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

[invokers] Should invokers on dialog have a cancel action? #938

Open
keithamus opened this issue Nov 6, 2023 · 8 comments
Open

[invokers] Should invokers on dialog have a cancel action? #938

keithamus opened this issue Nov 6, 2023 · 8 comments
Assignees
Labels
agenda+ Use this label if you'd like the topic to be added to the meeting agenda invokers needs edits This is ready for edits to be made

Comments

@keithamus
Copy link
Collaborator

keithamus commented Nov 6, 2023

Somewhat related to #937.

In the explainer today there exists a cancel action for dialog, which cancels the dialog (in other words it fires the cancel and close events, not just the close event).

<dialog id=d>
  <button commandfor=d command=request-close>This will cancel the dialog</button>
</dialog>

Should we keep this? The cancel event is fired when a CloseWatcher closes a dialog, it currently doesn't fire for buttons, so having these buttons cancel the dialog would be somewhat new behaviour.

My vote is that we should remove this, and cancel can be kept as an observation of the CloseWatcher behaviour, but I'd like to hear others' thoughts.

/cc @lukewarlow @mfreed7 @scottaohara @domenic

@lukewarlow
Copy link
Collaborator

lukewarlow commented Nov 6, 2023

For me it would make sense for cancel buttons to cancel the dialog. If the dialog is designed to make a decision or enter data and you don't then it's a "cancel" to me and the button should be able to signify this.

So I reckon we should keep it.

As a consumer of the dialog element I don't really care about close watchers Vs light dismiss (if that gets added) Vs my own cancel button. I just want an easy signal for "this didn't do the thing I was hoping it would"

@lukewarlow lukewarlow added the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Nov 7, 2023
@css-meeting-bot
Copy link

The Open UI Community Group just discussed [invokers] Should invokers on dialog have a cancel action?, and agreed to the following:

  • RESOLVED: Invokers will only provide a close action, and the cancel operation (and event) will be reserved for CloseWatcher or other UA gestures.
The full IRC log of that discussion <jarhar> keithamus: currently, dialogs can have two events. one is close and also closewatcher - confusing but the closewatcher wil close the dialog and will emit a close and cancel event
<jarhar> keithamus: closewatcher right now is escape key and back gesture on mobile
<jarhar> keithamus: with invokers, do we want to have a cancel action so that you can create a button that will also dispatch the cancel event and the close event
<masonf> q?
<bkardell_> no
<jarhar> keithamus: you can have a close action which dispatches close, so should we have one that does cancel and close
<jarhar> masonf: two choices are - just an action that is only close and another action that is close and cancel? or making the close action fire both?
<jarhar> brecht_dr: wondering about use case? why would you need both of them? i just dont understand
<jarhar> keithamus: close action ... theres a difference between canceling and escaping from the contents of the dialog
<jarhar> keithamus: and closing the dialog
<masonf> q+
<jarhar> keithamus: theres not a compelling use case, one of the questions that came up was: well we have close, which calls close() and that dispatches the close event
<jarhar> keithamus: and then there is the closewatcher which dispatches the cancel event
<jarhar> keithamus: you can determine whether it came from a closewatcher by listening to cancel
<jarhar> keithamus: you could argue that close button should fire cancel vs no button or cancel
<jarhar> q+
<bkardell_> q+
<jarhar> keithamus: theres a button in the footer, and you might want those things to be different
<jarhar> keithamus: when we have a dialog we put a close button in there
<jarhar> keithamus: ways to figure out if its the x button or another one of the buttons so i dont have a compelling use case
<jarhar> masonf: it feels like we shouldnt
<jarhar> masonf: were talking about invokers and modal dialogs
<jarhar> masonf: in that case, the invoker has to be within the modal dialog
<jarhar> masonf: which means its just another button in the dialog
<jarhar> masonf: theres nothing that tells you it has to have an x on it
<jarhar> masonf: feels like theres a nice separation that closewatcher does cancel, and button should just do close
<jarhar> masonf: that feels clean and easy
<scotto_> q+
<jarhar> masonf: trying to add somehow two kinds of actions is ... difference between cancel and close my heads gonna explode
<Luke> q+
<masonf> ack mason
<jarhar> keithamus: i think that the cancel event is that the user agent has made this gesture
<jarhar> keithamus: close button should only be for animations and cleanup, dialog could close because its accepted
<jarhar> keithamus: close is a generic this thing is going away, whereas cancel is like the user has dismissed this
<jarhar> keithamus: subsequent close event is now its gone away
<jarhar> keithamus: that same cancel behavior could be provided by js in a button that calls close
<masonf> jarhar: I helped implement the closewatcher integrations on Chromium, and it's complicated. I don't remember what cancel's point was.
<masonf> jarhar: lots of machinery around anti-abuse. We should just keep it close and leave cancel to the closewatcher.
<masonf> ack jarhar
<masonf> ack kardell
<jarhar> bkardell_: also wanted to say it should just be close
<masonf> ack bkarde
<jarhar> bkardell_: we have these nice invokers now and we want to map those to the ui
<brecht_dr> +1 (on just being close)
<jarhar> bkardell_: they cant be arbitrarily mapped to all things
<jarhar> bkardell_: a lot of things when you think of dialogs do have buttons in them that youre not going to be able to use an invoker for the same way
<jarhar> bkardell_: if the only action is close, then you can use an invoker
<jarhar> bkardell_: in many cases, you launch this and then clicking ok means you agree and it does cause the thing to close but that invoker also needs to do something else
<jarhar> keithamus: ideally clicking ok is a form submission where the method is dialog
<keithamus> Proposed resolution: Invokers will only provide a close action, and the cancel operation (and event) will be reserved for CloseWatcher or other UA gestures.
<jarhar> keithamus: does animations and event listeners
<Luke> q-
<masonf> ack scott
<jarhar> scotto_: reset button - what is that? is that a close or a cancel event?
<jarhar> scotto_: if one were thinking about closing a dialog submitting the dialog method on a form, thats a close event. is a reset a cancel or a close?
<jarhar> keithamus: i think reset does nothing and theres a conversation i want to ahve where it should close the dialog
<bkardell_> s/but that invoker also needs to do something else/but that isn't an invoker, it needs to do something else which also happens to, at the end, close the dialog somehow
<jarhar> scotto_: i thought it actually did close it
<jarhar> keithamus: i could be wrong
<jarhar> masonf: i dont think it closes it
<Luke> +1
<keithamus> RESOLVED: Invokers will only provide a close action, and the cancel operation (and event) will be reserved for CloseWatcher or other UA gestures.

