Add commandfor & command attributes to HTMLButtonElement

[keithamus]

This adds the commandfor & command attributes and a "command" event using the CommandEvent interface.

Button activation checks if the button has a "commandfor" target and if so performs invoker command behaviour depending on command and the target element.

• [x] At least two implementers are interested (and none opposed):
  • Mozilla Supportive & prototyping
  • Webkit TBA but propotype available
  • Chromium supportive; actively prototyping.
• [x] Tests are written and can be reviewed and commented upon at:
  • https://github.com/web-platform-tests/wpt/pull/42664 (PR to make the tests non-tentative)
• [x] Implementation bugs are filed:
  • Chromium Bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1490919
  • Chromium Patch: https://chromium-review.googlesource.com/c/chromium/src/+/4926471
  • Gecko Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1856430
  • Gecko Patch: https://phabricator.services.mozilla.com/D190449
  • WebKit Bug: https://bugs.webkit.org/show_bug.cgi?id=262840
  • Webkit Patch: https://github.com/WebKit/WebKit/pull/18794
  • Deno (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): …
• [x] MDN issue is filed: https://github.com/mdn/content/pull/30829
• [x] The top of this comment includes a clear commit message to use.

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

---

/dom.html ( diff ) /form-elements.html ( diff ) /index.html ( diff ) /indices.html ( diff ) /interaction.html ( diff ) /interactive-elements.html ( diff ) /popover.html ( diff ) /webappapis.html ( diff )

[scottaohara]

I realize this is also in the steps for getting the popover target element, but in both cases I'm wondering why its specified to return null if the node is in the disable state?

Doing so, at least with <button popovertarget=foo disabled> testing in chrome, results in that element not exposing whether the popover is in the expanded or collapsed state - as I'm assuming due to

"If node is disabled, then return null."

that state gets removed. That's unexpected, to have that state removed based on whether the button is disabled or not. And for invokertarget - if it is really is going to do more than just show/hide content - there are a lot of other states that should still be exposed, regarless of if the element is in the disabled state or not.

edit: I can file a bug for disabled / popovertarget if necessary - i just wanted to get insight on this first, before I went and made that issue. cc @mfreed7

[keithamus]

Yes actually this spec needs to change to reflect work done in the implementations that they always return the invokeTarget and it's only the activation behaviour that is influenced by the form/disabled state.

[lukewarlow]

Fwiw HTML requires a positive standards position or for chrome LGTMs on an intent to ship to be considered supportive.

Saying that Mozilla have marked their position as positive so that's 1 implementor interested.

I do wonder how this requirement works for a feature such as this which will require multiple PRs to add to the spec?

[domenic]

a feature such as this which will require multiple PRs to add to the spec?

I'm confused why this feature is being done as multiple PRs; it makes review a good deal harder.

[keithamus]

If it makes it easier to review I’m happy to put more into one PR. I figured it would be worthwhile splitting it into the core vs each elements behaviour as I imagine there will be more to discuss with each elements behaviour.

[domenic]

Well, it'd make it easier for me, but I haven't signed up to review yet, so no need to make any changes until we get some more opinions :)

Edited to add: the reason it makes it more difficult is that I don't think we want to accept the feature piecemeal.

[lukewarlow]

As per https://github.com/openui/open-ui/issues/900#issuecomment-1834448462 this'll need updating to only fire the event when the action is custom (has a hypen) or is recognised and valid (correct action name on correct element).

TLDR is that this will allow us to add default actions in future without conflicting with user land code.

[lukewarlow]

In https://github.com/openui/open-ui/issues/952#issuecomment-1834461595 we resolved that "Invokers v1 will be popover and dialog invoking."

This should help keep this initial PR as small as possible while also avoid the issue of reviewing stuff piecemeal.

So #9875 can be merged into this along with dialog related changes.

[mfreed7]

Fwiw HTML requires a positive standards position or for chrome LGTMs on an intent to ship to be considered supportive.

Saying that Mozilla have marked their position as positive so that's 1 implementor interested.

Chromium is explicitly supportive of this proposal, so I believe it has two implementer support (including Mozilla).

Is this PR in a state that it can get a review? I'm happy to do so, if it'd help.

