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 dialogmodaltarget attribute #9456

Closed

Conversation

keithamus
Copy link
Contributor

@keithamus keithamus commented Jun 26, 2023

This PR aims to implement a specification that could resolve #3567 based on the proposed comment here: #3567 (comment). That is, it aims to add the following:

interface mixin DialogInvokerElement {
  [CEReactions] attribute Element? dialogModalTargetElement;
};

The DialogInvokerElement mixin is also added to HTMLButtonElement and HTMLInputElement.

This PR specifies two new algorithms, dialog modal target attribute activation behavior which determines the behaviour for buttons that have dialogmodaltarget. This is an area of attention I'd like to discuss; it does the following:

  • <button dialogmodaltarget=foo> will call showModal() on <dialog id="foo">
  • <button dialogmodaltarget=foo> will call close() on <dialog id="foo" open>

In this way, the behaviour of dialogmodaltarget acts like a toggle. The reasoning here is that for a modal dialog to be open, the only buttons that would be active on the page would be those not made inert by the dialog, so likely within the tree of the dialog. We could further ratify this by adding checks within the algorithm but I think this is satisfactory.

The second algorithm dialog modal target element is, IMO, fairly vanilla. It gets the <dialog> element, much like popover target element, except instead of checking for [popover] it checks that the element is a dialog element.

There's one large piece here that remains somewhat undefined, which is what does this do?

Another key point to look at is the following:

<button dialogmodaltarget="foo" popovertarget="foo">Open</button>
<dialog id="foo" popover>

The proposed spec outlines that in the case of the invoker having a popovertarget, dialog modal target element will return null.


FAQ

Can't we make combine popovertarget & dialogmodaltarget or otherwise make them generic (commandtarget, openertarget, etc).

This was discussed in #9167 and it was decided that it would be undesirable for a few reasons. The first being popovertarget has already shipped, which means it can't be removed (easily). The second reason is that it is viable/desirable to have a button point to multiple forms of interactive elements, especially if/when popover=hint releases:

<button popovertarget="my-hint" dialogmodaltarget="my-dialog">Open dialog</button>
<p popover="hint" id="my-hint">Opens the dialog...</p>
<dialog id="my-dialog">The dialog</dialog>

Another justification for avoiding making these generic is avoiding invariants, for example these all have muddy heuristics:

<button openertarget="foo">Open</button>
<dialog id="foo" popover></dialog>
<button openertarget="foo" openeraction="showModal">Open</button>
<dialog id="foo" popover></dialog>
<button openertarget="foo" openeraction="showModal">Open</button>
<div id="foo"></div>

Can't we have dialogtarget and dialogtargetaction=show|showModal?

We could but #9376 goes into some detail as to why show() on a <dialog> has very limited use now <dialog popover> exists. In short, there's little good reason to use dialog.show(), which doesn't show on the top layer and doesn't handle light dismiss, which are both very desirable traits for a dialog. That doesn't negate the need for showModal() though, which is extremely useful for showing modal dialogs!

If we did chose dialogtarget, the majority of use cases would be dialogtarget=foo dialogtargetaction=showModal which is way more wordy than dialogmodaltarget=foo. If #9376 went ahead and deprecated .show() then it would stand to reason that dialogtargetaction=show would also be deprecated. One way to alleviate this issue is making showModal the default. There's more discussion around the separate attributes starting around here: #3567 (comment)

(See WHATWG Working Mode: Changes for more details.)


/form-elements.html ( diff )
/index.html ( diff )
/input.html ( diff )
/interactive-elements.html ( diff )
/scripting.html ( diff )

@nt1m
Copy link
Member

nt1m commented Jun 28, 2023

Not opposed to adding a declarative way to open modal dialogs, but it makes me uneasy that we have declarative attributes that can conflict with each other. It might be manageable with just 2, but what happens if we start introducing more that can conflict?

I wonder if we should rethink attribute names for popover if we do this:

popovertarget -> target
popovertargetaction -> targetaction

or something similar, then the show modal stuff can just reuse that infrastructure.

We had a similar design problem with the original popover declarative attributes (popovershowtarget/ popoverhidetarget/ popovertoggletarget).

It would be good to spend a bit of time thinking how we want to design declarative attributes moving forward, so we don't have more similar conflicts.

@jpzwarte
Copy link

I wonder if we should rethink attribute names for popover if we do this:

popovertarget -> target popovertargetaction -> targetaction

There is a scenario where you want to have a popover show on hover (TBD popover="hint") and have a dialog shown on click. That is not possible if both use the same attributes.

