-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Improve popover event handling in nested showing and hiding #9198
Conversation
Fixes whatwg#9197 Fixes whatwg#9196 But there was a case where the `beforetoggle` event could open a new popover while an old one was closing. This led to odd situations where asserts and infinite loops ensued. This PR breaks that loop, or rather converts it to an explicit one, by repeating the "hide all popovers until" algorithm until all popovers are closed. That admits an explicit loop, which is the popover equivalent of `while(true) {}`: when the `beforetoggle` handler for a popover re-shows the popover.
#9197 (comment) needs to be addressed first. |
beforetoggle
opens a new popover
@emilio or @cathiechen, can you please review this? Mainly interested in two things:
|
Yes
Instead of the |
The fix in Gecko, We have only made modifications to the loop in the Yet, I found another corner issue about nested popover, and I filed an issue in #9319 |
https://bugs.webkit.org/show_bug.cgi?id=256379 Reviewed by Tim Nguyen. Fix hideAllPopoversUntil for the case where hiding an auto popover changes the auto popover list (through beforetoggle) so the same auto popover is on top, causing a loop. To fix this, when this is detected, continue the loop without firing events. See: whatwg/html#9198 * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss.html: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt: * LayoutTests/platform/ios/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt: * Source/WebCore/dom/Document.cpp: (WebCore::Document::topmostAutoPopover const): (WebCore::Document::hideAllPopoversUntil): * Source/WebCore/dom/PopoverData.h: (WebCore::PopoverData::ScopedStartShowingOrHiding::ScopedStartShowingOrHiding): (WebCore::PopoverData::ScopedStartShowingOrHiding::~ScopedStartShowingOrHiding): (WebCore::PopoverData::ScopedStartShowingOrHiding::wasShowingOrHiding const): * Source/WebCore/html/HTMLElement.cpp: (WebCore::HTMLElement::showPopover): (WebCore::HTMLElement::hidePopoverInternal): Canonical link: https://commits.webkit.org/264623@main
Can you add "Fixes #9319" to the commit message? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some nits that would be nice to see fixed in a follow-up. As they already existed I'm not gonna hold this up over them as this PR is quite old now.
<li><p>If <var>foundEndpoint</var> is false, then run <var>closeAllOpenPopovers</var> and | ||
return.</p></li> | ||
<li> | ||
<p>For each <var>popover</var> in <var>document</var>'s <span>auto popover list</span>:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should link for each here.
<li> | ||
<p>While <var>lastToHide</var> is not null and <var>lastToHide</var>'s <span>popover | ||
visibility state</span> is <span data-x="popover-showing-state">showing</span> and | ||
<var>document</var>'s <span>auto popover list</span> is not empty:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should make is not empty an xref.
@cathiechen does this look good to you now as well? And Gecko bug we should link from OP? |
Yes, it looks good to me. And filed the Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1837360 |
https://bugs.webkit.org/show_bug.cgi?id=256379 Reviewed by Tim Nguyen. Fix hideAllPopoversUntil for the case where hiding an auto popover changes the auto popover list (through beforetoggle) so the same auto popover is on top, causing a loop. To fix this, when this is detected, continue the loop without firing events. See: whatwg/html#9198 * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss.html: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt: * LayoutTests/platform/ios/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt: * Source/WebCore/dom/Document.cpp: (WebCore::Document::topmostAutoPopover const): (WebCore::Document::hideAllPopoversUntil): * Source/WebCore/dom/PopoverData.h: (WebCore::PopoverData::ScopedStartShowingOrHiding::ScopedStartShowingOrHiding): (WebCore::PopoverData::ScopedStartShowingOrHiding::~ScopedStartShowingOrHiding): (WebCore::PopoverData::ScopedStartShowingOrHiding::wasShowingOrHiding const): * Source/WebCore/html/HTMLElement.cpp: (WebCore::HTMLElement::showPopover): (WebCore::HTMLElement::hidePopoverInternal): Canonical link: https://commits.webkit.org/264623@main
Fixes whatwg#9468 I mistakenly added and modified this check while speccing a chromium patch in this PR: whatwg#9198
I mistakenly added and modified this check in whatwg#9198. Fixes whatwg#9468.
Fixes #9196
Fixes #9197
Fixes #9319
This PR accounts for these 4 chromium patches whose commit messages have additional context:
beforetoggle
opens a new popover web-platform-tests/wpt#39647(See WHATWG Working Mode: Changes for more details.)
/popover.html ( diff )