Hide all popovers when beforetoggle shows a popover #10015
Conversation
From @mfreed7's notes in http://crrev.com/c/5141430 It was previously possible to hit some underspecified behavior with something like this: ```html <div popover id=p1>Popover 1 <div popover id=p2>Popover 2</div> </div> <script> p1.showPopover(); p1.addEventListener('beforetoggle',() => p2.showPopover()); p1.hidePopover(); </script> ``` The problem is that "hide all popovers until" doesn't end up with the desired "until" popover on the top of the stack in this case. There is already a similar situation within the "hide all..." algorithm itself, but that only handles the case where a popover being hidden by "hide all..." has the beforetoggle listener. This is the same problem, but for the case that the "until" popover has that listener.
|
I'm not sure I understand what bug this is fixing, in all browsers the above testcase ends up with both popovers hidden, which seems like what we want? Can you describe more in detail what the expected / actual result should be in this case? |
|
It looks like safari and firefox don't pass the newly added wpt yet: https://wpt.fyi/results/html/semantics/popovers/popover-open-in-beforetoggle.html |
So in this test case: <div popover id=p1>Popover 1
<div popover id=p2>Popover 2</div>
</div>
<script>
p1.showPopover();
p1.addEventListener('beforetoggle',() => p2.showPopover());
p1.hidePopover();
</script>the issue is that hiding p1 (last line of script) triggers There's already a very similar bit of the algorithm for "hide all popovers until" at step 6. Step 6.6 detects the newly opened popover and sets a flag to close everything and stop firing events in that case. This PR is the equivalent thing for hidePopover(). |
|
If this is the only edge case this changes that seems fine. @annevk Can you think of any unintended consequences of this PR? |
| @@ -84360,6 +84360,9 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> { | |||
| </ol> | |||
| </li> | |||
|
|
|||
| <li><p>Let <var>autoPopoverListContainsThis</var> be true if <var>document</var>'s <span>showing | |||
There was a problem hiding this comment.
Nit: it seems weird for this to end in "This" when there's no this in this algorithm.
There was a problem hiding this comment.
I changed it from "this" to "element"
|
OK, merged! Please remember to file bugs on Gecko and WebKit and update the OP with them. |
…emilio whatwg/html#10015 Differential Revision: https://phabricator.services.mozilla.com/D201772 UltraBlame original commit: 67f44aef7e5026fd3358bdc67a4ad3f302839d34
From @mfreed7's notes in http://crrev.com/c/5141430
It was previously possible to hit some underspecified behavior with something like this:
The problem is that "hide all popovers until" doesn't end up with the desired "until" popover on the top of the stack in this case. There is already a similar situation within the "hide all..." algorithm itself, but that only handles the case where a popover being hidden by "hide all..." has the beforetoggle listener. This is the same problem, but for the case that the "until" popover has that listener.
(See WHATWG Working Mode: Changes for more details.)
/popover.html ( diff )