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

Always fire CloseWatcher/dialog cancel events #10291

Merged
merged 1 commit into from
May 10, 2024
Merged

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 22, 2024

Previously, the anti-abuse mechanisms would sometimes skip these events. However, as discussed in #10047, it is more useful to fire them, and have the anti-abuse mechanisms set cancelable to false for them when appropriate. This allows observing close requests vs. other types of closes, by looking for the cancel event.

Closes #10047.

(See WHATWG Working Mode: Changes for more details.)


/interaction.html ( diff )
/interactive-elements.html ( diff )

Previously, the anti-abuse mechanisms would sometimes skip these events. However, as discussed in #10047, it is more useful to fire them, and have the anti-abuse mechanisms set cancelable to false for them when appropriate. This allows observing close requests vs. other types of closes, by looking for the cancel event.

Closes #10047.
@lukewarlow
Copy link
Member

If we ever added #10164 then this mechanism wouldn't be enough to actually differentiate close watchers Vs others?

Having said that I think the idea of sending the events but uncancelable is probably fine for now. My only reservation is that it's likely lots of dialog cancel event handlers won't actually be checking the cancelable value and I wonder if that could lead to some issues? prevent default would be a no-op which is fine, but could they be running a side effect they only want to run if the dialog doesn't close?

@domenic
Copy link
Member Author

domenic commented Apr 22, 2024

If we ever added #10164 then this mechanism wouldn't be enough to actually differentiate close watchers [presumably: requests] Vs others?

Well, it would become a mechanism for differentiating close requests vs. other closes, but those close requests could come from either web developer code or users. If developers want a further distinguisher for user-initiated, we would add a property to the event I think.

My only reservation is that it's likely lots of dialog cancel event handlers won't actually be checking the cancelable value and I wonder if that could lead to some issues? prevent default would be a no-op which is fine, but could they be running a side effect they only want to run if the dialog doesn't close?

I agree this is possible. I haven't seen it in any of the cases people reported, but I guess it's hard to tell...

@keithamus
Copy link
Contributor

I can't speak for Mozilla, but the implementation I'm writing in Gecko attaches event listeners to the close watcher to propagate the events for dialog & popover. In other words from the perspective of this implementation always dispatching the event is a big improvement and will make the implementation simpler.

With my developer hat on, always dispatching the event also massively simplifies a lot of complexity in manually wiring up close watchers. It's much harder to detect the absence of an event!

The biggest concern I had was making dialog cancels sometimes non-cancellable may result in lost work from the user if the dialog is forced to be cancelled; but scrutinizing the tests, it seems well covered that the only case here is if multiple dialogs/popovers were opened without explicit user gesture so it seems preferable for those to be unconditionally closed.

So 👍 all round from me.

Copy link
Member

@lukewarlow lukewarlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, can't forsee this being too difficult to change in my WebKit implementation.

Also thank you for the very extensive tests. Once this last patch is merged into HTML and WPT I should be able to finish off my the WebKit implementation with much more trust I've matched what's expected.

Given cancellable events are already backward compat with non-cancelable (aka preventDefault never throws) I think this change should be overall fine for web compat.

@emilio
Copy link
Contributor

emilio commented May 9, 2024

Yeah, seems reasonable to do this after looking a bit more into it. Still slightly weird but :)

@domenic domenic merged commit f239744 into main May 10, 2024
2 checks passed
@domenic domenic deleted the closewatcher-always-cancel branch May 10, 2024 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Close watchers: detecting close requests vs. other closes
5 participants