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

close() should not trigger cancel, at least by default #28

Closed
domenic opened this issue Jul 21, 2023 · 5 comments
Closed

close() should not trigger cancel, at least by default #28

domenic opened this issue Jul 21, 2023 · 5 comments

Comments

@domenic
Copy link
Collaborator

domenic commented Jul 21, 2023

@josepharhar noticed that for <dialog>, dialogEl.close() does not trigger the cancel event. Only a proper user-driven close signal will trigger a cancel event.

I tend to think the CloseWatcher design is more useful for developers: if you have code like

dialog.oncancel = e => {
  if (userHasUnsavedData) {
    e.preventDefault();
  }
};

dialog.querySelector(".close-button").onclick = () => dialog.close();

you probably would be surprised that clicking .close-button skips your unsaved-data check in the cancel event, and instead goes straight to the close event. You're forced to duplicate the unsaved-data check inside the onclick handler, which is silly.

However, symmetry between <dialog> and CloseWatcher is very important, to avoid web developer confusion. That's why we're using the close/cancel event names in the first place.

My proposal is that we provide both behaviors:

  • closeWatcher.close() behaves like dialog.close(), and goes straight to the close event.

  • closeWatcher.bikeshed() behaves as if a close signal has been sent, doing both cancel + close.

Naming for the cancel + close variant remains the tricky problem. I like signalClose() (meaning "send the close signal as if the user did"), but per #13 @annevk and @smaug---- did not. Some options:

  • cancelAndClose()
  • close({ withCancel: true })
  • Rename the whole "close signals" concept to something like "close command" or "close triggers", and then go with closeCommand() or triggerClose() or something like that.
  • Come up with some verb that is meant to indicate "the whole process of closing, which might possibly be canceled": e.g. exit(), shutdown().

The first two options seem nice and straightforward, but they have the minor drawback of making the cancel-and-close version more verbose, and demoting it to feeling second-class. (Whereas, per my above reasoning, I think it's actually the more likely behavior.) The latter two options are trickier but maybe worthwhile?

We can also consider porting whatever we come up with here to <dialog>.

@domenic domenic changed the title close() should maybe not trigger cancel, at least by default close() should not trigger cancel, at least by default Jul 21, 2023
@josepharhar
Copy link
Contributor

The first two options seem nice and straightforward, but they have the minor drawback of making the cancel-and-close version more verbose, and demoting it to feeling second-class

What about just calling it "cancel()"? Just a thought, I don't have strong opinions on this

@domenic
Copy link
Collaborator Author

domenic commented Jul 21, 2023

That's... pretty great! I'm promoting that to front-runner, pending any further opinions.

@tbondwilkinson
Copy link

cancel does really feel like a winner :)

Otherwise yeah I think focusing on the distinction between a UI close and a close action would be useful. requestClose would be my suggestion, but shrug

@domenic
Copy link
Collaborator Author

domenic commented Jul 27, 2023

I'm deciding on requestClose(), after writing some example code and realizing that stuff like

myPicker.querySelector(".close-button").onclick = () => watcher.requestClose();

seems more readable than

myPicker.querySelector(".close-button").onclick = () => watcher.cancel();

@domenic
Copy link
Collaborator Author

domenic commented Jul 27, 2023

Fixed this in the spec PR and in this repo's explainer. I also did a general update from "close signals" to "close requests".

Still TODO, updating Chromium.

@domenic domenic closed this as completed Jul 27, 2023
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 27, 2023
Or rather, rename close() to requestClose(), and then add close().
Follows whatwg/html@63fb283,
per WICG/close-watcher#28.

Bug: 1171318
Change-Id: Ic1c6dff2440f38c04f7ffaaa19188403a46b9adc
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 27, 2023
Or rather, rename close() to requestClose(), and then add close().
Follows whatwg/html@63fb283,
per WICG/close-watcher#28.

Bug: 1171318
Change-Id: Ic1c6dff2440f38c04f7ffaaa19188403a46b9adc
aarongable pushed a commit to chromium/chromium that referenced this issue Jul 28, 2023
Or rather, rename close() to requestClose(), and then add close().
Follows whatwg/html@63fb283,
per WICG/close-watcher#28.

Bug: 1171318
Change-Id: Ic1c6dff2440f38c04f7ffaaa19188403a46b9adc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4721666
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1176384}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 28, 2023
Or rather, rename close() to requestClose(), and then add close().
Follows whatwg/html@63fb283,
per WICG/close-watcher#28.

Bug: 1171318
Change-Id: Ic1c6dff2440f38c04f7ffaaa19188403a46b9adc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4721666
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1176384}
jonathan-j-lee pushed a commit to web-platform-tests/wpt that referenced this issue Jul 28, 2023
Or rather, rename close() to requestClose(), and then add close().
Follows whatwg/html@63fb283,
per WICG/close-watcher#28.

Bug: 1171318
Change-Id: Ic1c6dff2440f38c04f7ffaaa19188403a46b9adc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4721666
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1176384}

Co-authored-by: Domenic Denicola <domenic@chromium.org>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 2, 2023
…only

Automatic update from web-platform-tests
CloseWatcher: add requestClose() (#41201)

Or rather, rename close() to requestClose(), and then add close().
Follows whatwg/html@63fb283,
per WICG/close-watcher#28.

Bug: 1171318
Change-Id: Ic1c6dff2440f38c04f7ffaaa19188403a46b9adc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4721666
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1176384}

Co-authored-by: Domenic Denicola <domenic@chromium.org>
--

wpt-commits: d8c9a694e850e1715d83802f1b2adb0c2f8d41b7
wpt-pr: 41201
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Aug 4, 2023
…only

Automatic update from web-platform-tests
CloseWatcher: add requestClose() (#41201)

Or rather, rename close() to requestClose(), and then add close().
Follows whatwg/html@63fb283,
per WICG/close-watcher#28.

Bug: 1171318
Change-Id: Ic1c6dff2440f38c04f7ffaaa19188403a46b9adc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4721666
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1176384}

Co-authored-by: Domenic Denicola <domenic@chromium.org>
--

wpt-commits: d8c9a694e850e1715d83802f1b2adb0c2f8d41b7
wpt-pr: 41201
domenic added a commit to whatwg/html that referenced this issue Aug 10, 2023
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.
domenic added a commit to whatwg/html that referenced this issue Oct 18, 2023
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 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".
* Changing the "close request steps" to all be in a single queued task.

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. Closes #5667.
Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this issue Dec 11, 2023
Or rather, rename close() to requestClose(), and then add close().
Follows whatwg/html@63fb283,
per WICG/close-watcher#28.

Bug: 1171318
Change-Id: Ic1c6dff2440f38c04f7ffaaa19188403a46b9adc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4721666
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1176384}

Co-authored-by: Domenic Denicola <domenic@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants