feat(keyboard): support shadow DOM in tab order #1040
Conversation
Co-authored-by: Philipp Fritsche <[email protected]>
…DOM in setup(). Removed old custom elements test
Co-authored-by: Philipp Fritsche <[email protected]>
Co-authored-by: Philipp Fritsche <[email protected]>
Co-authored-by: Philipp Fritsche <[email protected]>
|
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 7756374:
|
|
Changes to be reviewed here: |
Codecov Report
@@ Coverage Diff @@
## main #1040 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 88 87 -1
Lines 2061 2099 +38
Branches 691 713 +22
=========================================
+ Hits 2061 2099 +38
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@Christian24 This should complete support for focus handling on shadow DOM |
|
Thanks @ph-fritsche! Really looking forward to seeing this in the next release. |
There was a problem hiding this comment.
I was thinking what I could do to help this along, so here is some very minor feedback @ph-fritsche :)
| * as it might delegate focus into a shadow tree. | ||
| */ | ||
| export function isFocusTarget(element: Element): element is HTMLElement { | ||
| if (element.tagName.includes('-')) { |
There was a problem hiding this comment.
I dunno but I think having this a separate helper function might be helpful. And makes it more clear as well.
|
|
||
| export function isFocusable(element: Element): element is HTMLElement { | ||
| return ( | ||
| !element.tagName.includes('-') && |
| // see https://github.com/jsdom/jsdom/issues/3418 | ||
| // We'll treat `undefined` as `true` | ||
| return ( | ||
| !!element.shadowRoot && |
There was a problem hiding this comment.
wouldn't element.shadowRoot be sufficient here? The !! shouldn't make a difference right?
| if (isFocusable(el)) { | ||
| return el | ||
| } else if (el.shadowRoot) { | ||
| const f = findFocusable(el.shadowRoot) |
There was a problem hiding this comment.
I would prefer a more describing name than f. Maybe focusable or focusableEl?
|
@ph-fritsche @Christian24 is there anything that can be done to get this, #1038 and #1033 back on the table? I would be glad to help or find someone from my team who can. |
|
I would love to get this merged. I think @ph-fritsche wanted to finish work on #1019 first though. I could potentially use this for something else I have been working on myself too. I am not a maintainer here though, so I cannot really comment on their goals. |
What:
Consider focusable elements in shadow trees.
Why:
#1026
Checklist:
on top of #1038 -- should be rebased once that is merged