[keithamus]

I'd be happy to get reviews, I think this is in a good position for that.

[mfreed7]

I'd be happy to get reviews, I think this is in a good position for that.

Done - I added a first set of comments.

[keithamus]

@domenic OpenUI settled on the MVP for Invokers being to open dialogs & popovers. The other proposals for element behaviours (details, video, input showpicker) are being discussed and will likely come after this. I've added popover & dialog to this PR, hopefully this is now not too large to review.

@scottaohara I don't know if your comment was addressed but it sounds like a good issue to file!

@mfreed7 if you have some time a second look at this would be great, now it has added the Dialog & Popover steps as well.

[keithamus]

Okay I think this one is as ready as it'll ever be.

@domenic or @annevk if you had some time I'd really appreciate a review on this.

[foolip]

Thanks for the updates, @keithamus! I see that the build is still failing with something about "attr-associated element", are you building locally so you can see and fix that? Let me know if you can't find an incantation that will work and I can give it a shot.

[keithamus]

I am building locally, and it builds fine. I'll look into this error though soon when I get some time, thanks @foolip, and thanks very much for the constructive review commentary!

[keithamus]

Thank you so much @foolip for the helpful and thorough review! I've added some commits that address most of your comments I believe.

[foolip]

@keithamus let me know if any of my comments aren't actionable and I can push whatever changes I intended myself. (In a few cases it was unwieldy to suggest edits over many lines.)

[foolip]

In order to prepare this for landing, can you also update https://github.com/web-platform-tests/wpt/pull/42664? I haven't reviewed the tests, but perhaps you could go through them and see if there's anything that was changed during the course of this review (or changes before this PR was opened) that need updating?

There's also the corner cases of currentTarget and the final "otherwise" fallback in the "invoke target attribute activation behavior" algorithm. I now see the guarantee that invokee is in the HTML namespace needs to hold there too, so the suggested assert might make sense earlier on in the algorithm than where I made the suggestion.

[keithamus]

I've got about half way through your new commentary, but I'll tackle the rest later. I've pushed up what I have so far.

[foolip]

@keithamus Thank you! Please let me know when I should review again! (I see you pushed some commits after your last comment, but I'm not sure if there's more coming.)

[keithamus]

I think I've addressed all your commentary @foolip if you would kindly review again.

[foolip]

These are the remaining loose ends in addition to the last round of review comments:

• More testing of retargeting against currentTarget, see https://github.com/whatwg/html/pull/9841#discussion_r1587611904 (not blocking)
• An assert for https://github.com/whatwg/html/pull/9841#discussion_r1589052164 (that line is also affected by a review comment, doesn't need "is an element" check, but does need "HTML namespace" check somehow)

That's all, I think this is ready to merge very soon!

[keithamus]

All of the above review comments have been addressed @foolip (including adding the assert), thanks again and as always. I'll go add some WPTs for currentTarget in the meantime.

[lukewarlow]

Before merging this I think it would be good to get a resolution on the naming discussions @annevk bought up in the webkit standards position issue. Specifically on if "invoke" is the right name for this feature.

[foolip]

@keithamus would you mind providing a simplified commit message? Is something like this enough information?

This adds the invoketarget & invokeaction attributes and an "invoke" event using the InvokeEvent interface.

Button activation checks if the button has an invoke target and if so performs invoke behavior depending on invokeaction and the target element.

[foolip]

@annevk Are you OK with invokers as the name of this feature? We have two implementers in support of this.

[mfreed7]

Is this ok to land now/soon?

[keithamus]

@mfreed7 I believe we're discussing it in the APAC friendly WHATNOT session in around 12 hours time.

[mfreed7]

@mfreed7 I believe we're discussing it in the APAC friendly WHATNOT session in around 12 hours time.

Ahh right, sorry, I forgot that was bumped from last week's agenda. I won't be able to attend, but I'm supportive of the name "invoker"/"invoke", and my rationale is listed here: https://github.com/WebKit/standards-positions/issues/264#issuecomment-2103026173

[annevk]

WebKit isn't okay with the naming. I've stated so in the issue now: https://github.com/whatwg/html/issues/9625#issuecomment-2115718679.