Skip to content

Commit

Permalink
Convert :open to :popover-open for popovers
Browse files Browse the repository at this point in the history
See [1]/[2] for more context, but there are cases where `:open` is
ambiguous for popovers. If multiple elements support `:open/:closed`,
and [popover] can be applied to any of them, there are situations
where an element is both open and closed. For example,
`<details popover>` can be closed as a details element and open as
a popover, which makes it match both `:open` and `:closed`. It seems
that really `:open` and `:closed` should match *elements* that can
open and close, and not things that can be made to open or close
via an attribute or other mechanism such as JS.

This CL adds `:popover-open` which only applies to popovers in the open
state, and it removes `:open` and `:closed` support for popovers. It also converts all of the popover WPTs to use `:popover-open` instead
of either `:open` or `:closed`.

[1] w3c/csswg-drafts#8637
[2] whatwg/html#9077

Bug: 1307772
Fixed: 1429670
Change-Id: I8d840512166ccdb5d5c8abbb7192bbce7177ee88
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4373888
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124869}
  • Loading branch information
Mason Freed authored and chromium-wpt-export-bot committed Mar 31, 2023
1 parent 720672c commit e68cb91
Show file tree
Hide file tree
Showing 29 changed files with 342 additions and 336 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -208,13 +208,13 @@
const selectMenu1Popover = document.getElementById("selectMenu1-popover");
const selectMenu1Button = document.getElementById("selectMenu1-button");
const selectMenu1Child2 = document.getElementById("selectMenu1-child2");
assert_false(selectMenu1Popover.matches(':open'));
assert_false(selectMenu1Popover.matches(':popover-open'));
selectMenu1Button.click();
assert_false(selectMenu1Popover.matches(':open'), "Clicking a button part that is a descendant of the listbox part should have no effect");
assert_false(selectMenu1Popover.matches(':popover-open'), "Clicking a button part that is a descendant of the listbox part should have no effect");

assert_equals(selectMenu1.value, "one");
await clickOn(selectMenu1);
assert_true(selectMenu1Popover.matches(':open'));
assert_true(selectMenu1Popover.matches(':popover-open'));
await clickOn(selectMenu1Child2);
assert_equals(selectMenu1.value, "two", "Clicking an <option> should change the value");
}, "To receive button part controller code, an element labeled as a button must not be a descendant of the listbox part in a flat tree traversal");
Expand All @@ -226,9 +226,9 @@
const selectMenu2Child2 = document.getElementById("selectMenu2-child2");
const selectMenu2Child4 = document.getElementById("selectMenu2-child4");

assert_false(selectMenu2Popover.matches(':open'));
assert_false(selectMenu2Popover.matches(':popover-open'));
await clickOn(selectMenu2Button);
assert_false(selectMenu2Popover.matches(':open'), "Clicking a button part should not show an invalid listbox part");
assert_false(selectMenu2Popover.matches(':popover-open'), "Clicking a button part should not show an invalid listbox part");

assert_equals(selectMenu2.value, "three");
await clickOn(selectMenu2Button);

This comment has been minimized.

Copy link
@mbrodesser-Igalia

mbrodesser-Igalia Apr 10, 2023

Contributor

It would've been clearer if the three file renamings below would've been done in a separate commit, because the change of :open to :popover-open and the removal of :closed don't directly affect them.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@
});
assert_throws_dom('InvalidStateError', () => popover2.showPopover(),
"popover1's beforetoggle event handler removes popover2 so showPopover should throw.");
assert_false(popover2.matches(':open'), 'popover2 should not match :open once it is closed.');
assert_false(popover2.matches(':popover-open'), 'popover2 should not match :popover-open once it is closed.');
}, 'Removing a popover while it is opening and force closing another popover should throw an exception.');
</script>
6 changes: 3 additions & 3 deletions html/semantics/popovers/light-dismiss-event-ordering.html
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
}, {capture, once: true});
// Click away from the popover to activate light dismiss.
await clickOn(target);
assert_equals(document.querySelectorAll(':open').length, 0,
assert_equals(document.querySelectorAll(':popover-open').length, 0,
'The popover should be closed via light dismiss even when preventDefault is called.');

popover.showPopover();
Expand All @@ -37,7 +37,7 @@
}, {capture, once: true});
// Click away from the popover to activate light dismiss.
await clickOn(target);
assert_equals(document.querySelectorAll(':open').length, 0,
assert_equals(document.querySelectorAll(':popover-open').length, 0,
'The popover should be closed via light dismiss even when stopPropagation is called.');

}, `Tests the interactions between popover light dismiss and pointer/mouse events. eventName: ${eventName}, capture: ${capture}`);
Expand Down Expand Up @@ -75,7 +75,7 @@
assert_array_equals(events, expectedEvents,
'pointer and popover events should be fired in the correct order.');

