-
Notifications
You must be signed in to change notification settings - Fork 1.3k
test: adds ShadowDOM / UNSAFE_PortalProvider tests #8806
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
base: main
Are you sure you want to change the base?
Changes from all commits
7a6dfcc
bbb7588
322c7cf
3098625
c636c8b
02deef6
2e0e86e
06a5380
a9b85aa
31b38f9
c543816
cc39458
dabe409
fc228f9
1f4a2ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -297,5 +297,8 @@ | |
}, | ||
"locales": [ | ||
"en-US" | ||
] | ||
], | ||
"dependencies": { | ||
"shadow-dom-testing-library": "^1.13.1" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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(); | ||
|
||
|
@@ -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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. interesting, when was this not defined? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
|
@@ -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}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 This test obfuscates this complexity, as 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(); | ||
}); | ||
}); |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.