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

Focus Management within Shadow DOM #6046

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

Conversation

MahmoudElsayad
Copy link

@MahmoudElsayad MahmoudElsayad commented Mar 11, 2024

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:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • 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 MahmoudElsayad marked this pull request as ready for review March 18, 2024 02:53
@ritz078
Copy link
Contributor

ritz078 commented Mar 18, 2024

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

@yihuiliao
Copy link
Collaborator

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.

@davidferguson
Copy link

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?

@MahmoudElsayad
Copy link
Author

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.

@davidferguson
Copy link

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?

@MahmoudElsayad
Copy link
Author

@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
Copy link
Author

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

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.

FocusScope not working when used inside shadowRoot
5 participants