Native cssstylesheet #853

samthor · 2019-11-17T22:51:24Z

Fixes #774. Supports native CSSStyleSheet if and only if native adopted style sheets on shadow roots are supported.

src/lib/css-tag.ts

justinfagnani · 2020-01-16T17:39:13Z

samthor · 2020-02-21T04:45:08Z

Sorry, I was on paternity leave and then I forgot this when I got back.

PTAL. I've changed the code a bit so we don't subclass CSSResult when we're provided with native CSSStyleSheet instances.

web-padawan

This breaks recently added getStyles() API, at least I don't see any call of it anymore. Please fix.

samthor · 2020-02-21T07:47:15Z

Oops, I didn't notice that change in my rebase. I've restored that line and modified a test to ensure we also call .getStyles().

web-padawan

Thanks, tested locally and it works for our ThemableElement, see vaadin/component-mixins#55

@justinfagnani could you please confirm if this change is planned to land in 2.3.0 release?

justinfagnani

Can you find a way around adding _nativeStyles? Seems like there should be a way to have _styles be a CSSResultOrNative[] and error only if there's a CSSStyleSheet in there on a platform that doesn't support it.

justinfagnani · 2020-03-20T21:05:43Z

src/lib/css-tag.ts

@@ -9,8 +9,9 @@ part of the polymer project is also subject to an additional IP rights grant
 found at http://polymer.github.io/PATENTS.txt
 */

-export const supportsAdoptingStyleSheets =
-    ('adoptedStyleSheets' in Document.prototype) &&
+export const supportsAdoptingShadowStyleSheets =


Why the name change? This is kind of public API, so I wouldn't...

Sure. I've left the new semantics though, since all we care about is support on a ShadowRoot. There's a polyfill for IE11 which only supports Document; in that case, the old behavior was wrong, and would cause a crash.

justinfagnani · 2020-03-20T21:07:18Z

src/lit-element.ts


  private static _styles: CSSResult[]|undefined;
+  private static _nativeStyles: CSSStyleSheet[]|undefined;


Why does this need to be separate from _styles? Doesn't total ordering between the two sets matter too?

We use one or the other depending on browser support. I also though about using a union TypeScript type but it got particularly awkward.

in case it helps, the type signature to merge the two might be like this:

private static _styles: (CSSResult | CSSStyleSheet)[] | undefined

then, when you're looping over each stylesheet, you'd distinguish whether it's a native stylesheet using the instanceof operator or something along those lines

it might be necessary if for some reason native stylesheets and CSSResult's might both be used simultaneously

Adding _nativeStyles lets us assign at once to each element—we just set .adoptedStyleSheets = this._nativeStyles. I think it's much more ergonomic than casting every element of the array when you assign it.

I want to hear what @justinfagnani thinks—mostly I hope I've made it clear that we use either array, so total ordering doesn't matter.

I think it's much more ergonomic than casting every element of the array when you assign it.

i'm not sure i follow.. casting every element when assigning to the array? maybe there's just a bit of confusion about the details of that type signature

the type signature (CSSResult | CSSStyleSheet)[] | undefined would allow the array to contain stylesheets of either type: so you'd be able to "assign" both types of stylesheets to the array without any type casting

const cssResult: CSSResult = getExampleCssResult() const nativeCss: CSSStyleSheet = getExampleNativeCss() this._styles.push(cssResult) // no casting this._styles.push(nativeCss) // no problem for (const style of this._styles) { // style has type: (CSSResult | CSSStyleSheet) }

however when you read one of the array values, then you might want to cast to distinguish it's type to handle them separately

The type signature should actually be (CSSResult[] | CSSStyleSheet[]), because you only have CSSStyleSheet where native adopted stylesheets are supported (regardless of input).

And then the only way you check the type is by doing something like this._styles[0] instanceof CSSStyleSheet, which is awkward.

justinfagnani · 2020-03-20T21:09:56Z

src/lit-element.ts

