-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add close requests and close watchers #9462
Conversation
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.
The examples and notes are illuminating, thanks.
Reviewed https://whatpr.org/html/9462/c3ca6c2...01ef747/popover.html and those changes look correct. |
In general the changes seem to be a strong improvement towards cross-platform compatibility. |
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.
Concerning tests for the popover attribute: there's already https://searchfox.org/mozilla-central/rev/7907fd85f6fd1cee3acaddbaf27a76b23d6d9993/testing/web-platform/tests/html/semantics/popovers/popover-change-type.html.
However, it's still concerning that the behavior is only specified for the Escape key, and it's implementation defined for other events. Consequently, WPT-coverage will have gaps on e.g. mobile platforms. That's problematic and I don't have solution for it. If that problem has been considered, thoughts about it would be appreciated.
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.
Thanks for writing all this!
I created a chromium patch to implement the missing popover integration: https://chromium-review.googlesource.com/c/chromium/src/+/4686066
source
Outdated
|
||
<ol> | ||
<li><p><span data-x="close-watcher-destroy">Destroy</span> <var>element</var>'s <span>popover | ||
close watcher</span>.</p></li> |
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.
Instead of putting this in cleanupSteps/cleanupHidingFlag, can't we just destroy the closewatcher at the very end of the hide popover algorithm?
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.
Then the close watcher won't be destroyed in the invalid cases, right? Hmm. I think that would be wrong. For example, if "hide all popovers until" or firing beforetoggle
caused the popover to get disconnected from the document, or become a modal dialog, then we should probably destroy the popover close watcher. Right?
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 think that the only way that beforetoggle can make hide popover abort is if beforetoggle caused a nested call to hide popover.
If it gets disconnected, then hide popover will get called again and successfully hide the popover and reach the end of the algorithm, where we would destroy the closewatcher.
beforetoggle and other stuff can't turn it into a modal dialog, because the show modal steps will throw an exception if it is an open popover.
Can you think of any other cases? If so I can write a test for it
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 can't think of any other cases. But now I'm confused about what the point of cleanupSteps is. Why can't we move "If nestedHide is false, then set element's popover showing or hiding to false." down to the bottom as well?
In other words, I'm confused why the logic for setting that flag to false, doesn't apply to close watcher destruction. What is different about those two? (Almost certainly there is something different and I just am not familiar enough with popover to understand.)
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.
In the case that beforetoggle causes hide popover to abort by causing a nested call to hide popover, we will reach the end of hide popover for this popover once. However, each of the two hide popover calls will have a different value for nestedHide that they each want to set when they return from the algorithm. We need the first, not nested call to set popover or showing or hiding to false whenever it returns - I suppose that the nested call could also set it to false and that might be OK.
This nestedHide/cleanupSteps/popover showing or hiding machinery was my translation of a stack-scoped variable that was implemented in chromium, and it certainly looks very clean in C++. I suppose that there may be a simpler way to write this in spec code.
In the meantime, I guess its fine to put it in either cleanupSteps or the end of hide popover.
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.
Thanks so much for the review and help Joey!!
source
Outdated
|
||
<ol> | ||
<li><p><span data-x="close-watcher-destroy">Destroy</span> <var>element</var>'s <span>popover | ||
close watcher</span>.</p></li> |
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.
Then the close watcher won't be destroyed in the invalid cases, right? Hmm. I think that would be wrong. For example, if "hide all popovers until" or firing beforetoggle
caused the popover to get disconnected from the document, or become a modal dialog, then we should probably destroy the popover close watcher. Right?
This HTML spec PR contains an assert about the document being active when a closewatcher is created, but I couldn't find a matching check so this patch creates one: whatwg/html#9462 Bug: 1171318 Change-Id: If52f8a5a3e8d522cab907ad5de05521b8d003d4f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4683914 Reviewed-by: Nate Chapin <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1171493}
This is being integrated in this HTML spec PR which adds CloseWatchers: whatwg/html#9462 CloseWatcher is not enabled right now, so this patch should not make any behavior changes to stable chrome. This patch makes it so that pressing the escape key uses the document/window's closewatcher stack to find a popover to light dismiss rather than HandlePopoverLightDismiss hiding whatever the TopmostPopoverOrHint is. Hopefully they agree with each other. The behavior change for popovers is that the android back button can close popovers, and this gives popovers the grouping-without-user-activation behavior that dialogs have (with the flag enabled). If you open 5 popovers with a single user activation, then any close signal (including Esc on desktop) will close all 5 of those popovers, not just the topmost one. Bug: 1171318 Change-Id: I73740c3eb9c16b0883862ae259be4867b824a79c
This is being integrated in this HTML spec PR which adds CloseWatchers: whatwg/html#9462 CloseWatcher is not enabled right now, so this patch should not make any behavior changes to stable chrome. This patch makes it so that pressing the escape key uses the document/window's closewatcher stack to find a popover to light dismiss rather than HandlePopoverLightDismiss hiding whatever the TopmostPopoverOrHint is. Hopefully they agree with each other. The behavior change for popovers is that the android back button can close popovers, and this gives popovers the grouping-without-user-activation behavior that dialogs have (with the flag enabled). If you open 5 popovers with a single user activation, then any close signal (including Esc on desktop) will close all 5 of those popovers, not just the topmost one. Bug: 1171318 Change-Id: I73740c3eb9c16b0883862ae259be4867b824a79c
This is being integrated in this HTML spec PR which adds CloseWatchers: whatwg/html#9462 CloseWatcher is not enabled right now, so this patch should not make any behavior changes to stable chrome. This patch makes it so that pressing the escape key uses the document/window's closewatcher stack to find a popover to light dismiss rather than HandlePopoverLightDismiss hiding whatever the TopmostPopoverOrHint is. Hopefully they agree with each other. The behavior change for popovers is that the android back button can close popovers, and this gives popovers the grouping-without-user-activation behavior that dialogs have (with the flag enabled). If you open 5 popovers with a single user activation, then any close signal (including Esc on desktop) will close all 5 of those popovers, not just the topmost one. Bug: 1171318 Change-Id: I73740c3eb9c16b0883862ae259be4867b824a79c
This is being integrated in this HTML spec PR which adds CloseWatchers: whatwg/html#9462 CloseWatcher is not enabled right now, so this patch should not make any behavior changes to stable chrome. This patch makes it so that pressing the escape key uses the document/window's closewatcher stack to find a popover to light dismiss rather than HandlePopoverLightDismiss hiding whatever the TopmostPopoverOrHint is. Hopefully they agree with each other. The behavior change for popovers is that the android back button can close popovers, and this gives popovers the grouping-without-user-activation behavior that dialogs have (with the flag enabled). If you open 5 popovers with a single user activation, then any close signal (including Esc on desktop) will close all 5 of those popovers, not just the topmost one. Bug: 1171318 Change-Id: I73740c3eb9c16b0883862ae259be4867b824a79c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4686066 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1174590}
This is being integrated in this HTML spec PR which adds CloseWatchers: whatwg/html#9462 CloseWatcher is not enabled right now, so this patch should not make any behavior changes to stable chrome. This patch makes it so that pressing the escape key uses the document/window's closewatcher stack to find a popover to light dismiss rather than HandlePopoverLightDismiss hiding whatever the TopmostPopoverOrHint is. Hopefully they agree with each other. The behavior change for popovers is that the android back button can close popovers, and this gives popovers the grouping-without-user-activation behavior that dialogs have (with the flag enabled). If you open 5 popovers with a single user activation, then any close signal (including Esc on desktop) will close all 5 of those popovers, not just the topmost one. Bug: 1171318 Change-Id: I73740c3eb9c16b0883862ae259be4867b824a79c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4686066 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1174590}
This is being integrated in this HTML spec PR which adds CloseWatchers: whatwg/html#9462 CloseWatcher is not enabled right now, so this patch should not make any behavior changes to stable chrome. This patch makes it so that pressing the escape key uses the document/window's closewatcher stack to find a popover to light dismiss rather than HandlePopoverLightDismiss hiding whatever the TopmostPopoverOrHint is. Hopefully they agree with each other. The behavior change for popovers is that the android back button can close popovers, and this gives popovers the grouping-without-user-activation behavior that dialogs have (with the flag enabled). If you open 5 popovers with a single user activation, then any close signal (including Esc on desktop) will close all 5 of those popovers, not just the topmost one. Bug: 1171318 Change-Id: I73740c3eb9c16b0883862ae259be4867b824a79c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4686066 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1174590}
…opovers, a=testonly Automatic update from web-platform-tests Implement CloseWatcher integration for popovers This is being integrated in this HTML spec PR which adds CloseWatchers: whatwg/html#9462 CloseWatcher is not enabled right now, so this patch should not make any behavior changes to stable chrome. This patch makes it so that pressing the escape key uses the document/window's closewatcher stack to find a popover to light dismiss rather than HandlePopoverLightDismiss hiding whatever the TopmostPopoverOrHint is. Hopefully they agree with each other. The behavior change for popovers is that the android back button can close popovers, and this gives popovers the grouping-without-user-activation behavior that dialogs have (with the flag enabled). If you open 5 popovers with a single user activation, then any close signal (including Esc on desktop) will close all 5 of those popovers, not just the topmost one. Bug: 1171318 Change-Id: I73740c3eb9c16b0883862ae259be4867b824a79c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4686066 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1174590} -- wpt-commits: 7f3c31af0515719d68659ae1bb87a46fc366a132 wpt-pr: 41080
…opovers, a=testonly Automatic update from web-platform-tests Implement CloseWatcher integration for popovers This is being integrated in this HTML spec PR which adds CloseWatchers: whatwg/html#9462 CloseWatcher is not enabled right now, so this patch should not make any behavior changes to stable chrome. This patch makes it so that pressing the escape key uses the document/window's closewatcher stack to find a popover to light dismiss rather than HandlePopoverLightDismiss hiding whatever the TopmostPopoverOrHint is. Hopefully they agree with each other. The behavior change for popovers is that the android back button can close popovers, and this gives popovers the grouping-without-user-activation behavior that dialogs have (with the flag enabled). If you open 5 popovers with a single user activation, then any close signal (including Esc on desktop) will close all 5 of those popovers, not just the topmost one. Bug: 1171318 Change-Id: I73740c3eb9c16b0883862ae259be4867b824a79c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4686066 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1174590} -- wpt-commits: 7f3c31af0515719d68659ae1bb87a46fc366a132 wpt-pr: 41080
This adds the generic close request concept, which allows interfaces like the Esc key, or back gestures and buttons, to be interpreted as closing various "close watchers". Two existing constructs are converted to close watchers: modal dialog elements, and auto popovers. Additionally, this adds the CloseWatcher API, which lets web developers create their own close watchers. Note that the close request steps formalize previously-informal parts of the Fullscreen API Standard. The specification text here is largely ported from https://github.com/WICG/close-watcher/blob/3a18e6811528d349df27a3e7b06b8dc018638c4c/spec.bs. Updates include: * Requiring requiring keyup to be the close request event for user agents which use the Esc key as a close request, per discussions in #9143. * Introducing requestClose(), per WICG/close-watcher#28. * Renaming the "close signal" concept to "close request". This change to dialog behavior is a potential compatibility risk, especially since it can cause the cancel event to be skipped if there has been no user activation since the last time it was canceled, or multiple dialogs to be closed at once. However, Chromium data shows these risks to be negligible: * https://chromestatus.com/metrics/feature/timeline/popularity/4422 shows 0.000015% of pages impacted by skipped cancel events * https://chromestatus.com/metrics/feature/timeline/popularity/4423 shows 0.000007% of pages impacted by skipped cancel events that would otherwise call preventDefault() * https://chromestatus.com/metrics/feature/timeline/popularity/4424 shows between 0.000000% and 0.000001% of pages impacted by multiple dialogs closed Closes #9143.
<p>This means that calling <code data-x="dom-CloseWatcher-destroy">destroy()</code>, <code | ||
data-x="dom-CloseWatcher-close">close()</code>, or <code | ||
data-x="dom-CloseWatcher-requestClose">requestClose()</code> properly is important. Doing so is | ||
the only way to get back the "free" ungrouped close watcher slot. Such close watchers created |
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.
If getting the free slot is important, a way to inspect whether one will get it or did get it could be useful. It could be a boolean getter that depends on "was created with user activation" and "is grouped with previous", or it could be by exposing the close watcher stack, and checking if its size is 1. I don't suggest any of the changes now, just making an observation.
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 agree, it seems like the kind of thing that could in theory be useful. However, I was unable to figure out what kind of application code might use it, and I didn't hear any requests from developers yet. So, I've punted this sort of introspection stuff.
This comment was marked as outdated.
This comment was marked as outdated.
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.
The "everything in a task" rewrite is a big improvement, thanks @domenic!
This is being integrated in this HTML spec PR which adds CloseWatchers: whatwg/html#9462 CloseWatcher is not enabled right now, so this patch should not make any behavior changes to stable chrome. This patch makes it so that pressing the escape key uses the document/window's closewatcher stack to find a popover to light dismiss rather than HandlePopoverLightDismiss hiding whatever the TopmostPopoverOrHint is. Hopefully they agree with each other. The behavior change for popovers is that the android back button can close popovers, and this gives popovers the grouping-without-user-activation behavior that dialogs have (with the flag enabled). If you open 5 popovers with a single user activation, then any close signal (including Esc on desktop) will close all 5 of those popovers, not just the topmost one. Bug: 1171318 Change-Id: I73740c3eb9c16b0883862ae259be4867b824a79c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4686066 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1174590}
BREAKING-CHANGE: Modal's and ConfirmModal's `onRequestClose` is now mandatory. Note that a Modal has to always be closed (`isOpen` must be set to `false`) when `onRequestClose` is called. This is driven by a change in the HTML spec that ensures a web page cannot indefinitely keep a dialog open. See whatwg/html#9462 and https://bugs.chromium.org/p/chromium/issues/detail?id=1511166#c1
BREAKING-CHANGE: Modal's and ConfirmModal's `onRequestClose` is now mandatory. Note that a Modal has to always be closed (`isOpen` must be set to `false`) when `onRequestClose` is called. This is driven by a change in the HTML spec that ensures a web page cannot indefinitely keep a dialog open. See whatwg/html#9462 and https://bugs.chromium.org/p/chromium/issues/detail?id=1511166#c1
This adds the generic close request concept, which allows interfaces like the Esc key, or back gestures and buttons, to be interpreted as closing various "close watchers". Two existing constructs are converted to close watchers: modal dialog elements, and auto popovers. Additionally, this adds the CloseWatcher API, which lets web developers create their own close watchers. Note that the close request steps formalize previously-informal parts of the Fullscreen API Standard.
The specification text here is largely ported from https://github.com/WICG/close-watcher/blob/3a18e6811528d349df27a3e7b06b8dc018638c4c/spec.bs. Updates include:
This change to dialog behavior is a potential compatibility risk, especially since it can cause the cancel event to be skipped if there has been no user activation since the last time it was canceled, or multiple dialogs to be closed at once. However, Chromium data shows these risks to be negligible:
Closes #9143. Closes #5667.
(See WHATWG Working Mode: Changes for more details.)
/acknowledgements.html ( diff )
/browsers.html ( diff )
/dnd.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/interaction.html ( diff )
/interactive-elements.html ( diff )
/popover.html ( diff )