assert_equals(document.querySelectorAll(':open').length, 0,
assert_equals(document.querySelectorAll(':popover-open').length, 0,
'The popover should be closed via light dismiss.');

}, 'Tests the order of pointer/mouse events during popover light dismiss.');
Expand Down
8 changes: 4 additions & 4 deletions html/semantics/popovers/popover-anchor-nesting.html
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,15 @@
setup({ explicit_done: true });

popover2.showPopover();
assert_false(popover1.matches(':open'));
assert_true(popover2.matches(':open'));
assert_false(popover1.matches(':popover-open'));
assert_true(popover2.matches(':popover-open'));
await clickOn(button1);
test(t => {
// Button1 is the anchor for popover1, and an ancestor of popover2.
// Since popover2 is open, but not popover1, button1 should not be
// the anchor of any open popover. So popover2 should be closed.
assert_false(popover2.matches(':open'));
assert_true(popover1.matches(':open'));
assert_false(popover2.matches(':popover-open'));
assert_true(popover1.matches(':popover-open'));
},'Nested popovers (inside anchor elements) do not affect light dismiss');

done();
Expand Down
67 changes: 40 additions & 27 deletions html/semantics/popovers/popover-attribute-basic.html
Original file line number Diff line number Diff line change
Expand Up @@ -154,25 +154,25 @@
test((t) => {
const popover = createPopover(t);
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover-open'));
if (popoverHintSupported()) {
popover.setAttribute('popover','hint'); // Change popover type
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover-open'));
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover-open'));
}
popover.setAttribute('popover','manual');
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover-open'));
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover-open'));
popover.setAttribute('popover','invalid');
assert_true(popover.matches(':open'),'From "manual" to "invalid" (which is interpreted as "manual") should not close the popover');
assert_true(popover.matches(':popover-open'),'From "manual" to "invalid" (which is interpreted as "manual") should not close the popover');
popover.setAttribute('popover','auto');
assert_false(popover.matches(':open'),'From "invalid" ("manual") to "auto" should hide the popover');
assert_false(popover.matches(':popover-open'),'From "invalid" ("manual") to "auto" should hide the popover');
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover-open'));
popover.setAttribute('popover','invalid');
assert_false(popover.matches(':open'),'From "auto" to "invalid" (which is interpreted as "manual") should close the popover');
assert_false(popover.matches(':popover-open'),'From "auto" to "invalid" (which is interpreted as "manual") should close the popover');
},'Changing attribute values should close open popovers');

const validTypes = popoverHintSupported() ? ["auto","hint","manual"] : ["auto","manual"];
Expand All @@ -181,21 +181,34 @@
const popover = createPopover(t);
popover.setAttribute('popover',type);
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover-open'));
popover.remove();
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover-open'));
document.body.appendChild(popover);
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover-open'));
},`Removing a visible popover=${type} element from the document should close the popover`);

test((t) => {
const popover = createPopover(t);
popover.setAttribute('popover',type);
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover-open'));
assert_false(popover.matches(':modal'));
popover.hidePopover();
},`A showing popover=${type} does not match :modal`);

test((t) => {
const popover = createPopover(t);
popover.setAttribute('popover',type);
assert_false(popover.matches(':popover-open'));
assert_false(popover.matches(':open'),'popovers never match :open');
assert_false(popover.matches(':closed'),'popovers never match :closed');
popover.showPopover();
assert_true(popover.matches(':popover-open'));
assert_false(popover.matches(':open'),'popovers never match :open');
assert_false(popover.matches(':closed'),'popovers never match :closed');
popover.hidePopover();
},`A popover=${type} never matches :open or :closed`);
});

test((t) => {
Expand All @@ -209,11 +222,11 @@
return;
popover.setAttribute('popover','manual');
},{once: true});
assert_true(other_popover.matches(':open'));
assert_false(popover.matches(':open'));
assert_true(other_popover.matches(':popover-open'));
assert_false(popover.matches(':popover-open'));
assert_throws_dom('InvalidStateError', () => popover.showPopover());
assert_false(other_popover.matches(':open'),'unrelated popover is hidden');
assert_false(popover.matches(':open'),'popover is not shown if its type changed during show');
assert_false(other_popover.matches(':popover-open'),'unrelated popover is hidden');
assert_false(popover.matches(':popover-open'),'popover is not shown if its type changed during show');
},`Changing the popover type in a "beforetoggle" event handler should throw an exception (during showPopover())`);

