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.

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

Remove css-tag stylesheet caching by css text #2978

Merged
merged 4 commits into from Jun 17, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/loud-dragons-hang.md
@@ -0,0 +1,7 @@
---
'@lit/reactive-element': patch
'lit': patch
'lit-element': patch
---

Changed the caching behavior of the css`` template literal tag so that same-text styles do not share a CSSStyleSheet. Note that this may be a breaking change in some very unusual scenarios on Chromium browsers only.
justinfagnani marked this conversation as resolved.
Show resolved Hide resolved
19 changes: 8 additions & 11 deletions packages/reactive-element/src/css-tag.ts
Expand Up @@ -31,8 +31,6 @@ export type CSSResultGroup = CSSResultOrNative | CSSResultArray;

const constructionToken = Symbol();

const styleSheetCache = new Map<string, CSSStyleSheet>();

/**
* A container for a string of CSS text, that may be used to create a CSSStyleSheet.
*
Expand All @@ -44,6 +42,7 @@ export class CSSResult {
// This property needs to remain unminified.
['_$cssResult$'] = true;
readonly cssText: string;
private _styleSheet?: CSSStyleSheet;

private constructor(cssText: string, safeToken: symbol) {
if (safeToken !== constructionToken) {
Expand All @@ -54,17 +53,15 @@ export class CSSResult {
this.cssText = cssText;
}

// Note, this is a getter so that it's lazy. In practice, this means
// stylesheets are not created until the first element instance is made.
// This is a getter so that it's lazy. In practice, this means stylesheets
// are not created until the first element instance is made.
get styleSheet(): CSSStyleSheet | undefined {
// Note, if `supportsAdoptingStyleSheets` is true then we assume
// CSSStyleSheet is constructable.
let styleSheet = styleSheetCache.get(this.cssText);
if (supportsAdoptingStyleSheets && styleSheet === undefined) {
styleSheetCache.set(this.cssText, (styleSheet = new CSSStyleSheet()));
styleSheet.replaceSync(this.cssText);
// If `supportsAdoptingStyleSheets` is true then we assume CSSStyleSheet is
// constructable.
if (supportsAdoptingStyleSheets && this._styleSheet === undefined) {
(this._styleSheet = new CSSStyleSheet()).replaceSync(this.cssText);
}
return styleSheet;
return this._styleSheet;
}

toString(): string {
Expand Down
65 changes: 32 additions & 33 deletions packages/reactive-element/src/test/css-tag_test.ts
Expand Up @@ -4,7 +4,12 @@
* SPDX-License-Identifier: BSD-3-Clause
*/

import {css, CSSResult, unsafeCSS} from '../css-tag.js';
import {
css,
CSSResult,
unsafeCSS,
supportsAdoptingStyleSheets,
} from '../css-tag.js';
import {assert} from '@esm-bundle/chai';

suite('Styling', () => {
Expand All @@ -14,46 +19,43 @@ suite('Styling', () => {
const cssValue = css;
const makeStyle = () => cssValue`foo`;
const style1 = makeStyle();
assert.equal(
(style1 as CSSResult).styleSheet,
(style1 as CSSResult).styleSheet
);
const style2 = makeStyle();
assert.equal(
(style1 as CSSResult).styleSheet,
(style2 as CSSResult).styleSheet
);
if (supportsAdoptingStyleSheets) {
assert.isDefined(style1.styleSheet);
assert.strictEqual(style1.styleSheet, style1.styleSheet);
const style2 = makeStyle();
assert.notStrictEqual(style1.styleSheet, style2.styleSheet);
} else {
assert.isUndefined(style1.styleSheet);
}
});

test('css with same values always produce the same stylesheet', () => {
test('css with same values produce unique stylesheets', () => {
// Alias avoids syntax highlighting issues in editors
const cssValue = css;
const makeStyle = () => cssValue`background: ${cssValue`blue`}`;
const style1 = makeStyle();
assert.equal(
(style1 as CSSResult).styleSheet,
(style1 as CSSResult).styleSheet
);
const style2 = makeStyle();
assert.equal(
(style1 as CSSResult).styleSheet,
(style2 as CSSResult).styleSheet
);
if (supportsAdoptingStyleSheets) {
assert.isDefined(style1.styleSheet);
assert.strictEqual(style1.styleSheet, style1.styleSheet);
const style2 = makeStyle();
assert.notStrictEqual(style1.styleSheet, style2.styleSheet);
} else {
assert.isUndefined(style1.styleSheet);
}
});

test('unsafeCSS() CSSResults always produce the same stylesheet', () => {
justinfagnani marked this conversation as resolved.
Show resolved Hide resolved
// Alias avoids syntax highlighting issues in editors
const makeStyle = () => unsafeCSS(`foo`);
const style1 = makeStyle();
assert.equal(
(style1 as CSSResult).styleSheet,
(style1 as CSSResult).styleSheet
);
const style2 = makeStyle();
assert.equal(
(style1 as CSSResult).styleSheet,
(style2 as CSSResult).styleSheet
);
if (supportsAdoptingStyleSheets) {
assert.isDefined(style1.styleSheet);
assert.strictEqual(style1.styleSheet, style1.styleSheet);
const style2 = makeStyle();
assert.notStrictEqual(style1.styleSheet, style2.styleSheet);
} else {
assert.isUndefined(style1.styleSheet);
}
});

test('`css` get styles throws when unsafe values are used', async () => {
Expand All @@ -75,10 +77,7 @@ suite('Styling', () => {
margin: ${spacer * 2}px;
}
`;
assert.equal(
(result as CSSResult).cssText.replace(/\s/g, ''),
'div{margin:4px;}'
);
assert.equal(result.cssText.replace(/\s/g, ''), 'div{margin:4px;}');
});

test('`CSSResult` cannot be constructed', async () => {
Expand Down