-
Notifications
You must be signed in to change notification settings - Fork 170
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
base: main
Are you sure you want to change the base?
Conversation
@@ -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; |
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.
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.
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.
I will have to double-check this, but my first thought is that the iframe's document is not a Node.DOCUMENT_FRAGMENT_NODE
.
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.
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;
}
|
||
it('renders the popover header correctly with portal', () => { | ||
const wrapper = renderPopoverInShadowDom({ children: 'Trigger', header: 'Memory error', renderWithPortal: true }); | ||
wrapper.findTrigger().click(); |
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.
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.
Description
This PR fixes a problem that prevents Popovers from working from inside Shadow DOM.
Currently, Popover bails on calculating dimensions of its body if its trigger does not belong to the document. This check is done using
nodeContains
utility. This is an optimization to prevent calculations from running in virtual environments where Popover wouldn't work anyway. However, this also prevents Popover from working from inside Shadow DOM, as thenodeContains
doesn't work across the Shadow DOM boundary and the check fails.This PR fixes this by checking if Popover is rendered inside Shadow DOM, and if so, checking that the trigger is contained inside it.
With this change customers will be able to use Popover in applications that use Shadow DOM.
Related links, issue #, if available: related to AWSUI-60527.
How has this been tested?
unit tests
I have built a small playground page with a webcomponent wrapped in Shadow DOM that contains a Popover. I have verified that the Popover does not work in the playground before the fix, and does work after
I can include the playground code into this PR, but the code is a bit hacky.
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.