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

Query shadow dom focusables #33

Merged
merged 3 commits into from
May 1, 2022

Conversation

vbabel
Copy link

@vbabel vbabel commented Mar 28, 2022

This PR aims to further improve support for focusable elements within shadow doms and nested shadow doms.

This primary changes are:

  • Searches shadow DOMs and nested shadow DOMs in getFocusables, previously this method would not search within shadow doms for focusable elements. This modification adds the shadow focusable elements in DOM order (depth first). This allows focusNextElement and focusPrevElement to appropriately pass focus to shadow DOMs or custom elements within a focus lock.
  • Adds contains to DOMUtils, used to determine whether an element is contained within another, including any nested shadow DOMs, this allows focusInside and focusIsHidden to correctly report when the focused element is within a shadow DOM. Previously, getActiveElement was added to get the active element from within a Shadow DOM, but the shadow dom was not being traversed when looking for that element (because Node.contains() does not check shadow doms).

It should be noted that this does add or fix support for marking shadow dom elements with any of the data- attributes found in the constants, used to control the behavior of the focus lock. This is outside of our use case for the focus lock, our team uses react-focus-lock (in preact), but consumes web components from our organizations design system that we need to be able to focus when within a focus lock.

This should fix theKashey/react-focus-lock#206

Victor Babel added 2 commits March 28, 2022 10:43
Search shadow roots for focusable elements in `getFocusables`
Search shadow roots for element containment in `getRelativeFocusable`
@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #33 (70ec30d) into master (1fe68ec) will increase coverage by 1.44%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   82.26%   83.70%   +1.44%     
==========================================
  Files          21       21              
  Lines         344      356      +12     
  Branches       68       73       +5     
==========================================
+ Hits          283      298      +15     
+ Misses         55       54       -1     
+ Partials        6        4       -2     
Impacted Files Coverage Δ
src/utils/tabUtils.ts 89.47% <75.00%> (+1.97%) ⬆️
src/focusInside.ts 91.66% <100.00%> (ø)
src/focusIsHidden.ts 77.77% <100.00%> (+27.77%) ⬆️
src/sibling.ts 87.50% <100.00%> (ø)
src/utils/DOMutils.ts 100.00% <100.00%> (ø)
src/utils/getActiveElement.ts 100.00% <100.00%> (+50.00%) ⬆️
src/utils/parenting.ts 96.87% <100.00%> (-3.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fe68ec...70ec30d. Read the comment docs.

@vbabel
Copy link
Author

vbabel commented Mar 31, 2022

https://github.com/theKashey/react-focus-lock/blob/master/src/Trap.js#L124 - This would need to be updated to use getActiveElement from this repository in order to support cycling when shadow dom elements are either the first or last focusable in a trap.

@theKashey theKashey self-requested a review April 1, 2022 06:39
(result, node) => result || node.contains(activeElement) || focusInsideIframe(node),
false as boolean
);
return getAllAffectedNodes(topNode).some((node) => contains(node, activeElement) || focusInsideIframe(node));
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@theKashey
Copy link
Owner

Still on it....

@theKashey theKashey merged commit 018b587 into theKashey:master May 1, 2022
@theKashey
Copy link
Owner

All focus-* family have got an update.

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.

Cannot focus elements in ShadowRoot mode open
2 participants