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

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

Merged
merged 14 commits into from Jun 12, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -35,7 +35,7 @@
"devDependencies": {
"@types/chai": "^4.1.0",
"@types/mocha": "^5.2.0",
"@webcomponents/shadycss": "^1.5.2",
"@webcomponents/shadycss": "^1.8.0",
"@webcomponents/webcomponentsjs": "^2.0.4",
"chai": "^4.1.2",
"clang-format": "^1.2.4",
Expand Down
4 changes: 4 additions & 0 deletions src/env.d.ts
Expand Up @@ -5,6 +5,10 @@ interface ShadyCSS {
prepareTemplateDom(template: Element, elementName: string): void;
prepareTemplateStyles(
template: Element, elementName: string, typeExtension?: string): void;
ScopingShim: undefined|{
prepareAdoptedCssText(
cssTextArray: Array<string>, elementName: string): void;
};
}

interface ShadyDOM {
Expand Down
23 changes: 15 additions & 8 deletions src/lib/shady-render.ts
Expand Up @@ -24,6 +24,7 @@
* Do not remove this comment; it keeps typedoc from misplacing the module
* docs.
*/
import {html} from '../lit-html.js';
import {removeNodes} from './dom.js';
import {insertNodeIntoTemplate, removeNodesFromTemplate} from './modify-template.js';
import {RenderOptions} from './render-options.js';
Expand Down Expand Up @@ -248,7 +249,7 @@ export const render =
const hasRendered = parts.has(container);
const needsScoping = compatibleShadyCSSVersion &&
container.nodeType === 11 /* Node.DOCUMENT_FRAGMENT_NODE */ &&
!!(container as ShadowRoot).host && result instanceof TemplateResult;
!!(container as ShadowRoot).host;
// Handle first render to a scope specially...
const firstScopeRender = needsScoping && !shadyRenderSet.has(scopeName);
// On first scope render, render into a fragment; this cannot be a single
Expand All @@ -272,12 +273,18 @@ export const render =
if (firstScopeRender) {
const part = parts.get(renderContainer)!;
parts.delete(renderContainer);
if (part.value instanceof TemplateInstance) {
prepareTemplateStyles(
renderContainer as DocumentFragment,
part.value.template,
scopeName);
}
// ShadyCSS might have style sheets (e.g. from `prepareAdoptedCssText`)
// that should apply to `renderContainer` even if the rendered value is
// not a TemplateInstance. However, it will only insert scoped styles
// 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

const template = part.value instanceof TemplateInstance ?
part.value.template :
new Template(html``, document.createElement('template'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this part for? Add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

prepareTemplateStyles(
renderContainer as DocumentFragment, template, scopeName);
removeNodes(container, container.firstChild);
container.appendChild(renderContainer);
parts.set(container, part);
Expand All @@ -286,7 +293,7 @@ export const render =
// initial render to this container.
// This is needed whenever dynamic changes are made so it would be
// safest to do every render; however, this would regress performance
// so we leave it up to the user to call `ShadyCSSS.styleElement`
// so we leave it up to the user to call `ShadyCSS.styleElement`
// for dynamic changes.
if (!hasRendered && needsScoping) {
window.ShadyCSS!.styleElement((container as ShadowRoot).host);
Expand Down
41 changes: 41 additions & 0 deletions src/test/lib/shady-render-scoping-shim_test.ts
@@ -0,0 +1,41 @@
/**
* @license
* Copyright (c) 2017 The Polymer Project Authors. All rights reserved.
* This code may only be used under the BSD style license found at
* http://polymer.github.io/LICENSE.txt
* The complete set of authors may be found at
* http://polymer.github.io/AUTHORS.txt
* The complete set of contributors may be found at
* http://polymer.github.io/CONTRIBUTORS.txt
* Code distributed by Google as part of the polymer project is also
* subject to an additional IP rights grant found at
* http://polymer.github.io/PATENTS.txt
*/

import {renderShadowRoot} from '../test-utils/shadow-root.js';

const assert = chai.assert;

suite('shady-render scoping shim', () => {
test('scoped styles are applied for non-TemplateResult values', function() {
if (typeof window.ShadyDOM === 'undefined' || !window.ShadyDOM.inUse) {
this.skip();
return;
}
if (typeof window.ShadyCSS === 'undefined' ||
window.ShadyCSS.nativeShadow ||
window.ShadyCSS.ScopingShim === undefined) {
this.skip();
return;
}
const container = document.createElement('scope-1');
window.ShadyCSS.ScopingShim.prepareAdoptedCssText(
[':host { border-top: 2px solid black; }'], 'scope-1');
document.body.appendChild(container);
renderShadowRoot(undefined, container);
assert.equal(
getComputedStyle(container).getPropertyValue('border-top-width').trim(),
'2px');
document.body.removeChild(container);
});
});
3 changes: 1 addition & 2 deletions src/test/test-utils/shadow-root.ts
Expand Up @@ -12,12 +12,11 @@
* http://polymer.github.io/PATENTS.txt
*/
import {render} from '../../lib/shady-render.js';
import {TemplateResult} from '../../lit-html.js';

/**
* A helper for creating a shadowRoot on an element.
*/
export const renderShadowRoot = (result: TemplateResult, element: Element) => {
export const renderShadowRoot = (result: unknown, element: Element) => {
if (!element.shadowRoot) {
element.attachShadow({mode: 'open'});
}
Expand Down
3 changes: 3 additions & 0 deletions test/index.html
Expand Up @@ -32,6 +32,9 @@
'shady-apply.html',
'shady-apply.html?shadydom=true',
'shady-apply.html?shadydom=true&shimcssproperties=true',
'shady-scoping-shim.html',
'shady-scoping-shim.html?shadydom=true',
'shady-scoping-shim.html?shadydom=true&shimcssproperties=true',
'no-template.html',
]);
</script>
Expand Down
21 changes: 21 additions & 0 deletions test/shady-scoping-shim.html
@@ -0,0 +1,21 @@
<!doctype html>
<html>
<head>
<script>
if (location.search.match('shadydom')) {
window.ShadyDOM = {force: true};
}
if (location.search.match('shimcssproperties')) {
window.ShadyCSS = {shimcssproperties: true};
}
</script>
<script src="../node_modules/mocha/mocha.js"></script>
<script src="../node_modules/chai/chai.js"></script>
<script src="../node_modules/wct-mocha/wct-mocha.js"></script>
<script src="../node_modules/@webcomponents/webcomponentsjs/webcomponents-bundle.js"></script>
<script src="../node_modules/@webcomponents/shadycss/scoping-shim.min.js"></script>
</head>
<body>
<script type="module" src="./lib/shady-render-scoping-shim_test.js"></script>
</body>
</html>