fix(compiler-sfc): scope nested rules under mixed :deep() selector lists - #15270
fix(compiler-sfc): scope nested rules under mixed :deep() selector lists#15270ValentinYoushkevich wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughScoped style processing now splits mixed ChangesScoped selector handling
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This narrowly scoped compiler change corrects selector scoping for mixed :deep() nested rules without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ScopedStyle
participant processRule
participant BranchRules
participant compileStyleTests
ScopedStyle->>processRule: process nested mixed selector list
processRule->>BranchRules: create deep and plain branches
BranchRules->>ScopedStyle: return rewritten scoped rules
compileStyleTests->>ScopedStyle: validate ordering, specificity, and exclusions
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/compiler-sfc/src/style/pluginScoped.ts`:
- Around line 105-106: Update the rule-check predicate in the scoped-style
processing flow to detect descendant rule nodes recursively through nested
at-rules, rather than only direct children, before deciding not to split.
Preserve the existing behavior for rules without any nested descendants, and add
a regression test covering a selector with :deep and a nested at-rule containing
a child combinator rule so the plain branch receives its scope attribute.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f300b76b-67cf-4448-8c8f-8e892fe6f76a
📒 Files selected for processing (2)
packages/compiler-sfc/__tests__/compileStyle.spec.tspackages/compiler-sfc/src/style/pluginScoped.ts
edison1105
left a comment
edison1105
left a comment
There was a problem hiding this comment.
The branch-local scope placement for the reported case is now correct, but I do not think this can be merged as-is because splitting the parent selector list changes native CSS Nesting specificity.
Per the CSS Nesting specification, the specificity of & is the largest specificity among the selectors in the original parent selector list: https://drafts.csswg.org/css-nesting/#nest-selector
For example:
.a,
#b :deep(.c) {
> span {}
}The nested selector matching through .a originally derives its nesting specificity from the whole parent list, including the higher-specificity #b :deep(.c) member. After this PR splits the rule, the plain copy is evaluated under .a alone:
.a {
> span[data-v-test] {}
}That can change which declaration wins in the cascade even though the set of matched elements is now correct. This does not require an exotic :where() or :not() combination; a regular higher-specificity member is enough.
Please preserve the original parent-list specificity, or explicitly establish that diverging from native CSS Nesting specificity is an intended scoped-CSS semantic.
Non-blocking: the regression test currently uses separate toContain() checks. It would still pass if the deep copy incorrectly emitted > span[data-v-test]. An exact inline snapshot would verify both rule boundaries and that the nested selector in the deep branch remains unscoped.
…:deep() Per the CSS nesting spec the specificity of `&` is the largest specificity in the parent selector list, so splitting a mixed list into two rules evaluated each branch against a smaller list and could change which declaration wins the cascade. Keep the list intact and give each kind of member its own copy of the body wrapped in `&:where(<members>)`, which narrows what a branch matches without adding specificity of its own. Members that cannot be expressed as a `:where()` argument - pseudo elements, `&`, `:global()`, and members that expand into several selectors - are left alone rather than silently reduced to matching nothing.
|
You're right, and rather than argue that the divergence is intended I changed the approach so there is no divergence to argue about. What changedThe selector list is no longer split. It stays whole, and each kind of member gets its own copy of the body, wrapped in /* input */
.a,
#b :deep(.c) {
> span { color: blue; }
}
/* output */
.a,
#b[data-v-test] .c {
&:where(.a) {
> span[data-v-test] { color: blue; }
}
&:where(#b[data-v-test] .c) {
> span { color: blue; }
}
}
For the example in your review the nested rule goes from
Members that cannot be a
|
ValentinYoushkevich
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/compiler-sfc/src/style/pluginScoped.ts`:
- Around line 512-518: Update isGlobalSelector() to recursively inspect child
nodes of functional pseudo selectors, detecting nested :global or ::v-global
such as within :is(). Preserve the existing direct pseudo checks, and add a
regression test covering a mixed selector list like :is(:global(.b)) :deep(.c),
.a.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a637ce10-21df-4f52-880c-9a502fdb560a
📒 Files selected for processing (2)
packages/compiler-sfc/__tests__/compileStyle.spec.tspackages/compiler-sfc/src/style/pluginScoped.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/compiler-sfc/tests/compileStyle.spec.ts
… splitting `:global()` can sit inside `:is()` and friends, where the member-level check did not see it. Such a member belongs to neither branch, so recurse the same way isDeepSelector does and leave the rule alone.
…d :deep() lists A :slotted() member gets its own '-s' attribute handling inside rewriteSelector, which the branch wrapper of splitMixedDeepRuleBody cannot reproduce - splitting the list would silently drop the scope suffix. Detect :slotted() members the same way :global() members are detected and fall back to the old behavior. Ref: vuejs#15205
|
Hi @ValentinYoushkevich — nice work on this fix, the While probing edge cases I found one gap: a Would you be open to incorporating it? Happy to adjust anything. |
|
Good catch, thank you — confirmed. Before your fix the branch indeed dropped the Verified the patch locally: for every slotted variant I checked ( Merged into the branch — thanks for the PR! |
|
@edison1105 Please take a look at my edits. |
3 participants
Closes #15205.
Problem
When a scoped rule has a comma-separated selector list where one member uses
:deep(), and the rule contains nested rules, the scope id is placed wrongly for the plain members. Depending on where the:deep()member sits, a plain member either loses the id entirely or keeps it while its nested rules go unscoped.__deepis stored on the rule, but "is this deep?" is a property of an individual list member. Two things are inherently per-rule and cannot distinguish members: the one-shotextractAndWrapNodes(rule), and the deep-ness that nested rules inherit by walkingrule.parent.Approach
Per the spec in #15205 (comment), each list member must keep its own scoped nesting semantics:
That needs the id in different positions inside the same body, which one shared body cannot express. So
processRulegives each kind of member its own copy of the body, wrapped in&:where(<members of that kind>), and the existing code paths process each branch unchanged — no new scoping logic.The selector list itself stays whole. Per css-nesting the specificity of
&is the largest specificity in the parent selector list, so splitting the list into two rules would evaluate each branch against a smaller list and could change which declaration wins the cascade (see review).&:where()narrows what a branch matches while adding no specificity of its own, so nested rules keep exactly the weight they have today, plus the scope attribute they are supposed to get.Input:
Output:
:where()is Chrome 88 / Safari 14 / Firefox 78; the native nesting this code path already emits is Chrome 112 / Safari 16.5 / Firefox 117, so any browser that can parse the output supports:where().The change is narrowly gated: it only fires when the body has a nested rule (including one below an at-rule such as
@media;@keyframesare excluded), the list has more than one member, and the list mixes deep and plain members.Because
:where()is a forgiving selector list — it drops an argument it cannot parse instead of invalidating the rule — a rule is left exactly as it is onmainwhenever a member cannot be a:where()argument: members with pseudo elements, members written on&(inside a branch it would resolve against the mixed list itself),:global()members, and members that expand into several selectors such as:is(:deep(.foo), .bar) .baz. Each bail-out has a test asserting unchanged output.Relation to #15206
#15206 resolves
rule.__deepup-front. That fixes the reported ".ais unscoped" symptom, but the nested rule then yields.a[data-v-xxx] > span— the shape explicitly called out as wrong in the issue, and visible in that PR's own inline snapshot. Pre-computing the flag cannot get to.a > span[data-v-xxx], because the body is still shared.Not covered
One related case stays as it is on
main, and can be a follow-up::is(.a, :deep(.c))with nested rules, which goes throughsplitSelectorForNestedDeepTrade-off
The body is duplicated in the output for mixed lists — unavoidable if each branch needs its own id placement.
Tests
Added to
compileStyle.spec.tsas exact inline snapshots: the reported case, the reversed order, a three-member list, a nested rule below@media, a case with an id member pinning the preserved nesting specificity, and controls asserting unchanged output for lists without nested rules, all-deep lists, and every bail-out above.vitest run --project unit --project unit-jsdom: 181 test files, 3675 tests passed, 5 skipped.Cascade behavior verified in Chrome by mounting the compiled output against a competing
.q.q.q.q.q.q.q.q > spanrule — specificity(0,8,1): the nested rule keeps winning, the:deep()branch applies, and nothing leaks outside the component.Summary by CodeRabbit
Bug Fixes
:deep()selectors used alongside regular selectors in nested rules.:global(), keyframes, and other complex selector combinations.Tests