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.

Sign up for GitHub

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

Jump to bottom

Close watchers: detecting close requests vs. other closes #10047

Closed
domenic opened this issue Jan 11, 2024 · 4 comments · Fixed by #10291
Closed

Close watchers: detecting close requests vs. other closes #10047

domenic opened this issue Jan 11, 2024 · 4 comments · Fixed by #10291
Labels
normative change topic: close watchers topic: dialog The <dialog> element.

Comments

@domenic
Copy link
Member

domenic commented Jan 11, 2024

What is the issue with the HTML Standard?

In https://bugs.chromium.org/p/chromium/issues/detail?id=1512224, especially this comment, I was made aware that developers were using <dialog>'s cancel event as a way of detecting when a close request was issued vs. whether the dialog was closed in some other way.

This conflicts a bit with the changes introduced in #9462. There, the close watcher infrastructure only fires cancel if it's preventDefault()-able, so that web developers can call cancelEvent.preventDefault() and stop the dialog or close watcher from closing. When designing that infrastructure, I didn't realize that people might be using cancel for this sort of informational purpose; I thought they would only use it to intervene.

To accomplish this use case, of detecting close requests, two solutions come to mind:

  • Add a property to the close event, e.g. fromCloseRequest or userInitiated or something.
  • Fire cancel anyway, but make cancelEvent.cancelable false when the anti-abuse protections do not allow calling preventDefault(). This automatically makes it so that cancelEvent.preventDefault() does nothing.

For both options, we have the choice of whether to apply them just to <dialog>, or to apply them also to CloseWatcher.

I'm currently leaning toward the second option, as it does not require web developers to change their code. However, based on our measurements from shipping #9462, only 0.000015% of page views experienced a change in the number of cancel events fired, so it isn't required that we take that path. (And, after fixing #10046, that 0.000015% number will go even lower.)

My plan is to write up the spec changes for "fire cancel anyway", for both <dialog> and CloseWatcher, and see how it feels. If it's very ugly or there's some unintended consequence that I didn't understand, I'll revisit and consider "add a property to the close event".

/cc @josepharhar @lukewarlow
@domenic domenic added normative change topic: dialog The <dialog> element. labels Jan 11, 2024
@domenic
Copy link
Member Author

domenic commented Mar 4, 2024

Hmm, I forgot that my plan was to fire cancel anyway. I think we can add that on top of #10168 though.

@zcorpan
Copy link
Member

zcorpan commented Apr 2, 2024

Did #10168 fix this issue?

@domenic
Copy link
Member Author

domenic commented Apr 2, 2024

No, we still need to update the spec and tests to fire uncancelable cancel events.

@KoKuToru

This comment was marked as off-topic.

Sign in to view
domenic added a commit that referenced this issue Apr 22, 2024
@domenic
Always fire close watcher/dialog cancel events
fec01a1 
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.
domenic added a commit that referenced this issue Apr 22, 2024
@domenic
Always fire CloseWatcher/dialog cancel events
9eec117 
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.
@domenic domenic mentioned this issue Apr 22, 2024
Merged
5 tasks
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 22, 2024
@domenic @chromium-wpt-export-bot
Close watchers: always fire cancel events
fd1eb0c 
Sometimes they will have cancelable = false, but they will now always
fire. See whatwg/html#10047.

Bug: 41484805, 40054591
Change-Id: Ica682043fb56c729f4c331e9f3bd0590d3b1d088
Merged
aarongable pushed a commit to chromium/chromium that referenced this issue Apr 23, 2024
@domenic
Close watchers: always fire cancel events
ea9a37a 
Sometimes they will have cancelable = false, but they will now always
fire. See whatwg/html#10047.

Bug: 41484805, 40054591
Change-Id: Ica682043fb56c729f4c331e9f3bd0590d3b1d088
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5465306
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1291009}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 23, 2024
@domenic @chromium-wpt-export-bot
Close watchers: always fire cancel events
9c386ea 
Sometimes they will have cancelable = false, but they will now always
fire. See whatwg/html#10047.

Bug: 41484805, 40054591
Change-Id: Ica682043fb56c729f4c331e9f3bd0590d3b1d088
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5465306
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1291009}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 23, 2024
@domenic @chromium-wpt-export-bot
Close watchers: always fire cancel events
69f5a6f 
Sometimes they will have cancelable = false, but they will now always
fire. See whatwg/html#10047.

Bug: 41484805, 40054591
Change-Id: Ica682043fb56c729f4c331e9f3bd0590d3b1d088
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5465306
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1291009}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 25, 2024
@domenic @moz-wptsync-bot
Bug 1892677 [wpt PR 45818] - Close watchers: always fire cancel event…
96f23de 
…s, a=testonly

Automatic update from web-platform-tests
Close watchers: always fire cancel events

Sometimes they will have cancelable = false, but they will now always
fire. See whatwg/html#10047.

Bug: 41484805, 40054591
Change-Id: Ica682043fb56c729f4c331e9f3bd0590d3b1d088
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5465306
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1291009}

--

wpt-commits: 69f5a6f1e296d9b51552db976b2ffe37c72dd472
wpt-pr: 45818
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this issue Apr 30, 2024
@domenic @moz-wptsync-bot
Bug 1892677 [wpt PR 45818] - Close watchers: always fire cancel event…
2ba37a7 
…s, a=testonly

Automatic update from web-platform-tests
Close watchers: always fire cancel events

Sometimes they will have cancelable = false, but they will now always
fire. See whatwg/html#10047.

Bug: 41484805, 40054591
Change-Id: Ica682043fb56c729f4c331e9f3bd0590d3b1d088
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5465306
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1291009}

--

wpt-commits: 69f5a6f1e296d9b51552db976b2ffe37c72dd472
wpt-pr: 45818
domenic added a commit that referenced this issue May 10, 2024
@domenic
Always fire CloseWatcher/dialog cancel events
f239744 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
normative change topic: close watchers topic: dialog The <dialog> element.
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Always fire CloseWatcher/dialog cancel events whatwg/html
3 participants
@zcorpan @domenic @KoKuToru