Scope styles even if the rendered value is not a TemplateResult. #910

bicknellr · 2019-05-13T22:33:13Z

This should have been part of #904. The ShadyDOM/CSS-aware renderer will only take the actions to scope styles if the value being rendered is a TemplateResult. Before #904, the rendered value was always assumed to be a TemplateResult and these checks were redundant. However, to implement lit/lit-element#618, the style-scoping steps need to happen even if the value isn't a TemplateResult because some user (e.g. lit-element) might have added adopted style sheets for the render container.

justinfagnani · 2019-05-13T23:13:53Z

src/lib/shady-render.ts

-        }
+        const template = part.value instanceof TemplateInstance ?
+            part.value.template :
+            new Template(html``, document.createElement('template'));


What's this part for? Add a comment.

Added, let me know what you think.

Is this really going to work? prepareTemplateStyles might insert styles into this template, but for what purpose if the template is never rendered anywhere?

I get the feeling that this isn't going to work with the original use-case of rendering undefined, then rendering a TemplateResult. Can we add a test specifically for that, using shady-render?

I think the gist is that the styling we are about in this case was sent to ShadyCSS via prepareAdoptedCssText. Styling in the non-template result would just not be supported.

So it seems like all we need to do in this case is ensure that (1) styles are removed from any lit template for the scope (would there be any if no TemplateResult is being rendered?, (2) ensure that prepareTemplate is called (because this is necessary to apply any styles defined with prepareAdoptedCssText). Why not change prepareTemplateStyles to optionally take a Template rather than creating a dummy one here?

Is this really going to work? prepareTemplateStyles might insert styles into this template, but for what purpose if the template is never rendered anywhere?

I get the feeling that this isn't going to work with the original use-case of rendering undefined, then rendering a TemplateResult. Can we add a test specifically for that, using shady-render?

Ok, I added another test specifically for this case.

I think the gist is that the styling we are about in this case was sent to ShadyCSS via prepareAdoptedCssText. Styling in the non-template result would just not be supported.

AFAICT, the current setup seems to be that all styles in the first render are scoped and any styles that appear later aren't. So I think that even styles inserted into NodeParts of a top-level TemplateInstance were collected / scoped / removed from their rendered position on the first render.

Am I correct in thinking that (local) prepareTemplateStyles' renderedDOM contains the complete rendered output of the first render by the time it runs?

So it seems like all we need to do in this case is ensure that (1) styles are removed from any lit template for the scope (would there be any if no TemplateResult is being rendered?, (2) ensure that prepareTemplate is called (because this is necessary to apply any styles defined with prepareAdoptedCssText). Why not change prepareTemplateStyles to optionally take a Template rather than creating a dummy one here?

I was avoiding this because it seemed difficult to pry apart the Template editing parts of (local) prepareTemplateStyles from the parts that were dealing with changing the output of the first render but it seems necessary at this point to match the earlier behavior - I'll update.

I made prepareTemplateStyles' template parameter optional and updated the logic in there to work with a new, empty template if it isn't passed. Also, I added a couple extra tests that ShadyCSS mixins and scoping are applied to styles that are part of an initial render for a scope of a non-TemplateInstance value.

…teStyles`.

justinfagnani · 2019-05-14T02:48:23Z

src/lib/shady-render.ts

-        }
+        const template = part.value instanceof TemplateInstance ?
+            part.value.template :
+            new Template(html``, document.createElement('template'));


Is this really going to work? prepareTemplateStyles might insert styles into this template, but for what purpose if the template is never rendered anywhere?

I get the feeling that this isn't going to work with the original use-case of rendering undefined, then rendering a TemplateResult. Can we add a test specifically for that, using shady-render?

sorvell · 2019-05-14T17:02:26Z

src/lib/shady-render.ts

-        }
+        const template = part.value instanceof TemplateInstance ?
+            part.value.template :
+            new Template(html``, document.createElement('template'));


I think the gist is that the styling we are about in this case was sent to ShadyCSS via prepareAdoptedCssText. Styling in the non-template result would just not be supported.

So it seems like all we need to do in this case is ensure that (1) styles are removed from any lit template for the scope (would there be any if no TemplateResult is being rendered?, (2) ensure that prepareTemplate is called (because this is necessary to apply any styles defined with prepareAdoptedCssText). Why not change prepareTemplateStyles to optionally take a Template rather than creating a dummy one here?

… rendered value was not a TemplateInstance.

…(...) - Adds a test that ShadyCSS mixins are processed when the first render is of a non-TemplateInstance value. - Adds a test that ShadyCSS scoping is applied when the first render is of a non-TemplateInstance value.

This reverts commit 86412ca.

…result

justinfagnani · 2019-06-03T19:57:19Z

The PR now has unrelated changes in it from master. Can you rebase?

justinfagnani · 2019-06-03T19:58:42Z

I guess that's the merge of #904

justinfagnani

LGTM. @sorvell ?

…result

sorvell

It's unclear why styling is incorrect after you eventually render a TemplateResult (without this modification). Perhaps the underlying issue is that the user expects :host styles to render initially (when rendering something like undefined)?

src/lib/parts.ts

sorvell · 2019-06-05T19:05:11Z

src/lib/shady-render.ts

+      if (template) {
+        insertNodeIntoTemplate(template, condensedStyle, content.firstChild);
+      } else {
+        content.insertBefore(condensedStyle, content.firstChild);


Because of the comment below this, I think this case breaks support for @apply when using native Shadow DOM. That might be ok, but at the least we need to document it.

We should be able to add a test that demonstrates this.

I added the comment we were working on in offline discussion.

sorvell · 2019-06-05T19:06:12Z

src/lib/shady-render.ts

+        // into the document if `prepareTemplateStyles` has already been called
+        // for the given scope name. `prepareTemplateStyles` requires a
+        // template element (and the local wrapper requires a Template), so we
+        // create an empty one here to satisfy it.


Update this comment since we're not creating an empty template here.

sorvell · 2019-06-05T19:07:28Z

src/test/lib/render_test.ts

+  // tests are really just a sanity check that they are accepted by `render`.
+  // Tests about rendering behavior for specific values should generally be
+  // grouped with those of `NodePart#setValue` and `#commit`.
+  suite('accepts types other than TemplateResult', () => {


Let's add a test for accepting a Node since that seems like it might not be uncommon.

Added at the bottom.

bicknellr · 2019-06-06T18:44:23Z

It's unclear why styling is incorrect after you eventually render a TemplateResult (without this modification). Perhaps the underlying issue is that the user expects :host styles to render initially (when rendering something like undefined)?

Yeah, the :host styles should render when the first result is undefined. (Trying to remember our offline chat about this...) Particularly, this is a problem when Shadow DOM is supported but adoptedStyleSheets is not. Assuming this check for TemplateResult in lit-element was removed, then lit-html's checks for TemplateResult and for TemplateInstance will prevent it from running ShadyCSS scoping on the first actual render if the rendered result was undefined, but lit-element still inserts the styles it created to simulate adoptedStyleSheets. Then, the first time that a TemplateResult is produced by render, it will run the scoping but also remove all of the content of renderContainer, which contained the styles that were inserted by lit-element.

…result

justinfagnani

Thanks for the additional comments

justinfagnani · 2019-06-07T18:20:08Z

src/lib/shady-render.ts

+      // Template's underlying template element. Otherwise, we create one here
+      // to give to ShadyCSS, which still requires one while scoping.
+      const templateElement =
+          template ? template.element : document.createElement('template');


Always prefer explicit comparisons over implicity ToBoolean conversions, here and in two if statements below.

Ok, these are explicit now.

…#910) * Scope styles even if the rendered value is not a TemplateResult. * Add a comment about why an empty Template is passed to `prepareTemplateStyles`. * Test that adopted styles still apply after later renders if the first rendered value was not a TemplateInstance. * shady-render: `prepareTemplateStyles` no longer requires a Template. (...) - Adds a test that ShadyCSS mixins are processed when the first render is of a non-TemplateInstance value. - Adds a test that ShadyCSS scoping is applied when the first render is of a non-TemplateInstance value. * Revert "Revert "Allow `render` to accept any type. (lit#904)" (lit#913)" This reverts commit 86412ca. * Add / update comments. * Test that `render` accepts a node. * Add a comment about interaction with @apply. * Explicitly convert to boolean in conditions.

Scope styles even if the rendered value is not a TemplateResult.

822b15f

bicknellr requested review from jridgewell, justinfagnani and sorvell May 13, 2019 22:33

justinfagnani requested changes May 13, 2019

View reviewed changes

bicknellr requested a review from dfreedm May 13, 2019 23:58

Add a comment about why an empty Template is passed to `prepareTempla…

434aa5f

…teStyles`.

justinfagnani requested changes May 14, 2019

View reviewed changes

sorvell requested changes May 14, 2019

View reviewed changes

bicknellr added 2 commits May 14, 2019 17:50

Test that adopted styles still apply after later renders if the first…

6e254e1

… rendered value was not a TemplateInstance.

bicknellr mentioned this pull request May 15, 2019

Revert "Allow render to accept any type. (#904)" #913

Merged

bicknellr added 3 commits May 17, 2019 18:19

Revert "Revert "Allow render to accept any type. (#904)" (#913)"

72c11e8

This reverts commit 86412ca.

Merge commit '72c11e8' into shady-non-template-result

9e1efa2

Merge remote-tracking branch 'github/master' into shady-non-template-…

da77ffd

…result

bicknellr requested review from sorvell and justinfagnani May 31, 2019 17:30

justinfagnani previously approved these changes Jun 3, 2019

View reviewed changes

bicknellr added 2 commits June 3, 2019 14:25

Merge remote-tracking branch 'github/master' into shady-non-template-…

da77bbd

…result

Merge remote-tracking branch 'github/master' into shady-non-template-…

2150067

…result

sorvell requested changes Jun 5, 2019

View reviewed changes

bicknellr added 2 commits June 5, 2019 15:34

Add / update comments.

4b8d150

Test that render accepts a node.

5d71c23

bicknellr dismissed justinfagnani’s stale review via 5d71c23 June 5, 2019 22:35

Add a comment about interaction with @apply.

bf1373b

bicknellr force-pushed the shady-non-template-result branch from da05c41 to bf1373b Compare June 6, 2019 01:10

sorvell previously approved these changes Jun 6, 2019

View reviewed changes

bicknellr requested a review from justinfagnani June 6, 2019 18:46

Merge remote-tracking branch 'github/master' into shady-non-template-…

68094d4

…result

justinfagnani previously approved these changes Jun 7, 2019

View reviewed changes

Explicitly convert to boolean in conditions.

572c103

bicknellr dismissed stale reviews from justinfagnani and sorvell via 572c103 June 7, 2019 20:28

bicknellr requested a review from justinfagnani June 7, 2019 20:29

justinfagnani approved these changes Jun 12, 2019

View reviewed changes

bicknellr merged commit 8655aa5 into master Jun 12, 2019

bicknellr deleted the shady-non-template-result branch June 12, 2019 20:52

bicknellr added a commit that referenced this pull request Jun 12, 2019

Update CHANGELOG.md for PR #910.

d83b530

bicknellr added a commit that referenced this pull request Jun 12, 2019

Update CHANGELOG.md for PR #910. (#945)

5fad61a

bicknellr mentioned this pull request Jun 13, 2019

Render any value returned by #render. lit/lit-element#712

Merged

2 tasks

bicknellr mentioned this pull request Jun 28, 2019

Update api.md for changes to render #953

Closed

neuronetio pushed a commit to neuronetio/lit-html that referenced this pull request Dec 2, 2019

Update CHANGELOG.md for PR lit#910. (lit#945)

238a436

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scope styles even if the rendered value is not a TemplateResult. #910

Scope styles even if the rendered value is not a TemplateResult. #910

bicknellr commented May 13, 2019

justinfagnani May 13, 2019

bicknellr May 14, 2019

justinfagnani May 14, 2019

sorvell May 14, 2019

bicknellr May 15, 2019

bicknellr May 15, 2019

justinfagnani May 14, 2019

sorvell May 14, 2019

justinfagnani commented Jun 3, 2019

justinfagnani commented Jun 3, 2019

justinfagnani left a comment

sorvell left a comment

sorvell Jun 5, 2019

sorvell Jun 5, 2019

bicknellr Jun 6, 2019

sorvell Jun 5, 2019

bicknellr Jun 5, 2019

sorvell Jun 5, 2019

bicknellr Jun 5, 2019

bicknellr commented Jun 6, 2019

justinfagnani left a comment

justinfagnani Jun 7, 2019

bicknellr Jun 7, 2019

Scope styles even if the rendered value is not a TemplateResult. #910

Scope styles even if the rendered value is not a TemplateResult. #910

Conversation

bicknellr commented May 13, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinfagnani commented Jun 3, 2019

justinfagnani commented Jun 3, 2019

justinfagnani left a comment

Choose a reason for hiding this comment

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

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

bicknellr commented Jun 6, 2019

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