Don't always consume user activation for close watchers #10048
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm implementing this in WebKit in a personal capacity there's no official standards position from WebKit yet. |
|
Fwiw this change makes sense to me |
|
My understanding is that this is bug fix to make CloseWatcher work as intended. OK for Mozilla. |
josepharhar
approved these changes
Jan 17, 2024
chromium-wpt-export-bot
pushed a commit
to web-platform-tests/wpt
that referenced
this pull request
Jan 17, 2024
This patch tweaks the CloseWatcher user activation checks so that opening a dialog with user activation does not consume the user activation to create the CloseWatcher. That way, when the user opens a dialog element with user activation and then presses escape before interacting with the page again, the cancel event still gets fired. This is done in order to address the attached regression bug. This patch also re-enables CloseWatcher now that the regression bug has been addressed. HTML spec PR: whatwg/html#10048 Bug: 1512224 Change-Id: I6f39d538090a832130c40eb30f06041c7a31b741
chromium-wpt-export-bot
pushed a commit
to web-platform-tests/wpt
that referenced
this pull request
Jan 23, 2024
This patch tweaks the CloseWatcher user activation checks so that opening a dialog with user activation does not consume the user activation to create the CloseWatcher. That way, when the user opens a dialog element with user activation and then presses escape before interacting with the page again, the cancel event still gets fired. This is done in order to address the attached regression bug. This patch also re-enables CloseWatcher now that the regression bug has been addressed. HTML spec PR: whatwg/html#10048 Bug: 1512224 Change-Id: I6f39d538090a832130c40eb30f06041c7a31b741
domenic
added
the
do not merge yet
|
Implementing this in Chromium has uncovered some significant bugs in the anti-abuse logic of close watchers. The new proposed spec allows infinite back-trapping, and the existing spec didn't have enough test coverage. A more significant overhaul will be needed. I think I have something almost working... |
|
@domenic is there a PR for your overhaul so I can take a look through it? Just wondering how much I'll need to change the draft WebKit implementation |
|
I have a work in progress Chromium CL. This version is passing all the tests but is ugly bad code; this version is my attempt at refactoring but it's now failing some of the tests, so we shouldn't trust it yet. But, it can give you an idea of what I expect the changes to look like:
|
domenic
added a commit
that referenced
this pull request
Feb 28, 2024
As noted in #10046, the close watcher anti-abuse measures were overly strict, and would prevent firing the cancel event even when the close watcher was created with user interaction. Additionally, the "is grouped with previous" infrastructure used to track close watcher groups was buggy, since close watchers could be destroyed and this would cause groups to spuriously collapse. To fix both of these problems, we re-do the close watcher anti-abuse protections. We now track the groups as a list-of-lists, and explicitly track how many groups should be allowed at a given time. We can then compare them when making decisions about whether to group a new close watcher, or whether to fire a cancel event. Note that the initial fix proposed in #10046 (and implemented in #10048) is incorrect, as it allows indefinite trapping of the user. Closes #10046.
5 tasks
|
Superseded by #10168; any review over there is appreciated! |
domenic
added a commit
that referenced
this pull request
Mar 14, 2024
As noted in #10046, the close watcher anti-abuse measures were overly strict, and would prevent firing the cancel event even when the close watcher was created with user interaction. Additionally, the "is grouped with previous" infrastructure used to track close watcher groups was buggy, since close watchers could be destroyed and this would cause groups to spuriously collapse. To fix both of these problems, we re-do the close watcher anti-abuse protections. We now track the groups as a list-of-lists, and explicitly track how many groups should be allowed at a given time. We can then compare them when making decisions about whether to group a new close watcher, or whether to fire a cancel event. Note that the initial fix proposed in #10046 (and implemented in #10048) is incorrect, as it allows indefinite trapping of the user. Closes #10046.
rubberyuzu
pushed a commit
to rubberyuzu/html
that referenced
this pull request
Mar 21, 2024
As noted in whatwg#10046, the close watcher anti-abuse measures were overly strict, and would prevent firing the cancel event even when the close watcher was created with user interaction. Additionally, the "is grouped with previous" infrastructure used to track close watcher groups was buggy, since close watchers could be destroyed and this would cause groups to spuriously collapse. To fix both of these problems, we re-do the close watcher anti-abuse protections. We now track the groups as a list-of-lists, and explicitly track how many groups should be allowed at a given time. We can then compare them when making decisions about whether to group a new close watcher, or whether to fire a cancel event. Note that the initial fix proposed in whatwg#10046 (and implemented in whatwg#10048) is incorrect, as it allows indefinite trapping of the user. Closes whatwg#10046.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #10046.
(See WHATWG Working Mode: Changes for more details.)
/interaction.html ( diff )