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

Support disabling CloseWatcher integration in <dialog> #10592

Open
lucacasonato opened this issue Aug 30, 2024 · 8 comments · May be fixed by #10593
Open

Support disabling CloseWatcher integration in <dialog> #10592

lucacasonato opened this issue Aug 30, 2024 · 8 comments · May be fixed by #10593
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: dialog The <dialog> element.

Comments

@lucacasonato
Copy link
Member

lucacasonato commented Aug 30, 2024

What problem are you trying to solve?

Right now one can not really use <dialog> to grab the users attention until they complete some action. This is because <dialog> always lets the user close it through native UI, even when the developer explicitly does not want to allow this (ie when using cancel events). This is done in HTML to prevent malicious sites from blocking the Android back button. This is good!

However, there are genuine use cases for unclosable <dialog>:

  • a upload progress modal that prevents the user from continuing until the upload is done
  • a "Please accept our terms of service" modal after signing in that should not be dismissable, because a user must accept TOS before using the service
  • a required action before progress can be made, such as a "Please enter your name" modal before you can join a video call / online game

As such, it would be great if we had a way to open an unclosable dialog. This dialog would not have any CloseWatcher integration at all: pressing escape or the Android back button would completely bypass the dialog and instead do their default behaviour (like navigate back in the navigation history).

What solutions exist today?

No solutions using the native <dialog> element. One would have to reimplement the behaviour.

A solution that will probably be gone soon: https://issues.chromium.org/issues/363225474

How would you solve it?

dialog.showModal({ closeBehaviour: "manual" });

Anything else?

No response

@lucacasonato lucacasonato added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Aug 30, 2024
@domenic domenic added the topic: dialog The <dialog> element. label Aug 30, 2024
@lucacasonato
Copy link
Member Author

I opened a tenative PR - it should be trivial by everyone that implements CloseWatcher to implement: #10593

@domenic
Copy link
Member

domenic commented Aug 30, 2024

I think this has large overlap with the discussions in #9373 (which is unfortunately pretty long). That ended up in #10157 which was recently closed, but I think @mfreed7 still wants to pursue it.

The big difference is that proposes a new HTML attribute as opposed to your new option to showModal(). (That makes it extra hard as you have to deal with attribute addition/removal/mutation while the dialog is open.) And that has more options, although I like the idea of just starting with two.

@lucacasonato
Copy link
Member Author

Oh I like that proposal a lot - that interacts much better with a future where one can open dialogs declaratively from HTML, like we support for popover now.

If that @mfreed7 is going to work on that issue, then I'm happy to have that be the solution for this problem :)

Maybe we can simplify that proposal by initially only doing closedby="none" and `closedby="closerequest" as a first pass? That allows us to do the backdrop click close machinery as a follow up.

(Also I think this should be called autocloseby, because closeby="none" is backwards with the fact that you can still call dialog.close().)

@lukewarlow
Copy link
Member

That ended up in #10157 which was recently closed, but I think @mfreed7 still wants to pursue it.

Yeah I recently closed it because I don't really have the time to really work out the spec details of it but I think what's there and from our discussions is pretty close.

Maybe we can simplify that proposal by initially only doing closedby="none" and `closedby="closerequest" as a first pass? That allows us to do the backdrop click close machinery as a follow up.

Fwiw the backdrop click is the relatively easy part, we already have the machinery from popover. The difficulty is actually the none option and how to handle it dynamically changing.

TLDR: What do you do with the close watcher stack if you go from none -> closerequest, same with the other way around? I think we need a new activation flag on close watchers but getting the spec handling right for that is tricky due to stuff like the stacking behaviour.

One option we do have is to just not support dynamic update, either by closing the dialog or by leaving it open and the change having no immediate effect. But I think @domenic has a preference for handling this properly?

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 31, 2024

Sorry for the delay getting back to this. But yes I'd like to pick #10157 back up. Might take me a minute to wade through the comments there and see what's needed from CloseWatcher. But I just wanted to register interest and intention to continue that work.

@keithamus
Copy link
Contributor

I’d also be happy to pick this up as it aligns with our goals at GitHub.

@lukewarlow
Copy link
Member

Happy to assist anyone who wants to take over the PR.
I think for the exact mechanics of changing the value the best bet is to prototype an implementation and ensure it doesn't break the anti-abuse mechanism and spec based on that. I struggled to think about it purely spec wise.

@mfreed7
Copy link
Contributor

mfreed7 commented Nov 1, 2024

Thanks to both of you! I revamped (slightly) your PR, and re-opened it as #10737. I'm going to get started prototyping it, to see how it works in practice. I'd love help just evaluating that the PR looks good and the impl makes sense, when the time comes.

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: dialog The <dialog> element.
Development

Successfully merging a pull request may close this issue.

5 participants