Skip to content

Commit

Permalink
Reland "Update popover post-toggle event naming and behavior"
Browse files Browse the repository at this point in the history 
This is a reland of commit 2e5ee120d7fc5df4b6c101e88d03a5f62a73d8b8

It seems that the event coalescing behavior changes cause a segfault
in the event dispatch code. This reland (compare to Patchset 1) just
includes the event renaming, and not the new behavior. I'll land
the new behavior in a separate CL to make it smaller if it gets
reverted again. This part should be safe.

Original change's description:
> Update popover post-toggle event naming and behavior
>
> This CL updates the post-toggle event in the following ways:
>  1. Rename the 'aftertoggle' event to 'toggle'.
>  2. Rename PopoverToggleEvent to ToggleEvent.
>  3. Rename the currentState attribute to oldState.
>  4. Add event coalescing behavior. If two transitions occur before the
>     first 'toggle' event has been fired, cancel the first event and
>     queue a replacement that has oldState === newState.
>
> These changes were driven by the corresponding changes to the spec PR:
>   whatwg/html#8717
>
> Bug: 1307772
> Change-Id: Iabc5a9093d7cef3bbd6e54e488d8e571c51ea568
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4195120
> Auto-Submit: Mason Freed <[email protected]>
> Commit-Queue: Joey Arhar <[email protected]>
> Reviewed-by: Joey Arhar <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1098728}

Bug: 1307772
Change-Id: Iede18bbf05516cec4ee881f1afc663c45380c82e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4205710
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1099397}
  • Loading branch information
@mfreed7 @marcoscaceres
mfreed7 authored and marcoscaceres committed Mar 28, 2023
1 parent 882f4ce commit d5d326d
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 120 deletions.
12 changes: 6 additions & 6 deletions html/semantics/popovers/idlharness.tentative.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,12 @@
'document.getElementById("b3")',
],
BeforeToggleEvent: [
'new PopoverToggleEvent("beforetoggle")',
'new PopoverToggleEvent("beforetoggle", {currentState: "open"})',
'new PopoverToggleEvent("beforetoggle", {currentState: "open",newState: "open"})',
'new PopoverToggleEvent("aftertoggle")',
'new PopoverToggleEvent("aftertoggle", {currentState: "open"})',
'new PopoverToggleEvent("aftertoggle", {currentState: "open",newState: "open"})',
'new ToggleEvent("beforetoggle")',
'new ToggleEvent("beforetoggle", {oldState: "open"})',
'new ToggleEvent("beforetoggle", {oldState: "open",newState: "open"})',
'new ToggleEvent("toggle")',
'new ToggleEvent("toggle", {oldState: "open"})',
'new ToggleEvent("toggle", {oldState: "open",newState: "open"})',
],
});
}
Expand Down
120 changes: 93 additions & 27 deletions html/semantics/popovers/popover-events.tentative.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@
<div popover>Popover</div>

