Skip to content

fix(compiler): scope nested CSS rules #50693

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

Closed
crisbeto wants to merge 1 commit into from
+422 −22
Closed

fix(compiler): scope nested CSS rules #50693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 29 additions & 6 deletions packages/compiler/src/shadow_css.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ const animationKeywords = new Set([
'start',
]);

/** Contains parts of selectors that should never be scoped. */
const unscopeableSelectorParts = new Set([
// Ampersands don't need scoping, because it's assumed that
// the selector above them has been scoped already.
'&',
]);

/**
* The following array contains all of the CSS at-rule identifiers which are scoped.
*/
Expand Down Expand Up @@ -615,14 +622,15 @@ export class ShadowCss {
return processRules(cssText, (rule: CssRule) => {
let selector = rule.selector;
let content = rule.content;
if (rule.selector[0] !== '@') {
if (rule.selector[0] !== '@' && rule.isBlock) {
selector = this._scopeSelector(rule.selector, scopeSelector, hostSelector);
content = this._scopeSelectors(rule.content, scopeSelector, hostSelector);
} else if (scopedAtRuleIdentifiers.some((atRule) => rule.selector.startsWith(atRule))) {
content = this._scopeSelectors(rule.content, scopeSelector, hostSelector);
} else if (rule.selector.startsWith('@font-face') || rule.selector.startsWith('@page')) {
content = this._stripScopingSelectors(rule.content);
}
return new CssRule(selector, content);
return new CssRule(selector, content, rule.isBlock);
});
}

Expand Down Expand Up @@ -652,7 +660,7 @@ export class ShadowCss {
const selector = rule.selector
.replace(_shadowDeepSelectors, ' ')
.replace(_polyfillHostNoCombinatorRe, ' ');
return new CssRule(selector, rule.content);
return new CssRule(selector, rule.content, rule.isBlock);
});
}

Expand Down Expand Up @@ -730,7 +738,11 @@ export class ShadowCss {
return p;
}

