Skip to content

Commit

Permalink
Combine the popoverhide/popovershow events into one toggle event
Browse files Browse the repository at this point in the history
See [1] for context. This eliminates the "popoverhide" and
"popovershow" events, and re-uses the "toggle" event for both
of these transitions. The event in this case is a ToggleEvent,
which has a `state` that is either "opening" or "closing".

[1] openui/open-ui#607 (comment)

Bug: 1307772
Change-Id: I404861a10579365b56e6e8eae7c29f88a5aac166
  • Loading branch information
mfreed7 authored and chromium-wpt-export-bot committed Nov 28, 2022
1 parent 22ecfc9 commit 9f5066f
Show file tree
Hide file tree
Showing 8 changed files with 182 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@
assert_true(isElementVisible(popover));
assert_equals(popover.getAnimations({subtree: true}).length,0);
let animation;
popover.addEventListener('popoverhide', () => {
popover.addEventListener('toggle', (e) => {
if (e.state !== "closing") return;
animation = popover.animate([{opacity: 1},{opacity: 0}],1000000);
});
assert_equals(popover.getAnimations({subtree: true}).length,0,'There should be no animations yet');
Expand All @@ -111,7 +112,7 @@
animation.finish();
await waitForRender();
assert_false(isElementVisible(popover),'Once the animation ends, the popover is hidden');
},'It should be possible to use the "popoverhide" event handler to animate the hide');
},'It should be possible to use the "toggle" event handler to animate the hide');


promise_test(async (t) => {
Expand All @@ -121,27 +122,26 @@
popover.showPopover();
assert_true(isElementVisible(popover));
assert_equals(popover.getAnimations({subtree: true}).length,0);
popover.addEventListener('popoverhide', () => {
popover.addEventListener('toggle', (e) => {
if (e.state !== "closing") return;
popover.animate([{opacity: 1},{opacity: 0}],1000000);
});
assert_equals(popover.getAnimations({subtree: true}).length,0,'There should be no animations yet');
dialog.showModal(); // Force hide the popover
await waitForRender();
assert_equals(popover.getAnimations({subtree: true}).length,1,'the hide animation should now be running');
assert_false(isElementVisible(popover),'But the animation should *not* keep the popover visible in this case');
},'It should *not* be possible to use the "popoverhide" event handler to animate the hide, if the hide is due to dialog.showModal');
},'It should *not* be possible to use the "toggle" event handler to animate the hide, if the hide is due to dialog.showModal');

promise_test(async (t) => {
const {popover, descendent} = createPopover(t,'');
popover.showPopover();
assert_true(isElementVisible(popover));
popover.addEventListener('popoverhide', (e) => {
e.preventDefault();
});
popover.addEventListener('toggle', (e) => e.preventDefault());
popover.hidePopover();
await waitForRender();
assert_false(isElementVisible(popover),'Even if hide event is cancelled, the popover still closes');
},'hide event cannot be cancelled');
},'toggle event cannot be cancelled');

