Add commandfor & command attributes to HTMLButtonElement #9841

keithamus · 2023-10-08T23:00:35Z

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.

At least two implementers are interested (and none opposed):
- Mozilla Supportive & prototyping
- Webkit
- Chromium supportive; actively prototyping.
Tests are written and can be reviewed and commented upon at:
- Mark invokers tests as non-tentative web-platform-tests/wpt#42664 (PR to make the tests non-tentative)
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: Add invoketarget & invokeaction attributes WebKit/WebKit#18794
MDN issue is filed: Add documentation for the "command" and "commandfor" attributes, and "command" event. mdn/content#30829
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 )
/webappapis.html ( diff )

scottaohara · 2023-10-18T16:54:02Z

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

See related spec PR whatwg/html#9841

lukewarlow · 2023-11-04T13:36:02Z

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 · 2023-11-05T00:52:12Z

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 · 2023-11-05T01:03:00Z

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 · 2023-11-05T01:04:05Z

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 · 2023-12-01T21:07:17Z

As per openui/open-ui#900 (comment) 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 · 2023-12-01T21:09:24Z

In openui/open-ui#952 (comment) 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 · 2024-01-17T22:12:20Z

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 · 2024-01-17T22:47:01Z

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

source

mfreed7 · 2024-01-17T23:09:42Z

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.

annevk · 2024-11-27T07:43:54Z

source

+     <td><dfn attr-value for="html-global/command"
+     data-x="attr-button-command-custom"><code>(valid custom command)</code></dfn>


I would make this:

A valid custom command

with "valid custom command" cross-referenced, but not code. However, I'm also thinking that we should rename it to "custom command keyword". There's no invalid custom commands after all.

Though also, now that we have a state, do we even need a separate definition of "custom command keyword" or can we just state here

A string whose first two code points are U+0002D (-)

? That would simplify this even more.

The notes column is non-normative though right?

I'm not sure I understand the question in light of the suggestion. This is about the keyword column.

Oh I thought you meant to add the statement to the notes section.

In light of the current changes, where the keyword says A <span data-x="attr-button-command-custom">custom command keyword</span>, do you think it makes more sense to add the definition to the keywords table or to keep it as is?

You have to update the other references to "attr-button-command-custom". Given that there is a reference in the Indices section I'm inclined to say we have to keep this as-is. It could potentially be inlined like:

A custom command keyword, which is any string whose first two code points are U+002D (-)

but I'm not sure if that's an improvement. Might be something Domenic has an opinion about, but otherwise I'd leave it as-is (modulo updating the references to it; I noticed some casing issues and weird parenthesis).

source

annevk · 2024-11-27T07:52:20Z

source

+   <p>The following code shows how buttons can use commandfor to show and hide a popover box when
+   activated:</p>


You need to cross-reference these terms and use code appropriately. Across all examples. I've left this comment many times now. :-(

Sorry maybe I’m failing to understand what you mean by “use code”?

@annevk I've added the references, using <code> elements. Is this what you meant? I'll leave you to click Resolve conversation if you're happy with it.

I would also expect "button" to be referenced and maybe "popover", but I guess it's okay like this...

Okay, I've linked both buttons and popovers here.

annevk

Nice. This is getting close now. I'd really appreciate it if someone else could do one detailed read through as after this many takes I'm probably missing things. Maybe @domenic or @domfarolino?

source

domfarolino

Took a brief scan before going OOO for Thanksgiving, and mostly only came up with editorial things.

source

domfarolino · 2024-11-28T13:26:37Z

source

+    </ol>
+   </li>
+
+   <li><p>Otherwise, run the <span>popover target attribute activation behavior</span> given


I wonder if we could elevate this to handle this case first, and early return. That way all of the entire invoker-is-null steps could be dedented by one, and we can remove a whole level of nesting in this algorithm. That seems just a bit more readable (just a suggestion though).

This was an intentional editorial choice and an intentional choice in the code to keep these steps last to emphasise their order of precedence. Happy to change though.

source

domfarolino · 2024-11-28T13:36:41Z

source

+     <li><p>If <var>continue</var> is false, then return.</p></li>
+
+     <li><p>If <var>command</var> is in the <span
+     data-x="attr-button-command-custom-state">Custom</span> state, then return.</p></li>


I don't think it matters much, but what if the result of firing the event changed the button's command state to "Custom"? In that case, this algorithm would not early-return here because we're looking at a snapshot of <var>command</var>. I assume that's fine, and maybe even desirable, but I just want to make sure that's what we expect and ask if this definitely matches what implementations would do.

Yeah I believe it was deemed quite early on that scripting shouldn't change the type of gesture halfway through. I think there could be some interesting use cases for it (such as just-in-time determining if you want something as show-modal vs show-popover), but I am not convinced they're practically worthwhile in the face of potential problematic use cases.

Wait, command is the attribute, so it's not a snapshot. If you want a snapshot you need to do something else.

Oh I'd need attribute value? And presumably also attribute state or similar, so that we can capture and continue to test the state?

Regardless, it sounds like you don't want to compute the state on the fly like that (i.e., by grabbing its "state" (unliked)). It sounds like you want to do this early, by pulling it out sometime before we run script, so that we have the snapshot you were expecting. Right?

With the prose above I'm assuming that command holds the attribute value in which case it's grabbed early. It's only the attribute that's live (and which most of the existing infrastructure assumes is used).

