react-spectrum
react-spectrum copied to clipboard
feat: Focus Management within Shadow DOM
Closes #1472
This PR enhances focus management capabilities in React Spectrum applications when used within Shadow DOM environments.
Changes
- Introduced a new utility function
getRootNode, designed to return a given element's contextually appropriate root (Document or ShadowRoot). This improves the library's ability to query and manipulate focus within shadow DOMs. - Added Shadow DOM support to the following:
FocusScopeuseFocususeFocusVisibleuseFocusWithinuseInteractionOutsideusePress
- Implemented a new utility function,
getRootBodythat determines the effective "body" element for an event's propagation path, supporting both Shadow DOM and traditional document structures. - implemented a new utility, ' getDeepActiveElement,` which retrieves the currently focused element across Shadow DOM boundaries, ensuring accurate focus management in complex DOM structures.
- Added tests for all new utility functions and affected hooks/components.
โ Pull Request Checklist:
- [x] Included link to corresponding React Spectrum GitHub Issue.
- [x] Added/updated unit tests and storybook for this change (for new code or code which already has tests).
- [x] Filled out test instructions.
- [ ] Updated documentation (if it already exists for this component).
- [ ] Looked at the Accessibility Practices for this feature - Aria Practices
๐ Test Instructions:
- Open any storybook example that uses any of the affected hooks/components
- Test the functionality and make sure it works when the story is encapsulated inside a shadow root.
// custom wrapper to run any story inside a shadow root
function ShadowDomWrapper({ children }) {
const container = useRef(null);
useEffect(() => {
const _container = container.current;
if (!_container) return;
const div = document.createElement('div');
_container.appendChild(div);
const shadowRoot = div.attachShadow({ mode: 'open' });
const main = document.createElement('main');
shadowRoot.appendChild(main);
// Wrap children with the ThemeProvider when rendering
ReactDOM.render(
children,
main
);
return () => {
ReactDOM.unmountComponentAtNode(main);
if (_container) {
_container.innerHTML = '';
}
};
}, [children]);
return <div ref={container} />;
}
// .storybook/preview.js
export const decorators = [
+ (Story) => (
+ <ShadowDomWrapper>
+ <Story />
+ </ShadowDomWrapper>
+ ),
withScrollingSwitcher,
...(process.env.NODE_ENV !== 'production' ? [withStrictModeSwitcher] : []),
withProviderSwitcher
];
๐งข Your Project:
PSPDFKit -
@snowystinger We are working on fixing the linting and type errors but if you want you can give an early eye to this PR.
You'll need to sign the Adobe CLA and then close and reopen this PR for the it to pass.
But also I'd like to let you know that we might be slow to review as we work through some other priorities and we apologize for the wait.
This is a great contribution, and it would be very useful for our team to see Shadow DOM support in React Spectrum.
I've had a play with it, and can see it works for some components, however it seems to not work with all overlays (AlertDialog, ContextualHelp, ActionMenu, Picker, etc.). There are a couple of issues, including that after closing the overlay, focus is not returned correctly to the element that triggered the open, and there also seems to be some state bug too where it thinks the overlay is still open.
See this screen recording for an example with Picker - you will see that focus is not returned after choosing a value, and the Picker trigger still looks "pressed":
I tried modifying useRestoreFocus of FocusScope.tsx to set const nodeToRestoreRef to be getDeepActiveElement(), and this fixed the issue of focus not being returned, however doesn't solve the state issue. In the following screen capture, you'll see that the focus ring is returned as it should be, however when I tab off the Picker, it still looks "pressed", and clicking on it to open it has no effect. I need to focus onto something else and hit enter to restore the state correctly:
I've not had too much of a dig around, but I can see there are quite a lot of uses of .activeElement in React Spectrum (I count 63 uses across 27 files, 51 of which are document.activeElement). I wonder if some/all of these also need to be updated to get the actual active element, rather than assuming it's the document's active element?
Thanks, @davidferguson, for looking at the PR; what you mentioned is really helpful, and finding the correct activeElement will, to some extent, solve most of the issues.
I will try to invest some time soon to fix the overlays raised issues and the state issue; most likely the overlay issues will share the same root cause.
Hi @MahmoudElsayad, I did spend some time looking into this, and fixing it for the use cases that we needed.
I'm attaching a patch file in the hope it may be useful for you. This is based off 3.34.1, so not the current main, or your PR, but it should be similar to apply. It makes overlays (menus, dialogs) and toasts and landmarks functional for our use case.
@yihuiliao and @snowystinger, is there a preferred approach of how you'd like to see webcomponent & React Spectrum support progress? Is working on small individual fixes like @MahmoudElsayad's work, and now my overlay work the way forward, or are you hoping for a more holistic, comprehensive approach that updates the entire library at once?
@davidferguson Thank you for the patch! I think the way to go would be in small individual fixes; it will be great if you open a PR for the patch changes as well, and you can branch off this PR if you find any useful utility that can be used and that way, we add support incrementally for shadow DOM.
@yihuiliao and @snowystinger, Your feedback would be greatly appreciated to move things forward.
@davidferguson One issue related to overlays, which I don't know if you have encountered yet or not, and that arose from accessibility testing, is that the ariaHideOutside function sets aria-hidden="true" on the document body, which is incorrectly inherited by elements within the shadow root making all overlays inherit aria-hidden="true".
It's been handled in #6133; I am working on it getting the PR ready for review as well.
@snowystinger @yihuiliao Can you have a look at this PR and suggest if there is something more that needs to be done ?
We've had some other priorities. How ready is this PR for a review? I'll try to find some time this week to look at it again.
It looks like there are some tests failing, do you need assistance figuring them out?
@snowystinger I fixed the failing tests in the last commit.
@snowystinger Sorry, the last commit doesn't handle versions 16 and 17, so I am adjusting the fix slightly; I am working on it now.
@snowystinger They are fixed now.
GET_BUILD
Build successful! ๐
## API Changes
unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any', access: 'private' } unknown top level export { type: 'any', access: 'private' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'identifier', name: 'Column' } unknown top level export { type: 'identifier', name: 'Column' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' }
@react-aria/focus
getFocusableTreeWalker
getFocusableTreeWalker {
- root: Element
+ root: Element | ShadowRoot
opts?: FocusManagerOptions
scope?: Array<Element>
returnVal: undefined
}
@react-aria/utils
useFormReset
-
+getRootNode {
+ el: Element | null | undefined
+ returnVal: undefined
+}
getRootNode
-
+getRootBody {
+ root: Document | ShadowRoot
+ returnVal: undefined
+}
getRootBody
-
+getDeepActiveElement {
+ returnVal: undefined
+}
@snowystinger I extended the tree walker to handle traversal between multiple shadow DOMs by dynamically detecting and navigating to focusable elements in adjacent shadow roots. Please feel free to recheck the PR changes.
Thanks, just a heads up, but the team is going on break. We'll be back in about a week. Until then it's unlikely we will look at this.
@snowystinger Any update on this ?
Thanks for the ping, I've not forgotten, I just haven't had a big enough solid chunk of time to dive back into this yet.
@snowystinger The failing tests didn't work as soon as I got a pull from the latest master before committing any updates to the PR. Is that expected?
In general, no, but I took a look, and the failures appear to all be testing the stuff you're working on. So I'd double-check your merge or that something on main didn't conflict with the changes you're trying to make. We are working on a lot of things, I cannot guarantee everything we do on main will be compatible with what you're doing. This PR is very ambitious.
I'll have a look at your latest changes soon.
Thanks for taking the time to look into this. I appreciate your efforts in reviewing it thoroughly ๐๐ผ. I understand it is a substantial change and can conflict with many things. I'll look carefully at potential conflicts with recent updates to the main branch; earlier, I wanted to confirm that it was unexpected.
@snowystinger The tests failed because the previous implementation used instanceof before your recommendation for changes here. instanceof checks fail across contexts due to different global constructors, by switching to nodeType checks, which are consistent across contexts, we ensure that the function accurately identifies Document and ShadowRoot nodes regardless of their origin.