Fixing button command early-bail steps

[jakearchibald]

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. Edit: here it is https://issues.chromium.org/issues/469115770

• At least two implementers are interested (and none opposed):
  • Firefox
  • WebKit
• Tests are written and can be reviewed and commented upon at:
  • Test known commands with invalid commandfor targets web-platform-tests/wpt#57032
• Implementation bugs are filed:
  • Chromium: https://issues.chromium.org/issues/469115770
  • Gecko: N/A
  • WebKit: N/A
• Corresponding HTML AAM & ARIA in HTML issues & PRs: N/A
• MDN issue is filed: N/A
• The top of this comment includes a clear commit message to use.

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

---

/form-elements.html ( diff )

[keithamus]

Nice. LGTM.

[lukewarlow]

Good catch!

[mfreed7]

Thanks! LGTM.

[jakearchibald]

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.

[jakearchibald]

@annevk finally got the participation stuff sorted

[annevk]

Looks good modulo minor nit. Thanks!

[annevk]

Is this covered by existing tests? I filled out OP for everything else (next time please update it in a similar manner as the PR progresses).

[lukewarlow]

Chromium doesn't seem to be failing any WPTs relevant to this so this will need some additional tests.

[lukewarlow]

I'm working on some tests for this and it turns out there's some bugs in Firefox and WebKit too.

Somehow all browsers are allowing show-popover commands to fire on non-popover elements too.

[lukewarlow]

web-platform-tests/wpt#57032 - this should provide test coverage.

Some of these fail in all 3 browsers so I will raise a bug for webkit and firefox too. (Please let me know if you think I've made a mistake in any of these tests)

[lukewarlow]

Looks like the chrome behaviour is a recent regression. The additional test coverage should prevent that extra weird behaviour.

One behaviour does exist across all 3 engines and is different from the old and new spec definitions that I want to flag just to make sure we all agree on a direction.

Currently show-popover, toggle-popover and hide-popover all fire events even if an element isn't a popover. This allows authors to dynamically make the target a popover within the command event handler. See https://jsfiddle.net/5y0q81jb/

If we want to keep this behaviour, we'd need to update determine if a command is valid for a target step 4 to remove the popover attribute check.

cc @keithamus and @mfreed7 in case either of them can provide context for where this spec vs implementation discrepency might have come from.

I'm agnostic, happy to update WebKit to match the spec here. Also happy to update my tests PR to allow this behaviour in which case only chrome needs fixing.

[keithamus]

I'm sure we determined at some point that the heuristic for valid commands should remain simple enough where each element essentially has an allow list; not to introduce more complex heuristics such as the presence of an attribute; as that might be too intuit as the complexity of the commands grows (for example we could prevent show-popover working if the popover is open, or close not working if the dialog isn't open). Of course this means that, given any element can have a popover attribute, any element can fire the event.

[jakearchibald]

@lukewarlow thanks for looking into the tests

[jakearchibald]

@keithamus any documentation of that decision? Or are you confident enough in it? I can update the PR.

[keithamus]

I'd have to dig to find out if it was ever documented anywhere. Having said that, the implementations exhibit that kind of behaviour, so this PR as written would be a change. So I think we're at a fork in the road anyway. Disregarding any prior conversations or design intent I think we can either:

1. Accept that the implementations just check based on the element and nothing further (e.g. attribute presence). We can encode the principle from this point forward with a well documented note in the spec.
2. Accept the changes to the spec as written in this PR now, and update implementations to include the additional heuristics, and likely follow up with documentation on MDN that describes this.

[jakearchibald]

I don't have strong feelings either way.

@mfreed7 what do you think? The simplicity with the popover stuff is compelling. And it's what browsers are already doing.

Note this only impacts popovers. The bug I filed is still a bug.