-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Conversation
33a85df
to
d466422
Compare
d466422
to
c7f3a68
Compare
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 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. |
There is a scenario where you want to have a popover show on hover (TBD |
Yes the combining of attributes was discussed in #9167 and we resolved not to because of @jpzwarte's excellent point.
FWIW this is already the case due to form controls. |
I'm not convinced we want to add more and more do-x attributes to elements.
(and if we figure out some generic API, then popovertarget/*action could we removed?) |
@smaug---- I've added an FAQ section to the OP to speak to your concerns (among others). To quote the OP:
|
@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) 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 |
@nt1m if we settled on |
@nt1m doesn't that affect more than just the attribute names? For example:
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 |
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 One way to migrate is to make It’s all quite doable. If it still captures all the use cases. |
It's basically what @smaug---- described earlier in #9456 (comment)
I think the attribute values could reflect the method names, I don't think the method names need to change.
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:
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 |
Overall it seems like a worthwhile direction to me, but I'm a little concerned that The other concern is around close. Does this call <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> |
@smaug----'s proposal is slightly less wordy, just
Given dialogs have a way to close using ...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 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 |
IMO an important use case is |
@keithamus what does |
@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. |
Popover is now enabled in stable chrome, so yeah it wouldn't be great to unship them. |
I agree that making showModal the default sounds like a good idea |
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 Having said that, I really don't love the idea of unshipping Perhaps instead of unshipping As for the proposals, I'm ok with something short like
I think it'd also be nice to define 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? |
Just like event listeners, we could do |
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 |
The benefit of having something like |
Can you say more? I think in both cases it would be possible to "ignore" invalid values. For
Thinking more about this, I'm not sure it makes sense. Perhaps there really are three things that need to be controlled:
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"> ... |
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
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 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 <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 ( |
From a spectator's point of view this all sounds very complicated to be honest. |
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.
We can still have a combined model for how these attributes function, so that using |
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 |
I'm happy to keep the proposal as it stands, with |
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. |
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. |
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. |
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. |
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
|
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. |
For those following along, the new proposal is to add Please follow #9625. |
I'm closing this as it's very unlikely to be merged; #9625 is gaining traction and supersedes this work. |
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:
The
DialogInvokerElement
mixin is also added toHTMLButtonElement
andHTMLInputElement
.This PR specifies two new algorithms,
dialog modal target attribute activation behavior
which determines the behaviour for buttons that havedialogmodaltarget
. This is an area of attention I'd like to discuss; it does the following:<button dialogmodaltarget=foo>
will callshowModal()
on<dialog id="foo">
<button dialogmodaltarget=foo>
will callclose()
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 likepopover 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:
The proposed spec outlines that in the case of the invoker having a
popovertarget
,dialog modal target element
will returnnull
.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/whenpopover=hint
releases:Another justification for avoiding making these generic is avoiding invariants, for example these all have muddy heuristics:
Can't we have
dialogtarget
anddialogtargetaction=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 usedialog.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 forshowModal()
though, which is extremely useful for showing modal dialogs!If we did chose
dialogtarget
, the majority of use cases would bedialogtarget=foo dialogtargetaction=showModal
which is way more wordy thandialogmodaltarget=foo
. If #9376 went ahead and deprecated.show()
then it would stand to reason thatdialogtargetaction=show
would also be deprecated. One way to alleviate this issue is makingshowModal
the default. There's more discussion around the separate attributes starting around here: #3567 (comment)<dialog>
elements without JavaScript #3567 (comment) I'm claiming that Chrome are interested.dialogmodaltarget
attribute mozilla/standards-positions#834dialogmodaltarget
attribute WebKit/standards-positions#213Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …(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 )