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

feat(pointer): move focus in shadow DOM #1038

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

Conversation

ph-fritsche
Copy link
Member

@ph-fritsche ph-fritsche commented Aug 18, 2022

What:

Handle shadow hosts when moving focus per mousedown.

Why:

One step closer to full shadow DOM support #1026

How:

Check for element.shadowRoot.delegatesFocus on the ancestors. If true, find the first focusable element in its shadow tree.
Jsdom doesn't implement delegatesFocus. Treat undefined as true.

Checklist:

  • Tests
  • Ready to be merged

Additional information:

Work on top of #1033 - should be rebased once that is merged

Christian24 and others added 29 commits August 7, 2022 17:34
Co-authored-by: Philipp Fritsche <[email protected]>
…DOM in setup(). Removed old custom elements test
@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 18, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b322220:

Sandbox Source
userEvent-dom Configuration
userEvent-react Configuration

@ph-fritsche
Copy link
Member Author

@Christian24 Interestingly we'll need to apply a workaround both on Jsdom and the browser to handle delegatesFocus. Invoking shadowHost.focus() does not delegate the focus in nested shadow trees while the mousedown which we will use this for does delegate through multiple shadow hosts to the first focusable in the browser.

@Christian24
Copy link

This entire conversation has caused me into looking into focus handling in Shadow DOM in a different light. It is a mess. It seems to work oddly differently everywhere. How do you want to get this consistent? Also do you want to apply the workarounds to the library's tests or in user land? How do you see chances of jsdom implementing delegatesFocus?

A bit of context maybe as well: We use delegatesFocus mostly to get an application that can be easily navigated by tab.

@ph-fritsche
Copy link
Member Author

This entire conversation has caused me into looking into focus handling in Shadow DOM in a different light. It is a mess. It seems to work oddly differently everywhere.

That isn't exclusive to this feature. When you work on this library, you get to know a few quirks of browsers and frameworks you might not expect. ;)

How do you want to get this consistent? Also do you want to apply the workarounds to the library's tests or in user land?

It looks as if the behavior in the browser regarding the user interaction is consistent - according to MDN:

The delegatesFocus read-only property of the ShadowRoot interface returns true if the shadow root delegates focus, and false otherwise.
If true, when a non-focusable part of the shadow DOM is clicked, the first focusable part is given focus, and the shadow host is given any available :focus styling.

On delegatesFocus: true we need to find the focusable element and apply focus there so that the click behavior matches the behavior in the browser.
I think it makes sense to handle delegatesFocus: undefined as true. Users who want a different behavior in Jsdom will need to set the property themselves.

How do you see chances of jsdom implementing delegatesFocus?

I filed an issue. But even if it's implemented, I don't think it is relevant enough to be backported. So most users would still use versions without it.

A bit of context maybe as well: We use delegatesFocus mostly to get an application that can be easily navigated by tab.

How keyboard navigation handles shadow trees needs to be researched, but there might be no special handling, so our tab order helper would just need to consider all focusable elements no matter if they are in a shadow tree.

@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

Merging #1038 (b322220) into main (1aa2027) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1038   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           88        87    -1     
  Lines         2061      2091   +30     
  Branches       691       711   +20     
=========================================
+ Hits          2061      2091   +30     
Impacted Files Coverage Δ
src/utils/focus/getTabDestination.ts 100.00% <ø> (ø)
src/clipboard/copy.ts 100.00% <100.00%> (ø)
src/clipboard/cut.ts 100.00% <100.00%> (ø)
src/clipboard/paste.ts 100.00% <100.00%> (ø)
src/event/behavior/click.ts 100.00% <100.00%> (ø)
src/event/behavior/keydown.ts 100.00% <100.00%> (ø)
src/event/focus.ts 100.00% <100.00%> (ø)
src/event/selection/updateSelectionOnFocus.ts 100.00% <100.00%> (ø)
src/utils/focus/focusable.ts 100.00% <100.00%> (ø)
src/utils/focus/getActiveElement.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ph-fritsche
Copy link
Member Author

@ph-fritsche ph-fritsche marked this pull request as ready for review August 19, 2022 11:08
if (effectiveTarget) {
doFocus(effectiveTarget, true, element)
} else {
// This is not consistent across browsers if there is a focusable ancestor.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document this and related gotchas in the docs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants