Skip to content

Commit

Permalink
Convert popup=popup to popup=auto or just popup
Browse files Browse the repository at this point in the history
Per the [1] resolution, `popup=popup` has been renamed to `popup=auto`.
Additionally, per the [2] resolution, "boolean-like" behavior is also
supported, such that `<div popup>` means the same thing as `<div popup=auto>`. This CL implements both of these changes.

Note that this CL has one case that still needs to be fixed:
  <div id=foo popup=invalid>
  <script>
    foo.popup === null; // false, should be true
  </script>

To fix the above, I need to figure out how to specify the ReflectMissing
and ReflectInvalid values so that they mean "null".

[1] openui/open-ui#491 (comment)
[2] openui/open-ui#533 (comment)

Bug: 1307772
Change-Id: I6037c5322f7408ebd2c91690f89ecbc513c66bdb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3668816
Auto-Submit: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Reviewed-by: Aaron Leventhal <[email protected]>
Reviewed-by: Yuki Shiino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1009383}
NOKEYCHECK=True
GitOrigin-RevId: 4010500cd24efb96413178987cfb35a0c2419192
  • Loading branch information
mfreed7 authored and copybara-github committed Jun 1, 2022
1 parent 2436308 commit 0df86e8
Show file tree
Hide file tree
Showing 49 changed files with 231 additions and 184 deletions.
9 changes: 6 additions & 3 deletions blink/renderer/core/css/popup.css
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@

@namespace "http://www.w3.org/1999/xhtml";

[popup=popup i]:not(:top-layer),
[popup="" i]:not(:top-layer),
[popup=auto i]:not(:top-layer),
[popup=hint i]:not(:top-layer),
[popup=async i]:not(:top-layer) {
display: none;
}

[popup=popup i],
[popup="" i],
[popup=auto i],
[popup=hint i],
[popup=async i] {
display: block;
Expand All @@ -34,7 +36,8 @@
color: -internal-light-dark(black, white);
}

