Skip to content
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.

Sign up for GitHub

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

Jump to bottom

breaking: scope :has(...) selectors #13567

Merged
merged 4 commits into from
Oct 14, 2024
+442 −86
Merged

breaking: scope :has(...) selectors #13567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
08b3a74
breaking: scope `:has(...)` selectors
dummdidumm Oct 11, 2024
0d68454
tweak
dummdidumm Oct 11, 2024
3e7ddce
regenerate
dummdidumm Oct 11, 2024
3d436d6
while we're at it ..
dummdidumm Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silly-houses-promise.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

breaking: scope `:has(...)` selectors
240 changes: 179 additions & 61 deletions packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ const visitors = {
selectors,
/** @type {Compiler.Css.Rule} */ (node.metadata.rule),
context.state.element,
context.state.stylesheet
context.state.stylesheet,
true
)
) {
mark(inner, context.state.element);
Expand All @@ -120,12 +121,18 @@ const visitors = {
};

/**
* Discard trailing `:global(...)` selectors, these are unused for scoping purposes
* Discard trailing `:global(...)` selectors without a `:has(...)` modifier, these are unused for scoping purposes
* @param {Compiler.Css.ComplexSelector} node
*/
function truncate(node) {
const i = node.children.findLastIndex(({ metadata }) => {
return !metadata.is_global && !metadata.is_global_like;
const i = node.children.findLastIndex(({ metadata, selectors }) => {
return (
!metadata.is_global_like &&
(!metadata.is_global ||
selectors.some(
(selector) => selector.type === 'PseudoClassSelector' && selector.name === 'has'
))
);
});

return node.children.slice(0, i + 1);
Expand All @@ -136,9 +143,10 @@ function truncate(node) {
* @param {Compiler.Css.Rule} rule
* @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element
* @param {Compiler.Css.StyleSheet} stylesheet
* @param {boolean} check_has Whether or not to check the `:has(...)` selectors
* @returns {boolean}
*/
function apply_selector(relative_selectors, rule, element, stylesheet) {
function apply_selector(relative_selectors, rule, element, stylesheet, check_has) {
const parent_selectors = relative_selectors.slice();
const relative_selector = parent_selectors.pop();

Expand All @@ -148,88 +156,121 @@ function apply_selector(relative_selectors, rule, element, stylesheet) {
relative_selector,
rule,
element,
stylesheet
stylesheet,
check_has
);

if (!possible_match) {
return false;
}

if (relative_selector.combinator) {
const name = relative_selector.combinator.name;

switch (name) {
case ' ':
case '>': {
let parent = /** @type {Compiler.TemplateNode | null} */ (element.parent);
return apply_combinator(
relative_selector.combinator,
relative_selector,
parent_selectors,
rule,
element,
stylesheet,
check_has
);
}

let parent_matched = false;
let crossed_component_boundary = false;
// if this is the left-most non-global selector, mark it — we want
// `x y z {...}` to become `x.blah y z.blah {...}`
const parent = parent_selectors[parent_selectors.length - 1];
if (!parent || is_global(parent, rule)) {
mark(relative_selector, element);
}

while (parent) {
if (parent.type === 'Component' || parent.type === 'SvelteComponent') {
crossed_component_boundary = true;
}
return true;
}

if (parent.type === 'RegularElement' || parent.type === 'SvelteElement') {
if (apply_selector(parent_selectors, rule, parent, stylesheet)) {
// TODO the `name === ' '` causes false positives, but removing it causes false negatives...
if (name === ' ' || crossed_component_boundary) {
mark(parent_selectors[parent_selectors.length - 1], parent);
}
/**
* @param {Compiler.Css.Combinator} combinator
* @param {Compiler.Css.RelativeSelector} relative_selector
* @param {Compiler.Css.RelativeSelector[]} parent_selectors
* @param {Compiler.Css.Rule} rule
* @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element
* @param {Compiler.Css.StyleSheet} stylesheet
* @param {boolean} check_has Whether or not to check the `:has(...)` selectors
* @returns {boolean}
*/
function apply_combinator(
combinator,
relative_selector,
parent_selectors,
rule,
element,
stylesheet,
check_has
) {
const name = combinator.name;

switch (name) {
case ' ':
case '>': {
let parent = /** @type {Compiler.TemplateNode | null} */ (element.parent);

let parent_matched = false;
let crossed_component_boundary = false;

while (parent) {
if (parent.type === 'Component' || parent.type === 'SvelteComponent') {
crossed_component_boundary = true;
}

parent_matched = true;
if (parent.type === 'RegularElement' || parent.type === 'SvelteElement') {
if (apply_selector(parent_selectors, rule, parent, stylesheet, check_has)) {
// TODO the `name === ' '` causes false positives, but removing it causes false negatives...
if (name === ' ' || crossed_component_boundary) {
mark(parent_selectors[parent_selectors.length - 1], parent);
}

if (name === '>') return parent_matched;
parent_matched = true;
}

parent = /** @type {Compiler.TemplateNode | null} */ (parent.parent);
if (name === '>') return parent_matched;
}

return parent_matched || parent_selectors.every((selector) => is_global(selector, rule));
parent = /** @type {Compiler.TemplateNode | null} */ (parent.parent);
}

case '+':
case '~': {
const siblings = get_possible_element_siblings(element, name === '+');
return parent_matched || parent_selectors.every((selector) => is_global(selector, rule));
}

let sibling_matched = false;
case '+':
case '~': {
const siblings = get_possible_element_siblings(element, name === '+');

for (const possible_sibling of siblings.keys()) {
if (possible_sibling.type === 'RenderTag' || possible_sibling.type === 'SlotElement') {
// `{@render foo()}<p>foo</p>` with `:global(.x) + p` is a match
if (parent_selectors.length === 1 && parent_selectors[0].metadata.is_global) {
mark(relative_selector, element);
sibling_matched = true;
}
} else if (apply_selector(parent_selectors, rule, possible_sibling, stylesheet)) {
let sibling_matched = false;

for (const possible_sibling of siblings.keys()) {
if (possible_sibling.type === 'RenderTag' || possible_sibling.type === 'SlotElement') {
// `{@render foo()}<p>foo</p>` with `:global(.x) + p` is a match
if (parent_selectors.length === 1 && parent_selectors[0].metadata.is_global) {
mark(relative_selector, element);
sibling_matched = true;
}
} else if (
apply_selector(parent_selectors, rule, possible_sibling, stylesheet, check_has)
) {
mark(relative_selector, element);
sibling_matched = true;
}

return (
sibling_matched ||
(get_element_parent(element) === null &&
parent_selectors.every((selector) => is_global(selector, rule)))
);
}

default:
// TODO other combinators
return true;
return (
sibling_matched ||
(get_element_parent(element) === null &&
parent_selectors.every((selector) => is_global(selector, rule)))
);
}
}

// if this is the left-most non-global selector, mark it — we want
// `x y z {...}` to become `x.blah y z.blah {...}`
const parent = parent_selectors[parent_selectors.length - 1];
if (!parent || is_global(parent, rule)) {
mark(relative_selector, element);
default:
// TODO other combinators
return true;
}

return true;
}

/**
Expand Down Expand Up @@ -295,10 +336,87 @@ const regex_backslash_and_following_character = /\\(.)/g;
* @param {Compiler.Css.Rule} rule
* @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element
* @param {Compiler.Css.StyleSheet} stylesheet
* @param {boolean} check_has Whether or not to check the `:has(...)` selectors
* @returns {boolean}
*/
function relative_selector_might_apply_to_node(relative_selector, rule, element, stylesheet) {
function relative_selector_might_apply_to_node(
relative_selector,
rule,
element,
stylesheet,
check_has
) {
// Sort :has(...) selectors in one bucket and everything else into another
const has_selectors = [];
const other_selectors = [];

for (const selector of relative_selector.selectors) {
if (selector.type === 'PseudoClassSelector' && selector.name === 'has' && selector.args) {
has_selectors.push(selector);
} else {
other_selectors.push(selector);
}
}

// If we're called recursively from a :has(...) selector, we're on the way of checking if the other selectors match.
// In that case ignore this check (because we just came from this) to avoid an infinite loop.
if (check_has && has_selectors.length > 0) {
// :has(...) is special in that it means "look downwards in the CSS tree". Since our matching algorithm goes
// upwards and back-to-front, we need to first check the selectors inside :has(...), then check the rest of the
// selector in a way that is similar to ancestor matching. In a sense, we're treating `.x:has(.y)` as `.x .y`.
for (const has_selector of has_selectors) {
const complex_selectors = /** @type {Compiler.Css.SelectorList} */ (has_selector.args)
.children;
let matched = false;

for (const complex_selector of complex_selectors) {
const selectors = truncate(complex_selector);
if (
selectors.length === 0 /* is :global(...) */ ||
apply_selector(selectors, rule, element, stylesheet, check_has)
) {
// Treat e.g. `.x:has(.y)` as `.x .y` with the .y part already being matched,
// and now looking upwards for the .x part.
if (
apply_combinator(
selectors[0]?.combinator ?? descendant_combinator,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe put selectors[0] as its own better named variable. Why only the first element too?

selectors[0] ?? [],
[relative_selector],
rule,
element,
stylesheet,
false
)
) {
complex_selector.metadata.used = true;
matched = true;
}
}
}

if (!matched) {
if (relative_selector.metadata.is_global && !relative_selector.metadata.is_global_like) {
// Edge case: `:global(.x):has(.y)` where `.x` is global but `.y` doesn't match.
// Since `used` is set to `true` for `:global(.x)` in css-analyze beforehand, and
// we have no way of knowing if it's safe to set it back to `false`, we'll mark
// the inner selector as used and scoped to prevent it from being pruned, which could
// result in a invalid CSS output (e.g. `.x:has(/* unused .y */)`). The result
// can't match a real element, so the only drawback is the missing prune.
// TODO clean this up some day
complex_selectors[0].metadata.used = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe put complex_selectors[0] as its own better named variable. Why only the first element too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would just be called first_complex_selector which IMO doesn't add any clarity.

As for why only the first: a complex selector is the a and b in :has(a, b). This logic is just there to prevent invalid CSS, which means we only need to mark one of them as used so there's at least one selector in there.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this caused:

    .dropzone:has(+ .dragenter-top),
    :global(.dragenter-bottom + .dropzone) {
        background-color: blue;
        position: relative;
    }

to trigger

editor:build: warnings when minifying css:
editor:build: ▲ [WARNING] Unexpected ")" [css-syntax-error]
editor:build: 
editor:build:     <stdin>:1230:71:
editor:build:       1230 │ ...) .dropzone:has(/* (unused) + .dragenter-top*/.svelte-12stbyo)*/
editor:build:            ╵

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please open an issue with a reproduction

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure: #13680

complex_selectors[0].children.forEach((selector) => {
selector.metadata.scoped = true;
});
}

return false;
}
}

return true;
}

for (const selector of other_selectors) {
if (selector.type === 'Percentage' || selector.type === 'Nth') continue;

const name = selector.name.replace(regex_backslash_and_following_character, '$1');
Expand All @@ -316,7 +434,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element,
) {
const args = selector.args;
const complex_selector = args.children[0];
return apply_selector(complex_selector.children, rule, element, stylesheet);
return apply_selector(complex_selector.children, rule, element, stylesheet, check_has);
}

// We came across a :global, everything beyond it is global and therefore a potential match
Expand All @@ -326,7 +444,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element,
let matched = false;

for (const complex_selector of selector.args.children) {
if (apply_selector(truncate(complex_selector), rule, element, stylesheet)) {
if (apply_selector(truncate(complex_selector), rule, element, stylesheet, check_has)) {
complex_selector.metadata.used = true;
matched = true;
}
Expand Down Expand Up @@ -400,7 +518,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element,
const parent = /** @type {Compiler.Css.Rule} */ (rule.metadata.parent_rule);

for (const complex_selector of parent.prelude.children) {
if (apply_selector(truncate(complex_selector), parent, element, stylesheet)) {
if (apply_selector(truncate(complex_selector), parent, element, stylesheet, check_has)) {
complex_selector.metadata.used = true;
matched = true;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/3-transform/css/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ const visitors = {
context.state.specificity.bumped = before_bumped;
},
PseudoClassSelector(node, context) {
if (node.name === 'is' || node.name === 'where') {
if (node.name === 'is' || node.name === 'where' || node.name === 'has') {
context.next();
}
}
Expand Down
4 changes: 4 additions & 0 deletions packages/svelte/src/compiler/types/css.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export namespace Css {
type: 'Rule';
prelude: SelectorList;
block: Block;
/** @internal */
metadata: {
parent_rule: null | Rule;
has_local_selectors: boolean;
Expand Down Expand Up @@ -60,8 +61,10 @@ export namespace Css {
* The `a`, `b` and `c` in `a b c {}`
*/
children: RelativeSelector[];
/** @internal */
metadata: {
rule: null | Rule;
/** True if this selector applies to an element. For global selectors, this is defined in css-analyze, for others in css-prune while scoping */
used: boolean;
};
}
Expand All @@ -79,6 +82,7 @@ export namespace Css {
* The `b:is(...)` in `> b:is(...)`
*/
selectors: SimpleSelector[];
/** @internal */
metadata: {
/**
* `true` if the whole selector is unscoped, e.g. `:global(...)` or `:global` or `:global.x`.
Expand Down
Loading