Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -297,5 +297,8 @@
},
"locales": [
"en-US"
]
],
"dependencies": {
"shadow-dom-testing-library": "^1.13.1"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: All dependencies within this package.json are devDependencies. I see no reason why shadow-dom-testing-library would be any different.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a mistake, I'll move it to dev deps.

}
}
9 changes: 5 additions & 4 deletions packages/@react-aria/focus/src/FocusScope.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
isChrome,
isFocusable,
isTabbable,
nodeContains,
ShadowTreeWalker,
useLayoutEffect
} from '@react-aria/utils';
Expand Down Expand Up @@ -440,7 +441,7 @@ function isElementInScope(element?: Element | null, scope?: Element[] | null) {
if (!scope) {
return false;
}
return scope.some(node => node.contains(element));
return scope.some(node => nodeContains(node, element));
}

function isElementInChildScope(element: Element, scope: ScopeRef = null) {
Expand Down Expand Up @@ -771,7 +772,7 @@ export function getFocusableTreeWalker(root: Element, opts?: FocusManagerOptions
{
acceptNode(node) {
// Skip nodes inside the starting node.
if (opts?.from?.contains(node)) {
if (opts?.from && nodeContains(opts.from, node)) {
return NodeFilter.FILTER_REJECT;
}

Expand Down Expand Up @@ -822,7 +823,7 @@ export function createFocusManager(ref: RefObject<Element | null>, defaultOption
let {from, tabbable = defaultOptions.tabbable, wrap = defaultOptions.wrap, accept = defaultOptions.accept} = opts;
let node = from || getActiveElement(getOwnerDocument(root));
let walker = getFocusableTreeWalker(root, {tabbable, accept});
if (root.contains(node)) {
if (nodeContains(root, node)) {
walker.currentNode = node!;
}
let nextNode = walker.nextNode() as FocusableElement;
Expand All @@ -843,7 +844,7 @@ export function createFocusManager(ref: RefObject<Element | null>, defaultOption
let {from, tabbable = defaultOptions.tabbable, wrap = defaultOptions.wrap, accept = defaultOptions.accept} = opts;
let node = from || getActiveElement(getOwnerDocument(root));
let walker = getFocusableTreeWalker(root, {tabbable, accept});
if (root.contains(node)) {
if (nodeContains(root, node)) {
walker.currentNode = node!;
} else {
let next = last(walker);
Expand Down
6 changes: 3 additions & 3 deletions packages/@react-aria/interactions/src/useFocusWithin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ export function useFocusWithin(props: FocusWithinProps): FocusWithinResult {

let onBlur = useCallback((e: FocusEvent) => {
// Ignore events bubbling through portals.
if (!e.currentTarget.contains(e.target)) {
if (!nodeContains(e.currentTarget as Element, e.target as Element)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (repeated): The fact that you have to add type assert highlights that nodeContains does not accept the optimal set of types. Requiring type assertions in order to use a function erodes trust that the function is being used correctly.

suggestion: While Adobe hasn't gotten back to either of us since we restarted this work last week on whether they'd prefer a targeted approach or a universal approach, I can say that the changes I made to DomFunctions.ts are an improvement.

return;
}

// We don't want to trigger onBlurWithin and then immediately onFocusWithin again
// when moving focus inside the element. Only trigger if the currentTarget doesn't
// include the relatedTarget (where focus is moving).
if (state.current.isFocusWithin && !(e.currentTarget as Element).contains(e.relatedTarget as Element)) {
if (state.current.isFocusWithin && !nodeContains(e.currentTarget as Element, e.relatedTarget as Element)) {
state.current.isFocusWithin = false;
removeAllGlobalListeners();

Expand All @@ -78,7 +78,7 @@ export function useFocusWithin(props: FocusWithinProps): FocusWithinResult {
let onSyntheticFocus = useSyntheticBlurEvent(onBlur);
let onFocus = useCallback((e: FocusEvent) => {
// Ignore events bubbling through portals.
if (!e.currentTarget.contains(e.target)) {
if (!nodeContains(e.currentTarget as Element, e.target as Element)) {
return;
}

Expand Down
4 changes: 2 additions & 2 deletions packages/@react-aria/interactions/src/useInteractOutside.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// NOTICE file in the root directory of this source tree.
// See https://github.com/facebook/react/tree/cc7c1aece46a6b69b41958d731e0fd27c94bfc6c/packages/react-interactions

import {getOwnerDocument, useEffectEvent} from '@react-aria/utils';
import {getOwnerDocument, nodeContains, useEffectEvent} from '@react-aria/utils';
import {RefObject} from '@react-types/shared';
import {useEffect, useRef} from 'react';

Expand Down Expand Up @@ -121,7 +121,7 @@ function isValidEvent(event, ref) {
if (event.target) {
// if the event target is no longer in the document, ignore
const ownerDocument = event.target.ownerDocument;
if (!ownerDocument || !ownerDocument.documentElement.contains(event.target)) {
if (!ownerDocument || !nodeContains(ownerDocument.documentElement, event.target)) {
return false;
}
// If the target is within a top layer element (e.g. toasts), ignore.
Expand Down
13 changes: 8 additions & 5 deletions packages/@react-aria/overlays/src/ariaHideOutside.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* governing permissions and limitations under the License.
*/

import {getOwnerWindow} from '@react-aria/utils';
import {createShadowTreeWalker, getOwnerDocument, getOwnerWindow, nodeContains} from '@react-aria/utils';
const supportsInert = typeof HTMLElement !== 'undefined' && 'inert' in HTMLElement.prototype;

interface AriaHideOutsideOptions {
Expand Down Expand Up @@ -85,16 +85,19 @@ export function ariaHideOutside(targets: Element[], options?: AriaHideOutsideOpt

// Skip this node but continue to children if one of the targets is inside the node.
for (let target of visibleNodes) {
if (node.contains(target)) {
if (nodeContains(node, target)) {
return NodeFilter.FILTER_SKIP;
}
}

return NodeFilter.FILTER_ACCEPT;
};

let walker = document.createTreeWalker(
root,
let rootElement = root?.nodeType === Node.ELEMENT_NODE ? (root as Element) : null;
let doc = getOwnerDocument(rootElement);
let walker = createShadowTreeWalker(
doc,
root || doc,
NodeFilter.SHOW_ELEMENT,
{acceptNode}
);
Expand Down Expand Up @@ -147,7 +150,7 @@ export function ariaHideOutside(targets: Element[], options?: AriaHideOutsideOpt

// If the parent element of the added nodes is not within one of the targets,
// and not already inside a hidden node, hide all of the new children.
if (![...visibleNodes, ...hiddenNodes].some(node => node.contains(change.target))) {
if (![...visibleNodes, ...hiddenNodes].some(node => nodeContains(node, change.target as Element))) {
for (let node of change.addedNodes) {
if (
(node instanceof HTMLElement || node instanceof SVGElement) &&
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-aria/utils/src/shadowdom/DOMFunctions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export const getActiveElement = (doc: Document = document): Element | null => {
* ShadowDOM safe version of event.target.
*/
export function getEventTarget<T extends Event>(event: T): Element {
if (shadowDOM() && (event.target as HTMLElement).shadowRoot) {
if (shadowDOM() && (event.target as HTMLElement)?.shadowRoot) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, when was this not defined?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a while ago I did this, I'll try and reproduce it and see exactly when this happened.

if (event.composedPath) {
return event.composedPath()[0] as Element;
}
Expand Down
4 changes: 2 additions & 2 deletions packages/react-aria-components/src/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
useContextProps,
useRenderProps
} from './utils';
import {filterDOMProps, mergeProps, useEnterAnimation, useExitAnimation, useLayoutEffect} from '@react-aria/utils';
import {filterDOMProps, mergeProps, nodeContains, useEnterAnimation, useExitAnimation, useLayoutEffect} from '@react-aria/utils';
import {focusSafely} from '@react-aria/interactions';
import {OverlayArrowContext} from './OverlayArrow';
import {OverlayTriggerProps, OverlayTriggerState, useOverlayTriggerState} from 'react-stately';
Expand Down Expand Up @@ -198,7 +198,7 @@ function PopoverInner({state, isExiting, UNSTABLE_portalContainer, clearContexts
// Focus the popover itself on mount, unless a child element is already focused.
// Skip this for submenus since hovering a submenutrigger should keep focus on the trigger
useEffect(() => {
if (isDialog && props.trigger !== 'SubmenuTrigger' && ref.current && !ref.current.contains(document.activeElement)) {
if (isDialog && props.trigger !== 'SubmenuTrigger' && ref.current && !nodeContains(ref.current, document.activeElement)) {
focusSafely(ref.current);
}
}, [isDialog, ref, props.trigger]);
Expand Down
57 changes: 55 additions & 2 deletions packages/react-aria-components/test/Popover.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@
* governing permissions and limitations under the License.
*/

import {act, pointerMap, render} from '@react-spectrum/test-utils-internal';
import {Button, Dialog, DialogTrigger, OverlayArrow, Popover, Pressable} from '../';
import {act, createShadowRoot, pointerMap, render} from '@react-spectrum/test-utils-internal';
import {Button, Dialog, DialogTrigger, Menu, MenuItem, MenuTrigger, OverlayArrow, Popover, Pressable} from '../';
import React, {useRef} from 'react';
import {screen} from 'shadow-dom-testing-library';
import {UNSAFE_PortalProvider} from '@react-aria/overlays';
import userEvent from '@testing-library/user-event';

Expand Down Expand Up @@ -273,4 +274,56 @@ describe('Popover', () => {
let dialog = getByRole('dialog');
expect(dialog).toBeInTheDocument();
});

it('test overlay and overlay trigger inside the same shadow root to have interactable content', async function () {
const {shadowRoot, cleanup} = createShadowRoot();

const appContainer = document.createElement('div');
appContainer.setAttribute('id', 'appRoot');
shadowRoot.appendChild(appContainer);

const portal = document.createElement('div');
portal.id = 'shadow-dom-portal';
shadowRoot.appendChild(portal);

const onAction = jest.fn();
const user = userEvent.setup({delay: null, pointerMap});

function ShadowApp() {
return (
<MenuTrigger>
<Button>
Open
</Button>
<Popover>
<Menu onAction={onAction}>
<MenuItem key="new">New…</MenuItem>
<MenuItem key="open">Open…</MenuItem>
<MenuItem key="save">Save</MenuItem>
<MenuItem key="save-as">Save as…</MenuItem>
<MenuItem key="print">Print…</MenuItem>
</Menu>
</Popover>
</MenuTrigger>
);
}
render(
<UNSAFE_PortalProvider getContainer={() => portal}>
Copy link

@pzaczkiewicz-athenahealth pzaczkiewicz-athenahealth Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question for Adobe: While I like the concept of having a PortalProvider so that each individual Popover shouldn't need to burden itself with knowing where to mount, there's a subtlety to juggling refs that is difficult to solve for the initial-render case. In React, we can't have access to the DOM node until the ref function has a chance to fire. This means that every React component in the render tree goes through one render cycle with no refs present. This is problematic for ReactDOM.createPortal, as getContainer() is forced to return document in the absence of resolved refs. This can cause the portalled content to briefly be rendered at document.body before re-rendering at shadowRoot in the next render cycle. Given that getContainer is a function, it may not even update once shadowRoot is resolved, keeping the Popover mounted at the wrong DOM node until the user interacts with it or something, causing getContainer to be reevaluated at its next render.

This test obfuscates this complexity, as shadowRoot is created outside of React. The most synchronous way to create a shadow root within React is do do so within a ref callback function, and store that DOM node in state rather than as a ref. This forces a re-render, and allows ReactDOM.createPortal to use the resolved DOM node. In order to prevent the initial ReactDOM.creaetPortal from mounting at document.body, I have to delay rendering until the mount point has a chance to resolve. None of this is done in react-aria's Popover, as I can see delaying rendering being problematic in server side rendering.

My question is this: Does react-aria have a plan for how to delay rendering for Popovers, or is it up to the application code to identify and correct this race condition?

<ShadowApp />
</UNSAFE_PortalProvider>,
{container: appContainer}
);

let button = await screen.findByShadowRole('button');
await user.click(button);
let menu = await screen.findByShadowRole('menu');
expect(menu).toBeVisible();
let items = await screen.findAllByShadowRole('menuitem');
let openItem = items.find(item => item.textContent?.trim() === 'Open…');
expect(openItem).toBeVisible();

await user.click(openItem);
expect(onAction).toHaveBeenCalledTimes(1);
cleanup();
});
});
10 changes: 10 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -26677,6 +26677,7 @@ __metadata:
recursive-readdir: "npm:^2.2.2"
regenerator-runtime: "npm:0.13.3"
rimraf: "npm:^2.6.3"
shadow-dom-testing-library: "npm:^1.13.1"
sharp: "npm:^0.33.5"
sinon: "npm:^7.3.1"
storybook: "npm:^8.6.14"
Expand Down Expand Up @@ -28055,6 +28056,15 @@ __metadata:
languageName: node
linkType: hard

"shadow-dom-testing-library@npm:^1.13.1":
version: 1.13.1
resolution: "shadow-dom-testing-library@npm:1.13.1"
peerDependencies:
"@testing-library/dom": ">= 8"
checksum: 10c0/cd0a5e7799f868af665235d0812bdbcfbfe4461681ef35ce0fba4d460d395f3fa0e95df5c8fec4686ba30286a62c4e7ba48013e67646977726aa13363479d70f
languageName: node
linkType: hard

"shallow-clone@npm:^3.0.0":
version: 3.0.1
resolution: "shallow-clone@npm:3.0.1"
Expand Down