feat(focus-scope): add shadow root support #2352
Conversation
commit: |
ldelhommeau
force-pushed
the
feat/focus-scope-shadow-root
branch
from
06f93d4 to
54ff6df
Compare
📝 WalkthroughWalkthroughFocusScope gained Shadow DOM compatibility: event listeners and MutationObserver now resolve and bind to the element's root (Document or ShadowRoot); tabbable discovery recurses into shadow roots; Tab handling and focus trapping operate relative to the resolved root; tests and Storybook examples for mixed light/shadow scenarios were added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FocusScope
participant getEventRoot
participant Root as Root(ShadowRoot/Document)
participant TabHandler
participant getTabbable
User->>FocusScope: focusin/focusout event or Tab key
FocusScope->>getEventRoot: resolve root for container
getEventRoot-->>FocusScope: return Root
FocusScope->>Root: attach/remove listeners / observe mutations
Root-->>FocusScope: emit focus event or keydown
alt Tab pressed
FocusScope->>TabHandler: handle Tab within resolved root
TabHandler->>getTabbable: collect tabbable candidates from container/root
getTabbable->>getTabbable: recurse into shadowRoots
getTabbable-->>TabHandler: merged candidates
TabHandler->>FocusScope: move focus to next candidate (support looping)
else focusin/focusout
FocusScope->>FocusScope: update internal focus state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧬 Code graph analysis (1)packages/core/src/FocusScope/FocusScope.test.ts (2)
🪛 ast-grep (0.40.5)packages/core/src/FocusScope/FocusScope.test.ts[warning] 262-262: 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. (dom-content-modification) 🔇 Additional comments (7)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/src/FocusScope/FocusScope.test.ts`:
- Around line 348-357: The test uses un-awaited waitFor calls after
userEvent.tab() which can cause flakiness; update the FocusScope.test assertions
to await each waitFor invocation (the ones asserting queryRoot.activeElement,
document.activeElement/shadowRoot.activeElement, and the final loop-back check)
so that the async assertions complete before proceeding—specifically add await
before each waitFor that wraps expect(...) to ensure the test waits for the
focus changes triggered by userEvent.tab().
In `@packages/core/src/FocusScope/story/shadowDom/DialogShadowRoot.vue`:
- Line 46: The template passes a :portal-target="portalTarget" to
FocusableLayersElements but FocusableLayersElements.vue declares no props, so
this attribute is a fallthrough; either remove the :portal-target binding from
DialogShadowRoot.vue if nested portals don’t need it, or add a prop definition
named portalTarget (kebab portal-target / camel portalTarget) to
FocusableLayersElements.vue (with an appropriate type like Element|String|null
and default null) and forward it where needed for nested portal mounting; update
usages inside FocusableLayersElements.vue to read this.portalTarget when
rendering portals.
🧹 Nitpick comments (7)
packages/core/src/FocusScope/utils.ts (1)
52-56: Side-effect inacceptNodefilter is unconventional but works.The recursive collection of shadow DOM nodes inside the
acceptNodecallback is a side-effect pattern that works but can be surprising. Consider extracting this to a separate traversal step for clarity.Also, the cast
as unknown as HTMLElementis technically imprecise sinceshadowRootis aShadowRoot(extendsDocumentFragment), but it works becausecreateTreeWalkeraccepts anyNode.packages/core/src/FocusScope/FocusScope.vue (2)
132-154: Consider reusing the storedrootreference for cleanup.The cleanup function recomputes
getEventRoot(container)at line 146. While unlikely in practice, if the DOM structure changed during the component lifecycle, this could remove listeners from a different root than where they were added.♻️ Suggested refactor to reuse root reference
const root = getEventRoot(container) if (root instanceof Document) { root.addEventListener('focusin', handleFocusIn) root.addEventListener('focusout', handleFocusOut) } else { root.addEventListener('focusin', handleFocusIn as EventListener) root.addEventListener('focusout', handleFocusOut as EventListener) } const mutationObserver = new MutationObserver(handleMutations) if (container) mutationObserver.observe(container, { childList: true, subtree: true }) cleanupFn(() => { - const cleanupRoot = getEventRoot(container) - if (cleanupRoot instanceof Document) { - cleanupRoot.removeEventListener('focusin', handleFocusIn) - cleanupRoot.removeEventListener('focusout', handleFocusOut) + if (root instanceof Document) { + root.removeEventListener('focusin', handleFocusIn) + root.removeEventListener('focusout', handleFocusOut) } else { - cleanupRoot.removeEventListener('focusin', handleFocusIn as EventListener) - cleanupRoot.removeEventListener('focusout', handleFocusOut as EventListener) + root.removeEventListener('focusin', handleFocusIn as EventListener) + root.removeEventListener('focusout', handleFocusOut as EventListener) } mutationObserver.disconnect() })
195-197: Fallback todocument.bodyon unmount may be suboptimal in shadow DOM contexts.When unmounting within a shadow root, if
previouslyFocusedElementis null, falling back todocument.bodymight not provide the best UX. Consider whether the shadow host or a designated element would be more appropriate.This is a minor edge case and acceptable for now.
packages/core/src/FocusScope/FocusScope.test.ts (2)
155-160: Consider handling regular document root for robustness.The
getActiveElementhelper returnsnullwhen the root is not a ShadowRoot. While this works for current shadow DOM-only usage, returningdocument.activeElementfor regular document roots would make the helper more versatile.♻️ Optional improvement
function getActiveElement(container: Element): Element | null { const root = container.getRootNode() if ((root as ShadowRoot).host) return (root as ShadowRoot).activeElement - return null + return (root as Document).activeElement }
339-339: Test description may be misleading forbodyOnlycase.The test name "should loop focus within the FocusScope in the ShadowRoot" doesn't accurately describe the
bodyOnlycase, which tests focus looping in the regular document body without a ShadowRoot.♻️ Suggested improvement
- it('should loop focus within the FocusScope in the ShadowRoot', async () => { + it('should loop focus within the FocusScope', async () => {packages/core/src/FocusScope/story/shadowDom/ShadowRootContainer.vue (2)
25-27: Vue app instance not unmounted on cleanup - potential memory leak.The
createAppcreates a new Vue app instance, but it's never stored or explicitly unmounted whenresetShadowRootis called or when the component unmounts. For story/demo purposes this is likely acceptable, but in production usage this could cause memory leaks.♻️ Suggested fix to properly manage app lifecycle
+import type { App, Component } from 'vue' -import type { Component } from 'vue' -import { createApp, useTemplateRef, watch } from 'vue' +import { createApp, onBeforeUnmount, useTemplateRef, watch } from 'vue' import DialogShadowRoot from './DialogShadowRoot.vue' import FocusableLayersElements from './FocusableLayersElements.vue' const props = defineProps<{ withDialog?: boolean }>() +let currentApp: App | 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, { + currentApp = createApp(component, { portalTarget: shadowPortalTarget, }).mount(shadowMountPoint) } function resetShadowRoot(container: HTMLDivElement) { + if (currentApp) { + currentApp.unmount() + currentApp = null + } const elementWithShadow = container as Element & { shadowRoot: ShadowRoot | null } if (elementWithShadow.shadowRoot) { elementWithShadow.shadowRoot.innerHTML = '' } } +onBeforeUnmount(() => { + if (currentApp) { + currentApp.unmount() + currentApp = null + } +})
21-23: Static portal target ID may cause conflicts with multiple containers.The portal target uses a hardcoded ID
portal-shadow-root. If multipleShadowRootContainerinstances exist in the same document, this could cause ID conflicts. Since this is a story/demo component, this is likely acceptable, but consider using a unique ID if this pattern is reused elsewhere.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/core/src/FocusScope/FocusScope.test.tspackages/core/src/FocusScope/FocusScope.vuepackages/core/src/FocusScope/story/FocusScopeShadowRoot.story.vuepackages/core/src/FocusScope/story/shadowDom/DialogBodyOnly.vuepackages/core/src/FocusScope/story/shadowDom/DialogMixedBodyAndShadowRoot.vuepackages/core/src/FocusScope/story/shadowDom/DialogShadowRoot.vuepackages/core/src/FocusScope/story/shadowDom/FocusableLayersElements.vuepackages/core/src/FocusScope/story/shadowDom/ShadowRootContainer.vuepackages/core/src/FocusScope/utils.ts
🧰 Additional context used
🪛 ast-grep (0.40.5)
packages/core/src/FocusScope/FocusScope.test.ts
[warning] 263-263: 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 (11)
packages/core/src/FocusScope/FocusScope.vue (1)
71-76: LGTM!The
getEventRoothelper correctly usesgetRootNode()to determine the appropriate event target for shadow DOM vs. document contexts. The fallback todocumentwhen not in a shadow root is appropriate.packages/core/src/FocusScope/story/shadowDom/FocusableLayersElements.vue (1)
1-15: LGTM!Simple and clean story component for testing focus behavior within shadow DOM contexts.
packages/core/src/FocusScope/story/FocusScopeShadowRoot.story.vue (1)
1-15: LGTM!Clean Storybook story setup for demonstrating FocusScope behavior with shadow DOM.
packages/core/src/FocusScope/story/shadowDom/DialogShadowRoot.vue (1)
22-59: LGTM!The Dialog composition correctly uses
DialogPortalwith a custom target for shadow DOM rendering. The structure follows established patterns and properly demonstrates focus management in shadow DOM contexts.packages/core/src/FocusScope/utils.ts (1)
80-92:isHiddenlimitation with shadow boundaries is an existing design constraint.The
parentElementtraversal inisHiddendoesn't cross shadow DOM boundaries. However, this is intentional by design: the function is always called with anupToparameter (fromfindVisible()) that explicitly stops visibility checks at the container boundary. Elements are only checked for visibility within their containing scope, and the container itself (whether in light DOM or shadow DOM) serves as the boundary—not something this function checks beyond.Extensive shadow DOM test coverage confirms this design works correctly across all scenarios (shadow-only, mixed body/shadow, and body-only cases).
packages/core/src/FocusScope/story/shadowDom/DialogBodyOnly.vue (1)
1-18: LGTM - Story component for light DOM dialog testing.The component correctly demonstrates dialog usage with proper accessibility attributes and test IDs. Minor note: there's a trailing empty line in the import block (line 12) that could be cleaned up for consistency.
packages/core/src/FocusScope/FocusScope.test.ts (3)
1-12: LGTM - Appropriate imports for shadow DOM testing.The new imports correctly support the shadow DOM test scenarios being added.
30-30: Good clarity improvement.Renaming the test suite to explicitly indicate "(light DOM)" improves readability and distinguishes it from the new shadow DOM tests.
263-264: Static analysis false positive - safe test cleanup.The
document.body.innerHTML = ''on line 264 is safe test cleanup code that resets the DOM between tests. No user input is involved, so there's no XSS risk.packages/core/src/FocusScope/story/shadowDom/DialogMixedBodyAndShadowRoot.vue (1)
1-57: LGTM - Well-structured mixed DOM story component.The component correctly demonstrates the mixed scenario where the dialog lives in the document body while the form content is rendered inside a shadow root via
ShadowRootContainer. This is an important test case for validating focus management across DOM boundaries.packages/core/src/FocusScope/story/shadowDom/ShadowRootContainer.vue (1)
51-56: LGTM - Clean template structure.The template is straightforward and correctly sets up the container element for shadow root attachment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/core/src/FocusScope/story/shadowDom/DialogShadowRoot.vue
Outdated
Show resolved
Hide resolved
ldelhommeau
force-pushed
the
feat/focus-scope-shadow-root
branch
from
3624989 to
c221d9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/src/FocusScope/FocusScope.vue`:
- Around line 132-154: Capture the event root once after calling
getEventRoot(container) (the existing const root) and close over that value in
the cleanupFn instead of calling getEventRoot(container) again; update cleanupFn
to reference the previously computed root when removing listeners (for both
Document and non-Document branches) so handleFocusIn/handleFocusOut are removed
from the same root they were added to, and do not re-resolve container which can
be detached and return a different root.
In `@packages/core/src/FocusScope/story/shadowDom/ShadowRootContainer.vue`:
- Around line 25-27: createApp(...).mount(...) is called without keeping the
returned app instance so the Vue app isn't unmounted before resetShadowRoot
clears innerHTML; store the result of createApp(...) in a variable (e.g.,
shadowApp) when mounting to shadowMountPoint, and call shadowApp.unmount()
inside resetShadowRoot (or before setting shadowRoot.innerHTML = '') to properly
teardown the Vue app and avoid leaks; apply the same change for the other
occurrences around lines 30-35 that mount into
shadowPortalTarget/shadowMountPoint.
♻️ Duplicate comments (2)
packages/core/src/FocusScope/story/shadowDom/DialogShadowRoot.vue (1)
46-46:FocusableLayersElementsdoes not accept aportal-targetprop.This was flagged in a previous review. The prop will be ignored or treated as a fallthrough attribute.
packages/core/src/FocusScope/FocusScope.test.ts (1)
348-356: MissingawaitonwaitForcalls will cause flaky tests.The
waitForcalls are not awaited, which means assertions may complete after the test proceeds. This can lead to false positives or intermittent failures.🐛 Proposed fix
await userEvent.tab() - waitFor(() => expect(queryRoot.activeElement).toBe(emailInput)) + await waitFor(() => expect(queryRoot.activeElement).toBe(emailInput)) await userEvent.tab() - waitFor(() => expect(queryRoot.activeElement).toBe(submitButton)) + await waitFor(() => expect(queryRoot.activeElement).toBe(submitButton)) await userEvent.tab() - waitFor(() => expect(testCase === 'shadowDomOnly' ? shadowRoot.activeElement : document.activeElement).toBe(closeDialogButton)) + await waitFor(() => expect(testCase === 'shadowDomOnly' ? shadowRoot.activeElement : document.activeElement).toBe(closeDialogButton)) // Tab again should loop back to the first focusable element await userEvent.tab() - waitFor(() => expect(queryRoot.activeElement).toBe(nameInput)) + await waitFor(() => expect(queryRoot.activeElement).toBe(nameInput))
🧹 Nitpick comments (1)
packages/core/src/FocusScope/FocusScope.test.ts (1)
339-339: Test description is misleading for thebodyOnlycase.The description "should loop focus within the FocusScope in the ShadowRoot" doesn't accurately describe the
bodyOnlytest case, which operates entirely in the light DOM.✏️ Suggested improvement
- it('should loop focus within the FocusScope in the ShadowRoot', async () => { + it('should loop focus within the FocusScope', async () => {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/core/src/FocusScope/FocusScope.test.tspackages/core/src/FocusScope/FocusScope.vuepackages/core/src/FocusScope/story/FocusScopeShadowRoot.story.vuepackages/core/src/FocusScope/story/shadowDom/DialogBodyOnly.vuepackages/core/src/FocusScope/story/shadowDom/DialogMixedBodyAndShadowRoot.vuepackages/core/src/FocusScope/story/shadowDom/DialogShadowRoot.vuepackages/core/src/FocusScope/story/shadowDom/FocusableLayersElements.vuepackages/core/src/FocusScope/story/shadowDom/ShadowRootContainer.vuepackages/core/src/FocusScope/utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/FocusScope/story/shadowDom/DialogBodyOnly.vue
- packages/core/src/FocusScope/story/FocusScopeShadowRoot.story.vue
- packages/core/src/FocusScope/story/shadowDom/FocusableLayersElements.vue
🧰 Additional context used
🪛 ast-grep (0.40.5)
packages/core/src/FocusScope/FocusScope.test.ts
[warning] 263-263: 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 (9)
packages/core/src/FocusScope/utils.ts (1)
42-65: LGTM! Shadow DOM traversal implemented correctly.The recursive handling properly collects tabbable candidates from shadow roots while still evaluating the host element's tabbability independently. The approach correctly handles nested shadow DOMs.
packages/core/src/FocusScope/story/shadowDom/DialogMixedBodyAndShadowRoot.vue (1)
1-57: LGTM!The story component is well-structured and demonstrates the mixed body/shadow-root dialog scenario effectively. Accessibility is properly handled with the
aria-labelon the close button.packages/core/src/FocusScope/story/shadowDom/DialogShadowRoot.vue (1)
1-45: LGTM!The dialog structure and accessibility handling look correct. The component properly uses the
portalTargetprop to render the dialog portal inside the shadow root.Also applies to: 47-60
packages/core/src/FocusScope/FocusScope.vue (1)
71-76: LGTM on the helper function.
getEventRootcorrectly determines whether to use the ShadowRoot or document based on the container's root node.packages/core/src/FocusScope/FocusScope.test.ts (5)
1-12: LGTM!The new imports are well-organized and all appear to be utilized in the shadow DOM test suite.
30-30: LGTM!Good practice to distinguish the existing light DOM tests from the new shadow root tests.
145-160: LGTM!The helper functions are well-designed for shadow DOM testing:
renderInShadowRootproperly creates and manages the shadow DOM containergetActiveElementcorrectly retrieves the active element from the shadow root context
162-198: LGTM!This test directly validates the core issue from the PR objectives - ensuring focus remains on an input inside a Shadow DOM when content dynamically updates. Good use of
try/finallyfor cleanup.
263-264: Test cleanup pattern is safe here.The static analysis warning about
innerHTMLmodification is a false positive in this test context. Clearingdocument.body.innerHTMLbefore each test is a standard practice for test isolation and doesn't involve user input.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/core/src/FocusScope/story/shadowDom/ShadowRootContainer.vue
Outdated
Show resolved
Hide resolved
ldelhommeau
force-pushed
the
feat/focus-scope-shadow-root
branch
2 times, most recently
from
932bfbd to
6971453
Compare
There was a problem hiding this comment.
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/FocusScope/FocusScope.test.ts (1)
65-65: MissingawaitonwaitFormay cause flaky test.This
waitForcall is not awaited, which means the assertion may not complete before the test ends. This pattern also appears at lines 108 and 138.🐛 Proposed fix
- waitFor(() => expect(tabbableLast).toBe(document.activeElement)) + await waitFor(() => expect(tabbableLast).toBe(document.activeElement))
🧹 Nitpick comments (3)
packages/core/src/FocusScope/story/shadowDom/ShadowRootContainer.vue (2)
9-9: Module-levelshadowAppmay cause issues with multiple instances.The
shadowAppvariable is scoped to the module rather than the component instance. If multipleShadowRootContainercomponents are mounted simultaneously, they would share this variable, causing the second instance to overwrite the first's reference.For a story/demo component with single-instance usage, this is likely acceptable. However, if you anticipate multiple simultaneous instances, consider moving this into the component's reactive state.
44-55: Consider adding cleanup on component unmount.The
watchhandles mounting and resetting when the container ref changes, but there's no cleanup when the component itself is unmounted. If the parent component destroysShadowRootContainer, the Vue app inside the shadow root will remain mounted.♻️ Suggested fix
-import { createApp, useTemplateRef, watch } from 'vue' +import { createApp, onUnmounted, useTemplateRef, watch } from 'vue'Then add after the watch:
onUnmounted(() => { if (shadowApp) { shadowApp.unmount() shadowApp = null } })packages/core/src/FocusScope/FocusScope.test.ts (1)
270-276: Consider replacingsleep(1)with explicit wait conditions.Fixed delays can cause flaky tests. The
sleep(1)calls after clicking the trigger could be replaced withwaitForon the expected dialog state.♻️ Suggested alternative
- await fireEvent.click(trigger) - await sleep(1) - const dialogOverlay = getDialogOverlay() - expect(dialogOverlay).toBeTruthy() + await fireEvent.click(trigger) + await waitFor(() => expect(getDialogOverlay()).toBeTruthy()) + const dialogOverlay = getDialogOverlay()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/src/FocusScope/FocusScope.test.tspackages/core/src/FocusScope/FocusScope.vuepackages/core/src/FocusScope/story/FocusScopeShadowRoot.story.vuepackages/core/src/FocusScope/story/shadowDom/FocusableLayersElements.vuepackages/core/src/FocusScope/story/shadowDom/ShadowRootContainer.vuepackages/core/src/FocusScope/story/shadowDom/_Dialog.vuepackages/core/src/FocusScope/utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/FocusScope/FocusScope.vue
- packages/core/src/FocusScope/story/shadowDom/FocusableLayersElements.vue
- packages/core/src/FocusScope/story/FocusScopeShadowRoot.story.vue
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/FocusScope/FocusScope.test.ts (2)
packages/core/src/shared/index.ts (1)
getActiveElement(6-6)packages/plugins/src/namespaced/index.ts (1)
Dialog(275-293)
🪛 ast-grep (0.40.5)
packages/core/src/FocusScope/FocusScope.test.ts
[warning] 262-262: 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 (4)
packages/core/src/FocusScope/utils.ts (1)
42-66: Shadow DOM recursion approach looks correct for typical use cases.The recursive traversal into shadow roots is a good approach for discovering tabbable elements across shadow boundaries. The signature expansion to accept
ShadowRootis appropriate sinceTreeWalkeraccepts anyNodeas root.One edge case to be aware of: if a shadow host element itself has
tabIndex >= 0, the shadow children will be added to the array before the host element (due to the push insideacceptNodehappening before the walker loop's push). This would produce[shadowChild1, shadowChild2, host]instead of[host, shadowChild1, shadowChild2]. In practice, tabbable shadow hosts are rare, so this is likely acceptable.packages/core/src/FocusScope/story/shadowDom/_Dialog.vue (1)
1-67: LGTM!The dialog story component is well-structured with clear conditional rendering logic for demonstrating both shadow DOM and light DOM scenarios. The component properly integrates with the existing Dialog primitives.
packages/core/src/FocusScope/FocusScope.test.ts (2)
143-197: Well-structured shadow DOM test utilities.The
renderInShadowRoothelper andgetActiveElementfunction provide clean abstractions for testing within shadow DOM contexts. The test properly validates focus behavior during dynamic content updates within a shadow root.
199-349: Comprehensive shadow DOM focus loop test coverage.The test matrix covering
shadowDomOnly,mixedBodyAndShadowDom, andbodyOnlyscenarios provides excellent coverage for the FocusScope changes. The helper functions handle query root differences cleanly, and the focus loop assertions properly awaitwaitForcalls.Note: The static analysis warning about
document.body.innerHTML = ''on line 263 is a false positive—this is standard test cleanup code, not user input handling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ldelhommeau
force-pushed
the
feat/focus-scope-shadow-root
branch
from
755008e to
e57629c
Compare
1 participant
🔗 Linked issue
#1667
This PR only addresses the FocusScope issue, not the dismissible layer issue mentioned in the original issue.
❓ Type of change
📚 Description
Resolves #1667 partially, just about the focus scope issue:
📸 Screenshots (if appropriate)
Now the FocusScope behaves correctly for the dialog components when they are teleported inside the shadow root:
https://github.com/user-attachments/assets/084fa28d-9cb6-4d82-be6b-7bfe9845e1f8
📝 Checklist
docs/content/docs/utilities/focus-scope.md, but I don't think it's useful to add implementation details about the shadow root inside.Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.