fix(autocomplete,search,base-select,cascader,date-panel, date-range,date-picker,dropdown,input,select,tree): The component under the shadowRoot node event is invalid. #3546
Conversation
|
""" WalkthroughThe update refines click outside event handlers in both the Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/renderless/src/search/index.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/vue-directive/src/clickoutside.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 2
🧹 Nitpick comments (1)
packages/vue-directive/src/clickoutside.ts (1)
36-41: Apply optional chaining for better code safety.The static analysis suggestion is valid - the shadowRoot check can be simplified using optional chaining.
-const isInElementOrShadow = (el, target) => { - if (el.contains(target)) return true - if (el.shadowRoot && el.shadowRoot.contains(target)) return true - return false -} +const isInElementOrShadow = (el, target) => { + if (el.contains(target)) return true + if (el.shadowRoot?.contains(target)) return true + return false +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/vue-directive/src/clickoutside.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*`: 用于写产品需求和开发文档的文件，需求应尽量写得简单清晰易懂。 在需求设计阶段应遵从逐步迭代的原则，早期版本可以尽量简洁。
**/*: 用于写产品需求和开发文档的文件，需求应尽量写得简单清晰易懂。
在需求设计阶段应遵从逐步迭代的原则，早期版本可以尽量简洁。
📄 Source: CodeRabbit Inference Engine (.cursor/rules/product.mdc)
List of files the instruction was applied to:
packages/vue-directive/src/clickoutside.ts
🪛 Biome (1.9.4)
packages/vue-directive/src/clickoutside.ts
[error] 39-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 45-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
packages/vue-directive/src/clickoutside.ts (2)
50-51: LGTM: Correct usage of Shadow DOM helper function.The replacement of direct
el.containscalls with theisInElementOrShadowhelper function properly extends click detection to include Shadow DOM boundaries.
65-77: LGTM: Well-structured Shadow DOM listener management.The helper functions correctly handle adding and removing event listeners on shadowRoot elements with proper capture phase usage.
| // 移除 shadowRoot 事件监听 | ||
| removeShadowRootListeners(el, el[nameSpace].documentHandler) |
There was a problem hiding this comment.
Fix handler reference for proper cleanup.
The current code cannot properly remove event listeners because it references el[nameSpace].documentHandler instead of the actual registered handler.
// 移除 shadowRoot 事件监听
- removeShadowRootListeners(el, el[nameSpace].documentHandler)
+ removeShadowRootListeners(el, el[nameSpace].shadowHandler)📝 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.
| // 移除 shadowRoot 事件监听 | |
| removeShadowRootListeners(el, el[nameSpace].documentHandler) | |
| // 移除 shadowRoot 事件监听 | |
| removeShadowRootListeners(el, el[nameSpace].shadowHandler) |
🤖 Prompt for AI Agents
In packages/vue-directive/src/clickoutside.ts at lines 140-141, the event
listener removal uses el[nameSpace].documentHandler, which is not the correct
reference to the registered handler. To fix this, ensure you store the actual
handler function when adding the event listener and then use that exact
reference here to remove the listener properly, replacing
el[nameSpace].documentHandler with the stored handler variable.
| // 监听 shadowRoot 内部事件 | ||
| addShadowRootListeners(el, (event) => { | ||
| // 只处理 shadowRoot 内部事件 | ||
| nodeList | ||
| .filter((node) => node[nameSpace].mousedownTrigger) | ||
| .forEach((node) => node[nameSpace].documentHandler(event, event)) | ||
| nodeList | ||
| .filter((node) => !node[nameSpace].mousedownTrigger) | ||
| .forEach((node) => node[nameSpace].documentHandler(event, event)) | ||
| }) |
There was a problem hiding this comment.
Fix event handler reference management and scope issues.
This implementation has two critical issues:
-
Handler reference problem: The anonymous function registered here cannot be properly removed in
unbindbecause you're trying to remove a different function reference. -
Incorrect scope: The handler processes ALL nodes in
nodeListinstead of just the current element, which could trigger unintended clickoutside events.
+ // Store handler reference for proper cleanup
+ const shadowHandler = (event) => {
+ // Only process the current element, not all nodes
+ if (el[nameSpace].mousedownTrigger) {
+ el[nameSpace].documentHandler(event, event)
+ } else {
+ el[nameSpace].documentHandler(event, event)
+ }
+ }
+ el[nameSpace].shadowHandler = shadowHandler
+
// 监听 shadowRoot 内部事件
- addShadowRootListeners(el, (event) => {
- // 只处理 shadowRoot 内部事件
- nodeList
- .filter((node) => node[nameSpace].mousedownTrigger)
- .forEach((node) => node[nameSpace].documentHandler(event, event))
- nodeList
- .filter((node) => !node[nameSpace].mousedownTrigger)
- .forEach((node) => node[nameSpace].documentHandler(event, event))
- })
+ addShadowRootListeners(el, shadowHandler)📝 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.
| // 监听 shadowRoot 内部事件 | |
| addShadowRootListeners(el, (event) => { | |
| // 只处理 shadowRoot 内部事件 | |
| nodeList | |
| .filter((node) => node[nameSpace].mousedownTrigger) | |
| .forEach((node) => node[nameSpace].documentHandler(event, event)) | |
| nodeList | |
| .filter((node) => !node[nameSpace].mousedownTrigger) | |
| .forEach((node) => node[nameSpace].documentHandler(event, event)) | |
| }) | |
| // Store handler reference for proper cleanup | |
| const shadowHandler = (event) => { | |
| // Only process the current element, not all nodes | |
| if (el[nameSpace].mousedownTrigger) { | |
| el[nameSpace].documentHandler(event, event) | |
| } else { | |
| el[nameSpace].documentHandler(event, event) | |
| } | |
| } | |
| el[nameSpace].shadowHandler = shadowHandler | |
| // 监听 shadowRoot 内部事件 | |
| addShadowRootListeners(el, shadowHandler) |
🤖 Prompt for AI Agents
In packages/vue-directive/src/clickoutside.ts around lines 101 to 110, the event
handler is an anonymous function causing removal issues in unbind, and it
incorrectly processes all nodes instead of just the current element. To fix
this, define the handler as a named function stored on the element's namespace
for proper reference and removal, and modify the handler logic to only process
the current element rather than iterating over all nodes in nodeList.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/vue-directive/src/clickoutside.ts (2)
91-100: Fix handler reference management and scope issues.This implementation has the same critical issues mentioned in previous reviews:
- Handler reference problem: The anonymous function cannot be properly removed in
unbindbecause you're trying to remove a different reference.- Incorrect scope: Processing ALL nodes in
nodeListinstead of just the current element.Store the handler reference properly:
+ // Store handler reference for proper cleanup + const shadowHandler = (event) => { + if (el[nameSpace].mousedownTrigger) { + el[nameSpace].documentHandler(event, event) + } else { + el[nameSpace].documentHandler(event, event) + } + } + el[nameSpace].shadowHandler = shadowHandler + // 监听 shadowRoot 内部事件 - addShadowRootListeners(el, (event) => { - // 只处理 shadowRoot 内部事件 - nodeList - .filter((node) => node[nameSpace].mousedownTrigger) - .forEach((node) => node[nameSpace].documentHandler(event, event)) - nodeList - .filter((node) => !node[nameSpace].mousedownTrigger) - .forEach((node) => node[nameSpace].documentHandler(event, event)) - }) + addShadowRootListeners(el, shadowHandler)
131-131: Fix handler reference for proper cleanup.The event listener removal cannot work correctly because it references the wrong handler function.
Apply this fix:
- removeShadowRootListeners(el, el[nameSpace].documentHandler) + removeShadowRootListeners(el, el[nameSpace].shadowHandler)
🧹 Nitpick comments (1)
packages/vue-directive/src/clickoutside.ts (1)
38-44: Consider using optional chaining for cleaner code.The logic for Shadow DOM support looks correct, but the code can be simplified using optional chaining as suggested by static analysis tools.
Apply this diff to use optional chaining:
- const popperElm = vnode.context.popperElm || (vnode.context.state && vnode.context.state.popperElm) - const mousedownPath = (mousedown.composedPath && mousedown.composedPath()) || [mousedown.target] - const mouseupPath = (mouseup.composedPath && mouseup.composedPath()) || [mouseup.target] + const popperElm = vnode.context.popperElm || vnode.context.state?.popperElm + const mousedownPath = mousedown.composedPath?.() || [mousedown.target] + const mouseupPath = mouseup.composedPath?.() || [mouseup.target]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/vue-directive/src/clickoutside.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*`: 用于写产品需求和开发文档的文件，需求应尽量写得简单清晰易懂。 在需求设计阶段应遵从逐步迭代的原则，早期版本可以尽量简洁。
**/*: 用于写产品需求和开发文档的文件，需求应尽量写得简单清晰易懂。
在需求设计阶段应遵从逐步迭代的原则，早期版本可以尽量简洁。
📄 Source: CodeRabbit Inference Engine (.cursor/rules/product.mdc)
List of files the instruction was applied to:
packages/vue-directive/src/clickoutside.ts
🪛 Biome (1.9.4)
packages/vue-directive/src/clickoutside.ts
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 39-39: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
packages/vue-directive/src/clickoutside.ts (1)
55-67: Helper functions look good for Shadow DOM support.The
addShadowRootListenersandremoveShadowRootListenersfunctions are well-implemented and provide clean abstraction for managing shadowRoot event listeners.
discreted66
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/vue-directive/src/clickoutside.ts (2)
91-100: Fix handler reference management and scope issues.This implementation has the same critical issues identified in previous reviews:
- Handler reference problem: The anonymous function registered here cannot be properly removed in
unbindbecause you're trying to remove a different function reference.- Incorrect scope: The handler processes ALL nodes in
nodeListinstead of just the current element, which could trigger unintended clickoutside events.Apply this diff to fix both issues:
+ // Store handler reference for proper cleanup + const shadowHandler = (event) => { + // Only process the current element, not all nodes + if (el[nameSpace].mousedownTrigger) { + el[nameSpace].documentHandler(event, event) + } else { + el[nameSpace].documentHandler(event, event) + } + } + el[nameSpace].shadowHandler = shadowHandler + // 监听 shadowRoot 内部事件 - addShadowRootListeners(el, (event) => { - // 只处理 shadowRoot 内部事件 - nodeList - .filter((node) => node[nameSpace].mousedownTrigger) - .forEach((node) => node[nameSpace].documentHandler(event, event)) - nodeList - .filter((node) => !node[nameSpace].mousedownTrigger) - .forEach((node) => node[nameSpace].documentHandler(event, event)) - }) + addShadowRootListeners(el, shadowHandler)
130-131: Fix handler reference for proper cleanup.The current code cannot properly remove event listeners because it references
el[nameSpace].documentHandlerinstead of the actual registered handler.Apply this diff to fix the handler reference:
// 移除 shadowRoot 事件监听 - removeShadowRootListeners(el, el[nameSpace].documentHandler) + removeShadowRootListeners(el, el[nameSpace].shadowHandler)
🧹 Nitpick comments (2)
packages/renderless/src/search/index.ts (1)
79-80: Enhance with optional chaining for cleaner code.The shadow DOM support enhancement is excellent and aligns well with the broader changes to improve click detection within shadow DOM boundaries. However, the condition can be simplified using optional chaining as suggested by the static analysis tool.
Apply this diff to use optional chaining for cleaner code:
- const path = event.composedPath && event.composedPath() - if (path ? !path.includes(parent.$el) : !parent.$el.contains(event.target)) { + const path = event.composedPath?.() + if (path ? !path.includes(parent.$el) : !parent.$el.contains(event.target)) {packages/vue-directive/src/clickoutside.ts (1)
38-42: Improve with optional chaining for consistency.The shadow DOM support enhancement using
composedPath()is excellent and provides better event detection within shadow DOM boundaries. However, the code can be improved with optional chaining for consistency.Apply this diff to use optional chaining:
const popperElm = vnode.context.popperElm || (vnode.context.state && vnode.context.state.popperElm) - const mousedownPath = (mousedown.composedPath && mousedown.composedPath()) || [mousedown.target] - const mouseupPath = (mouseup.composedPath && mouseup.composedPath()) || [mouseup.target] + const mousedownPath = mousedown.composedPath?.() || [mousedown.target] + const mouseupPath = mouseup.composedPath?.() || [mouseup.target] const isClickInEl = mousedownPath.includes(el) || mouseupPath.includes(el) const isClickInPopper = popperElm && (mousedownPath.includes(popperElm) || mouseupPath.includes(popperElm))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/renderless/src/search/index.ts(1 hunks)packages/vue-directive/src/clickoutside.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*`: 用于写产品需求和开发文档的文件，需求应尽量写得简单清晰易懂。 在需求设计阶段应遵从逐步迭代的原则，早期版本可以尽量简洁。
**/*: 用于写产品需求和开发文档的文件，需求应尽量写得简单清晰易懂。
在需求设计阶段应遵从逐步迭代的原则，早期版本可以尽量简洁。
📄 Source: CodeRabbit Inference Engine (.cursor/rules/product.mdc)
List of files the instruction was applied to:
packages/renderless/src/search/index.tspackages/vue-directive/src/clickoutside.ts
🧬 Code Graph Analysis (1)
packages/vue-directive/src/clickoutside.ts (1)
packages/vue-directive/src/repeat-click.ts (1)
el(16-46)
🪛 Biome (1.9.4)
packages/renderless/src/search/index.ts
[error] 79-80: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/vue-directive/src/clickoutside.ts
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 39-39: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PR Unit Test
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (1)
packages/vue-directive/src/clickoutside.ts (1)
55-67: LGTM! Clean helper functions for shadow DOM event management.These helper functions provide a clean abstraction for managing shadow DOM event listeners with proper cleanup support.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/vue-directive/src/clickoutside.ts (1)
40-48: Excellent Shadow DOM support implementation.The refactor to use
event.composedPath()correctly addresses Shadow DOM scenarios where events cross shadow boundaries. The implementation properly:
- Uses
composedPath()when available to get the full event path- Falls back to the target element when
composedPath()is not supported- Uses
includes()to check element presence in the pathHowever, consider using optional chaining to improve readability:
-const mousedownPath = (mousedown.composedPath && mousedown.composedPath()) || [mousedown.target] -const mouseupPath = (mouseup.composedPath && mouseup.composedPath()) || [mouseup.target] +const mousedownPath = mousedown.composedPath?.() || [mousedown.target] +const mouseupPath = mouseup.composedPath?.() || [mouseup.target]packages/renderless/src/search/index.ts (1)
79-80: Excellent consistent Shadow DOM support implementation.This change aligns perfectly with the directive implementation and correctly handles Shadow DOM scenarios. The ternary operator approach cleanly chooses between the modern
composedPath()method and the fallbackcontains()check.Consider using optional chaining for improved readability:
-const path = event.composedPath && event.composedPath() +const path = event.composedPath?.()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/renderless/src/search/index.ts(1 hunks)packages/vue-directive/src/clickoutside.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*`: 用于写产品需求和开发文档的文件，需求应尽量写得简单清晰易懂。 在需求设计阶段应遵从逐步迭代的原则，早期版本可以尽量简洁。
**/*: 用于写产品需求和开发文档的文件，需求应尽量写得简单清晰易懂。
在需求设计阶段应遵从逐步迭代的原则，早期版本可以尽量简洁。
📄 Source: CodeRabbit Inference Engine (.cursor/rules/product.mdc)
List of files the instruction was applied to:
packages/renderless/src/search/index.tspackages/vue-directive/src/clickoutside.ts
🪛 Biome (1.9.4)
packages/renderless/src/search/index.ts
[error] 79-80: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/vue-directive/src/clickoutside.ts
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 45-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 46-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
packages/vue-directive/src/clickoutside.ts (2)
38-38: Good change fromlettoconst.The variable
popperElmis not reassigned, so usingconstis more appropriate and communicates intent better.
49-49: Logic correctly updated to use the new path-based detection.The condition properly uses the new
isClickInElandisClickInPoppervariables that leverage the composed path detection.
PR
fix：shadow-root节点下组件点击事件失效以及search面板闪动问题
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