fix(grid): fix target error in shadow dom #3651
Conversation
WalkthroughIntroduces a safer event target utility via optional chaining in utils and adopts it across Grid keyboard, table, and global mousedown handlers. Replaces direct event.target usage with getActualTarget(event). Also bumps the Grid package version to 3.25.1 and reorders a dependency entry. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DOM Event
participant getActualTarget
participant Grid Components
User->>DOM Event: Click/Scroll/MouseDown
DOM Event->>getActualTarget: Resolve actual target (shadow DOM aware)
getActualTarget-->>Grid Components: actualTarget
Grid Components->>Grid Components: Containment checks, blur/clear/scroll logic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/utils/src/event/index.ts (1)
61-61: Guard composedPath presence to avoid rare runtime errorsOptional chaining on target is good. For extra safety, also guard that composedPath exists before calling it, since some environments may not implement it even if
composedis truthy.Apply this diff:
-return e.target?.shadowRoot && e.composed ? e.composedPath()[0] || e.target : e.target +return e.target?.shadowRoot && e.composed && typeof e.composedPath === 'function' + ? e.composedPath()[0] || e.target + : e.targetpackages/vue/src/grid/src/table/src/methods.ts (3)
1216-1216: Avoid potential TypeError when args.cell is undefinedIf
argsexists butargs.cellis undefined,args?.cell.contains(...)can throw. Chain thecellas well.Apply this diff:
- if (args?.cell.contains(getActualTarget(event))) { + if (args?.cell?.contains(getActualTarget(event))) { return true }
1628-1632: Defensive access for scrollTop to handle unexpected targetsIn rare cases,
getActualTarget(event)might not be a scrollable element. Use optional chaining with a sane default to avoid runtime errors and keep pagination math stable.Apply this diff:
- let scrollTop = getActualTarget(event).scrollTop + const target = getActualTarget(event) + let scrollTop = (target && 'scrollTop' in (target as any)) ? (target as any).scrollTop : 0
1652-1654: Same here: guard scrollTop destructuringAvoid destructuring from a possibly nullish or non-scrollable target.
Apply this diff:
- let { scrollTop } = getActualTarget(event) + const target = getActualTarget(event) + const scrollTop = (target && 'scrollTop' in (target as any)) ? (target as any).scrollTop : 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/utils/src/event/index.ts(1 hunks)packages/vue/src/grid/package.json(3 hunks)packages/vue/src/grid/src/keyboard/src/methods.ts(2 hunks)packages/vue/src/grid/src/table/src/methods.ts(5 hunks)packages/vue/src/grid/src/table/src/utils/handleGlobalMousedownEvent.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/vue/src/grid/src/table/src/methods.ts (1)
packages/utils/src/event/index.ts (1)
getActualTarget(57-62)
packages/vue/src/grid/src/table/src/utils/handleGlobalMousedownEvent.ts (1)
packages/utils/src/event/index.ts (1)
getActualTarget(57-62)
packages/vue/src/grid/src/keyboard/src/methods.ts (1)
packages/utils/src/event/index.ts (1)
getActualTarget(57-62)
🔇 Additional comments (8)
packages/vue/src/grid/src/keyboard/src/methods.ts (2)
25-25: LGTM: centralizing target resolutionImporting
getActualTargethere aligns target handling with Shadow DOM cases across the Grid.
670-677: Robust outside-click detection with Shadow DOM-aware targetUsing
actualTarget = getActualTarget(event)and the unifiedequalOrContainchecks addresses retargeting issues under Shadow DOM while remaining null-safe (Node.contains accepts null).packages/vue/src/grid/src/table/src/utils/handleGlobalMousedownEvent.ts (2)
25-25: LGTM: shared utility importPulling
getActualTargetfrom utils is consistent with other modules.
82-89: Containment checks now honor Shadow DOM internalsSwitching to
actualTarget = getActualTarget(event)and using it for header/table containment removes false negatives when events originate inside shadow roots. Logic remains equivalent otherwise.packages/vue/src/grid/src/table/src/methods.ts (2)
26-26: LGTM: import addition for Shadow DOM-aware targetingThis keeps target handling consistent across Grid internals.
1751-1751: LGTM: wheel containment uses actual targetChecking
$el.contains(getActualTarget(event))correctly treats wheel events originating inside shadow descendants as “inside”.packages/vue/src/grid/package.json (2)
4-4: Version bump looks correct3.25.1 aligns with a bugfix-level change.
15-16: Dependency reorder is fineRelocating
@opentiny/utilsto the top keeps the dependency set unchanged.Also applies to: 30-31
2 participants
PR
修复在shadow dom下，表格部分target指向错误问题
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Bug Fixes
Chores