Skip to content

fix: Do not bail on calculating position of popover if its trigger is inside Shadow DOM #3429

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
93 changes: 93 additions & 0 deletions src/popover/__tests__/popover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@
}
return this.findByClassName(styles.body);
}
findContainer({ renderWithPortal } = { renderWithPortal: false }): ElementWrapper | null {
if (renderWithPortal) {
return createWrapper().findByClassName(styles.container);
}
return this.findByClassName(styles.container);
}
}

function renderPopover(props: PopoverProps & { ref?: React.Ref<PopoverProps.Ref> }) {
Expand Down Expand Up @@ -415,3 +421,90 @@

expect(document.querySelectorAll('body > div')).toHaveLength(1);
});

describe('inside shadow DOM', () => {
// React below v17 does not dispatch events to components inside Shadow DOM, so to make clicking
// on a Popover trigger work inside Shadow DOM we need to catch the click event on the topmost
// node inside Shadow DOM, look back through all the nodes it bubbled through, find React component
// instances corresponding to those nodes, and manually call onClick on them. See
// https://github.com/facebook/react/issues/10422#issuecomment-674928774
function retargetClicks(root: HTMLElement) {
function findReactComponent(item: EventTarget) {
for (const key in item) {
if (Object.prototype.hasOwnProperty.call(item, key) && key.indexOf('_reactInternal') !== -1) {
return item[key as keyof typeof item] as any;
}
}
}

root.addEventListener('click', event => {
const eventPath = event.composedPath();
for (const i in eventPath) {
const item = eventPath[i];

const internalComponent = findReactComponent(item);
if (internalComponent && internalComponent.memoizedProps) {
internalComponent.memoizedProps.onClick?.(event);
break;
}

if (item === root) {
break;
}
}
});
}

function renderPopoverInShadowDom(props: PopoverProps & { ref?: React.Ref<PopoverProps.Ref> }) {
const shadowContainer = document.createElement('div');
shadowContainer.setAttribute('id', 'shadow-container');
shadowContainer.attachShadow({ mode: 'open' });

const shadowChild = document.createElement('div');
shadowChild.setAttribute('id', 'shadow-child');
shadowContainer.shadowRoot!.appendChild(shadowChild);

document.body.appendChild(shadowContainer);

retargetClicks(shadowChild);

const { container } = render(<Popover {...props} />, { container: shadowChild });

return new PopoverInternalWrapper(container);
}

afterEach(() => {
// cleanup test container after each test
document.getElementById('shadow-container')?.remove();
});

it('renders text trigger correctly', () => {
const wrapper = renderPopoverInShadowDom({ children: 'Trigger', triggerAriaLabel: 'Test aria label' });
expect(wrapper.findTrigger().getElement().tagName).toBe('BUTTON');
expect(wrapper.findTrigger().getElement()).toHaveTextContent('Trigger');
expect(wrapper.findTrigger().getElement()).toHaveAccessibleName('Test aria label');
});

it('renders the popover header correctly', () => {
const wrapper = renderPopoverInShadowDom({ children: 'Trigger', header: 'Memory error' });
wrapper.findTrigger().click();

expect(wrapper.findHeader()!.getElement()).toHaveTextContent('Memory error');
// this assertion fails if Popover bails early on calculating its position
expect(wrapper.findContainer()!.getElement()).toHaveAttribute(
'style',
'inset-block-start: 0; inset-inline-start: 13px;'
);
});

it('renders the popover header correctly with portal', () => {
const wrapper = renderPopoverInShadowDom({ children: 'Trigger', header: 'Memory error', renderWithPortal: true });
wrapper.findTrigger().click();
Copy link
Member

Choose a reason for hiding this comment

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

Considering the complications of supporting the click() event, how about we scope the tests to the popover container (internal) component instead? You can add a new test file popover-container.test.tsx and assert the expected placement, but without the need to make it visible first.

expect(wrapper.findHeader({ renderWithPortal: true })!.getElement()).toHaveTextContent('Memory error');
// this assertion fails if Popover bails early on calculating its position
expect(wrapper.findContainer({ renderWithPortal: true })!.getElement()).toHaveAttribute(

Check failure on line 505 in src/popover/__tests__/popover.test.tsx

View workflow job for this annotation

GitHub Actions / build / build

inside shadow DOM › renders the popover header correctly with portal

expect(element).toHaveAttribute("style", "z-index: 7000; inset-block-start: 18px; inset-inline-start: 10px;") // element.getAttribute("style") === "z-index: 7000; inset-block-start: 18px; inset-inline-start: 10px;" Expected the element to have attribute: style="z-index: 7000; inset-block-start: 18px; inset-inline-start: 10px;" Received: style="z-index: 7000;" at Object.<anonymous> (src/popover/__tests__/popover.test.tsx:505:77)

Check failure on line 505 in src/popover/__tests__/popover.test.tsx

View workflow job for this annotation

GitHub Actions / dry-run / Components unit tests

inside shadow DOM › renders the popover header correctly with portal

expect(element).toHaveAttribute("style", "z-index: 7000; inset-block-start: 18px; inset-inline-start: 10px;") // element.getAttribute("style") === "z-index: 7000; inset-block-start: 18px; inset-inline-start: 10px;" Expected the element to have attribute: style="z-index: 7000; inset-block-start: 18px; inset-inline-start: 10px;" Received: style="z-index: 7000;" at Object.<anonymous> (src/popover/__tests__/popover.test.tsx:505:77)
'style',
'z-index: 7000; inset-block-start: 18px; inset-inline-start: 10px;'
);
});
});
6 changes: 5 additions & 1 deletion src/popover/use-popover-position.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,14 @@ export default function usePopoverPosition({
const document = popover.ownerDocument;
const track = trackRef.current;

const rootNode = popover.getRootNode();
const isInShadowDom = rootNode !== document && rootNode.nodeType === Node.DOCUMENT_FRAGMENT_NODE;
Copy link
Member

Choose a reason for hiding this comment

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

I think this check rootNode !== document might result in false positives when popover is rendered within an iframe, in which case the getRootNode() will return the iframe's document, not top document.

Copy link
Author

@n1313 n1313 Apr 16, 2025

Choose a reason for hiding this comment

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

I will have to double-check this, but my first thought is that the iframe's document is not a Node.DOCUMENT_FRAGMENT_NODE.

Copy link
Member

Choose a reason for hiding this comment

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

You're right. I haven't realised that is not the global document but popover.ownerDocument. I think it would be nice to add a new util to utils/dom, something like:

function isInShadowDom(element: Element) {
  const rootNode = element.getRootNode();
  return rootNode !== element.ownerDocument && rootNode.nodeType === Node.DOCUMENT_FRAGMENT_NODE;
}


// If the popover body isn't being rendered for whatever reason (e.g. "display: none" or JSDOM),
// or track does not belong to the document - bail on calculating dimensions.
const { offsetWidth, offsetHeight } = getOffsetDimensions(popover);
if (offsetWidth === 0 || offsetHeight === 0 || !nodeContains(document.body, track)) {

if (offsetWidth === 0 || offsetHeight === 0 || !nodeContains(isInShadowDom ? rootNode : document.body, track)) {
return;
}

Expand Down
Loading