promise_test(async (t) => {
const {popover, descendent} = createPopover(t,'animation');
Expand Down Expand Up @@ -169,7 +169,8 @@
popover.showPopover();
assert_true(isElementVisible(popover));
assert_equals(popover.getAnimations({subtree: true}).length,0);
popover.addEventListener('popoverhide', () => {
popover.addEventListener('toggle', (e) => {
if (e.state !== "closing") return;
popover.animate([{opacity: 1},{opacity: 0}],1000000);
});
assert_equals(popover.getAnimations({subtree: true}).length,0,'There should be no animations yet');
Expand All @@ -193,7 +194,8 @@
promise_test(async (t) => {
const {popover, descendent} = createPopover(t,'');
popover.showPopover();
popover.addEventListener('popoverhide', () => {
popover.addEventListener('toggle', (e) => {
if (e.state !== "closing") return;
popover.animate([{opacity: 1},{opacity: 0}],1000000);
});
popover.hidePopover();
Expand Down
18 changes: 11 additions & 7 deletions html/semantics/popovers/popover-attribute-basic.tentative.html
Original file line number Diff line number Diff line change
Expand Up @@ -275,15 +275,16 @@
other_popover.showPopover();
const popover = createPopover(t);
popover.setAttribute('popover','auto');
other_popover.addEventListener('popoverhide',() => {
other_popover.addEventListener('toggle', (e) => {
if (e.state !== "closing") return;
popover.setAttribute('popover','manual');
},{once: true});
assert_true(other_popover.matches(':open'));
assert_false(popover.matches(':open'));
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');
},`Changing the popover type in a "popoverhide" event handler should not cause problems (during showPopover())`);
},`Changing the popover type in a "toggle" event handler should not cause problems (during showPopover())`);

test((t) => {
const popover = createPopover(t);
Expand All @@ -294,11 +295,13 @@
popover.showPopover();
other_popover.showPopover();
let nested_popover_hidden=false;
other_popover.addEventListener('popoverhide',() => {
other_popover.addEventListener('toggle', (e) => {
if (e.state !== "closing") return;
nested_popover_hidden = true;
popover.setAttribute('popover','manual');
},{once: true});
popover.addEventListener('popoverhide',() => {
popover.addEventListener('toggle', (e) => {
if (e.state !== "closing") return;
assert_true(nested_popover_hidden,'The nested popover should be hidden first');
},{once: true});
assert_true(popover.matches(':open'));
Expand All @@ -307,7 +310,7 @@
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_throws_dom("InvalidStateError",() => other_popover.hidePopover(),'Nested popover should already be hidden');
},`Changing the popover type in a "popoverhide" event handler should not cause problems (during hidePopover())`);
},`Changing the popover type in a "toggle" event handler should not cause problems (during hidePopover())`);

function interpretedType(typeString,method) {
if (validTypes.includes(typeString))
Expand Down Expand Up @@ -344,7 +347,8 @@
popover.showPopover();
assert_true(popover.matches(':open'));
let gotEvent = false;
popover.addEventListener('popoverhide',() => {
popover.addEventListener('toggle', (e) => {
if (e.state !== "closing") return;
gotEvent = true;
setPopoverValue(popover,inEventType,method);
},{once:true});
Expand Down Expand Up @@ -380,7 +384,7 @@
}
}
}
},`Changing a popover from ${type} to ${newType} (via ${method}), and then ${inEventType} during 'popoverhide' works`);
},`Changing a popover from ${type} to ${newType} (via ${method}), and then ${inEventType} during 'toggle' works`);
});
});
});
Expand Down
40 changes: 19 additions & 21 deletions html/semantics/popovers/popover-events.tentative.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,28 @@
assert_false(popover.matches(':open'));
let showCount = 0;
let hideCount = 0;
function showListener(e) {
assert_true(e.target.matches(':closed'),'The popover should be in the :closed state when the popovershow event fires.');
assert_false(e.target.matches(':open'),'The popover should *not* be in the :open state when the popovershow event fires.');
++showCount;
};
function hideListener(e) {
assert_true(e.target.matches(':open'),'The popover should be in the :open state when the popoverhide event fires.');
assert_false(e.target.matches(':closed'),'The popover should *not* be in the :closed state when the popoverhide event fires.');
++hideCount;
function listener(e) {
if (e.state === "opening") {
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 {
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;
}
};
switch (method) {
case "listener":
const controller = new AbortController();
const signal = controller.signal;
t.add_cleanup(() => controller.abort());
document.addEventListener('popovershow',showListener, {signal});
document.addEventListener('popoverhide',hideListener, {signal});
document.addEventListener('toggle', listener, {signal});
break;
case "attribute":
assert_false(popover.hasAttribute('onpopovershow'));
assert_false(popover.hasAttribute('onpopoverhide'));
t.add_cleanup(() => popover.removeAttribute('onpopovershow'));
t.add_cleanup(() => popover.removeAttribute('onpopoverhide'));
popover.onpopovershow = showListener;
popover.onpopoverhide = hideListener;
assert_false(popover.hasAttribute('ontoggle'));
t.add_cleanup(() => popover.removeAttribute('ontoggle'));
popover.ontoggle = listener;
break;
default: assert_unreached();
}
Expand All @@ -62,7 +59,7 @@
assert_false(popover.matches(':open'));
assert_equals(1,showCount);
assert_equals(1,hideCount);
}, `Popovershow and popoverhide events (${method}) get properly dispatched for popovers`);
}, `Toggle event (${method}) get properly dispatched for popovers`);
}