<script>
function getPopoverAndSignal(t) {
const popover = document.querySelector('[popover]');
const controller = new AbortController();
const signal = controller.signal;
t.add_cleanup(() => controller.abort());
return {popover, signal};
}
window.onload = () => {
for(const method of ["listener","attribute"]) {
promise_test(async t => {
Expand All @@ -22,52 +29,48 @@
function listener(e) {
if (e.type === "beforetoggle") {
if (e.newState === "open") {
assert_equals(e.currentState,"closed",'The "beforetoggle" event should be fired before the popover is 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.');
++showCount;
} else {
++hideCount;
assert_equals(e.newState,"closed",'Popover toggleevent states should be "open" and "closed"');
assert_equals(e.currentState,"open",'The "beforetoggle" event should be fired before the popover is 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.');
++hideCount;
}
} else {
assert_equals(e.type,"aftertoggle",'Popover events should be "beforetoggle" and "aftertoggle"')
assert_equals(e.type,"toggle",'Popover events should be "beforetoggle" and "toggle"')
if (e.newState === "open") {
assert_equals(e.currentState,"open",'Aftertoggle should be fired after the popover is 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.');
}
++afterShowCount;
} else {
++afterHideCount;
assert_equals(e.newState,"closed",'Popover toggleevent states should be "open" and "closed"');
assert_equals(e.currentState,"closed",'Aftertoggle should be fired after the popover is 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.');
++afterHideCount;
}
e.preventDefault(); // "aftertoggle" should not be cancelable.
e.preventDefault(); // "toggle" should not be cancelable.
}
};
switch (method) {
case "listener":
const controller = new AbortController();
const signal = controller.signal;
t.add_cleanup(() => controller.abort());
const {signal} = getPopoverAndSignal(t);
// These events bubble.
document.addEventListener('beforetoggle', listener, {signal});
document.addEventListener('aftertoggle', listener, {signal});
document.addEventListener('toggle', listener, {signal});
break;
case "attribute":
assert_false(popover.hasAttribute('onbeforetoggle'));
t.add_cleanup(() => popover.removeAttribute('onbeforetoggle'));
popover.onbeforetoggle = listener;
assert_false(popover.hasAttribute('onaftertoggle'));
t.add_cleanup(() => popover.removeAttribute('onaftertoggle'));
popover.onaftertoggle = listener;
assert_false(popover.hasAttribute('ontoggle'));
t.add_cleanup(() => popover.removeAttribute('ontoggle'));
popover.ontoggle = listener;
break;
default: assert_unreached();
}
Expand All @@ -82,7 +85,7 @@
assert_equals(0,afterShowCount);
assert_equals(0,afterHideCount);
await waitForRender();
assert_equals(1,afterShowCount,'aftertoggle show is fired asynchronously');
assert_equals(1,afterShowCount,'toggle show is fired asynchronously');
assert_equals(0,afterHideCount);
assert_true(popover.matches(':open'));
popover.hidePopover();
Expand All @@ -93,7 +96,7 @@
assert_equals(0,afterHideCount);
await waitForRender();
assert_equals(1,afterShowCount);
assert_equals(1,afterHideCount,'aftertoggle hide is fired asynchronously');
assert_equals(1,afterHideCount,'toggle hide is fired asynchronously');
// No additional events
await waitForRender();
await waitForRender();
Expand All @@ -106,10 +109,7 @@
}

promise_test(async t => {
const popover = document.querySelector('[popover]');
const controller = new AbortController();
const signal = controller.signal;
t.add_cleanup(() => controller.abort());
const {popover,signal} = getPopoverAndSignal(t);
let cancel = true;
popover.addEventListener('beforetoggle',(e) => {
if (e.newState !== "open")
Expand All @@ -128,10 +128,7 @@
}, 'The "beforetoggle" event is cancelable for the "opening" transition');

promise_test(async t => {
const popover = document.querySelector('[popover]');
const controller = new AbortController();
const signal = controller.signal;
t.add_cleanup(() => {controller.abort();});
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});
Expand All @@ -144,5 +141,74 @@
await waitForRender(); // Check for async events also
assert_false(popover.matches(':open'));
}, 'The "beforetoggle" event is not fired for element removal');

promise_test(async t => {
const {popover,signal} = getPopoverAndSignal(t);
let events;
function resetEvents() {
events = {
singleShow: false,
singleHide: false,
coalescedShow: false,
coalescedHide: false,
};
}
function setEvent(type) {
assert_equals(events[type],false,'event repeated');
events[type] = true;
}
function assertOnly(type,msg) {
Object.keys(events).forEach(val => {
assert_equals(events[val],val===type,`${msg} (${val})`);
});
}
popover.addEventListener('toggle',(e) => {
switch (e.newState) {
case "open":
switch (e.oldState) {
case "open": setEvent('coalescedShow'); break;
case "closed": setEvent('singleShow'); break;
default: assert_unreached();
}
break;
case "closed":
switch (e.oldState) {
case "closed": setEvent('coalescedHide'); break;
case "open": setEvent('singleHide'); break;
default: assert_unreached();
}
break;
default: assert_unreached();
}
}, {signal});

resetEvents();
assertOnly('none');
assert_false(popover.matches(':open'));
popover.showPopover();
await waitForRender();
assert_true(popover.matches(':open'));
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'));
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'));

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'));
}, 'The "toggle" event is coalesced');
};
</script>
Loading

0 comments on commit d5d326d

Please sign in to comment.