Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combine the popoverhide/popovershow events into one beforetoggle event #37031

Merged
merged 1 commit into from
Dec 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,9 @@
assert_true(isElementVisible(popover));
assert_equals(popover.getAnimations({subtree: true}).length,0);
let animation;
popover.addEventListener('popoverhide', () => {
popover.addEventListener('beforetoggle', (e) => {
if (e.newState !== "closed")
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 +113,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 "beforetoggle" event handler to animate the hide');


promise_test(async (t) => {
Expand All @@ -121,8 +123,10 @@
popover.showPopover();
assert_true(isElementVisible(popover));
assert_equals(popover.getAnimations({subtree: true}).length,0);
let animation
popover.addEventListener('popoverhide', () => {
let animation;
popover.addEventListener('beforetoggle', (e) => {
if (e.newState !== "closed")
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 @@ -133,19 +137,17 @@
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, even when the hide is due to dialog.showModal');
},'It should be possible to use the "beforetoggle" event handler to animate the hide, even when 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('beforetoggle', (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 @@ -173,7 +175,9 @@
popover.showPopover();
assert_true(isElementVisible(popover));
assert_equals(popover.getAnimations({subtree: true}).length,0);
popover.addEventListener('popoverhide', () => {
popover.addEventListener('beforetoggle', (e) => {
if (e.newState !== "closed")
return;
popover.animate([{opacity: 1},{opacity: 0}],1000000);
});
assert_equals(popover.getAnimations({subtree: true}).length,0,'There should be no animations yet');
Expand All @@ -197,7 +201,9 @@
promise_test(async (t) => {
const {popover, descendent} = createPopover(t,'');
popover.showPopover();
popover.addEventListener('popoverhide', () => {
popover.addEventListener('beforetoggle', (e) => {
if (e.newState !== "closed")
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
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
requestAnimationFrame(() => {++frameCount;});
const popover = document.querySelector('[popover]');
const testText = 'Show Event Occurred';
popover.addEventListener('popovershow',() => {
popover.addEventListener('beforetoggle',(e) => {
if (e.newState !== "open")
return;
popover.textContent = testText;
})
popover.offsetHeight;
Expand All @@ -25,5 +27,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 `beforetoggle` event can be used to populate content before the popover renders');
</script>
45 changes: 24 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,32 @@
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.newState === "open") {
assert_equals(e.currentState,"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 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_equals(e.currentState,"open",'Popover toggleevent states should be "open" and "closed"')
assert_equals(e.newState,"closed",'Popover toggleevent states should be "open" and "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;
}
};
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});
// The 'beforetoggle' event bubbles.
document.addEventListener('beforetoggle', 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('onbeforetoggle'));
t.add_cleanup(() => popover.removeAttribute('onbeforetoggle'));
popover.onbeforetoggle = listener;
break;
default: assert_unreached();
}
Expand All @@ -62,7 +63,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 +72,20 @@
const signal = controller.signal;
t.add_cleanup(() => controller.abort());
let cancel = true;
popover.addEventListener('popovershow',(e) => {
popover.addEventListener('beforetoggle',(e) => {
if (e.newState !== "open")
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 "beforetoggle" 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('beforetoggle',(e) => {
if (e.newState === "open")
++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,9 @@
}
async_test(t => {
for(let popover of popovers) {
popover.addEventListener('popoverhide',e => {
popover.addEventListener('beforetoggle',e => {
if (e.newState !== "closed")
return;
assert_unreached('Scrolling should not light-dismiss a popover');
});
}
Expand Down
16 changes: 11 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,18 @@
const afterp1 = document.querySelector('#after_p1');

let popover1HideCount = 0;
popover1.addEventListener('popoverhide',(e) => {
popover1.addEventListener('beforetoggle',(e) => {
if (e.newState !== "closed")
return;
++popover1HideCount;
e.preventDefault(); // 'popoverhide' should not be cancellable.
e.preventDefault(); // 'beforetoggle' should not be cancellable.
});
let popover2HideCount = 0;
popover2.addEventListener('popoverhide',(e) => {
popover2.addEventListener('beforetoggle',(e) => {
if (e.newState !== "closed")
return;
++popover2HideCount;
e.preventDefault(); // 'popoverhide' should not be cancellable.
e.preventDefault(); // 'beforetoggle' should not be cancellable.
});
promise_test(async () => {
assert_false(popover1.matches(':open'));
Expand Down Expand Up @@ -482,7 +486,9 @@
p13.showPopover();
p14.showPopover();
p15.showPopover();
p15.addEventListener('popoverhide',() => {
p15.addEventListener('beforetoggle', (e) => {
if (e.newState !== "closed")
return;
p14.hidePopover();
},{once:true});
assert_true(p13.matches(':open') && p14.matches(':open') && p15.matches(':open'),'all three should be open');
Expand Down
Loading