promise_test(async t => {
Expand All @@ -71,18 +68,19 @@
const signal = controller.signal;
t.add_cleanup(() => controller.abort());
let cancel = true;
popover.addEventListener('popovershow',(e) => {
popover.addEventListener('toggle',(e) => {
if (e.state !== "opening") return;
if (cancel)
e.preventDefault();
}, {signal});
assert_false(popover.matches(':open'));
popover.showPopover();
assert_false(popover.matches(':open'),'The "popovershow" event should be cancelable');
assert_false(popover.matches(':open'),'The "toggle" event should be cancelable for the "opening" transition');
cancel = false;
popover.showPopover();
assert_true(popover.matches(':open'));
popover.hidePopover();
assert_false(popover.matches(':open'));
}, 'Popovershow event is cancelable');
}, 'Toggle event is cancelable for the "opening" transition');
};
</script>
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,12 @@
const button = document.querySelector('button');
let showCount = 0;
let hideCount = 0;
popover.addEventListener('popovershow',() => ++showCount);
popover.addEventListener('popoverhide',() => ++hideCount);
popover.addEventListener('toggle',(e) => {
if (e.state === "opening")
++showCount;
else
++hideCount;
});

async function assertState(expectOpen,expectShow,expectHide) {
assert_equals(popover.matches(':open'),expectOpen,'Popover open state is incorrect');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@
}
async_test(t => {
for(let popover of popovers) {
popover.addEventListener('popoverhide',e => {
popover.addEventListener('toggle',e => {
if (e.state !== "closing") return;
assert_unreached('Scrolling should not light-dismiss a popover');
});
}
Expand Down
13 changes: 8 additions & 5 deletions html/semantics/popovers/popover-light-dismiss.tentative.html
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,16 @@
const afterp1 = document.querySelector('#after_p1');

let popover1HideCount = 0;
popover1.addEventListener('popoverhide',(e) => {
popover1.addEventListener('toggle',(e) => {
if (e.state !== "closing") return;
++popover1HideCount;
e.preventDefault(); // 'popoverhide' should not be cancellable.
e.preventDefault(); // 'toggle' should not be cancellable.
});
let popover2HideCount = 0;
popover2.addEventListener('popoverhide',(e) => {
popover2.addEventListener('toggle',(e) => {
if (e.state !== "closing") return;
++popover2HideCount;
e.preventDefault(); // 'popoverhide' should not be cancellable.
e.preventDefault(); // 'toggle' should not be cancellable.
});
promise_test(async () => {
assert_false(popover1.matches(':open'));
Expand Down Expand Up @@ -482,7 +484,8 @@
p13.showPopover();
p14.showPopover();
p15.showPopover();
p15.addEventListener('popoverhide',() => {
p15.addEventListener('toggle', (e) => {
if (e.state !== "closing") return;
p14.hidePopover();
},{once:true});
assert_true(p13.matches(':open') && p14.matches(':open') && p15.matches(':open'),'all three should be open');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
requestAnimationFrame(() => {++frameCount;});
const popover = document.querySelector('[popover]');
const testText = 'Show Event Occurred';
popover.addEventListener('popovershow',() => {
popover.addEventListener('toggle',(e) => {
if (e.state !== "opening") return;
popover.textContent = testText;
})
popover.offsetHeight;
Expand All @@ -25,5 +26,5 @@
assert_equals(popover.textContent,testText);
assert_equals(frameCount,0,'nothing should be rendered before the popover is updated');
popover.hidePopover(); // Cleanup
},'Ensure the `show` event can be used to populate content before the popover renders');
},'Ensure the `toggle` event can be used to populate content before the popover renders');
</script>
Loading

0 comments on commit 9f5066f

Please sign in to comment.