FocusScope not working when used inside shadowRoot

[sventschui]

🐛 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]

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]

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]

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]

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

[padmkris123]

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]

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]

@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

react-spectrum/packages/@react-aria/interactions/src/useInteractOutside.ts

Line 116 in aa52464

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]

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]

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

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

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’ve made, only one document.activeElement and null means we’ve 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]

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.