Skip to content

Commit

Permalink
Rename show->popupshow, hide->popuphide, and add global handlers
Browse files Browse the repository at this point in the history
Per [1], there's feedback that "show" and "hide" are too generic
and could be confusing. Developers might think anything that shows
or hides (e.g. via display:none) should fire these events. So this
CL renames them to "popupshow" and "popuphide". It also adds global
event handlers, behind the flag.

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

Bug: 1307772
Change-Id: Iaec1306176e7e08dc367c25b801d6b21f19a1f88
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3938538
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1056698}
  • Loading branch information
mfreed7 authored and chromium-wpt-export-bot committed Oct 8, 2022
1 parent eee708c commit 0e16224
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
assert_true(isElementVisible(popUp));
assert_equals(popUp.getAnimations({subtree: true}).length,0);
let animation;
popUp.addEventListener('hide', () => {
popUp.addEventListener('popuphide', () => {
animation = popUp.animate([{opacity: 1},{opacity: 0}],1000000);
});
assert_equals(popUp.getAnimations({subtree: true}).length,0,'There should be no animations yet');
Expand All @@ -105,7 +105,7 @@
animation.finish();
await waitForRender();
assert_false(isElementVisible(popUp),'Once the animation ends, the popup is hidden');
},'It should be possible to use the "hide" event handler to animate the hide');
},'It should be possible to use the "popuphide" event handler to animate the hide');


promise_test(async (t) => {
Expand All @@ -115,21 +115,21 @@
popUp.showPopUp();
assert_true(isElementVisible(popUp));
assert_equals(popUp.getAnimations({subtree: true}).length,0);
popUp.addEventListener('hide', () => {
popUp.addEventListener('popuphide', () => {
popUp.animate([{opacity: 1},{opacity: 0}],1000000);
});
assert_equals(popUp.getAnimations({subtree: true}).length,0,'There should be no animations yet');
dialog.showModal(); // Force hide the popup
await waitForRender();
assert_equals(popUp.getAnimations({subtree: true}).length,1,'the hide animation should now be running');
assert_false(isElementVisible(popUp),'But the animation should *not* keep the popup visible in this case');
},'It should *not* be possible to use the "hide" event handler to animate the hide, if the hide is due to dialog.showModal');
},'It should *not* be possible to use the "popuphide" event handler to animate the hide, if the hide is due to dialog.showModal');

