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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

FocusScope not working when used inside shadowRoot #1472

Open
sventschui opened this issue Jan 15, 2021 · 12 comments 路 May be fixed by #6046
Open

FocusScope not working when used inside shadowRoot #1472

sventschui opened this issue Jan 15, 2021 · 12 comments 路 May be fixed by #6046

Comments

@sventschui
Copy link

馃悰 Bug Report

When used inside a shadowRoot the FocusScope currently has some issues:

  • Focused element will not be determined correctly as document.activeElement will refer to the parent node of the shadowRoot the currently focused element is in. The activeElement needs to be determined recursively traversing (open) shadowRoots (document.activeElement.shadowRoot.activeElement.shadowRoot.activeElement......). This leads to invalid detection whether an element is in scope and also restores focus wrongly.
  • e.target in focus/blur events is referring to the parent node of the shadowRoot of the focused element. e.composedPath()[0] can be used to determine the actually focused element inside the custom element. ('composedPath' in e ? e.composedPath()[0] : e.target)
  • Chrome: TypeError: Cannot read property 'focus' of null in the onBlur of useFocusContainment due to e.target being null inside the rAF (not sure this is really related to custom element / shadowRoot or a general bug
  • Safari: Focus is looped through focusable elements

I patched this in our libs using patch-package so I'm happy to file a PR if you'd like to support custom elements. I'm not sure how this affects other packages of the spectrum ecosystem and thus wanted to check in prior to opening a PR.

馃 Expected Behavior

FocusRing can be used with custom elements / shadowRoots

馃槸 Current Behavior

See Bug Report above

馃拋 Possible Solution

  • Instead of document.activeElement use activeElement()
function activeElement() {
    let activeElement = document.activeElement;
    while (activeElement && activeElement.shadowRoot && activeElement.shadowRoot.activeElement) {
        activeElement = activeElement.shadowRoot.activeElement;
    }
    return activeElement;
}
  • Instead of e.target use 'composedPath' in e ? e.composedPath()[0] : e.target

馃敠 Context

In our Microfrontend Framework we encapsulate modules inside custom elements. To escape them we use a portal-root custom element in the body where all modals will be rendered into. Inside this we use FocusScope to contain focus.

馃捇 Code Sample

https://codesandbox.io/s/vigilant-hofstadter-3wf4i?file=/src/index.js

馃實 Your Environment

Software Version(s)
@react-aria/focus 3.2.3
Browser Chrome 87.0.4280.141 / Safari 14.0.2
Operating System Mac OS 10.15

馃Б Your Company/Team

Undisclosed

馃暦 Tracking Issue (optional)

n/a

@devongovett
Copy link
Member

Shadow DOM support is definitely something we're interested in. I'm sure quite a few things are broken at the moment beyond just FocusScope. If you're interested in contributing, that would be amazing!

A few other places I can think of that will need work:

  • Any other place we use document.activeElement
  • Places where we use Node#contains, querySelector, TreeWalker, or other DOM traversal functions
  • event.target usages

There are probably others as well.

In general, is it safe to assume that we can traverse into a shadow root though? What about closed shadow roots? Is it ok to not support those you think? Or assume that the closed shadow roots will forward focus internally as necessary, for example?

@nouman91
Copy link

I love this project, I will contribute to adding support for shadow DOM. Any pointers on what should I keep in mind while working on it?

@snowystinger
Copy link
Member

snowystinger commented May 24, 2021

Thank you! I think just the things @devongovett mentioned in the previous comment to start. Otherwise, make sure to check out the contribution page on our docs site https://react-spectrum.adobe.com/contribute.html

@nouman91
Copy link

Ok sure, I will check the contribution guide and work on the initial RFC.

@padmkris123
Copy link

I would love to use this fix as well. I too need this for implementing micro-frontends in our application. Is there any pending work that needs an extra helping hand?

@snowystinger
Copy link
Member

If you want to have a look at it, that'd be a great help! We don't have any current work in the team going on for it and I don't think we've heard from the other people in this thread in a long while.

@snowystinger
Copy link
Member

@theomessin
Sorry for the delay; we've been in release prep mode. During our grooming session, we discussed this PR and what we're hoping for in the future of shadowDOM support. The team is a bit worried; we think this needs to be considered as a general approach for the whole library, and it is an area where we lack expertise. We'd be looking for an overview of what needs to be done to support ShadowDOM so that we aren't just reactive. For instance, this PR has done a good job of fixing one particular issue. However, a few lines up is another call to element.contains

if (!ownerDocument || !ownerDocument.documentElement.contains(event.target)) {
which we think would lead to Toasts and the top layer not working.

If this is something you would be interested in, we would be open to discussing how you think a shadow dom solution in React Aria would work before committing any individual patches.

@josephmarkus
Copy link

I ran into the same issue where keyboard focus doesn't work when an app that uses { useFocusRing } from "@react-aria/focus" is rendered in ShadowDOM

@joealden
Copy link

@snowystinger I know that the scope of react-aria is much larger than https://github.com/downshift-js/downshift, but their approach to this is an environment prop, which allows the caller to pass their own values/functions to be used. For reference, here is their type definition at the time of writing:

https://github.com/downshift-js/downshift/blob/a5d6310281e50e4fe91037de77f99d67955e7700/typings/index.d.ts#L83-L88

I'm wondering if a similar approach could be followed, but maybe using context instead (similar to RouterProvider), as that way it wouldn't have to be passed to individual hooks/components, and likely provides a better DX for your team (as a default "environment" could be the default context value, and use that instead of any direct window or document calls).

Just an idea - I don't know my way around this codebase, so maybe there are problems with following a similar pattern?

@ritz078
Copy link
Contributor

ritz078 commented Feb 12, 2024

@snowystinger Shouldn't this be easier now that we already support dynamic iframe starting next release ? I guess the change needed will be mostly in getOwnerDocument/getOwnerWindow?

@snowystinger
Copy link
Member

Possibly? I do not know much about ShadowDOM or ShadowRoot.
I imagine there are other issues as well, but might be able to support just the FocusScope.

Here's a list of things we identified as potentially problematic with iframes. I'm not sure if it holds for ShadowDom as well.
useTable -> useDescription (no access to a dom node)
FocsuScope -> focusSafely -> runAfterTransition (global transition event listeners)
useMenuItem/useOption -> useSlotId (no access to a dom node)
useGrid -> useSelectableCollection -> scrollIntoView (maths might be hard, especially if the iframe is out of view)
useTable -> LiveAnnouncer (appends element to document, no access to a dom node)
useCombobox -> ariaHideOutside (unable to watch in other iframes or at application root)
Some other assumptions we鈥檝e made, only one document.activeElement and null means we鈥檝e lost focus to the body. We know the dom order for moving focus around.
disable/restoreTextSelection used by usePress and useMove

The biggest help for support would be unit tests demonstrating real life use cases and expected behaviors. Or an example minimal app. Something so we can discuss the approach here. If it's just FocusScope, we can also determine how much would be required to support it more easily this way.

@ritz078
Copy link
Contributor

ritz078 commented Feb 16, 2024

Thanks for the list. This is very helpful. We will be soon implementing react-aria inside shadow DOM so we will be able to look into this more.

One good starting point will be to support iframes for all react aria hooks. It will make sure that the document extraction logic is at the same place, i.e getOwnerDocument. This will make it easier to support shadow root.

Do you see any problem in replacing all instances of window and document in react-aria with getOwnerWindow and getOwnerDocument ? It is only implemented in only selected components right now and I don't see a problem with enabling the lint rule across react-aria packages. It shouldn't break anything in theory.

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 a pull request may close this issue.

8 participants