if (p.includes(_polyfillHostNoCombinator)) {
if (unscopeableSelectorParts.has(scopedP)) {
return scopedP;
}

if (p.indexOf(_polyfillHostNoCombinator) > -1) {
scopedP = this._applySimpleSelectorScope(p, scopeSelector, hostSelector);
} else {
// remove :host since it should be unnecessary
Expand Down Expand Up @@ -905,6 +917,7 @@ const _ruleRe = new RegExp(
'g',
);
const CONTENT_PAIRS = new Map([['{', '}']]);
const QUOTES = new Set([`'`, `"`]);

const COMMA_IN_PLACEHOLDER = '%COMMA_IN_PLACEHOLDER%';
const SEMI_IN_PLACEHOLDER = '%SEMI_IN_PLACEHOLDER%';
Expand All @@ -918,6 +931,7 @@ export class CssRule {
constructor(
public selector: string,
public content: string,
readonly isBlock: boolean,
) {}
}

Expand All @@ -930,12 +944,14 @@ export function processRules(input: string, ruleCallback: (rule: CssRule) => Css
let content = '';
let suffix = m[4];
let contentPrefix = '';
let isBlock = false;
if (suffix && suffix.startsWith('{' + BLOCK_PLACEHOLDER)) {
content = inputWithEscapedBlocks.blocks[nextBlockIndex++];
suffix = suffix.substring(BLOCK_PLACEHOLDER.length + 1);
contentPrefix = '{';
isBlock = true;
}
const rule = ruleCallback(new CssRule(selector, content));
const rule = ruleCallback(new CssRule(selector, content, isBlock));
return `${m[1]}${rule.selector}${m[3]}${contentPrefix}${rule.content}${suffix}`;
});
return unescapeInStrings(escapedResult);
Expand All @@ -960,11 +976,18 @@ function escapeBlocks(
let blockStartIndex = -1;
let openChar: string | undefined;
let closeChar: string | undefined;
let currentQuote: string | undefined;

for (let i = 0; i < input.length; i++) {
const char = input[i];
if (char === '\\') {
i++;
} else if (QUOTES.has(char)) {
if (!currentQuote) {
currentQuote = char;
} else if (currentQuote === char) {
currentQuote = undefined;
}
} else if (char === closeChar) {
openCharCount--;
if (openCharCount === 0) {
Expand All @@ -976,7 +999,7 @@ function escapeBlocks(
}
} else if (char === openChar) {
openCharCount++;
} else if (openCharCount === 0 && charPairs.has(char)) {
} else if (openCharCount === 0 && charPairs.has(char) && !currentQuote) {
openChar = char;
closeChar = charPairs.get(char);
openCharCount = 1;
Expand Down
16 changes: 8 additions & 8 deletions packages/compiler/test/shadow_css/keyframes_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,12 @@ describe('ShadowCss, keyframes and animations', () => {
const css = `.test {
animation:my-anim 1s,my-anim2 2s, my-anim3 3s,my-anim4 4s;
}

@keyframes my-anim {
0% {color: red}
100% {color: blue}
}

@keyframes my-anim2 {
0% {font-size: 1em}
100% {font-size: 1.2em}
Expand Down Expand Up @@ -162,12 +162,12 @@ describe('ShadowCss, keyframes and animations', () => {
const css = `.test {
animation:, my-anim 1s,my-anim2 2s, my-anim3 3s,my-anim4 4s;
}

@keyframes my-anim {
0% {color: red}
100% {color: blue}
}

@keyframes my-anim2 {
0% {font-size: 1em}
100% {font-size: 1.2em}
Expand Down Expand Up @@ -460,14 +460,14 @@ describe('ShadowCss, keyframes and animations', () => {
div {
animation: 1s 'foo', 1.5s 'bar';
animation: 2s 'fo\\'o', 2.5s 'bar';
animation: 3s 'foo\\'', 3.5s 'bar', 3.7s 'ba\\'r';
animation: 3s 'foo', 3.5s 'bar', 3.7s 'ba\\'r';
animation: 4s 'foo\\\\', 4.5s 'bar', 4.7s 'baz\\'';
animation: 5s 'fo\\\\\\'o', 5.5s 'bar', 5.7s 'baz\\'';
}

@keyframes foo {}
@keyframes 'fo\\'o' {}
@keyframes 'foo'' {}

@crisbeto crisbeto Jun 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't know the context of this assertion, however this is actually invalid syntax (note the mismatching quotes). I had to update the test, because after my changes, invalid syntax would prevent styles from being encapsulated correctly.

@keyframes 'foo' {}
@keyframes 'foo\\\\' {}
@keyframes "bar" {}
@keyframes 'ba\\'r' {}
Expand All @@ -476,15 +476,15 @@ describe('ShadowCss, keyframes and animations', () => {
const result = shim(css, 'host-a');
expect(result).toContain('@keyframes host-a_foo {}');
expect(result).toContain("@keyframes 'host-a_fo\\'o' {}");
expect(result).toContain("@keyframes 'host-a_foo'' {}");
expect(result).toContain("@keyframes 'host-a_foo' {}");
expect(result).toContain("@keyframes 'host-a_foo\\\\' {}");
expect(result).toContain('@keyframes "host-a_bar" {}');
expect(result).toContain("@keyframes 'host-a_ba\\'r' {}");
expect(result).toContain(`@keyframes "host-a_fo\\\\\\'o" {}`);
expect(result).toContain("animation: 1s 'host-a_foo', 1.5s 'host-a_bar';");
expect(result).toContain("animation: 2s 'host-a_fo\\'o', 2.5s 'host-a_bar';");
expect(result).toContain(
"animation: 3s 'host-a_foo\\'', 3.5s 'host-a_bar', 3.7s 'host-a_ba\\'r';",
"animation: 3s 'host-a_foo', 3.5s 'host-a_bar', 3.7s 'host-a_ba\\'r';",
);
expect(result).toContain("animation: 4s 'host-a_foo\\\\', 4.5s 'host-a_bar', 4.7s 'baz\\'';");
expect(result).toContain("animation: 5s 'host-a_fo\\\\\\'o', 5.5s 'host-a_bar', 5.7s 'baz\\''");
Expand Down
Loading