@keithamus
Copy link
Contributor Author

keithamus commented Jun 28, 2023

Yes the combining of attributes was discussed in #9167 and we resolved not to because of @jpzwarte's excellent point.

declarative attributes that can conflict with each other

FWIW this is already the case due to form controls. <form><button type="submit" popovertarget="foo">Open</button></form> will not open a popover, and while <input type="button" popovertarget="foo"> will, <input type="text" popovertarget="foo"> will, understandably, not. I don't see these as substantially different to <input type="button" dialogmodaltarget="foo" popovertarget="foo"> being equally invalid.

@smaug----
Copy link

I'm not convinced we want to add more and more do-x attributes to elements.
There might be opportunity to have some more generic command API to control stuff.
Maybe something like:

<dialog id="dialogelement"></dialog
<button command="show" commandtarget="dialogelement">
<button command="showModal" commandtarget="dialogelement">

(and if we figure out some generic API, then popovertarget/*action could we removed?)

@keithamus
Copy link
Contributor Author

@smaug---- I've added an FAQ section to the OP to speak to your concerns (among others). To quote the OP:

Can't we make combine popovertarget & dialogmodaltarget or otherwise make them generic (commandtarget, openertarget, etc).

This was discussed in #9167 and it was decided that it would be undesirable for a few reasons. The first being popovertarget has already shipped, which means it can't be removed (easily). The second reason is that it is viable/desirable to have a button point to multiple forms of interactive elements, especially if/when popover=hint releases:

<button popovertarget="my-hint" dialogmodaltarget="my-dialog">Open dialog</button>
<p popover="hint" id="my-hint">Opens the dialog...</p>
<dialog id="my-dialog">The dialog</dialog>

Another justification for avoiding making these generic is avoiding invariants, for example these all have muddy heuristics:

<button openertarget="foo">Open</button>
<dialog id="foo" popover></dialog>
<button openertarget="foo" openeraction="showModal">Open</button>
<dialog id="foo" popover></dialog>
<button openertarget="foo" openeraction="showModal">Open</button>
<div id="foo"></div>

@nt1m
Copy link
Member

nt1m commented Jun 28, 2023

@keithamus @jpzwarte If we need to have the hover use case work, I'd rather opt for an event listener attributes style design:

command/commandtarget (click)
hovercommand/hovercommandtarget ("hover" tbd, could be "hint", etc.)

My worry is that we might actually reach a point where we have too many declarative attributes in the future, where precedence becomes very fuzzy and unclear.

Probably a bit late to change the attribute names though... I think Chrome just shipped popover in 114? @mfreed7

@keithamus
Copy link
Contributor Author

@nt1m if we settled on commandtarget what would it do with <dialog popover>? How would a web developer tell the commandtarget to call showModal?

@jpzwarte
Copy link

@nt1m doesn't that affect more than just the attribute names? For example:

  • popovertarget -> show/hide/togglePopover()`
  • dialogmodaltarget -> showModal()/close()`

Does it make sense to have a common set of attributes that map to methods depending on the instance of the HTMLElement? You'd have a switch statement in the implementation?

@keithamus
Copy link
Contributor Author

Changing behavior based on the target element is already true of popover today, and it doesn’t make the implementation more costly IMO. The action could default to “toggle” but allow for “open” or “close” and if the element is a dialog with no popover state then the “open” and “toggle” actions could call showModal (assuming dialog show deprecates). This could also expand to supporting <details>.

One way to migrate is to make popovertarget/action an alias for these attributes.

It’s all quite doable. If it still captures all the use cases.

@nt1m
Copy link
Member

nt1m commented Jun 28, 2023

@nt1m if we settled on commandtarget what would it do with <dialog popover>? How would a web developer tell the commandtarget to call showModal?

It's basically what @smaug---- described earlier in #9456 (comment)

doesn't that affect more than just the attribute names?

I think the attribute values could reflect the method names, I don't think the method names need to change.

Does it make sense to have a common set of attributes that map to methods depending on the instance of the HTMLElement? You'd have a switch statement in the implementation?

<button dialogmodaltarget="foo" popovertarget="bar">Open</button>

The issue arises when you have something like this (where bar is not a hint popover) ^ What should this do, does this open foo & bar at the same time? Does one take precedence over the other?

If it opens both at the same time, then we have the issue of:

<button dialogmodaltarget="foo" popovertarget="foo">Open</button>

If there is some precedence, then it is sort of arbitrary here, and it would just get worse as we add more of these.

Fwiw, we had a similar design problem when the popover attributes were popovershowtarget / popoverhidetarget / popovertoggletarget instead of popovertarget / popovertargetaction.

@keithamus
Copy link
Contributor Author

Overall it seems like a worthwhile direction to me, but I'm a little concerned that <button commandtarget="foo" commandtargetaction="showModal"> is quite wordy for <dialog> especially considering any other combination would be invalid.

The other concern is around close. Does this call close() or hidePopover()?

<button commandtarget="foo" commandtargetaction="hide">Close</button>
<dialog id="foo"></dialog>

Do we need to make those distinct? In that case is this invalid?

<button commandtarget="foo" commandtargetaction="close">Close</button>
<dialog id="foo" popover></dialog>

@nt1m
Copy link
Member

nt1m commented Jun 28, 2023

Overall it seems like a worthwhile direction to me, but I'm a little concerned that <button commandtarget="foo" commandtargetaction="showModal"> is quite wordy for <dialog> especially considering any other combination would be invalid.

@smaug----'s proposal is slightly less wordy, just command (instead of commandtargetaction) and commandtarget. Not as short as dialogmodaltarget I admit, but it avoids weird ambiguities.

The other concern is around close. Does this call close() or hidePopover()?

<button commandtarget="foo" commandtargetaction="hide">Close</button>
<dialog id="foo"></dialog>

Do we need to make those distinct? In that case is this invalid?

<button commandtarget="foo" commandtargetaction="close">Close</button>
<dialog id="foo" popover></dialog>

Given dialogs have a way to close using <form method="dialog">, maybe we only need the counterpart calling hidePopover.

...but before we get too much into the details of this, I want to check-in if Chrome folks are OK with such a late stage rename, because this does imply unshipping popovertarget/popovertargetaction. cc @josepharhar @mfreed7

I think it's technically still possible (though not ideal), given Safari's implementation is still in beta, and Firefox hasn't shipped.

It seems like a good time to think about design of declarative attributes, especially with popover="hint" coming as well.

@keithamus
Copy link
Contributor Author

Given dialogs have a way to close using <form method="dialog">, maybe we only need the counterpart calling hidePopover.

IMO an important use case is <input type="reset" commandtarget="the-dialog">

@annevk
Copy link
Member

annevk commented Jul 7, 2023

@keithamus what does type=reset mean there?

@keithamus
Copy link
Contributor Author

@annevk a button which resets the forms contents to their original values. Allowing this button too also close a dialog is useful as often times a dialog dismissal should also reset the forms content.

@josepharhar
Copy link
Contributor

I want to check-in if Chrome folks are OK with such a late stage rename, because this does imply unshipping popovertarget/popovertargetaction

Popover is now enabled in stable chrome, so yeah it wouldn't be great to unship them.

@josepharhar
Copy link
Contributor

One way to alleviate this issue is making showModal the default

I agree that making showModal the default sounds like a good idea

@mfreed7
Copy link
Contributor

mfreed7 commented Jul 10, 2023

...but before we get too much into the details of this, I want to check-in if Chrome folks are OK with such a late stage rename, because this does imply unshipping popovertarget/popovertargetaction. cc @josepharhar @mfreed7

Thanks for checking in, and sorry for the delay. I was OOO for a few weeks.

Generally, I'd like to get this right. As mentioned a few times above, there are more things coming, such as popovertargetaction=hover (or similar). So I want to make sure we're on a good path.

Having said that, I really don't love the idea of unshipping popovertarget and popovertargetaction. We did ship all of Popover in 114, on May 30, and the usage is rising very quickly. There's also all of the MDN documentation which would need to be updated. Etc.

Perhaps instead of unshipping popovertarget*, it could be defined in terms of whatever comes out of this PR? We could mark it deprecated ASAP and hope most new usage migrates to the new thing over time, assuming it's designed well and is at least as ergonomic?

As for the proposals, I'm ok with something short like

<button actiontarget=dialogelement action="showModal">
<button actiontarget=popover action="toggle">

I think it'd also be nice to define action="auto" and have that be the default, so that the action could automatically depend on the target. E.g. toggle if the target is a popover, showModal if the target is a dialog, etc.

The one thing I haven't seen answered above is how to allow for the "multiple actions" thing:

<button popovertarget=hint popovertargetaction=hover dialogmodaltarget=dialog>Open dialog</button>
<p popover=hint id=hint>Description of the button...</p>
<dialog id=dialog>The dialog</dialog>

Is there a proposal above that achieves something like this?

@nt1m
Copy link
Member

nt1m commented Jul 10, 2023

I think it'd also be nice to define action="auto" and have that be the default, so that the action could automatically

depend on the target. E.g. toggle if the target is a popover, showModal if the target is a dialog, etc.

The one thing I haven't seen answered above is how to allow for the "multiple actions" thing:

Open dialog

Description of the button...

The dialog Is there a proposal above that achieves something like this?

Just like event listeners, we could do hoveraction="..." hoveractiontarget=""

@mfreed7
Copy link
Contributor

mfreed7 commented Jul 10, 2023

Just like event listeners, we could do hoveraction="..." hoveractiontarget=""

Yeah, I suppose that would be pretty clear. It's either that (a duplicate set of the two attributes) or some kind of value syntax like action="show showModal" actiontarget="popover_id dialog_id". I'm not sure which is more ergonomic.

@keithamus
Copy link
Contributor Author

The benefit of having something like hoveraction= (although I'd really like to avoid hover) is that we can limit actions for that interaction. For example hoveraction=showModal should never be allowed. Passing a list to action= does not allow us to restrict like this.

@mfreed7
Copy link
Contributor

mfreed7 commented Jul 11, 2023

The benefit of having something like hoveraction= (although I'd really like to avoid hover) is that we can limit actions for that interaction. For example hoveraction=showModal should never be allowed. Passing a list to action= does not allow us to restrict like this.

Can you say more? I think in both cases it would be possible to "ignore" invalid values. For hoveraction=showModal we just don't do anything. Akin to hoveraction=invalid_value. The same would work with the list syntax - just ignore invalid values, which you'll have to do anyway. Right?

Just like event listeners, we could do hoveraction="..." hoveractiontarget=""

Thinking more about this, I'm not sure it makes sense. Perhaps there really are three things that need to be controlled:

  1. The target element (popover, dialog, etc.)
  2. What should happen to the target (show, hide, toggle, showModal)
  3. What action ("trigger") on the invoker causes that action (activate, hover, etc.)

If you put that together with the desire to control multiple targets, what about this?

<button
    invoketarget="id1 id2"
    invokeaction="toggle showModal"
    invoketrigger="hover activate"> Hover for tooltip, click for dialog </button>
<div popover=hint id=id1> Hover tooltip </div>
<dialog id=id2"> Dialog </dialog>

(I changed to "invoke" because that seems the best way to describe what's happening. Or at least more so than "command" or "action".)

Another option would be to cram it all into a single attribute:

<button invoke="id1 toggle hover, id2 showModal activate"> ...

@keithamus
Copy link
Contributor Author

Can you say more?

It's easier for users to reason at a glance, or for downstream tooling (e.g. linters) to check an allow list of values and report a violation for something like hoveraction=showModal. It's a more difficult task when presented with a list which is valid or not depending on the contents of another list.

If you put that together with the desire to control multiple targets, what about this?

I worry the lists might cause confusion if we elide when defaults make sense. For example, what does this mean?

<button
    invoketarget="id1 id2"
    invokeaction="toggle"
    invoketrigger="hover activate">Show</button>

If the lists have a size mismatch, do we assume toggle for all? That's the current behaviour for a missing popoveraction but the concept of default values is fairly common in HTML, and I fear that equal sized DOMTokenList might be a surprising new behaviour.

Now let's say for some reason some new action is added (let's say "touchhold"), what does this mean:

<button
    invoketarget="id1 id2 id3"
    invokeaction="toggle"
    invoketrigger="hover activate touchhold">Show</button>

To customise the touchold action I've either got to specify the active action, or refactor all three lists:

<button
    invoketarget="id1 id3 id2"
    invokeaction="toggle showModal"
    invoketrigger="hover touchhold activate">Show</button>

The same could be expressed with explicit individual attributes, which are a little less concise but, IMO, much clearer:

<button
    overtarget="id1"
    invoketarget="id2"
    touchholdtarget="id3" touchholdaction="showModal"
>Show</button>

Here, each pair wires up a specific behaviour (over -> hover & focus, invoke -> click/touch/keyboard, touchhold -> touch hold), the default value for any *action is toggle, and overaction cannot have the value showModal.

@jpzwarte
Copy link

From a spectator's point of view this all sounds very complicated to be honest. dialogmodaltarget was really simple...

@mfreed7
Copy link
Contributor

mfreed7 commented Jul 13, 2023

From a spectator's point of view this all sounds very complicated to be honest. dialogmodaltarget was really simple...

I can't say I disagree with this. We have a general aversion to adding more attributes to the platform, and I'm not sure I understand why. There's no performance/implementation reason - attributes are roughly free.

  1. Separate attributes for separate features (popovertarget and dialogtarget). Pro: very clear what they do and what feature they relate to. Con: more attributes on the platform.
  2. Combined attributes for all features (actiontarget). Pro: fewer attributes. Con: more convoluted and hard to understand.

We can still have a combined model for how these attributes function, so that using popovertarget is very close to using dialogtarget. That seems to be the developer benefit people are nominally aiming for when they suggest combining everything into one set of attributes.

@hidde
Copy link
Member

hidde commented Jul 13, 2023

Reading through how the discussion here, and considering the various possibilities people have put forward, I understand the want to find something more generic, but I'm not sure if there will be many more similar attributes after these two.

I'll say I agree with @jpzwarte and @mfreed7 that dialogmodaltarget brings a lot of clarity, it's my favourite. I feel it would be the easiest to teach and learn, as well as the clearest when coming across it in code.

@keithamus
Copy link
Contributor Author

I'm happy to keep the proposal as it stands, with dialogmodaltarget. @smaug---- and @nt1m do you want to make comment, based on the discussion so far?

@nt1m
Copy link
Member

nt1m commented Jul 13, 2023

We have a general aversion to adding more attributes to the platform, and I'm not sure I understand why. There's no performance/implementation reason - attributes are roughly free.

My concern is more about extensibility and developer experience. If we add more declarative attributes in the future, there is just going to be a lot of confusion about what takes precedence.

@bramus
Copy link

bramus commented Jul 13, 2023

A benefit of individual properties which hasn't been mentioned yet (after only quickly skimming this thread) is that they're easier to feature detect.

@mfreed7
Copy link
Contributor

mfreed7 commented Jul 13, 2023

My concern is more about extensibility and developer experience. If we add more declarative attributes in the future, there is just going to be a lot of confusion about what takes precedence.

This certainly needs to be defined, but I don't think it adds to confusion in the "normal" use case. I.e. adding both attributes is "weird" so if you (as a developer) are doing that, then it's probably normal to have to look up how it works.

As for exactly how to decide on precedence, I'd say that if the developer uses both (e.g. <button popovertarget=foo dialogtarget=bar>) then the dialog "wins". That naturally follows, since if you do "both", the popover will either be closed by the dialog or rendered inert by the dialog, depending on which one you do first. So let's just define the behavior to say popovertarget is ignored in this case.

@nt1m
Copy link
Member

nt1m commented Jul 13, 2023

This isn't about just two attributes, this is also about being able to extend the platform for more declarative attributes too.

I imagine we don't want to end up with a flow chart of attributes to define precedence.

@keithamus
Copy link
Contributor Author

I'm not sure if there will be many more similar attributes after these two.

It depends on how many interactive elements we wish to add our own buttons for. I could easily see requests for many other interactive elements in the future. In openui/open-ui#702 there is a discussion about <selectmenu> which I feel relates:

@domenic

My suggested starting point is to figure out how you'd replace the file selector button in <input type="file">

@keithamus

In order to have a button declaratively replace the button in <input type="file">, which takes no children, we may end up with a solution that relates to the existing work done on popovertarget and subsequent proposal for dialogmodaltarget which has requests to unify toward something more generic like invokertarget. Perhaps there's some space to explore here about giving buttons the general ability to declaratively invoke interactive elements on the page?

An invokertarget could also apply nicely to a video; where invokeraction could be on of the video control actions e.g. play/pause. <input type="range" invokertarget="video"> could also have an invokeraction=playbackrate|volume|time which would create an easy way to make sliders that act on a video.

@brechtDR
Copy link

brechtDR commented Jul 17, 2023

The benefit of having something like hoveraction= (although I'd really like to avoid hover) is that we can limit actions for that interaction....

There are some cases (for example on longer informative tooltips) where on a desktop a popover/tooltip is shown and mobile users get a modal on touch. Just wanted to add this behavior as something we should think about since it might conflict with touchhold actions of popover. (Q?)

This could be a benefit of having the action in the attribute instead of the value as well. Although it might be a bit more niche, It's something to think about. So having a "hover" action and "click" at the same time. Whether this is a good way of implementing information is not my point, but as a developer I had to implement this kind of behavior more than once in the past.

@keithamus
Copy link
Contributor Author

For those following along, the new proposal is to add invokertarget which will allow for opening of <dialog> or popover and in a more extensible manner.

Please follow #9625.

@keithamus
Copy link
Contributor Author

I'm closing this as it's very unlikely to be merged; #9625 is gaining traction and supersedes this work.

@keithamus keithamus closed this Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Have some way of opening <dialog> elements without JavaScript
10 participants