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

Add requestClose() function to HTMLDialogElement #10164

Open
lukewarlow opened this issue Feb 26, 2024 · 7 comments
Open

Add requestClose() function to HTMLDialogElement #10164

lukewarlow opened this issue Feb 26, 2024 · 7 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: close watchers topic: dialog The <dialog> element

Comments

@lukewarlow
Copy link
Member

lukewarlow commented Feb 26, 2024

What problem are you trying to solve?

Dialog elements have an internal close watcher which can trigger cancel events to give web developers a chance to prevent the closure. Currently there's no way for developers to get access to this functionality via anything other than the internal close watcher.

Developers will often want to trigger this same behaviour if closed through other mechanisms such as custom light dismiss for dialogs or even cancel buttons themselves.

What solutions exist today?

The developer would have to implement the cancellation logic twice once for the cancel event handler and another time for their custom cancel behaviour.

How would you solve it?

Exposing a new requestClose() function on dialogs that that simplified would be firing the cancel event and then firing close event (and associated dialog closing steps) if cancel wasn't prevented.

Anything else?

@lukewarlow lukewarlow added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Feb 26, 2024
@lukewarlow
Copy link
Member Author

lukewarlow commented Feb 26, 2024

This has come up in a few conversations but primarily from a discussion about adding a 'cancel' (nameTBD) action to invokers for dialogs, and also the light dismiss for dialog discussions.

openui/open-ui#950 (reply in thread) - @domenic mentions that this is a useful functionality that we should probably expose.

@mfreed7
Copy link
Contributor

mfreed7 commented Nov 15, 2024

As I'm speccing and implementing light dismiss behaviors for <dialog> over on #10737, @dbaron's code review made me realize a potential issue/question:

I had been thinking this new requestClose() method on dialog would work for both modal and non-modal dialogs. And the natural way to implement it would be for that method to just call the dialog's close watcher's "request to close" method. That nicely takes care of several kinds of issues, such as recursion checks ("is running cancel action") and the like.

However, non-modal dialogs don't get a close watcher. The "easy" thing is to say that requestClose() only works on modal dialogs. And I'm ok with that, but wanted to run it by especially @domenic who requested this feature. The alternative doesn't feel web compatible: add a close watcher to non-modal dialogs. I'm fairly positive (without supporting data) that will break the web.

Suggestions? Would we be ok with dialog.requestClose() throwing an exception if dialog isn't modal? That would nicely solve the problem, and feels like yet another reason for people to prefer either modal dialogs, or <dialog popover>'s, but not old-school modeless dialogs. Which I support. 😄

@lukewarlow
Copy link
Member Author

Non-modal dialogs can get a close watcher if the closedby value isn't in the none state (though that state is the default). Perhaps requestClose() should no-op or throw when in this state, for both modal and non-modal dialogs.

@mfreed7
Copy link
Contributor

mfreed7 commented Nov 15, 2024

Non-modal dialogs can get a close watcher if the closedby value isn't in the none state (though that state is the default). Perhaps requestClose() should no-op or throw when in this state, for both modal and non-modal dialogs.

Right. Sorry. I suppose I should have said that an exception gets thrown for a dialog whose closedby attribute is in the none state. (I was mostly thinking about existing dialogs that lack a closedby attribute, which will be in the none state.) Thanks!

@lukewarlow
Copy link
Member Author

I guess the three options are:

  • no-op
  • throw
  • fallthrough to directly calling 'close the dialog' algorithm?

My initial hunch is that throwing is best but I might be wrong.,

@lukewarlow
Copy link
Member Author

Alternatively and this is just off the top of my head idea so apologies if I'm missing something obvious.

Is it possible that perhaps dialogs should always have an internal close watcher and its enabled Boolean is what gets toggled by this new attribute? And then we just only care about that Boolean when processing close watchers? But not when calling the algorithms such as 'request to close' directly? This way existing non-modal dialogs wouldn't close on escape press but you could still reuse the close watcher infra for this function?

@mfreed7
Copy link
Contributor

mfreed7 commented Nov 15, 2024

Thanks for the thoughts. So I've spent today implementing the "throw" behavior, and adding it to the spec. After doing that, it feels pretty natural, and seems to have fewer footguns than either "no-op" or "fallthrough". Both of those seem like surprising (and semi-silent) behavior, vs. getting an exception that tells you exactly why things are weird.

But please take a look at the new spec PR text #10737 and also the updated set of tests right here, and let me know what you think.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 18, 2024
1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 18, 2024
1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 18, 2024
1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 20, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Nov 21, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Nov 22, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Nov 26, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Commit-Queue: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223

UltraBlame original commit: 6b098292cbe3fd32f9a6e982d47edfdd882c84cc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Nov 27, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Commit-Queue: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223

UltraBlame original commit: 6b098292cbe3fd32f9a6e982d47edfdd882c84cc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Nov 27, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Commit-Queue: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223

UltraBlame original commit: 6b098292cbe3fd32f9a6e982d47edfdd882c84cc
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 27, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Nov 30, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Dec 1, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Commit-Queue: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223

UltraBlame original commit: 1f244e2980abb756ba68b021b1000a60e03d054e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Dec 1, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Commit-Queue: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223

UltraBlame original commit: 1f244e2980abb756ba68b021b1000a60e03d054e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Dec 1, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Commit-Queue: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223

UltraBlame original commit: 1f244e2980abb756ba68b021b1000a60e03d054e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: close watchers topic: dialog The <dialog> element
Development

No branches or pull requests

3 participants