react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

feat: Focus Management within Shadow DOM

Open MahmoudElsayad opened this issue 1 year ago โ€ข 29 comments

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:
    • FocusScope
    • useFocus
    • useFocusVisible
    • useFocusWithin
    • useInteractionOutside
    • usePress
  • Implemented a new utility function, getRootBody that 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 -

MahmoudElsayad avatar Mar 11 '24 21:03 MahmoudElsayad

@snowystinger We are working on fixing the linting and type errors but if you want you can give an early eye to this PR.

ritz078 avatar Mar 18 '24 11:03 ritz078

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.

yihuiliao avatar Mar 20 '24 23:03 yihuiliao

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":

ezgif-2-6a42c41408

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:

ezgif-2-4480c9e82b

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?

davidferguson avatar Apr 30 '24 17:04 davidferguson

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.

MahmoudElsayad avatar May 03 '24 01:05 MahmoudElsayad

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.

overlay-toast-fixes.patch.txt

@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 avatar May 03 '24 19:05 davidferguson

@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.

MahmoudElsayad avatar May 07 '24 02:05 MahmoudElsayad

@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.

MahmoudElsayad avatar May 09 '24 14:05 MahmoudElsayad

@snowystinger @yihuiliao Can you have a look at this PR and suggest if there is something more that needs to be done ?

ritz078 avatar May 21 '24 09:05 ritz078

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.

snowystinger avatar May 21 '24 10:05 snowystinger

It looks like there are some tests failing, do you need assistance figuring them out?

snowystinger avatar May 21 '24 21:05 snowystinger

@snowystinger I fixed the failing tests in the last commit.

MahmoudElsayad avatar May 21 '24 22:05 MahmoudElsayad

@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.

MahmoudElsayad avatar May 21 '24 22:05 MahmoudElsayad

@snowystinger They are fixed now.

MahmoudElsayad avatar May 22 '24 01:05 MahmoudElsayad

GET_BUILD

snowystinger avatar May 23 '24 07:05 snowystinger

## 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
+}

rspbot avatar May 23 '24 07:05 rspbot

@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.

MahmoudElsayad avatar Jun 28 '24 02:06 MahmoudElsayad

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 avatar Jun 28 '24 06:06 snowystinger

@snowystinger Any update on this ?

ritz078 avatar Jul 12 '24 08:07 ritz078

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 avatar Jul 16 '24 04:07 snowystinger

@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?

MahmoudElsayad avatar Aug 14 '24 03:08 MahmoudElsayad

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.

snowystinger avatar Aug 14 '24 03:08 snowystinger

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.

MahmoudElsayad avatar Aug 14 '24 03:08 MahmoudElsayad

@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.

MahmoudElsayad avatar Sep 26 '24 03:09 MahmoudElsayad