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

Ensure dialog cancel events fired even without interaction #44301

Conversation

m-akinc
Copy link

@m-akinc m-akinc commented Jan 30, 2024

The cancel events should not be dependent on the user having first interacted with the dialog. Because this test includes user interaction before sending the close request, it failed to catch a regression in Chromium.

Removing the user interaction (i.e. call to blessTopLayer) so this test covers that scenario as well.

@domenic
Copy link
Member

domenic commented Jan 30, 2024

This is not correct. The spec requires a user interaction to either open the dialog, or to interact with the dialog while it's open, if cancel is going to be fired.

There is a slight improvement we can make to the spec, which is being discussed in whatwg/html#10046 . But the fix for that is proving pretty complicated, and will involve significant test changes beyond deleting one line. See https://chromium-review.googlesource.com/c/chromium/src/+/5232387 for my current work in progress there.

@domenic domenic closed this Jan 30, 2024
@m-akinc
Copy link
Author

m-akinc commented Jan 31, 2024

This is not correct. The spec requires a user interaction to either open the dialog, or to interact with the dialog while it's open, if cancel is going to be fired.

Ah, I hadn't realized that the new CloseWatcher stuff was already part of the official specification (submitted a few months ago). But your description in that PR acknowledges that this was a breaking change:

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.

I would just like to raise awareness that this has real consequences. Part of the justification for this change was that only 0.000015% of page loads would have been affected (possibly broken) by this, but what qualifies as an acceptable percentage? It's also worth noting that since you cited that number back in October, it has apparently grown by 37x:

image

@domenic

@domenic
Copy link
Member

domenic commented Feb 1, 2024

The growth is due to a bug where when we remove the use counter, the denominator trends toward 0 (depending on how fast old Chrome versions get out of circulation), so the values swing wildly. There are no valid data in the chart after November.

@m-akinc
Copy link
Author

m-akinc commented Feb 1, 2024

In retrospect, that makes a lot more sense than a steady, drastic increase in a that specific usage pattern. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants