Skip to content

Commit

Permalink
Convert :open to :popover for popovers
Browse files Browse the repository at this point in the history
See [1] 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` which only applies to popovers in the open
state, but it does not yet remove `:open` and `:closed` support for
popovers. However, it does convert all of the popover WPTs to use
`:popover` instead of either `:open` or `:closed`.

[1] w3c/csswg-drafts#8637

Bug: 1307772
Change-Id: I8d840512166ccdb5d5c8abbb7192bbce7177ee88
  • Loading branch information
mfreed7 authored and chromium-wpt-export-bot committed Mar 27, 2023
1 parent de685a7 commit 80d0037
Show file tree
Hide file tree
Showing 25 changed files with 324 additions and 331 deletions.
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'), 'popover2 should not match :popover 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').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').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').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'));
assert_true(popover2.matches(':popover'));
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'));
assert_true(popover1.matches(':popover'));
},'Nested popovers (inside anchor elements) do not affect light dismiss');

done();
Expand Down
54 changes: 27 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'));
if (popoverHintSupported()) {
popover.setAttribute('popover','hint'); // Change popover type
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
}
popover.setAttribute('popover','manual');
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
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'),'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'),'From "invalid" ("manual") to "auto" should hide the popover');
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
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'),'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,18 +181,18 @@
const popover = createPopover(t);
popover.setAttribute('popover',type);
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
popover.remove();
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
document.body.appendChild(popover);
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
},`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'));
assert_false(popover.matches(':modal'));
popover.hidePopover();
},`A showing popover=${type} does not match :modal`);
Expand All @@ -209,11 +209,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'));
assert_false(popover.matches(':popover'));
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'),'unrelated popover is hidden');
assert_false(popover.matches(':popover'),'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 +236,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'));
assert_true(other_popover.matches(':popover'));
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'),'unrelated popover is hidden');
assert_false(popover.matches(':popover'),'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 +277,7 @@
const popover = createPopover(t);
setPopoverValue(popover,type,method);
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
let gotEvent = false;
popover.addEventListener('beforetoggle', (e) => {
if (e.newState !== "closed")
Expand All @@ -288,7 +288,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'),'popover should remain open when not changing the type');
assert_false(gotEvent);
try {
popover.hidePopover(); // Cleanup
Expand All @@ -297,7 +297,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'));
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 +306,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'),'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'),'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'),'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'));
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'),'popover1 should have been hidden when it was removed from the document');
assert_false(popover1.matches(':popover'),'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'),'popover2 should be able to be shown');
popover2.hidePopover();
},'document.open should not break popovers');
};
Expand Down
46 changes: 21 additions & 25 deletions html/semantics/popovers/popover-events.html
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
for(const method of ["listener","attribute"]) {
promise_test(async t => {
const {popover,signal} = getPopoverAndSignal(t);
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
let showCount = 0;
let afterShowCount = 0;
let hideCount = 0;
Expand All @@ -33,15 +33,13 @@
if (e.newState === "open") {
++showCount;
assert_equals(e.oldState,"closed",'The "beforetoggle" event should be fired before the popover is open');
assert_true(e.target.matches(':closed'),'The popover should be in the :closed state when the opening event fires.');
assert_false(e.target.matches(':open'),'The popover should *not* be in the :open state when the opening event fires.');
assert_false(e.target.matches(':popover'),'The popover should *not* be in the :popover state when the opening event fires.');
assert_true(e.cancelable,'beforetoggle should be cancelable only for the "show" transition');
} else {
++hideCount;
assert_equals(e.newState,"closed",'Popover toggleevent states should be "open" and "closed"');
assert_equals(e.oldState,"open",'The "beforetoggle" event should be fired before the popover is closed')
assert_true(e.target.matches(':open'),'The popover should be in the :open state when the hiding event fires.');
assert_false(e.target.matches(':closed'),'The popover should *not* be in the :closed state when the hiding event fires.');
assert_true(e.target.matches(':popover'),'The popover should be in the :popover state when the hiding event fires.');
assert_false(e.cancelable,'beforetoggle should be cancelable only for the "show" transition');
e.preventDefault(); // beforetoggle should be cancelable only for the "show" transition
}
Expand All @@ -52,14 +50,12 @@
if (e.newState === "open") {
++afterShowCount;
if (document.body.contains(e.target)) {
assert_true(e.target.matches(':open'),'The popover should be in the :open state when the after opening event fires.');
assert_false(e.target.matches(':closed'),'The popover should *not* be in the :closed state when the after opening event fires.');
assert_true(e.target.matches(':popover'),'The popover should be in the :popover state when the after opening event fires.');
}
} else {
++afterHideCount;
assert_equals(e.newState,"closed",'Popover toggleevent states should be "open" and "closed"');
assert_true(e.target.matches(':closed'),'The popover should be in the :closed state when the after hiding event fires.');
assert_false(e.target.matches(':open'),'The popover should *not* be in the :open state when the after hiding event fires.');
assert_false(e.target.matches(':popover'),'The popover should *not* be in the :popover state when the after hiding event fires.');
}
e.preventDefault(); // "toggle" should not be cancelable.
}
Expand All @@ -85,17 +81,17 @@
assert_equals(0,afterShowCount);
assert_equals(0,afterHideCount);
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
assert_equals(1,showCount);
assert_equals(0,hideCount);
assert_equals(0,afterShowCount);
assert_equals(0,afterHideCount);
await waitForRender();
assert_equals(1,afterShowCount,'toggle show is fired asynchronously');
assert_equals(0,afterHideCount);
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
popover.hidePopover();
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
assert_equals(1,showCount);
assert_equals(1,hideCount);
assert_equals(1,afterShowCount);
Expand All @@ -106,7 +102,7 @@
// No additional events
await waitForRender();
await waitForRender();
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
assert_equals(1,showCount);
assert_equals(1,hideCount);
assert_equals(1,afterShowCount);
Expand All @@ -123,29 +119,29 @@
if (cancel)
e.preventDefault();
}, {signal});
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
popover.showPopover();
assert_false(popover.matches(':open'),'The "beforetoggle" event should be cancelable for the "opening" transition');
assert_false(popover.matches(':popover'),'The "beforetoggle" event should be cancelable for the "opening" transition');
cancel = false;
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
popover.hidePopover();
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
}, 'The "beforetoggle" event is cancelable for the "opening" transition');