test((t) => {
Expand All @@ -236,11 +249,11 @@
return;
assert_true(nested_popover_hidden,'The nested popover should be hidden first');
},{once: true});
assert_true(popover.matches(':open'));
assert_true(other_popover.matches(':open'));
assert_true(popover.matches(':popover-open'));
assert_true(other_popover.matches(':popover-open'));
assert_throws_dom('InvalidStateError', () => popover.hidePopover());
assert_false(other_popover.matches(':open'),'unrelated popover is hidden');
assert_false(popover.matches(':open'),'popover is still hidden if its type changed during hide event');
assert_false(other_popover.matches(':popover-open'),'unrelated popover is hidden');
assert_false(popover.matches(':popover-open'),'popover is still hidden if its type changed during hide event');
assert_throws_dom("InvalidStateError",() => other_popover.hidePopover(),'Nested popover should already be hidden');
},`Changing the popover type in a "beforetoggle" event handler should throw an exception (during hidePopover())`);

Expand Down Expand Up @@ -277,7 +290,7 @@
const popover = createPopover(t);
setPopoverValue(popover,type,method);
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover-open'));
let gotEvent = false;
popover.addEventListener('beforetoggle', (e) => {
if (e.newState !== "closed")
Expand All @@ -288,7 +301,7 @@
setPopoverValue(popover,newType,method);
if (type===interpretedType(newType,method)) {
// Keeping the type the same should not hide it or fire events.
assert_true(popover.matches(':open'),'popover should remain open when not changing the type');
assert_true(popover.matches(':popover-open'),'popover should remain open when not changing the type');
assert_false(gotEvent);
try {
popover.hidePopover(); // Cleanup
Expand All @@ -297,7 +310,7 @@
// Changing the type at all should hide the popover. The hide event
// handler should run, set a new type, and that type should end up
// as the final result.
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover-open'));
if (inEventType === undefined || (method ==="idl" && inEventType === null)) {
assert_throws_dom("NotSupportedError",() => popover.showPopover(),'We should have removed the popover attribute, so showPopover should throw');
} else {
Expand All @@ -306,16 +319,16 @@
assert_equals(popover.popover, interpretedType(inEventType,method),'IDL attribute');
// Make sure the type is really correct, via behavior.
popover.showPopover(); // Show it
assert_true(popover.matches(':open'),'Popover should function');
assert_true(popover.matches(':popover-open'),'Popover should function');
await clickOn(outsideElement); // Try to light dismiss
switch (interpretedType(inEventType,method)) {
case 'manual':
assert_true(popover.matches(':open'),'A popover=manual should not light-dismiss');
assert_true(popover.matches(':popover-open'),'A popover=manual should not light-dismiss');
popover.hidePopover();
break;
case 'auto':
case 'hint':
assert_false(popover.matches(':open'),'A popover=auto should light-dismiss');
assert_false(popover.matches(':popover-open'),'A popover=auto should light-dismiss');
break;
}
}
Expand Down
8 changes: 4 additions & 4 deletions html/semantics/popovers/popover-document-open.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,17 @@
test((t) => {
const popover1 = document.querySelector('#popover1');
popover1.showPopover();
assert_true(popover1.matches(':open'));
assert_true(popover1.matches(':popover-open'));
assert_true(!document.querySelector('#popover2'));
document.open();
document.write('<!DOCTYPE html><div popover id=popover2>Popover</div>');
document.close();
assert_true(!document.querySelector('#popover1'),'popover1 should be removed from the document');
assert_true(!!document.querySelector('#popover2'),'popover2 should be in the document');
assert_false(popover1.matches(':open'),'popover1 should have been hidden when it was removed from the document');
assert_false(popover1.matches(':open'),'popover2 shouldn\'t be showing yet');
assert_false(popover1.matches(':popover-open'),'popover1 should have been hidden when it was removed from the document');
assert_false(popover1.matches(':popover-open'),'popover2 shouldn\'t be showing yet');
popover2.showPopover();
assert_true(popover2.matches(':open'),'popover2 should be able to be shown');
assert_true(popover2.matches(':popover-open'),'popover2 should be able to be shown');
popover2.hidePopover();
},'document.open should not break popovers');
};
Expand Down
Loading

0 comments on commit e68cb91

Please sign in to comment.