[popup=popup i]::backdrop,
[popup="" i]::backdrop,
[popup=auto i]::backdrop,
[popup=hint i]::backdrop,
[popup=async i]::backdrop {
position: fixed;
Expand Down
15 changes: 8 additions & 7 deletions blink/renderer/core/dom/element.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2399,8 +2399,9 @@ void Element::UpdatePopupAttribute(String value) {
if (!RuntimeEnabledFeatures::HTMLPopupAttributeEnabled())
return;
PopupValueType type = PopupValueType::kNone;
if (EqualIgnoringASCIICase(value, kPopupTypeValuePopup)) {
type = PopupValueType::kPopup;
if (EqualIgnoringASCIICase(value, kPopupTypeValueAuto) ||
(!value.IsNull() && value.IsEmpty())) {
type = PopupValueType::kAuto;
} else if (EqualIgnoringASCIICase(value, kPopupTypeValueHint)) {
type = PopupValueType::kHint;
} else if (EqualIgnoringASCIICase(value, kPopupTypeValueAsync)) {
Expand Down Expand Up @@ -2465,12 +2466,12 @@ void Element::showPopup(ExceptionState& exception_state) {
"Invalid on already-showing or disconnected popup elements");
}
bool should_restore_focus = false;
if (PopupType() == PopupValueType::kPopup ||
if (PopupType() == PopupValueType::kAuto ||
PopupType() == PopupValueType::kHint) {
if (GetDocument().HintShowing()) {
GetDocument().HideTopmostPopupOrHint(HidePopupFocusBehavior::kNone);
}
if (PopupType() == PopupValueType::kPopup) {
if (PopupType() == PopupValueType::kAuto) {
// Only hide other popups up to this popup's ancestral popup.
GetDocument().HideAllPopupsUntil(NearestOpenAncestralPopup(this),
HidePopupFocusBehavior::kNone);
Expand Down Expand Up @@ -2514,7 +2515,7 @@ void Element::hidePopupInternal(HidePopupFocusBehavior focus_behavior) {
DCHECK(isConnected());
DCHECK(HasValidPopupAttribute());
DCHECK(popupOpen());
if (PopupType() == PopupValueType::kPopup ||
if (PopupType() == PopupValueType::kAuto ||
PopupType() == PopupValueType::kHint) {
// Hide any popups/hints above us in the stack.
GetDocument().HideAllPopupsUntil(this, focus_behavior);
Expand Down Expand Up @@ -2635,7 +2636,7 @@ const Element* Element::NearestOpenAncestralPopup(Node* start_node) {
int indx = 0;
Document& document = start_node->GetDocument();
for (auto popup : document.PopupAndHintStack()) {
DCHECK(popup->PopupType() == PopupValueType::kPopup ||
DCHECK(popup->PopupType() == PopupValueType::kAuto ||
popup->PopupType() == PopupValueType::kHint);
popup_position.Set(popup, indx++);
if (const auto* anchor = popup->anchorElement())
Expand Down Expand Up @@ -2669,7 +2670,7 @@ const Element* Element::NearestOpenAncestralPopup(Node* start_node) {
continue;
if (current_element->HasValidPopupAttribute() &&
current_element->GetPopupData()->open() &&
(current_element->PopupType() == PopupValueType::kPopup ||
(current_element->PopupType() == PopupValueType::kAuto ||
current_element->PopupType() == PopupValueType::kHint)) {
// Case #1: a showing popup.
update_highest(current_element);
Expand Down
4 changes: 2 additions & 2 deletions blink/renderer/core/dom/element.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,11 @@ enum class NamedItemType {

enum class PopupValueType {
kNone,
kPopup,
kAuto,
kHint,
kAsync,
};
constexpr const char* kPopupTypeValuePopup = "popup";
constexpr const char* kPopupTypeValueAuto = "auto";
constexpr const char* kPopupTypeValueHint = "hint";
constexpr const char* kPopupTypeValueAsync = "async";

Expand Down
2 changes: 1 addition & 1 deletion blink/renderer/core/dom/element.idl
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ dictionary IsVisibleOptions {
// The Popup API
[MeasureAs=ElementShowPopup,RuntimeEnabled=HTMLPopupAttribute,RaisesException] void showPopup();
[MeasureAs=ElementHidePopup,RuntimeEnabled=HTMLPopupAttribute,RaisesException] void hidePopup();
[Unscopable,CEReactions,RuntimeEnabled=HTMLPopupAttribute,Reflect,ReflectOnly=("popup","hint","async"),ReflectMissing="",ReflectInvalid=""] attribute DOMString popup;
[Unscopable,CEReactions,RuntimeEnabled=HTMLPopupAttribute,Reflect,ReflectOnly=("auto","hint","async"),ReflectEmpty="auto"] attribute DOMString? popup;
[CEReactions,RuntimeEnabled=HTMLPopupAttribute,Reflect] attribute boolean defaultOpen;

// Experimental accessibility API
Expand Down
2 changes: 1 addition & 1 deletion blink/renderer/core/html/forms/html_select_menu_element.cc
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ void HTMLSelectMenuElement::DidAddUserAgentShadowRoot(ShadowRoot& root) {

Element* new_popup;
new_popup = MakeGarbageCollected<HTMLDivElement>(document);
new_popup->setAttribute(html_names::kPopupAttr, kPopupTypeValuePopup);
new_popup->setAttribute(html_names::kPopupAttr, kPopupTypeValueAuto);
new_popup->setAttribute(html_names::kPartAttr, kListboxPartName);
new_popup->setAttribute(html_names::kBehaviorAttr, kListboxPartName);
new_popup->SetShadowPseudoId(AtomicString("-internal-selectmenu-listbox"));
Expand Down
2 changes: 1 addition & 1 deletion blink/renderer/core/html/keywords.json5
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@

// popup attribute (experimental)
// https://github.com/openui/open-ui/blob/main/research/src/pages/popup/popup.research.explainer.mdx
"popup",
"auto",
"hint",
"async",

Expand Down
2 changes: 1 addition & 1 deletion blink/renderer/modules/accessibility/ax_node_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1843,7 +1843,7 @@ AccessibilityExpanded AXNodeObject::IsExpanded() const {
// aria-expanded=true when it is showing.
if (auto* form_control = DynamicTo<HTMLFormControlElement>(element)) {
if (auto popup = form_control->togglePopupElement().element;
popup && popup->PopupType() == PopupValueType::kPopup) {
popup && popup->PopupType() == PopupValueType::kAuto) {
return popup->popupOpen() ? kExpandedExpanded : kExpandedCollapsed;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
</style>

<selectmenu id="selectMenu0">
<div popup=popup slot="listbox" behavior="listbox" id="selectMenu0-popup">
<div popup slot="listbox" behavior="listbox" id="selectMenu0-popup">
<option>bottom left</option>
<option>two</option>
<option>three</option>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
<script src="/resources/testdriver-vendor.js"></script>

<selectmenu id="selectMenu0">
<div popup=popup slot="listbox" behavior="listbox">
<div popup slot="listbox" behavior="listbox">
<selectmenu id="nested0">
<option id="child1">one</option>
<option id="child2">two</option>
Expand All @@ -18,7 +18,7 @@
</selectmenu>

<selectmenu id="selectMenu1">
<div popup=popup slot="listbox" behavior="listbox">
<div popup slot="listbox" behavior="listbox">
<select>
<option>one</option>
<option>two</option>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<link rel="stylesheet" href="/fonts/ahem.css">

<selectmenu id="selectMenu0">
<div popup=popup slot="listbox" behavior="listbox">
<div popup slot="listbox" behavior="listbox">
<option>
option with image displayed
<img src="/images/green-256x256.png">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
</style>

<selectmenu id="selectMenu0">
<div popup=popup slot="listbox" behavior="listbox">
<div popup slot="listbox" behavior="listbox">
<option>
option with image displayed
<img src="/images/green-256x256.png">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
<script src="/resources/testdriver-vendor.js"></script>

<selectmenu id="selectMenu0">
<div popup=popup slot="listbox" behavior="listbox">
<div popup slot="listbox" behavior="listbox">
<option id="selectMenu0-child1">one</option>
<option id="selectMenu0-child2">two</option>
<div behavior="option" id="selectMenu0-child3">three</div>
Expand All @@ -18,7 +18,7 @@
</selectmenu>

<selectmenu id="selectMenu1">
<div popup=popup slot="listbox" behavior="listbox" id="selectMenu1-popup">
<div popup slot="listbox" behavior="listbox" id="selectMenu1-popup">
<div behavior="button" id="selectMenu1-button">
Custom button
</div>
Expand All @@ -30,7 +30,7 @@
<selectmenu id="selectMenu2">
<div slot="button" behavior="button" id="selectMenu2-button">
Custom button
<div popup=popup behavior="listbox" id="selectMenu2-popup">
<div popup behavior="listbox" id="selectMenu2-popup">
<option>one</option>
<option id="selectMenu2-child2">two</option>
</div>
Expand All @@ -53,7 +53,7 @@
<selectmenu id="selectMenu4">
<div slot="button" behavior="button" id="selectMenu4-button0">button0</div>
<div slot="listbox" id="selectMenu4-listbox-slot">
<div popup=popup behavior="listbox" id="selectMenu4-listbox0">
<div popup behavior="listbox" id="selectMenu4-listbox0">
<option>one</option>
<option id="selectMenu4-option2">two</option>
</div>
Expand All @@ -72,7 +72,7 @@
<!-- No associated JS test -- just don't crash when parsing! -->
<selectmenu id="selectMenu6">
<div slot="button"></div>
<div popup=popup slot="listbox" behavior="listbox"></div>
<div popup slot="listbox" behavior="listbox"></div>
</selectmenu>

<!-- No associated JS test -- just don't crash when parsing! -->
Expand All @@ -89,7 +89,7 @@

<selectmenu id="selectMenu9">
<div slot="listbox" id="selectMenu9-listbox-slot">
<div popup=popup behavior="listbox" id="selectMenu9-originalListbox">
<div popup behavior="listbox" id="selectMenu9-originalListbox">
<option>one</option>
<option id="selectMenu9-option2">two</option>
</div>
Expand All @@ -103,7 +103,7 @@
</selectmenu>

<selectmenu id="selectMenu11">
<div popup=popup slot="listbox" behavior="listbox">
<div popup slot="listbox" behavior="listbox">
<option>one</option>
</div>
<div slot="button" behavior="listbox" id="selectMenu11-button">Test</div>
Expand All @@ -114,7 +114,7 @@
<div behavior="button" id="selectMenu12-button0">button0</div>
</div>
<div slot="listbox" id="selectMenu12-listbox-slot">
<div popup=popup behavior="listbox" id="selectMenu12-originalListbox">
<div popup behavior="listbox" id="selectMenu12-originalListbox">
<option id="selectMenu12-option1">one</option>
<option>two</option>
</div>
Expand All @@ -131,12 +131,12 @@
</div>
<div slot="listbox" id="selectMenu13-listbox-slot">
<div id="selectMenu13-removeContent-listbox">
<div popup=popup behavior="listbox" id="selectMenu13-originalListbox">
<div popup behavior="listbox" id="selectMenu13-originalListbox">
<option id="selectMenu13-option1">one</option>
<option id="selectMenu13-option2">two</option>
</div>
</div>
<div popup=popup behavior="listbox" id="selectMenu13-newListbox">
<div popup behavior="listbox" id="selectMenu13-newListbox">
<option>three</option>
<option id="selectMenu13-option4">four</option>
</div>
Expand Down Expand Up @@ -313,7 +313,7 @@
assert_equals(selectMenu.value, "one", "Initial value should be the first option");

let newListbox = document.createElement("div");
newListbox.setAttribute("popup", "popup");
newListbox.setAttribute("popup", "auto");
newListbox.setAttribute("behavior", "listbox");
let newOption = document.createElement("option");
newOption.innerText = "three";
Expand Down Expand Up @@ -402,7 +402,7 @@
assert_false(selectMenu.open);

let newListbox = document.createElement("div");
newListbox.setAttribute("popup", "popup");
newListbox.setAttribute("popup", "auto");
newListbox.setAttribute("behavior", "listbox");
let newOption = document.createElement("option");
newOption.innerText = "three";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@

<selectmenu id="selectMenu0">
<div slot="button" behavior="button" id="selectMenu0-button">Custom bottom left</div>
<div popup=popup slot="listbox" behavior="listbox" id="selectMenu0-popup">
<div popup slot="listbox" behavior="listbox" id="selectMenu0-popup">
<option>bottom left</option>
<option>two</option>
<option>three</option>
Expand All @@ -58,7 +58,7 @@

<selectmenu id="selectMenu1">
<div slot="button" behavior="button" id="selectMenu1-button">Custom top left</div>
<div popup=popup slot="listbox" behavior="listbox" id="selectMenu1-popup">
<div popup slot="listbox" behavior="listbox" id="selectMenu1-popup">
<option>top left</option>
<option>two</option>
<option>three</option>
Expand All @@ -67,7 +67,7 @@

<selectmenu id="selectMenu2">
<div slot="button" behavior="button" id="selectMenu2-button">Custom bottom right</div>
<div popup=popup slot="listbox" behavior="listbox" id="selectMenu2-popup">
<div popup slot="listbox" behavior="listbox" id="selectMenu2-popup">
<option>bottom right</option>
<option>two</option>
<option>three</option>
Expand All @@ -76,7 +76,7 @@

<selectmenu id="selectMenu3">
<div slot="button" behavior="button" id="selectMenu3-button">Custom top right</div>
<div popup=popup slot="listbox" behavior="listbox" id="selectMenu3-popup">
<div popup slot="listbox" behavior="listbox" id="selectMenu3-popup">
<option>top right</option>
<option>two</option>
<option>three</option>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
</style>

<selectmenu id="selectMenu0">
<div popup=popup slot="listbox" behavior="listbox" id="selectMenu0-popup">
<div popup slot="listbox" behavior="listbox" id="selectMenu0-popup">
<option>bottom left</option>
<option>two</option>
<option>three</option>
Expand All @@ -44,23 +44,23 @@
<br>

<selectmenu id="selectMenu1">
<div popup=popup slot="listbox" behavior="listbox" id="selectMenu1-popup">
<div popup slot="listbox" behavior="listbox" id="selectMenu1-popup">
<option>top left</option>
<option>two</option>
<option>three</option>
</div>
</selectmenu>

<selectmenu id="selectMenu2">
<div popup=popup slot="listbox" behavior="listbox" id="selectMenu2-popup">
<div popup slot="listbox" behavior="listbox" id="selectMenu2-popup">
<option>bottom right</option>
<option>two</option>
<option>three</option>
</div>
</selectmenu>

<selectmenu id="selectMenu3">
<div popup=popup slot="listbox" behavior="listbox" id="selectMenu3-popup">
<div popup slot="listbox" behavior="listbox" id="selectMenu3-popup">
<option>top right</option>
<option>two</option>
<option>three</option>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
<div slot="button" behavior="button" class="button">
Custom button
</div>
<div popup=popup slot="listbox" behavior="listbox">
<div popup slot="listbox" behavior="listbox">
<option>one</option>
<option class="child2">two</option>
<option class="child3">three</option>
Expand All @@ -32,7 +32,7 @@

<selectmenu id="selectMenu3">
<div slot="listbox">
<div popup=popup behavior="listbox" id="selectMenu3-listbox">
<div popup behavior="listbox" id="selectMenu3-listbox">
<option>one</option>
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
<selectmenu id="selectmenu1">
<template shadowroot=open>
<button behavior="button">Custom button</button>
<div popup=popup behavior="listbox">
<div popup behavior="listbox">
<slot></slot>
</div>
</template>
Expand Down Expand Up @@ -58,7 +58,7 @@
</style>
<button behavior="button">Open select!</button>
<div id="value" behavior="selected-value"></div>
<div popup=popup behavior="listbox">
<div popup behavior="listbox">
<input type="text" placeholder="Filter options">
<option>Thing 1</option>
<option>Thing 2</option>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
<div slot="button" behavior="button">
<div behavior="selected-value" id="selectMenu5-custom-selected-value">Default custom selected-value text</div>
</div>
<div popup=popup slot="listbox" behavior="listbox">
<div popup slot="listbox" behavior="listbox">
<option id="selectMenu5-option1">one</option>
<option id="selectMenu5-option2">two</option>
</div>
Expand Down
Loading

0 comments on commit 0df86e8

Please sign in to comment.