search

whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.01k stars 2.62k forks source link

WPT testing non-standard behavior for popover focusing steps #8994

Closed nt1m closed 1 year ago

nt1m commented 1 year ago

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;drc=74c5b3d45c81fcc9c5cd86213c8c265335e6e1a6;l=1799

The ternary checking if the popover is autofocusable is not specified in the spec.

Is this intentional behavior? If so, should it be specified or should the implementation+WPT be adapted to match?

The relevant WPT is: html/semantics/popovers/popover-focus.html

cc @mfreed7 @josepharhar @annevk

nt1m commented 1 year ago

So I looked a bit into this, I think this was trying to match the updated dialog focusing steps, notably this step:

If isModal is true and subject has the autofocus attribute, then set control to subject.

I don't think we should follow the same pattern as dialog here. This is a step specific to modal dialogs, because they render the rest of the page inert, so it isn't relevant to popovers.

cc @scottaohara @domenic as well for opinions.

nt1m commented 1 year ago

Ah, seems like Chromium doesn't use the focus delegate, but rather the autofocus delegate, in which case the ternary makes slightly more sense?

Still though, the spec says to use the focus delegate, not the autofocus delegate.

nt1m commented 1 year ago

Ah ok, I see the "given other and true", the "and true" part refers to the autofocusOnly argument. Not sure why we can't refer to the autofocus delegate directly though.

nt1m commented 1 year ago

https://github.com/whatwg/html/pull/8998 cleans this up a bit.

It refers to the autofocus delegate directly (and adapts the definition a bit to match), and adds the extra step that Chromium does to matches to target the popover itself it has the autofocus attribute.

josepharhar commented 1 year ago

I tried using autofocus delegate instead of focus delegate in chromium and it didn't make any tests fail, so I'm generally supportive. I am working on additional tests for shadowdom.

I think that using autofocus delegate may also make it easier to account for nested autofocus popovers/dialogs which I implemented but haven't specced yet.

josepharhar commented 1 year ago

I wrote some WPTs for this behavior: https://github.com/web-platform-tests/wpt/pull/38994 If we set focus target to the shadow root in the case I mentioned in the PR, then I am supportive of doing this