@lukewarlow lukewarlow added needs edits This is ready for edits to be made and removed agenda+ Use this label if you'd like the topic to be added to the meeting agenda labels Nov 9, 2023
@lukewarlow
Copy link
Collaborator

Screenshot_20231116-090439.png

Interestingly Android has a similar cancel Vs close situation but they do allow a cancel function for cancel buttons

@lukewarlow
Copy link
Collaborator

#950 (reply in thread) - this discussion suggests that our assumption of a UA only cancel event isn't necessarily correct.

@lukewarlow
Copy link
Collaborator

Semi related to this I've raised whatwg/html#10164 - to discuss adding a new requestClose() (name tbd) function to dialogs, if this is added we should update the 'cancel' name to be requestClose to match.

Copy link

There hasn't been any discussion on this issue for a while, so we're marking it as stale. If you choose to kick off the discussion again, we'll remove the 'stale' label.

@github-actions github-actions bot added the stale label Aug 25, 2024
@lukewarlow lukewarlow added agenda+ Use this label if you'd like the topic to be added to the meeting agenda and removed stale labels Dec 9, 2024
@lukewarlow
Copy link
Collaborator

Our previous resolution no longer holds true as soon as whatwg/html#10737 is merged (it adds a dialog.requestClose(returnValue) function).

So I'm adding this back to the agenda to discuss adding a command="request-close" value for command invokers. That would just be a declarative form of that.

@css-meeting-bot
Copy link

The Open UI Community Group just discussed [invokers] Should invokers on dialog have a cancel action?, and agreed to the following:

  • RESOLVED: Add a request-close command for dialogs which uses the button value attribute for the returnValue parameter.
The full IRC log of that discussion <hdv> lwarlow: we include some basic commands for popover and some basic ones for dialog
<hdv> lwarlow: for dialog close and showModal, and for popover we have @@@
<hdv> lwarlow: we were debating whether to add a cancel event as well
<hdv> lwarlow: we discussed it before that we'd only add close as cancel is reserved for the UA
<masonf> q?
<hdv> lwarlow: should we add a request-close command ?
<hdv> masonf: I agree, makes sense to me
<scott> q+
<hdv> scott: when I reviewed it, I did find it strange that it was just close for dialog… can it work for non modal dialogs?
<hdv> lwarlow: you can close them but can't open them
<hdv> masonf: the idea is, all the commands follow precisely what the name of the JS method is
<hdv> lwarlow: we had a discussion about this… the decision not toclose non modal dialogs through this… there is a use case to have it declarative way to open, but not to close
<hdv> scott: was curious if there's a scenario where one has a close button outside of the dialog?
<hdv> scott: then maybe it is like popover? primary use case seems to be closing modal dialogs. In that case you dont want the extra a11y mappings like aria-expnanded
<hdv> s/aria-expnanded/aria-expanded
<hdv> lwarlow: there is an edge case where the mappings might not be correct
<hdv> scott: if they are not correct that would be my fautl, but I was wrong
<hdv> s/fautl/fault
<masonf> q?
<hdv> masonf: seems like an edge case though, two things we hope you won't do
<masonf> ack scott
<lwarlow> Proposed Resolution: Add a request-close command for dialogs which uses the button value attribute for the returnValue parameter.
<masonf> +1
<hdv> masonf: I'm good with that
<hdv> +1
<lwarlow> RESOLVED: Add a request-close command for dialogs which uses the button value attribute for the returnValue parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda+ Use this label if you'd like the topic to be added to the meeting agenda invokers needs edits This is ready for edits to be made
Projects
None yet
Development

No branches or pull requests

3 participants