Ah. I read #9841 (comment) as you saying the current prose grabs the live version of the attribute state/value, not a snapshot.

The current prose grabs the attribute, which is live by definition. If you grab the value instead you'll have to change how you inspect state, which is what the above wording is for.

Okay I've pushed up something I think works.

keithamus · 2024-11-28T16:40:30Z

Thanks for the review @domfarolino - I wonder if you'd be interested in another sweep given the changes to the if statements?

source

This was referenced Oct 8, 2023

"command" and "commandfor" attributes WebKit/standards-positions#264

Closed

Invokers (invoketarget & invokeaction) mozilla/standards-positions#902

Closed

keithamus force-pushed the add-invoke branch from c004fd4 to 0c56df8 Compare October 9, 2023 09:31

keithamus changed the title ~~Add invoke~~ Add InvokeElement & InvokeEvent IDLs for Invoke proposal Oct 9, 2023

keithamus changed the title ~~Add InvokeElement & InvokeEvent IDLs for Invoke proposal~~ Add InvokeElement & InvokeEvent IDLs & invocation steps for Invoke proposal Oct 9, 2023

Schweinepriester mentioned this pull request Oct 10, 2023

Add Invokers, currently invoketarget & invokeaction Fyrd/caniuse#6856

Open

keithamus force-pushed the add-invoke branch from 0c56df8 to 98adb9f Compare October 10, 2023 22:26

keithamus marked this pull request as ready for review October 10, 2023 22:28

keithamus force-pushed the add-invoke branch 3 times, most recently from 45e4032 to 9579336 Compare October 17, 2023 20:32

This comment was marked as outdated.

Sign in to view

keithamus force-pushed the add-invoke branch 2 times, most recently from 7f0f9ea to 834a8b1 Compare October 18, 2023 21:36

keithamus mentioned this pull request Oct 20, 2023

Mark invokers tests as non-tentative web-platform-tests/wpt#42664

Open

keithamus added a commit to keithamus/wpt that referenced this pull request Oct 20, 2023

Mark invokers tests as non-tentative

858bcd8

See related spec PR whatwg/html#9841

keithamus force-pushed the add-invoke branch from 834a8b1 to bd64801 Compare November 1, 2023 12:30

keithamus mentioned this pull request Nov 9, 2023

refactor Dialog to use <dialog> internally primer/view_components#2364

Merged

10 tasks

keithamus mentioned this pull request Jan 11, 2024

Refactor Dialog to use <dialog> internally primer/view_components#2496

Merged

10 tasks

mfreed7 reviewed Jan 17, 2024

View reviewed changes

introduce the "custom" state on command attribute table

767c4c3

annevk reviewed Nov 27, 2024

View reviewed changes

keithamus added 7 commits November 27, 2024 11:03

rename "valid custom command" to "custom command keyword"

a033ef8

drop redundant note

8e6b74e

use Custom state instead of isCustom bool

8d2a17b

title case all states

8e6ae5b

drop redundant check

d6536f1

properly reference commandfor in code examples

25d8c9b

remove uneccessary parens

eb62a52

annevk reviewed Nov 27, 2024

View reviewed changes

keithamus added 6 commits November 27, 2024 14:39

suggestions from recent code review

8621f0a

add definition to attr-button-command-custom

44a398d

link to buttons and popovers in example

0c1dd19

fix casing

e47a66c

make "custom command keyword" consistent

c1c8f42

consitently use span for "custom command keyword"

34e7de8

domfarolino reviewed Nov 28, 2024

View reviewed changes

keithamus added 6 commits November 28, 2024 16:32

use code not span

b5c402a

s/not false/true

8b6b4db

simplify the show/hidepopover command steps

15142b8

f code

397c490

comma

f07be5a

s/is the .*state/is in the .*state

224c61d

domfarolino reviewed Nov 28, 2024

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

keithamus added 3 commits November 28, 2024 17:59

more casing fixes

0ce6b7e

link example commandfor attribute and dialog

c71a1fb

rework state & attribute value to be a snapshot

2cf379a

domfarolino reviewed Nov 28, 2024

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

redo title casing

d2eb8df

		<td><dfn attr-value for="html-global/command"
		data-x="attr-button-command-custom"><code>(valid custom command)</code></dfn>

		<p>The following code shows how buttons can use commandfor to show and hide a popover box when
		activated:</p>

Add commandfor & command attributes to HTMLButtonElement #9841

Are you sure you want to change the base?

Add commandfor & command attributes to HTMLButtonElement #9841

Conversation

keithamus commented Oct 8, 2023 • edited by pr-preview bot Loading

scottaohara commented Oct 18, 2023 • edited Loading

This comment was marked as outdated.

lukewarlow commented Nov 4, 2023

domenic commented Nov 5, 2023

keithamus commented Nov 5, 2023

domenic commented Nov 5, 2023 • edited Loading

lukewarlow commented Dec 1, 2023

lukewarlow commented Dec 1, 2023

mfreed7 commented Jan 17, 2024

keithamus commented Jan 17, 2024

mfreed7 commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

domfarolino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithamus commented Nov 28, 2024

keithamus commented Oct 8, 2023 •

edited by pr-preview bot

Loading

scottaohara commented Oct 18, 2023 •

edited

Loading

domenic commented Nov 5, 2023 •

edited

Loading

annevk Nov 28, 2024 •

edited

Loading