-
-
Notifications
You must be signed in to change notification settings - Fork 472
feat(dismissable-layer): add shadow root support #2380
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: v2
Are you sure you want to change the base?
feat(dismissable-layer): add shadow root support #2380
Conversation
4f3483c to
41bab9b
Compare
📝 WalkthroughWalkthroughAdds Shadow DOM awareness to DismissableLayer: root-aware utilities, pointer/focus handling tied to an element's root, Escape-key logic that respects ShadowRoot boundaries, extensive shadow-DOM-focused tests, and new stories/components demonstrating nested shadow-root scenarios. Changes
Sequence DiagramsequenceDiagram
participant User
participant Layer as DismissableLayer
participant Escape as EscapeHandler
participant Utils as getRootNode/getAllDismissableLayers
participant DOM as DOM
User->>Layer: Press Escape
Layer->>Escape: handle Escape
Escape->>DOM: composedPath() -> eventTarget
Escape->>Utils: getRootNode(eventTarget)
Utils-->>Escape: ShadowRoot or Document
Escape->>Utils: getAllDismissableLayers(eventTarget)
Utils-->>Escape: list of active layers
alt Event target root differs from top layer root
Escape->>Escape: roots differ -> return (no dismiss)
else Same root
Escape->>Escape: determine topmost layer
Escape->>Layer: dismiss topmost layer
Layer->>DOM: hide/remove overlay/content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
41bab9b to
bbeb90f
Compare
| <strong>The shadow root teleported dialog should behave exactly like a dialog in the document's body</strong> | ||
|
|
||
| <ul class="list-disc ml-4"> | ||
| <li>✅ focus should move inside `Dialog` when mounted</li> |
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 one will not work here as the fix for that is in this PR:
#2352
What I can do is stack the dismissable PR (this one) on the FocusScope fix PR so it makes more sense. The FocusScope PR has to be merged first anyway
| }, layerElement) | ||
| onKeyStroke('Escape', (event) => { | ||
| const composedPath = event.composedPath() |
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.
We get the event rootNode via event.composedPath.
Then we just compare the current dismissable layer's layer element (so the current DismissableLayer instance) root node with the event's root node.
If the two elements are not the same and we have some dismissable layers in the DOM of the event (via allDismissableLayers.length > 0), it means we are not in the same DOM, so better to return early.
This way we don't have conflicts between different DOMs.
| ) { | ||
| const ownerDocument: Document | ||
| = element?.value?.ownerDocument ?? globalThis?.document | ||
| const ownerDocument: Document = element?.value?.ownerDocument ?? globalThis?.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.
Minor formatting
| const isFocusInsideDOMTree = ref(false) | ||
| watchEffect((cleanupFn) => { | ||
| if (!element?.value) { | ||
| return |
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.
if the element is not yet defined, we don't assume anything and we do an early return.
Because the risk otherwise is to add a focusin listener on the wrong rootNode
| if (event.target && !isFocusInsideDOMTree.value) { | ||
| const eventDetail = { originalEvent: event } | ||
| if (target && !isFocusInsideDOMTree.value) { | ||
| const eventDetail = { originalEvent: { ...event, target } } |
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.
Not sure why, but here the target is not in the event variable object.
| if (!isClient || !toValue(enabled)) | ||
| return | ||
| const handleFocus = async (event: FocusEvent) => { | ||
| const target = event.target as HTMLElement | undefined |
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 had to move this target here, otherwise it would fail the tests.
@zernonia are the two nextTick still needed?
Removing them is not making the unit test fail (all the tests in the repo)
|
|
||
| type ShadowRootTestCase = { | ||
| description: string | ||
| testCase: 'shadowDomOnly' | 'mixedBodyAndShadowDom' | 'bodyOnly' |
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.
| if (!isClient || !toValue(enabled)) | ||
| return | ||
| const handleFocus = async (event: FocusEvent) => { | ||
| const handleFocus = async (event: Event) => { |
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 changed the function signature here, because the shadowRoot addEventListener('focusin', ... expects a type Event.
I added a check just after to assert the event is of type FocusEvent.
commit: |
| let shadowHost: HTMLElement | ||
| let shadowRoot: ShadowRoot | ||
|
|
||
| globalThis.ResizeObserver = class ResizeObserver { |
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.
Needed for Combobox I believe, otherwise the tests would crash.
This is taken from other test files.
| disconnect() {} | ||
| } | ||
|
|
||
| function getDialogOverlay(): HTMLElement | null { |
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.
Small utility function to get the component from the correct rootNode, because query selectors can't traverse different doms
|
|
||
| beforeEach(async () => { | ||
| document.body.innerHTML = '' | ||
| if (testCase === 'shadowDomOnly') { |
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.
Here I completely separated the three test cases beforeEach steps as it's more maintainable.
The rest of the tests are the same for each case.
| </ul> | ||
|
|
||
| <div class="flex flex-col gap-4 mt-12"> | ||
| <ShadowRootContainer with-dialog /> |
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.
Here we directly have the shadow root container that will mount the Dialog inside the shadow root.
| const shadowMountPoint = document.createElement('div') | ||
| shadowRoot.appendChild(shadowMountPoint) | ||
| const shadowPortalTarget = document.createElement('div') |
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.
Used to teleport dismissable layers into shadow root.
|
|
||
| import DismissableLayersElements from './DismissableLayersElements.vue' | ||
|
|
||
| defineProps<{ portalTarget?: HTMLDivElement }>() |
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.
Used for both body and shadow root tests, hence the optional portalTarget
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.
Dropdown copied from the Dropdown story.
I didn't want to update the original, but I can if needed.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/DismissableLayer/story/shadowDom/components/_Popover.vue`:
- Around line 50-55: Replace invalid React JSX attribute defaultValue with the
proper Vue input attribute: use value="..." for these uncontrolled inputs (or
use :value="..." if you intend them to be reactive/controlled). Update the
inputs with id="width", id="maxWidth", id="height", and id="maxHeight" in
_Popover.vue to remove defaultValue and set value (or :value) accordingly so Vue
treats them correctly.
🧹 Nitpick comments (6)
packages/core/src/DismissableLayer/story/shadowDom/ShadowRootContainer.vue (2)
25-27: Consider unmounting the Vue app instance on cleanup.The
createAppinstance is never unmounted when the container changes or the component is destroyed. This could lead to memory leaks, especially during hot module replacement or repeated mounting/unmounting.♻️ Suggested improvement
+let app: ReturnType<typeof createApp> | null = null + function mountShadowRoot(container: HTMLDivElement, component: Component) { const elementWithShadow = container as Element & { shadowRoot: ShadowRoot | null } const shadowRoot = elementWithShadow.shadowRoot || elementWithShadow.attachShadow({ mode: 'open' }) document.querySelectorAll('style, link[rel="stylesheet"]').forEach((node) => { shadowRoot.appendChild(node.cloneNode(true)) }) const shadowMountPoint = document.createElement('div') shadowRoot.appendChild(shadowMountPoint) const shadowPortalTarget = document.createElement('div') shadowPortalTarget.id = `portal-shadow-root` shadowRoot.appendChild(shadowPortalTarget) - createApp(component, { + app = createApp(component, { portalTarget: shadowPortalTarget, - }).mount(shadowMountPoint) + }) + app.mount(shadowMountPoint) } function resetShadowRoot(container: HTMLDivElement) { + if (app) { + app.unmount() + app = null + } const elementWithShadow = container as Element & { shadowRoot: ShadowRoot | null } if (elementWithShadow.shadowRoot) { elementWithShadow.shadowRoot.innerHTML = '' } }
21-22: Hardcoded portal ID may conflict with multiple instances.The portal target ID
portal-shadow-rootis hardcoded. If multipleShadowRootContainerinstances exist, they would all use the same ID within their respective shadow roots. While shadow DOM isolation prevents cross-container conflicts, using a unique ID (e.g., viacrypto.randomUUID()) would be more robust.packages/core/src/DismissableLayer/story/shadowDom/components/_ComboboxMultiple.vue (1)
16-30:searchTermis declared but never updated.The
ComboboxInputis missing av-modelbinding tosearchTerm. As a result,filteredPeoplealways filters against an empty string, rendering all options regardless of user input.If filtering is intended for this story component, bind the input:
Proposed fix
<ComboboxInput + v-model="searchTerm" data-testid="combobox-input" class="bg-transparent outline-none text-grass11 placeholder-gray-400" placeholder="Test" />packages/core/src/DismissableLayer/utils.ts (1)
211-212: SpreadingFocusEventmay lose event properties.Spreading a
FocusEventinto a plain object ({ ...event, target }) may not preserve non-enumerable properties or prototype methods. If consumers oforiginalEventneed full event functionality (e.g.,relatedTarget,preventDefault), this could cause issues.Consider preserving the original event reference:
Suggested approach
if (target && !isFocusInsideDOMTree.value) { - const eventDetail = { originalEvent: { ...event, target } } + const eventDetail = { originalEvent: event as FocusEvent } handleAndDispatchCustomEvent(If
targetoverride is necessary for shadow DOM scenarios, consider a wrapper object or ensure consumers only access specific properties.packages/core/src/DismissableLayer/DismissableLayer.test.ts (2)
104-108: ResizeObserver mock placement could be hoisted.The mock is redefined for each iteration of the
forEachloop during test collection. While this works, moving it outside theforEach(or into abeforeAllat the outer describe level) would be slightly cleaner.That said, since this is intentional for Combobox support, this is a minor point.
♻️ Optional: Move mock outside forEach
+globalThis.ResizeObserver = class ResizeObserver { + observe() {} + unobserve() {} + disconnect() {} +} + testSuite.forEach(({ description, testCase }) => { describe(description, () => { let wrapper: VueWrapper<InstanceType<typeof ShadowRootContainer>> let shadowHost: HTMLElement let shadowRoot: ShadowRoot - - globalThis.ResizeObserver = class ResizeObserver { - observe() {} - unobserve() {} - disconnect() {} - }
151-152: Consider simplifying shadowRoot access.The double cast
as unknown as { shadowRoot: ShadowRoot }is verbose. SinceHTMLElementalready has ashadowRootproperty returningShadowRoot | null, you can access it directly:♻️ Simplified shadowRoot access
-shadowRoot = (shadowHost as unknown as { shadowRoot: ShadowRoot }).shadowRoot! +shadowRoot = shadowHost.shadowRoot!Also applies to: 175-176
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/core/src/DismissableLayer/DismissableLayer.test.tspackages/core/src/DismissableLayer/DismissableLayer.vuepackages/core/src/DismissableLayer/story/DismissableLayerDialogShadowRoot.story.vuepackages/core/src/DismissableLayer/story/DismissableLayerDialogWithNestedShadowRootContent.story.vuepackages/core/src/DismissableLayer/story/shadowDom/DismissableLayersElements.vuepackages/core/src/DismissableLayer/story/shadowDom/ShadowRootContainer.vuepackages/core/src/DismissableLayer/story/shadowDom/_Dialog.vuepackages/core/src/DismissableLayer/story/shadowDom/components/_ComboboxMultiple.vuepackages/core/src/DismissableLayer/story/shadowDom/components/_Dropdown.vuepackages/core/src/DismissableLayer/story/shadowDom/components/_Popover.vuepackages/core/src/DismissableLayer/utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/DismissableLayer/DismissableLayer.test.ts (1)
packages/plugins/src/namespaced/index.ts (1)
Dialog(275-293)
🪛 ast-grep (0.40.5)
packages/core/src/DismissableLayer/DismissableLayer.test.ts
[warning] 146-146: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (20)
packages/core/src/DismissableLayer/story/DismissableLayerDialogShadowRoot.story.vue (1)
1-97: Well-structured story with comprehensive test documentation.The story file provides excellent coverage documentation for shadow DOM behavior testing. The test scenarios are clearly organized and cover focus management, escape dismissal, and interaction behaviors for Dialog, Combobox, Popover, and Dropdown Menu components within a shadow root context.
packages/core/src/DismissableLayer/DismissableLayer.vue (1)
134-153: Shadow DOM-aware escape key handling looks correct.The implementation properly:
- Uses
composedPath()to get the actual event target within shadow DOM- Compares root nodes to determine if the event originated from a different DOM boundary
- Early-returns when the event is from a different shadow root that has its own dismissable layers
This design correctly isolates escape key handling per DOM boundary, preventing cross-DOM interference.
packages/core/src/DismissableLayer/story/shadowDom/DismissableLayersElements.vue (1)
1-30: Clean component composition for shadow DOM story elements.The component correctly composes the various dismissable layer elements (Combobox, Dropdown, Popover) with both modal and non-modal variants, properly forwarding the
portalTargetprop. This provides a comprehensive set of test cases within the shadow root context.packages/core/src/DismissableLayer/story/shadowDom/_Dialog.vue (1)
1-65: Well-implemented dialog component with shadow DOM flexibility.The component correctly:
- Supports portal targeting via the
portalTargetprop- Enables nested shadow DOM testing via
hasShadowRootInsideprop- Uses Vue's
v-model:openpattern for state management- Includes proper transitions and accessibility attributes
The conditional rendering logic (lines 47-52) cleanly handles both nested shadow DOM and direct element scenarios.
packages/core/src/DismissableLayer/story/shadowDom/components/_ComboboxMultiple.vue (1)
38-72: Portal targeting for shadow DOM looks correct.The
ComboboxPortalcorrectly uses theportalTargetprop, enabling content rendering within a shadow root when provided.packages/core/src/DismissableLayer/story/DismissableLayerDialogWithNestedShadowRootContent.story.vue (1)
1-96: Well-documented story for shadow DOM testing.The story provides a comprehensive checklist of expected behaviors for nested shadow root scenarios, covering focus trapping, escape dismissal, and interactions with nested components (Combobox, Popover, Dropdown Menu). This serves as valuable documentation alongside the test suite.
packages/core/src/DismissableLayer/story/shadowDom/components/_Popover.vue (1)
19-36: Modal/non-modal pattern and portal targeting look correct.The component properly passes the
modalprop toPopoverRootand uses conditionaldata-testidattributes to distinguish variants in tests. Portal targeting viaportalTargetenables shadow DOM support.packages/core/src/DismissableLayer/story/shadowDom/components/_Dropdown.vue (2)
139-261: Deeply nested submenu structure is repetitive but acceptable for a story.The 4-level deep nested submenu structure contains significant repetition. For a story/demo component testing shadow DOM behavior at various nesting depths, this is acceptable. If maintenance becomes burdensome, consider extracting a recursive submenu component.
48-72: Modal/non-modal dropdown pattern and portal targeting look correct.The component properly implements modal mode via props, uses conditional
data-testidattributes, and correctly passesportalTargettoDropdownMenuPortalfor shadow DOM support.packages/core/src/DismissableLayer/utils.ts (4)
16-21:getRootNodeutility correctly handles shadow DOM detection.The function properly checks for
ShadowRootinstance and falls back todocument. This provides consistent root node resolution for both shadow DOM and regular DOM contexts.
26-34:getAllDismissableLayerscorrectly queries within the appropriate root.The function properly handles both
DocumentandShadowRootcontexts, returning an empty array for edge cases like detached nodes.
62-72:getPointerEventTargetcorrectly resolves targets within shadow DOM.Using
composedPath()[0]when the target is a shadow host ensures the actual interacted element inside the shadow DOM is retrieved, which is essential for accurate outside-click detection.
187-223: Focus outside handling correctly attaches listeners to the computed root node.The implementation properly computes the root node for shadow DOM support and attaches/detaches
focusinlisteners accordingly. The early return guard for missing elements (lines 191-193) prevents errors when the component is not yet mounted.packages/core/src/DismissableLayer/DismissableLayer.test.ts (7)
77-96: Well-structured parameterized test setup.The use of a typed array with
ShadowRootTestCaseto drive three different test scenarios is a clean pattern that ensures consistent coverage across body-only, shadow DOM-only, and mixed configurations.
146-147: Static analysis false positive – safe to ignore.The ast-grep warning about
document.body.innerHTML = ''is a false positive. In test files, clearing the DOM between tests is standard practice and poses no XSS risk since no user input is involved.
110-144: Helper functions effectively abstract DOM queries.The
getDialogOverlay,getDialogTrigger,getDialogContent, andgetQueryRoothelpers cleanly encapsulate the query logic differences between test cases, improving readability and maintainability.
193-196: Good cleanup practice with afterEach.Properly unmounting the wrapper and awaiting
nextTickensures clean state between tests and prevents potential memory leaks or cross-test pollution.
216-247: Comprehensive Combobox nested interaction tests.The tests cover the key scenarios: interaction inside content, Escape key behavior, and outside interaction. The assertions correctly verify that the non-modal Combobox closes appropriately while respecting the dialog's state.
249-324: Thorough modal vs non-modal Popover coverage.Good differentiation between modal and non-modal behaviors:
- Modal popover closes on outside click but preserves the dialog
- Non-modal popover closes along with the dialog on outside interaction
This matches expected dismissable layer semantics.
326-391: Dropdown Menu tests mirror Popover patterns correctly.The modal/non-modal distinction is properly tested, and the submenu interaction test (line 334-339) adds valuable coverage for nested menu scenarios.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <input | ||
| id="width" | ||
| class="w-full inline-flex items-center justify-center flex-1 rounded px-2.5 text-[13px] leading-none text-violet11 shadow-[0_0_0_1px] shadow-violet7 h-[25px] focus:shadow-[0_0_0_2px] focus:shadow-violet8 outline-none" | ||
| defaultValue="100%" | ||
| data-testid="popover-first-input" | ||
| > |
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.
defaultValue is not a valid Vue attribute.
defaultValue is React JSX syntax. In Vue templates, use the value attribute for uncontrolled inputs or :value binding for controlled inputs.
Proposed fix for all inputs
<input
id="width"
class="w-full inline-flex items-center justify-center flex-1 rounded px-2.5 text-[13px] leading-none text-violet11 shadow-[0_0_0_1px] shadow-violet7 h-[25px] focus:shadow-[0_0_0_2px] focus:shadow-violet8 outline-none"
- defaultValue="100%"
+ value="100%"
data-testid="popover-first-input"
>Apply similar changes to maxWidth (line 67), height (line 80), and maxHeight (line 93).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <input | |
| id="width" | |
| class="w-full inline-flex items-center justify-center flex-1 rounded px-2.5 text-[13px] leading-none text-violet11 shadow-[0_0_0_1px] shadow-violet7 h-[25px] focus:shadow-[0_0_0_2px] focus:shadow-violet8 outline-none" | |
| defaultValue="100%" | |
| data-testid="popover-first-input" | |
| > | |
| <input | |
| id="width" | |
| class="w-full inline-flex items-center justify-center flex-1 rounded px-2.5 text-[13px] leading-none text-violet11 shadow-[0_0_0_1px] shadow-violet7 h-[25px] focus:shadow-[0_0_0_2px] focus:shadow-violet8 outline-none" | |
| value="100%" | |
| data-testid="popover-first-input" | |
| > |
🤖 Prompt for AI Agents
In `@packages/core/src/DismissableLayer/story/shadowDom/components/_Popover.vue`
around lines 50 - 55, Replace invalid React JSX attribute defaultValue with the
proper Vue input attribute: use value="..." for these uncontrolled inputs (or
use :value="..." if you intend them to be reactive/controlled). Update the
inputs with id="width", id="maxWidth", id="height", and id="maxHeight" in
_Popover.vue to remove defaultValue and set value (or :value) accordingly so Vue
treats them correctly.
8feed5a to
7cd4052
Compare
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/DismissableLayer/utils.ts (1)
84-86: MakeownerDocumentreactive to matchuseFocusOutsidepattern.The asymmetry between
usePointerDownOutsideanduseFocusOutsideis intentional—focus events don't bubble across shadow boundaries, souseFocusOutsidecorrectly attaches torootNode(which returns a ShadowRoot if applicable). Pointer events, however, are composed by default and bubble to the document.However, there's a real issue:
ownerDocumentis assigned once at lines 84-85, outside thewatchEffect. If theelementref changes after initialization, the listener remains attached to the old document. In contrast,useFocusOutsidecomputesrootNodereactively (line 187) so it updates when the element changes.Use
computed()instead of a plain variable:const ownerDocument = computed( () => element?.value?.ownerDocument ?? globalThis?.document )Then reference it as
ownerDocument.valueinside thewatchEffect.
🧹 Nitpick comments (3)
packages/core/src/DismissableLayer/story/shadowDom/components/_ComboboxMultiple.vue (1)
16-18: Filtering logic is unused —searchTermis never updated.The
searchTermref andfilteredPeoplecomputed are defined butComboboxInputlacks av-modelbinding to updatesearchTerm. As a result, filtering never activates and all items always display.If filtering is intended for this demo, bind the input:
♻️ Optional fix to enable filtering
<ComboboxInput + v-model="searchTerm" data-testid="combobox-input" class="bg-transparent outline-none text-grass11 placeholder-gray-400" placeholder="Test" />If showing all items is intentional for the story, consider removing the unused
searchTermandfilteredPeopleto avoid confusion.Also applies to: 26-30
packages/core/src/DismissableLayer/story/shadowDom/components/_Dropdown.vue (1)
31-41: Consider consolidating duplicate class definitions.The
item,checkboxItem, andradioItemclass strings are identical. For demo code this is acceptable, but consolidating would reduce duplication:♻️ Optional consolidation
const item = 'group w-full text-sm leading-none text-violet11 flex items-center h-7 relative pl-7 pr-2 select-none outline-none data-[state=open]:bg-violet4 data-[state=open]:text-violet11 data-[disabled]:text-mauve8 data-[disabled]:pointer-events-none data-[highlighted]:bg-violet9 data-[highlighted]:text-violet1 data-[highlighted]:data-[state=open]:bg-violet9 data-[highlighted]:data-[state=open]:text-violet1 cursor-default rounded' - -const checkboxItem = 'group w-full text-sm leading-none text-violet11 flex items-center h-7 relative pl-7 pr-2 select-none outline-none data-[state=open]:bg-violet4 data-[state=open]:text-violet11 data-[disabled]:text-mauve8 data-[disabled]:pointer-events-none data-[highlighted]:bg-violet9 data-[highlighted]:text-violet1 data-[highlighted]:data-[state=open]:bg-violet9 data-[highlighted]:data-[state=open]:text-violet1 cursor-default rounded' - -const radioItem = 'group w-full text-sm leading-none text-violet11 flex items-center h-7 relative pl-7 pr-2 select-none outline-none data-[state=open]:bg-violet4 data-[state=open]:text-violet11 data-[disabled]:text-mauve8 data-[disabled]:pointer-events-none data-[highlighted]:bg-violet9 data-[highlighted]:text-violet1 data-[highlighted]:data-[state=open]:bg-violet9 data-[highlighted]:data-[state=open]:text-violet1 cursor-default rounded'Then use
itemin place ofcheckboxItemandradioItemin the template.packages/core/src/DismissableLayer/DismissableLayer.test.ts (1)
104-108: Consider moving ResizeObserver mock outside the test loop.The
ResizeObservermock is redefined for each test case iteration. Moving it outside theforEachloop would be cleaner since it only needs to be set once.♻️ Suggested refactor
+globalThis.ResizeObserver = class ResizeObserver { + observe() {} + unobserve() {} + disconnect() {} +} + testSuite.forEach(({ description, testCase }) => { describe(description, () => { let wrapper: VueWrapper<InstanceType<typeof ShadowRootContainer>> let shadowHost: HTMLElement let shadowRoot: ShadowRoot - globalThis.ResizeObserver = class ResizeObserver { - observe() {} - unobserve() {} - disconnect() {} - }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/core/src/DismissableLayer/DismissableLayer.test.tspackages/core/src/DismissableLayer/DismissableLayer.vuepackages/core/src/DismissableLayer/story/DismissableLayerDialogShadowRoot.story.vuepackages/core/src/DismissableLayer/story/DismissableLayerDialogWithNestedShadowRootContent.story.vuepackages/core/src/DismissableLayer/story/shadowDom/DismissableLayersElements.vuepackages/core/src/DismissableLayer/story/shadowDom/ShadowRootContainer.vuepackages/core/src/DismissableLayer/story/shadowDom/_Dialog.vuepackages/core/src/DismissableLayer/story/shadowDom/components/_ComboboxMultiple.vuepackages/core/src/DismissableLayer/story/shadowDom/components/_Dropdown.vuepackages/core/src/DismissableLayer/story/shadowDom/components/_Popover.vuepackages/core/src/DismissableLayer/utils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/DismissableLayer/DismissableLayer.vue
- packages/core/src/DismissableLayer/story/shadowDom/_Dialog.vue
- packages/core/src/DismissableLayer/story/shadowDom/ShadowRootContainer.vue
- packages/core/src/DismissableLayer/story/shadowDom/components/_Popover.vue
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/DismissableLayer/DismissableLayer.test.ts (1)
packages/plugins/src/namespaced/index.ts (1)
Dialog(275-293)
🪛 ast-grep (0.40.5)
packages/core/src/DismissableLayer/DismissableLayer.test.ts
[warning] 146-146: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (17)
packages/core/src/DismissableLayer/story/DismissableLayerDialogShadowRoot.story.vue (1)
1-97: Well-structured story for shadow root testing.The story comprehensively documents expected behaviors for DismissableLayer inside a shadow root, covering focus management, escape dismissal, outside interactions, and nested component scenarios (Combobox, Popover, Dropdown Menu). The implementation is clean and serves as good functional documentation.
packages/core/src/DismissableLayer/story/shadowDom/DismissableLayersElements.vue (1)
1-30: Clean composition for shadow DOM test scenarios.The component correctly composes multiple dismissable layer components (Combobox, Dropdown, Popover) with both modal and non-modal variants, forwarding the
portalTargetprop appropriately for shadow DOM testing.packages/core/src/DismissableLayer/story/DismissableLayerDialogWithNestedShadowRootContent.story.vue (1)
1-97: Comprehensive story for mixed body/shadow-root scenario.This story tests the important case where a Dialog renders in the document body but contains nested shadow root content with dismissable layers. The documented test scenarios cover all critical interactions for this mixed context.
packages/core/src/DismissableLayer/story/shadowDom/components/_Dropdown.vue (1)
44-320: Deep nesting is appropriate for shadow DOM layer testing.The 4-level nested submenu structure thoroughly exercises dismissable layer behavior in shadow DOM contexts. The conditional
data-testidattributes based onmodalprop enable targeted testing of both modal and non-modal variants.packages/core/src/DismissableLayer/utils.ts (5)
16-21: LGTM! Clean utility for root node resolution.The implementation correctly handles the shadow DOM boundary detection. Returning
documentas fallback when the element is null or not in a ShadowRoot is appropriate.
26-34: LGTM! Properly scoped layer enumeration.The function correctly resolves the root node and queries within the appropriate DOM boundary. The fallback to an empty array for unexpected root node types is a safe default.
62-72: LGTM! Correct handling of pointer events in shadow DOM.Using
event.composedPath()[0]when the target is a shadow host correctly resolves the actual event target within the shadow DOM boundary.
47-50: LGTM! Correct root scoping for layer existence check.Using
getRootNode(layerElement)ensures the dismissable layer lookup is scoped to the correct DOM boundary (shadow root or document).
221-223: Potential stale closure in cleanup whenrootNodechanges.The cleanup function captures
rootNode.valueat the time the effect runs. If the element changes (causingrootNodeto change), the cleanup from the previous effect run will correctly reference the oldrootNode.valuedue to how closures work—so this is actually fine. Vue'swatchEffectwill re-run whenrootNodechanges (since it's a computed dependency), and each cleanup correctly captures its own snapshot.packages/core/src/DismissableLayer/DismissableLayer.test.ts (8)
77-96: Well-structured parameterized test suite.The approach of defining test cases as a typed array and running the same tests across different shadow DOM configurations (shadowDomOnly, mixedBodyAndShadowDom, bodyOnly) provides comprehensive coverage while maintaining DRY principles.
110-144: LGTM! Clear helper functions for DOM queries.The helper functions correctly abstract the differences between test cases, querying from the appropriate root node based on the scenario. The distinction between
getDialogOverlay/getDialogContent(dialog location) andgetQueryRoot(nested component location) is well-thought-out.
146-191: LGTM! Comprehensive test setup for each scenario.The
beforeEachcorrectly handles the three different test cases with appropriate component mounting and shadow root extraction. The static analysis warning aboutinnerHTMLis a false positive—clearing the DOM in test setup is a standard practice and doesn't involve user input.
198-202: LGTM! Conditional test registration is appropriate.Wrapping the
shadowRootexistence test in a condition ensures it only runs for shadow DOM scenarios whereshadowRootis expected to be defined.
204-214: LGTM! Core dialog dismissal tests are well-defined.The tests for Escape key and overlay click dismissal are clear and validate the fundamental DismissableLayer behavior across all DOM configurations.
216-247: Excellent coverage of nested Combobox interactions.These tests directly address the issue from
#1667where interactions with nested components were incorrectly closing the parent dialog. The distinction between:
- Interacting inside (should keep both open)
- Pressing Escape (should close combobox only)
- Interacting outside (should close both)
...validates the correct layer hierarchy behavior.
249-324: LGTM! Modal vs non-modal Popover behavior is correctly tested.The tests correctly differentiate:
- Modal: Overlay click closes popover only, preserving dialog (expected: popover intercepts dismiss)
- Non-modal: Overlay click closes both (expected: dismiss propagates through)
This validates the shadow DOM fix maintains proper modal semantics.
326-391: LGTM! Dropdown Menu tests with sub-menu coverage.The inclusion of sub-menu interaction testing (line 334-335) adds valuable coverage for complex nested menu scenarios within shadow DOM boundaries.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
7cd4052 to
363d728
Compare
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.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/core/src/DismissableLayer/story/shadowDom/ShadowRootContainer.vue`:
- Around line 9-28: mountShadowRoot currently creates a Vue app with
createApp(...) but never stores the app instance, so when resetShadowRoot clears
shadowRoot.innerHTML the app is not unmounted; modify mountShadowRoot to capture
the app instance returned by createApp and attach it to a stable place (e.g., a
property on elementWithShadow or shadowRoot like elementWithShadow.__vueApp or
shadowRoot.__vueApp) and update resetShadowRoot to check for that stored app and
call app.unmount() before clearing or removing shadowRoot contents (also ensure
any references like shadowMountPoint and shadowPortalTarget are removed after
unmount).
In `@packages/core/src/DismissableLayer/utils.ts`:
- Around line 221-223: The cleanup closure currently captures rootNode.value at
cleanup time which can be stale; change to capture a local constant when adding
the listener: assign const node = rootNode.value (or bail if null), call
node.addEventListener('focusin', handleFocus), and then pass a cleanup that
calls node.removeEventListener('focusin', handleFocus); ensure you reference the
same local node variable and guard for null to avoid runtime errors (symbols:
rootNode.value, handleFocus, cleanupFn).
- Around line 62-72: The getPointerEventTarget function currently checks
target?.shadowRoot which misses events originating inside shadow DOM; change it
to use event.composedPath()[0] as the primary source and fall back to
event.target if composedPath[0] is falsy so that getPointerEventTarget(event)
reliably returns (composedPath[0] || event.target) as an HTMLElement |
undefined.
♻️ Duplicate comments (1)
packages/core/src/DismissableLayer/story/shadowDom/components/_Popover.vue (1)
50-55:defaultValueis not a valid Vue attribute - usevalueinstead.The
defaultValueattribute is React JSX syntax. In Vue templates, usevaluefor setting initial input values.🐛 Proposed fix for all inputs
<input id="width" class="w-full inline-flex items-center justify-center flex-1 rounded px-2.5 text-[13px] leading-none text-violet11 shadow-[0_0_0_1px] shadow-violet7 h-[25px] focus:shadow-[0_0_0_2px] focus:shadow-violet8 outline-none" - defaultValue="100%" + value="100%" data-testid="popover-first-input" >Apply similar changes to lines 67, 80, and 93.
🧹 Nitpick comments (3)
packages/core/src/DismissableLayer/story/shadowDom/components/_Dropdown.vue (1)
31-42: Consider consolidating duplicate style constants.The
item,checkboxItem, andradioItemconstants are nearly identical. Since this is a story component, this is acceptable, but for maintainability you could consolidate them into a single constant.♻️ Optional: Consolidate duplicate styles
const item = 'group w-full text-sm leading-none text-violet11 flex items-center h-7 relative pl-7 pr-2 select-none outline-none data-[state=open]:bg-violet4 data-[state=open]:text-violet11 data-[disabled]:text-mauve8 data-[disabled]:pointer-events-none data-[highlighted]:bg-violet9 data-[highlighted]:text-violet1 data-[highlighted]:data-[state=open]:bg-violet9 data-[highlighted]:data-[state=open]:text-violet1 cursor-default rounded' -const checkboxItem = 'group w-full text-sm leading-none text-violet11 flex items-center h-7 relative pl-7 pr-2 select-none outline-none data-[state=open]:bg-violet4 data-[state=open]:text-violet11 data-[disabled]:text-mauve8 data-[disabled]:pointer-events-none data-[highlighted]:bg-violet9 data-[highlighted]:text-violet1 data-[highlighted]:data-[state=open]:bg-violet9 data-[highlighted]:data-[state=open]:text-violet1 cursor-default rounded' - -const radioItem = 'group w-full text-sm leading-none text-violet11 flex items-center h-7 relative pl-7 pr-2 select-none outline-none data-[state=open]:bg-violet4 data-[state=open]:text-violet11 data-[disabled]:text-mauve8 data-[disabled]:pointer-events-none data-[highlighted]:bg-violet9 data-[highlighted]:text-violet1 data-[highlighted]:data-[state=open]:bg-violet9 data-[highlighted]:data-[state=open]:text-violet1 cursor-default rounded' +// checkboxItem and radioItem share the same styles as item +const checkboxItem = item +const radioItem = itempackages/core/src/DismissableLayer/DismissableLayer.test.ts (2)
104-108: Consider moving ResizeObserver mock outside the forEach loop.The mock is reassigned for each test case iteration (3 times), which is redundant. Moving it outside the
testSuite.forEachwould clarify that it's a one-time global mock.Suggested placement
+globalThis.ResizeObserver = class ResizeObserver { + observe() {} + unobserve() {} + disconnect() {} +} + testSuite.forEach(({ description, testCase }) => { describe(description, () => { let wrapper: VueWrapper<InstanceType<typeof ShadowRootContainer>> let shadowHost: HTMLElement let shadowRoot: ShadowRoot - - globalThis.ResizeObserver = class ResizeObserver { - observe() {} - unobserve() {} - disconnect() {} - }
251-261: Stale DOM reference in assertion.At line 259,
popoverContentis the element reference captured before the interaction. AfterpointerDown, you're asserting on this stale reference rather than re-querying the DOM. While this happens to work (the element isn't removed in this case), it's inconsistent with other tests that re-query.For consistency with tests like line 270 that re-query the DOM for the assertion, consider:
await fireEvent.pointerDown(firstInput) await sleep(1) - expect(popoverContent).toBeTruthy() + expect(getQueryRoot().querySelector('[data-testid="popover-content-modal"]')).toBeTruthy() expect(getDialogContent()).toBeTruthy()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/core/src/DismissableLayer/DismissableLayer.test.tspackages/core/src/DismissableLayer/DismissableLayer.vuepackages/core/src/DismissableLayer/story/DismissableLayerDialogShadowRoot.story.vuepackages/core/src/DismissableLayer/story/DismissableLayerDialogWithNestedShadowRootContent.story.vuepackages/core/src/DismissableLayer/story/shadowDom/DismissableLayersElements.vuepackages/core/src/DismissableLayer/story/shadowDom/ShadowRootContainer.vuepackages/core/src/DismissableLayer/story/shadowDom/_Dialog.vuepackages/core/src/DismissableLayer/story/shadowDom/components/_ComboboxMultiple.vuepackages/core/src/DismissableLayer/story/shadowDom/components/_Dropdown.vuepackages/core/src/DismissableLayer/story/shadowDom/components/_Popover.vuepackages/core/src/DismissableLayer/utils.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/DismissableLayer/story/DismissableLayerDialogWithNestedShadowRootContent.story.vue
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/DismissableLayer/DismissableLayer.vue
- packages/core/src/DismissableLayer/story/shadowDom/components/_ComboboxMultiple.vue
- packages/core/src/DismissableLayer/story/shadowDom/_Dialog.vue
- packages/core/src/DismissableLayer/story/shadowDom/DismissableLayersElements.vue
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/DismissableLayer/DismissableLayer.test.ts (1)
packages/plugins/src/namespaced/index.ts (1)
Dialog(275-293)
🪛 ast-grep (0.40.5)
packages/core/src/DismissableLayer/DismissableLayer.test.ts
[warning] 146-146: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
packages/core/src/DismissableLayer/story/shadowDom/components/_Dropdown.vue (1)
1-321: LGTM for the shadow DOM story component.The component correctly demonstrates nested dropdown menus with shadow DOM portal support. The
portalTargetprop enables proper teleportation within shadow roots, and the multi-level submenu nesting provides good test coverage for dismissable layer behavior.packages/core/src/DismissableLayer/story/shadowDom/ShadowRootContainer.vue (1)
39-48: Watch does not react towithDialogprop changes.The watcher only observes
container, so changingwithDialogafter mount won't re-render with the new component. If this is intentional for the story, consider adding a comment. Otherwise, includeprops.withDialogin the watch source.♻️ Fix if dynamic switching is needed
watch( - container, + [container, () => props.withDialog], - (newVal) => { + ([newVal]) => { if (newVal) { resetShadowRoot(newVal) mountShadowRoot(newVal, props.withDialog ? Dialog : DismissableLayersElements) } }, { immediate: true }, )packages/core/src/DismissableLayer/story/DismissableLayerDialogShadowRoot.story.vue (1)
1-97: LGTM!The story file provides comprehensive documentation of expected behaviors and a clear test case for shadow DOM dialog interactions. The checklist format is helpful for manual testing and verification.
packages/core/src/DismissableLayer/story/shadowDom/components/_Popover.vue (1)
1-108: Overall structure is correct for shadow DOM portal support.The component properly accepts
portalTargetand passes it toPopoverPortal, enabling correct rendering within shadow roots.packages/core/src/DismissableLayer/utils.ts (3)
16-21: LGTM!Clean implementation of root node resolution. Correctly handles null elements and returns the appropriate root (ShadowRoot or document).
187-193: Good defensive guard for undefined element.The early return when
element?.valueis undefined prevents attaching listeners to the wrong root node. This aligns with the author's comment about avoiding incorrect listener attachment.
26-34: LGTM!The function correctly queries dismissable layers within the appropriate root context (Document or ShadowRoot), supporting the shadow DOM isolation requirement.
packages/core/src/DismissableLayer/DismissableLayer.test.ts (5)
77-96: Well-structured parameterized test setup.Good use of a typed test suite array to cover all three DOM context scenarios (shadow-only, mixed, body-only). This approach ensures consistent test coverage across all cases while keeping the test logic DRY.
110-144: LGTM on helper functions.The query helpers correctly distinguish where elements live across the three scenarios: Dialog elements are in the body for
mixedBodyAndShadowDomandbodyOnly, while nested content (combobox, popover, dropdown) usesshadowRootfor shadow DOM cases. The logic is sound.
146-191: Test setup is comprehensive and correctly handles all three scenarios.The
beforeEachproperly initializes the DOM context for each test case. The static analysis warning aboutinnerHTML = ''is a false positive—this is standard test cleanup to ensure a fresh DOM state between tests.The type assertion for
shadowRoot(lines 152, 176) is verbose but necessary given the typing constraints.
193-214: Cleanup and basic dismiss tests look good.The
afterEachproperly unmounts the wrapper to prevent test pollution. The conditional test registration at line 198 works correctly since the condition is evaluated at describe-time for each test case iteration.
216-391: Excellent test coverage for nested dismissable layer interactions.The tests comprehensively cover:
- Combobox (non-modal): inner interactions, Escape, outside dismiss
- Popover (modal & non-modal): inner interactions, Escape, outside dismiss
- Dropdown Menu (modal & non-modal): inner interactions, Escape, outside dismiss
The distinction between modal (protects parent layer) and non-modal (doesn't protect parent) behaviors is clearly tested. This coverage validates the shadow DOM support across all relevant component combinations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/core/src/DismissableLayer/story/shadowDom/ShadowRootContainer.vue
Show resolved
Hide resolved
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/DismissableLayer/utils.ts (1)
211-217: Keep the originalFocusEventto maintain type safety and preserve event methods.
{ ...event, target }creates a plain object that loses methods likepreventDefault()andstopPropagation(). While current code only reads properties, this creates a type-safety mismatch (type saysFocusEvent, runtime is plain object) and breaks if consumers expect to call these methods. Keep the original event object; thetargetproperty is already accessible via the event.Suggested fix
- if (target && !isFocusInsideDOMTree.value) { - const eventDetail = { originalEvent: { ...event, target } } + if (target && !isFocusInsideDOMTree.value) { + const eventDetail = { originalEvent: event } handleAndDispatchCustomEvent( FOCUS_OUTSIDE, onFocusOutside, eventDetail, )
🤖 Fix all issues with AI agents
In `@packages/core/src/DismissableLayer/utils.ts`:
- Around line 16-21: The helper getRootNode currently returns the global
document for any non-ShadowRoot result, which breaks iframe-safety; change
getRootNode to return the actual Document when Element.getRootNode() yields a
Document (i.e., if rootNode instanceof Document return rootNode), keep returning
ShadowRoot when appropriate, and fall back to the global document only when
element is null or getRootNode() returns something unexpected; update any
callers such as isLayerExist that rely on getRootNode to query the correct
document/iframe context.
| export function getRootNode(element: HTMLElement | null): Document | ShadowRoot { | ||
| const rootNode = element?.getRootNode() | ||
| if (rootNode instanceof ShadowRoot) | ||
| return rootNode | ||
| return 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.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full context of the file and find all usages of getRootNode
fd -e ts -e tsx | head -20Repository: unovue/reka-ui
Length of output: 809
🏁 Script executed:
# Look at the file in question
cat -n packages/core/src/DismissableLayer/utils.ts | head -50Repository: unovue/reka-ui
Length of output: 2086
🏁 Script executed:
# Search for all usages of getRootNode function
rg "getRootNode" --type ts --type tsx -B 2 -A 2Repository: unovue/reka-ui
Length of output: 85
🏁 Script executed:
# Check for any iframe-related tests or usage
rg "iframe|Document|ownerDocument" packages/core/src/DismissableLayer/ -B 1 -A 1Repository: unovue/reka-ui
Length of output: 4487
🏁 Script executed:
# Let's verify the Element.getRootNode() behavior and check if there are tests
cat packages/core/src/DismissableLayer/DismissableLayer.test.ts | head -100Repository: unovue/reka-ui
Length of output: 3830
🏁 Script executed:
# Check how getAllDismissableLayers uses the root node to understand impact
sed -n '26,34p' packages/core/src/DismissableLayer/utils.tsRepository: unovue/reka-ui
Length of output: 373
Return the actual Document root (iframe-safe).
getRootNode discards the Document returned by Element.getRootNode() for light DOM and always returns the global document, breaking iframe-safety. When this utility is used in isLayerExist() and similar functions that query dismissable layers, it causes queries to run against the wrong document if the element is in an iframe.
🔧 Suggested fix
export function getRootNode(element: HTMLElement | null): Document | ShadowRoot {
const rootNode = element?.getRootNode()
- if (rootNode instanceof ShadowRoot)
- return rootNode
- return document
+ if (rootNode instanceof ShadowRoot || rootNode instanceof Document)
+ return rootNode
+ return element?.ownerDocument ?? document
}🤖 Prompt for AI Agents
In `@packages/core/src/DismissableLayer/utils.ts` around lines 16 - 21, The helper
getRootNode currently returns the global document for any non-ShadowRoot result,
which breaks iframe-safety; change getRootNode to return the actual Document
when Element.getRootNode() yields a Document (i.e., if rootNode instanceof
Document return rootNode), keep returning ShadowRoot when appropriate, and fall
back to the global document only when element is null or getRootNode() returns
something unexpected; update any callers such as isLayerExist that rely on
getRootNode to query the correct document/iframe context.



🔗 Linked issue
#1667
This PR only addresses the DismissableLayer issue, not the FocusScope issue mentioned in the original issue.
FocusScope is fixed here: #2352
So this PR fixes:
DismissableLayerDialogWithNestedShadowRootContent.story.vueand DismissableLayerDialogShadowRoot.story.vue` stories❓ Type of change
📚 Description
Resolves #1667 partially, just about the focus scope issue:
📸 Screenshots (if appropriate)
Now the dismissable layers behave correctly in Shadow DOM.
Screen.Recording.2026-01-15.at.00.28.57.1.1.mp4
Screen.Recording.2026-01-15.at.00.34.19.mp4
Screen.Recording.2026-01-15.at.00.38.12.mp4
📝 Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.