-
-
Notifications
You must be signed in to change notification settings - Fork 9k
fix(v-model.trim): correct the activeElement discovery for shadow DOM #14196
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: main
Are you sure you want to change the base?
fix(v-model.trim): correct the activeElement discovery for shadow DOM #14196
Conversation
WalkthroughReplace the direct Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
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
🧹 Nitpick comments (1)
packages/runtime-dom/src/directives/vModel.ts (1)
54-58: HardenactiveElement()fornullinner activeElement + nested shadow rootsRight now, if
document.activeElement.shadowRootexists butshadowRoot.activeElementisnull, the helper returnsnullinstead of falling back to the document’s active element. Also, nested shadow roots won’t be fully resolved.Consider:
-function activeElement() { - return document.activeElement && document.activeElement.shadowRoot - ? document.activeElement.shadowRoot.activeElement - : document.activeElement -} +function activeElement(): Element | null { + let el: Element | null = document.activeElement + while (el && el.shadowRoot?.activeElement) { + el = el.shadowRoot.activeElement + } + return el +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-dom/src/directives/vModel.ts(2 hunks)
🔇 Additional comments (1)
packages/runtime-dom/src/directives/vModel.ts (1)
111-119: Good focus detection fix for Shadow DOM; please add a regression test (likely e2e)Using
activeElement()here matches the intended “don’t clobber the user’s in-progress text while focused” behavior in shadow trees. The remaining risk is regression without coverage, and jsdom focus behavior is known to be flaky for shadow DOM.
The current approach grants support for nested shadow doms
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Cheers everyone
Issue:
The issue is, that using
v-model.trimin the shadow dom does not behave the same as in the light dom. When an input should get trimmed in the light dom, the ref value itself gets trimmed correctly, but the displayed characters in the input won't get trimmed until you refocus the input. I like that approach since it reduces noise for the cursor and doesn't seem glitchy. However, when trimming in the shadow dom, the displayed characters (not equivalent to thevalueproperty of the input element at that moment!) are trimmed, which increases visual noise.A demo with reproduction steps can be seen here: https://vue-3-v-model-trim-misbehaviour.vercel.app/
With the repository: https://github.com/LordSalmon/vue-3-v-model-trim-misbehaviour
The problem seems to be that line which compares the currently focused element to the activeElement of the document. However, when the input is placed inside the shadow dom,
document.activeElementreturns the shadow dom root node instead of the input.Fix idea:
Initial Idea: - Checking at that point of execution whether
document.activeElementis the root node for a shadow dom and if so, return theactiveElementof the shadow dom.Improvement: To support nested shadow doms,
el.getRootNode()is used to directly verify theactiveElementproperty from that root instead of starting at the root of the root document itself.Tests:
I tried to create a test for that, but it seems as if js-dom's focus api is not entirely consistent. Reproducing the step where the displayed characters in the input still contained the spaces but were gone on refocus was not possible. But If someone has a flash of inspiration, let me know or feel free to contribute.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.