@@ -152,30 +172,33 @@ export class LitElement extends UpdatingElement {
  }

  /**
-   * Applies styling to the element shadowRoot using the `static get styles`
-   * property. Styling will apply using `shadowRoot.adoptedStyleSheets` where
+   * Applies styling to the element shadowRoot using the private styles


I don't think we should mention a private field in a protected API doc

I've reverted this comment. I think it's better the way it was.

My confusion came from... I think the tense is a bit ambiguous. By the time this function is called, we've already created the private "mapping" of styles (via _getUniqueStyles()), and this function doesn't call the static get styles method itself. Not a big deal, because we've already called it, but that's why I was eager to modify the comment.

justinfagnani · 2020-03-20T21:11:21Z

src/lit-element.ts

+      work.push(userStyles);
+    }
+
+    if (supportsAdoptingShadowStyleSheets) {


Can we do one loop with this check inside? I think it'll be more compact.

We're creating two different arrays. I could change the _styles and _nativeStyles to be a type union but I think it'll be more confusing when we apply them in adoptStyles.

samthor · 2020-03-29T21:40:23Z

ping @justinfagnani

I've rebased the PR again.

samthor · 2020-03-29T21:44:28Z

ugh, sorry for noise—I merged badly. I've just made the changes against master again.

justinfagnani · 2020-04-07T02:56:36Z

@samthor I tweaked this change to only use one array for both CSSSyleSheets and CSSResults. It's only a small conditional in adoptStyles. I think the rest is simpler. See the diff against master here: https://github.com/Polymer/lit-element/compare/native-stylesheets#diff-2ab97d0f347f8a3887d614f36018898bR187

samthor · 2020-04-07T03:33:08Z

@justinfagnani I pulled your change in and fixed some comments. PTAL

src/lit-element.ts

src/lib/css-tag.ts

samthor · 2020-04-07T23:01:48Z

I've addressed feedback and pushed again. I presume the PR will pass Travis :)

sorvell

This looks ok, but I'm a little concerned about lack of polyfill support. This is masked here because the test is only running if shadowRoot has adoptedStylesheets and the polyfill does not.

Let's look into either:

failing in some more obvious way if the polyfill is on (note, it can be forced on) and a CSSStyleSheet is provided (like just a clear console error or something) or
trying to pass the cssText from the provided stylesheet to ShadyCSS (presumably this would only work for a subset of stylesheets) and fail with a clear error otherwise.

justinfagnani · 2020-04-09T16:48:03Z

To be clear, you want to test the case where the platform has native constructible stylesheet support, but the polyfill is also forced on. Otherwise there's no way yet to get a constructible stylesheet.

samthor · 2020-04-09T23:07:55Z

I think what y'all mean is, you're worried about the case where there is native constructible stylesheets support but the WC polyfills are forced on. This can only happen in Chromium right now, since that's the only place native constructible stylesheets are supported. Does that sound right?

If the target doesn't have constructible stylesheets at all (or that is polyfilled—although that space is a bit fuzzy, we did it for Santa Tracker), I don't think there's an issue there.

src/lit-element.ts

Co-Authored-By: Steve Orvell <[email protected]>

sorvell

Just the little tweak to the "supports" flag and making sure we understand that the tests are all passing and I think we're g2g.

sorvell · 2020-04-10T01:06:55Z

src/lib/css-tag.ts

@@ -12,7 +12,8 @@ found at http://polymer.github.io/PATENTS.txt
 /**
 * Whether the current browser supports `adoptedStyleSheets`.
 */
-export const supportsAdoptingStyleSheets =
+export const supportsAdoptingStyleSheets = (window.ShadowRoot) &&


Think we need to change this to ('adoptedStyleSheets' in window.ShadowRoot.prototype), instead of ('adoptedStyleSheets' in Document.prototype). This makes sure it'll be false when ShadyDOM is enabled.

Seems like this change should be needed to get the test running under the polyfill to run?

@justinfagnani had an issue with this. To quote him:

We need to keep this checks against Document, since we have developers that delete the properties on Document.prototype to disable the use of adopted style sheets so they can edit in DevTools.

samthor · 2020-04-10T01:25:00Z

@sorvell @justinfagnani We're good for another review now.

we flatten CSSStyleSheet rules where native constructible stylesheets are supported but the WC polyfill is enabled
we throw when a user tries to pass a CSSStyleSheet from <style> tag (rather than trying to flatten)

And I've updated the tests a bit. We now explicitly check that, in the fully natively supported case, changing the rules of a CSSStyleSheet will change the rendered style.

src/lib/css-tag.ts

Co-Authored-By: Steve Orvell <[email protected]>

sorvell · 2020-04-10T15:44:14Z

src/test/lit-element_styling_test.ts

+  // adoptedStyleSheets, we can return a CSSStyleSheet that will be flattened
+  // and play nice with others.
+  const testShadyCSSWithAdoptedStyleSheetSupport = (typeof ShadowRoot === 'function') &&
+      ('adoptedStyleSheets' in window.ShadowRoot.prototype) &&


I don't think this test ever runs. When using the Shadow DOM polyfill, ShadowRoot does not have an adoptedStyleSheets property. Just taking out that check should make this run.

Well, that makes the test fail, because I can't construct CSSStyleSheet without adoptedStyleSheets support.

I've changed the logic a bit:

const testShadyCSSWithAdoptedStyleSheetSupport = (window.ShadowRoot) && ('replace' in CSSStyleSheet.prototype) && (window.ShadyCSS !== undefined && !window.ShadyCSS.nativeShadow);

This uses ShadyCSS the same way we do in css-tag.ts, and basically says whether we're flattening the stylesheets because the polyfill is included.