promise_test(async (t) => {
const {popUp, descendent} = createPopUp(t,'');
popUp.showPopUp();
assert_true(isElementVisible(popUp));
popUp.addEventListener('hide', (e) => {
popUp.addEventListener('popuphide', (e) => {
e.preventDefault();
});
popUp.hidePopUp();
Expand Down
77 changes: 46 additions & 31 deletions html/semantics/popups/popup-events.tentative.html
Original file line number Diff line number Diff line change
Expand Up @@ -11,53 +11,68 @@

<script>
window.onload = () => {
promise_test(async t => {
const popup = document.querySelector('[popup]');
let showCount = 0;
let hideCount = 0;
assert_false(popup.matches(':open'));
const controller = new AbortController();
const signal = controller.signal;
t.add_cleanup(() => controller.abort());
document.addEventListener('show',() => ++showCount, {signal});
document.addEventListener('hide',() => ++hideCount, {signal});
assert_equals(0,showCount);
assert_equals(0,hideCount);
popup.showPopUp();
assert_true(popup.matches(':open'));
assert_equals(1,showCount);
assert_equals(0,hideCount);
await waitForRender();
assert_true(popup.matches(':open'));
popup.hidePopUp();
assert_false(popup.matches(':open'));
assert_equals(1,showCount);
assert_equals(1,hideCount);
await waitForRender();
// No additional events after animation frame
assert_false(popup.matches(':open'));
assert_equals(1,showCount);
assert_equals(1,hideCount);
}, 'Show and hide events get properly dispatched for popups');
for(const method of ["listener","attribute"]) {
promise_test(async t => {
const popup = document.querySelector('[popup]');
assert_false(popup.matches(':open'));
let showCount = 0;
let hideCount = 0;
switch (method) {
case "listener":
const controller = new AbortController();
const signal = controller.signal;
t.add_cleanup(() => controller.abort());
document.addEventListener('popupshow',() => ++showCount, {signal});
document.addEventListener('popuphide',() => ++hideCount, {signal});
break;
case "attribute":
assert_false(popup.hasAttribute('onpopupshow'));
assert_false(popup.hasAttribute('onpopuphide'));
t.add_cleanup(() => popup.removeAttribute('onpopupshow'));
t.add_cleanup(() => popup.removeAttribute('onpopuphide'));
popup.onpopupshow = () => ++showCount;
popup.onpopuphide = () => ++hideCount;
break;
default: assert_unreached();
}
assert_equals(0,showCount);
assert_equals(0,hideCount);
popup.showPopUp();
assert_true(popup.matches(':open'));
assert_equals(1,showCount);
assert_equals(0,hideCount);
await waitForRender();
assert_true(popup.matches(':open'));
popup.hidePopUp();
assert_false(popup.matches(':open'));
assert_equals(1,showCount);
assert_equals(1,hideCount);
await waitForRender();
// No additional events after animation frame
assert_false(popup.matches(':open'));
assert_equals(1,showCount);
assert_equals(1,hideCount);
}, `Popupshow and popuphide events (${method}) get properly dispatched for popups`);
}

promise_test(async t => {
const popUp = document.querySelector('[popup]');
const controller = new AbortController();
const signal = controller.signal;
t.add_cleanup(() => controller.abort());
let cancel = true;
popUp.addEventListener('show',(e) => {
popUp.addEventListener('popupshow',(e) => {
if (cancel)
e.preventDefault();
}, {signal});
assert_false(popUp.matches(':open'));
popUp.showPopUp();
assert_false(popUp.matches(':open'),'The "show" event should be cancelable');
assert_false(popUp.matches(':open'),'The "popupshow" event should be cancelable');
cancel = false;
popUp.showPopUp();
assert_true(popUp.matches(':open'));
popUp.hidePopUp();
assert_false(popUp.matches(':open'));
}, 'Show event is cancelable');
}, 'Popupshow event is cancelable');
};
</script>
4 changes: 2 additions & 2 deletions html/semantics/popups/popup-invoking-attribute.tentative.html
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,8 @@
const button = document.querySelector('button');
let showCount = 0;
let hideCount = 0;
popUp.addEventListener('show',() => ++showCount);
popUp.addEventListener('hide',() => ++hideCount);
popUp.addEventListener('popupshow',() => ++showCount);
popUp.addEventListener('popuphide',() => ++hideCount);

async function assertState(expectOpen,expectShow,expectHide) {
assert_equals(popUp.matches(':open'),expectOpen,'Popup open state is incorrect');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
}
async_test(t => {
for(let popup of popups) {
popup.addEventListener('hide',e => {
popup.addEventListener('popuphide',e => {
assert_unreached('Scrolling should not light-dismiss a popup');
});
}
Expand Down
8 changes: 4 additions & 4 deletions html/semantics/popups/popup-light-dismiss.tentative.html
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,14 @@
const afterp1 = document.querySelector('#after_p1');

let popup1HideCount = 0;
popup1.addEventListener('hide',(e) => {
popup1.addEventListener('popuphide',(e) => {
++popup1HideCount;
e.preventDefault(); // 'hide' should not be cancellable.
e.preventDefault(); // 'popuphide' should not be cancellable.
});
let popup2HideCount = 0;
popup2.addEventListener('hide',(e) => {
popup2.addEventListener('popuphide',(e) => {
++popup2HideCount;
e.preventDefault(); // 'hide' should not be cancellable.
e.preventDefault(); // 'popuphide' should not be cancellable.
});
promise_test(async () => {
assert_false(popup1.matches(':open'));
Expand Down
2 changes: 1 addition & 1 deletion html/semantics/popups/popup-show-event.tentative.html
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
requestAnimationFrame(() => {++frameCount;});
const popUp = document.querySelector('[popup]');
const testText = 'Show Event Occurred';
popUp.addEventListener('show',() => {
popUp.addEventListener('popupshow',() => {
popUp.textContent = testText;
})
popUp.offsetHeight;
Expand Down

0 comments on commit 0e16224

Please sign in to comment.