promise_test(async t => {
const {popover,signal} = getPopoverAndSignal(t);
popover.addEventListener('beforetoggle',(e) => {
assert_not_equals(e.newState,"closed",'The "beforetoggle" event was fired for the closing transition');
}, {signal});
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
popover.showPopover();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
t.add_cleanup(() => {document.body.appendChild(popover);});
popover.remove();
await waitForRender(); // Check for async events also
await waitForRender(); // Check for async events also
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
}, 'The "beforetoggle" event is not fired for element removal');

promise_test(async t => {
Expand Down Expand Up @@ -190,31 +186,31 @@

resetEvents();
assertOnly('none');
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
popover.showPopover();
await waitForRender();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
assertOnly('singleShow','Single event should have been fired, which is a "show"');

resetEvents();
popover.hidePopover();
popover.showPopover(); // Immediate re-show
await waitForRender();
assert_true(popover.matches(':open'));
assert_true(popover.matches(':popover'));
assertOnly('coalescedShow','Single coalesced event should have been fired, which is a "show"');

resetEvents();
popover.hidePopover();
await waitForRender();
assertOnly('singleHide','Single event should have been fired, which is a "hide"');
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));

resetEvents();
popover.showPopover();
popover.hidePopover(); // Immediate re-hide
await waitForRender();
assertOnly('coalescedHide','Single coalesced event should have been fired, which is a "hide"');
assert_false(popover.matches(':open'));
assert_false(popover.matches(':popover'));
}, 'The "toggle" event is coalesced');
};
</script>
Loading

0 comments on commit 80d0037

Please sign in to comment.