Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nt1m opened this issue Mar 8, 2023 · 6 comments · Fixed by #8998
Closed

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

nt1m opened this issue Mar 8, 2023 · 6 comments · Fixed by #8998
Labels
topic: focus topic: popover The popover attribute and friends

Comments

@nt1m
Copy link
Member

nt1m commented Mar 8, 2023

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 nt1m added the topic: popover The popover attribute and friends label Mar 8, 2023
@nt1m
Copy link
Member Author

nt1m commented Mar 8, 2023

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
Copy link
Member Author

nt1m commented Mar 8, 2023

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
Copy link
Member Author

nt1m commented Mar 8, 2023

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
Copy link
Member Author

nt1m commented Mar 8, 2023

#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
Copy link
Contributor

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
Copy link
Contributor

I wrote some WPTs for this behavior: web-platform-tests/wpt#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

nt1m added a commit to nt1m/html that referenced this issue Jun 7, 2023
annevk pushed a commit that referenced this issue Jun 8, 2023
Refer to the autofocus delegate directly and add an extra step which focuses the popover itself if it has the autofocus attribute.

Fixes #8994.
rubberyuzu pushed a commit to rubberyuzu/html that referenced this issue Jul 20, 2023
Refer to the autofocus delegate directly and add an extra step which focuses the popover itself if it has the autofocus attribute.

Fixes whatwg#8994.
rubberyuzu pushed a commit to rubberyuzu/html that referenced this issue Jul 21, 2023
Refer to the autofocus delegate directly and add an extra step which focuses the popover itself if it has the autofocus attribute.

Fixes whatwg#8994.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: focus topic: popover The popover attribute and friends
Development

Successfully merging a pull request may close this issue.

3 participants