fix(grid): fix target error in shadow root #3659
Conversation
WalkthroughEvent handling updated to resolve the actual DOM target via a shared utility, replacing direct event.target usage across grid keyboard, table scroll/blur, and global mousedown handlers. The utility gained optional chaining safety. Grid package version bumped with a utils dependency adjustment. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser as Browser Event
participant Utils as getActualTarget()
participant Grid as Grid Components
participant Table as Table (Keyboard/Scroll)
User->>Browser: Click/Scroll Event
Browser->>Utils: Resolve actual target (event)
Utils-->>Browser: actualTarget
Browser->>Grid: Containment checks with actualTarget
Browser->>Table: Blur/Scroll handling with actualTarget.scrollTop
alt Outside element
Table->>Table: Clear selections/copies
else Inside element
Table->>Table: Maintain state / compute scroll
end
note over Utils,Table: New: all checks use actualTarget instead of event.target
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/utils/src/event/index.ts (1)
57-62: Harden getActualTarget for broader Shadow DOM cases and better typingCurrent logic relies on
e.composedand assumescomposedPath()returns an element with the right shape. Two tweaks improve resilience:
- Prefer
composedPath?.()when available (independent ofe.composed), and gracefully fall back.- Normalize Text nodes to their parentElement; add TS types for clarity.
Apply this diff:
-export const getActualTarget = (e) => { - if (!e || !e.target) { - return null - } - return e.target?.shadowRoot && e.composed ? e.composedPath()[0] || e.target : e.target -} +export const getActualTarget = (e: Event & { target?: EventTarget | null }): Node | null => { + if (!e) return null + const t = (e as any).target as EventTarget | null + const path = typeof (e as any).composedPath === 'function' ? (e as any).composedPath() : [] + let first = (path && path.length ? path[0] : t) as Node | null + // Promote Text nodes to their parent element when possible + if (first && first.nodeType === Node.TEXT_NODE) { + first = (first as any).parentElement ?? first + } + return first +}packages/vue/src/grid/src/table/src/utils/handleGlobalMousedownEvent.ts (1)
82-89: Condition reads well; optional micro-optimization to cache header elementLogic is correct and
Node.contains(null)is safe (returns false). If you want to shave a lookup and improve readability, cache the header element and boolean.Apply this diff:
- const actualTarget = getActualTarget(event) + const actualTarget = getActualTarget(event) + const headerEl = _vm.$refs.tableHeader?.$el + const inHeader = headerEl ? headerEl.contains(actualTarget) : false @@ - (_vm.$refs.tableHeader && _vm.$refs.tableHeader.$el.contains(actualTarget)) || + inHeader || (tableContent && !tableContent.contains(actualTarget))packages/vue/src/grid/src/table/src/methods.ts (1)
1747-1747: Minor: avoid recomputing actualTarget and aid TS inferenceThis is minor, but caching the node improves clarity and prevents recomputation.
Apply this diff:
- if (scrollLoad && $el.contains(getActualTarget(event))) { + const t = getActualTarget(event) as Node | null + if (scrollLoad && $el.contains(t)) {
📜 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/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)
packages/vue/src/grid/src/table/src/methods.ts (1)
packages/utils/src/event/index.ts (1)
getActualTarget(57-62)
🔇 Additional comments (6)
packages/vue/src/grid/src/table/src/utils/handleGlobalMousedownEvent.ts (1)
25-25: Good call: centralizing target resolution via getActualTargetImporting
getActualTargetkeeps Shadow DOM behavior consistent across handlers. No issues.packages/vue/src/grid/src/keyboard/src/methods.ts (2)
25-25: Importing getActualTarget is aligned with the Shadow DOM fixThis keeps keyboard interactions consistent with the new target resolution utility.
670-677: No lingering directevent.targetchecks found
Verified with ripgrep acrosspackages/vue/src/grid/src/keyboard—no matches forevent.targetremain.packages/vue/src/grid/package.json (1)
4-4: Confirmed: getActualTarget is properly re-exportedThe
getActualTargetfunction is exported frompackages/utils/src/index.ts(line 155), ensuring that imports will resolve correctly outside the workspace.No further action required.
packages/vue/src/grid/src/table/src/methods.ts (2)
26-26: Importing getActualTarget here is consistent with the rest of the PRKeeps table methods aligned with the shared target-resolution behavior.
1216-1217: Blur-inside check updated — LGTM
args?.cell.contains(getActualTarget(event))correctly preserves edit state when clicking within the active cell, even in Shadow DOM.
| @@ -23,7 +23,7 @@ | |||
| * | |||
| */ | |||
| import { getColumnList, assemColumn, repairFixed } from '@opentiny/vue-renderless/grid/utils' | |||
| import { toDecimal } from '@opentiny/utils' | |||
| import { toDecimal, getActualTarget } from '@opentiny/utils' | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Repository-wide consistency check for event.target → getActualTarget migration
To ensure there are no remaining shadow-root-sensitive call sites in the grid/table code using raw event.target, run:
Also applies to: 1216-1217, 1624-1625, 1648-1649, 1747-1747
🏁 Script executed:
#!/bin/bash
# 1) Direct event.target usage in grid table code
rg -nP -C2 --type=ts '\bevent\.target\b' packages/vue/src/grid/src/table
# 2) Places reading scrollTop from event target
rg -nP -C2 --type=ts '\b(event|e)\.target\.scrollTop\b' packages/vue/src/grid
# 3) Containment checks that still pass event.target directly
rg -nP -C2 --type=ts 'contains\(\s*event\.target\s*\)' packages/vue/src/gridLength of output: 707
Migrate remaining event.target usage in triggerCellClickEvent.ts
A raw event.target reference was detected in the grid/table utils and needs to be updated to use getActualTarget so that shadow-root elements are handled correctly.
• packages/vue/src/grid/src/table/src/utils/triggerCellClickEvent.ts:26
- const target = event.target
+ const target = getActualTarget(event)Please replace all direct event.target references in this file with getActualTarget(event), and ensure any downstream logic (e.g. containment checks or property reads) operates on the resolved target. Once updated, rerun the repository-wide grep from the original comment to confirm no raw event.target calls remain.
📝 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.
| import { toDecimal, getActualTarget } from '@opentiny/utils' | |
| // Replace this: | |
| - const target = event.target | |
| // With: | |
| const target = getActualTarget(event) |
🤖 Prompt for AI Agents
In packages/vue/src/grid/src/table/src/utils/triggerCellClickEvent.ts around
line 26, the file still uses raw event.target which breaks with shadow DOM;
replace every direct event.target access with const target =
getActualTarget(event) (import getActualTarget if not already) and update all
downstream checks and property reads to use that resolved target (e.g.,
containment checks, attribute/property reads, parent traversal). After changes,
run the repository-wide grep from the original comment to confirm no remaining
raw event.target usages.
| let scrollTop = getActualTarget(event).scrollTop | ||
|
|
There was a problem hiding this comment.
Guard against non-scrollable targets to avoid NaN pagination math
getActualTarget(event) can return nodes without scrollTop (e.g., a Text or non-scrollable element via composedPath[0]). If scrollTop is undefined, Math.ceil(scrollTop / rowHeight) becomes NaN, and pageCurrentChange(NaN) may fire.
Apply this defensive fallback to the table body wrapper:
- let scrollTop = getActualTarget(event).scrollTop
+ const t = getActualTarget(event) as any
+ const bodyEl = this.$refs.tableBody?.$el as any
+ let scrollTop = (typeof t?.scrollTop === 'number' ? t.scrollTop : bodyEl?.scrollTop) ?? 0📝 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.
| let scrollTop = getActualTarget(event).scrollTop | |
| const t = getActualTarget(event) as any | |
| const bodyEl = this.$refs.tableBody?.$el as any | |
| let scrollTop = (typeof t?.scrollTop === 'number' ? t.scrollTop : bodyEl?.scrollTop) ?? 0 |
🤖 Prompt for AI Agents
In packages/vue/src/grid/src/table/src/methods.ts around lines 1624-1625,
getActualTarget(event).scrollTop can be undefined for non-scrollable nodes which
causes NaN in pagination math; to fix, first capture the actual target into a
variable, then compute scrollTop with a defensive fallback (e.g., if target is
null or its scrollTop is not a number, use 0) and use that numeric value for the
subsequent Math.ceil/rowHeight calculation; apply the same defensive check when
reading scrollTop from the table body wrapper.
| let { scrollTop } = getActualTarget(event) | ||
| let toVisibleIndex = Math.ceil(scrollTop / rowHeight) |
There was a problem hiding this comment.
Same robustness issue in virtual Y scroll calculation
Destructuring from getActualTarget(event) will throw if it returns null, and even when not null, scrollTop may be undefined. Mirror the fallback used above.
Apply this diff:
- let { scrollTop } = getActualTarget(event)
+ const t = getActualTarget(event) as any
+ const bodyEl = this.$refs.tableBody?.$el as any
+ const scrollTop = (typeof t?.scrollTop === 'number' ? t.scrollTop : bodyEl?.scrollTop) ?? 0📝 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.
| let { scrollTop } = getActualTarget(event) | |
| let toVisibleIndex = Math.ceil(scrollTop / rowHeight) | |
| const t = getActualTarget(event) as any | |
| const bodyEl = this.$refs.tableBody?.$el as any | |
| const scrollTop = (typeof t?.scrollTop === 'number' ? t.scrollTop : bodyEl?.scrollTop) ?? 0 | |
| let toVisibleIndex = Math.ceil(scrollTop / rowHeight) |
🤖 Prompt for AI Agents
In packages/vue/src/grid/src/table/src/methods.ts around lines 1648-1649, avoid
destructuring directly from getActualTarget(event) because it can return null
and scrollTop can be undefined; instead retrieve the target with a safe fallback
(e.g., const target = getActualTarget(event) || {}) and read scrollTop using a
nullish/defined fallback (e.g., const scrollTop = target.scrollTop ?? 0) before
computing toVisibleIndex = Math.ceil(scrollTop / rowHeight) so it mirrors the
robustness used earlier.
PR
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