Fixing button command early-bail steps #12021
Conversation
|
Good catch! |
|
Thanks! LGTM. |
source
Outdated
| <div algorithm> | ||
| <p>To <dfn data-x="determine-if-command-is-valid">determine if a command is valid for a | ||
| target</dfn> given a <code data-x="attr-button-command">command</code> attribute | ||
| <var>command</var> and an <code>HTMLElement</code> <var>target</var>:</p> |
There was a problem hiding this comment.
| <var>command</var> and an <code>HTMLElement</code> <var>target</var>:</p> | |
| <var>command</var> and an element <var>target</var>:</p> |
As far as I can tell it's not restricted to an HTML element.
There was a problem hiding this comment.
There was an assert in the other algorithm. But maybe that was an error?
There was a problem hiding this comment.
I think so. [CEReactions, Reflect] attribute Element? commandForElement; suggests to me anyway that any kind of element can be assigned here.
Perhaps this is lacking test coverage @keithamus?
There was a problem hiding this comment.
Custom commands work for non-HTML elements, previously there was an assert but importantly it was in a conditional.
If isPopover is false and command is not in the Custom state:
- Assert: target's namespace is the HTML namespace.
Custom commands never went down the branch of checking for valid commands, so it was fine.
There was a problem hiding this comment.
So this suggested change + my extra step below should fix this.
source
Outdated
| <li><p><span data-x="attr-button-command-hide-popover-state">Hide Popover</span></p></li> | ||
| </ul> | ||
|
|
||
| <p>Then return true.</p> |
There was a problem hiding this comment.
| <p>Then return true.</p> | |
| <p>then return true.</p> |
| <li><p>If this standard does not define <span>is valid command steps</span> for | ||
| <var>target</var>'s <span data-x="concept-element-local-name">local name</span>, then return | ||
| false.</p></li> |
There was a problem hiding this comment.
It seems there's a problem as these steps are restricted to HTML elements.
Also, it would have been better if we always defined these steps. They just return false unless overridden.
|
|
||
| <li><p>If <var>command</var> is in the <span | ||
| data-x="attr-button-command-custom-state">Custom</span> state, then return true.</p></li> | ||
|
|
There was a problem hiding this comment.
We need to add an extra step here (so it would become step 3) that says if target's namespace is not the HTML namespace then return false.
|
I think I've addressed the feedback. I've also made my Mozilla membership public, which should sort out the participation thing. The build failure seems unrelated. |
|
@annevk finally got the participation stuff sorted |
The previous spec seemed to allow all commands through if the element was a popover, whereas it was only intended to allow popover commands through.
Firefox and Safari follow the intent of the spec. Chrome kinda gets it wrong but wasn't following the previous wording of the spec either. I'll file a bug with them.
(See WHATWG Working Mode: Changes for more details.)
/form-elements.html ( diff )