samthor · 2020-04-10T22:56:39Z

Thanks @sorvell for pointing out the test wasn't running. Fixed now.

sorvell

LGTM. Thanks for seeing it through!

samthor · 2020-04-11T01:52:22Z

Thanks for the thorough review. I look forward to the next release so I can pin Santa against it.

samthor requested a review from justinfagnani as a code owner November 17, 2019 22:51

googlebot added the cla: yes label Nov 17, 2019

justinfagnani reviewed Dec 3, 2019

View reviewed changes

src/lib/css-tag.ts Outdated Show resolved Hide resolved

web-padawan suggested changes Feb 21, 2020

View reviewed changes

web-padawan mentioned this pull request Feb 21, 2020

Prepare for lit-element 2.3.0 version bump vaadin/component-mixins#55

Merged

web-padawan approved these changes Feb 21, 2020

View reviewed changes

justinfagnani requested changes Mar 20, 2020

View reviewed changes

samthor requested a review from arthurevans as a code owner March 29, 2020 21:40

samthor and others added 10 commits April 7, 2020 13:27

support native CSSStyleSheet

fbf95c0

fix test

ee83057

comment tweaks

6e5cc41

use getStyles(), check with test

b716590

fix test

024abcb

review feedback

e535bc9

fix bad merge

f7009ad

Merge branch 'master' into native-cssstylesheet

9c988d9

Use only one styles array

a24cb63

fix comments after justin's change

0c2821a

justinfagnani reviewed Apr 7, 2020

View reviewed changes

src/lit-element.ts Outdated Show resolved Hide resolved

justinfagnani requested changes Apr 7, 2020

View reviewed changes

src/lit-element.ts Outdated Show resolved Hide resolved

src/lib/css-tag.ts Outdated Show resolved Hide resolved

review feedback

d90c070

justinfagnani approved these changes Apr 7, 2020

View reviewed changes

justinfagnani requested a review from sorvell April 8, 2020 21:43

sorvell reviewed Apr 9, 2020

View reviewed changes

samthor added 7 commits April 10, 2020 10:08

flatten sheets where possible

94286e4

remove unused import

8742ab5

fix missing whitespace

5dc558e

use the same test as every other element

f64ed51

change to throw for <style>.sheet, add tests

33f3b71

semicolon

626ef41

missed test name

b8f878f

sorvell reviewed Apr 10, 2020

View reviewed changes

src/lit-element.ts Outdated Show resolved Hide resolved

samthor and others added 3 commits April 10, 2020 11:04

Update src/lit-element.ts

3c42e5b

Co-Authored-By: Steve Orvell <[email protected]>

fix bad suggestion merge

0e4df4a

fix test running cases

2db7cd9

sorvell reviewed Apr 10, 2020

View reviewed changes

src/lib/css-tag.ts Outdated Show resolved Hide resolved

samthor and others added 5 commits April 10, 2020 11:44

Update src/lib/css-tag.ts

b0daa04

Co-Authored-By: Steve Orvell <[email protected]>

fix test and change flattening behavior

3c25762

eslint

edd8668

move throws check earlier

9b01c1c

fix tests

2aa7ec7

sorvell reviewed Apr 10, 2020

View reviewed changes

restructure tests a bit

c172c04

sorvell approved these changes Apr 11, 2020

View reviewed changes

sorvell merged commit c8e182d into lit:master Apr 11, 2020

samthor deleted the native-cssstylesheet branch April 11, 2020 01:52


		private static _styles: CSSResult[]\|undefined;
		private static _nativeStyles: CSSStyleSheet[]\|undefined;

Native cssstylesheet #853

Native cssstylesheet #853

Conversation

samthor commented Nov 17, 2019 • edited Loading

justinfagnani commented Jan 16, 2020

samthor commented Feb 21, 2020

web-padawan left a comment

Choose a reason for hiding this comment

samthor commented Feb 21, 2020

web-padawan left a comment

Choose a reason for hiding this comment

justinfagnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chase-moskal Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chase-moskal Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samthor commented Mar 29, 2020

samthor commented Mar 29, 2020

justinfagnani commented Apr 7, 2020

samthor commented Apr 7, 2020

samthor commented Apr 7, 2020

sorvell left a comment • edited Loading

Choose a reason for hiding this comment

justinfagnani commented Apr 9, 2020

samthor commented Apr 9, 2020

sorvell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samthor commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samthor commented Apr 10, 2020

sorvell left a comment

Choose a reason for hiding this comment

samthor commented Apr 11, 2020

samthor commented Nov 17, 2019 •

edited

Loading

chase-moskal Mar 30, 2020 •

edited

Loading

chase-moskal Mar 31, 2020 •

edited

Loading

sorvell left a comment •

edited

Loading