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 all 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
9 changes: 6 additions & 3 deletions src/lib/parts.ts
Expand Up @@ -245,15 +245,18 @@ export class NodePart implements Part {
private __commitText(value: unknown): void {
const node = this.startNode.nextSibling!;
value = value == null ? '' : value;
// If `value` isn't already a string, we explicitly convert it here in case
// it can't be implicitly converted - i.e. it's a symbol.
const valueAsString: string =
justinfagnani marked this conversation as resolved.
Show resolved Hide resolved
typeof value === 'string' ? value : String(value);
if (node === this.endNode.previousSibling &&
node.nodeType === 3 /* Node.TEXT_NODE */) {
// If we only have a single text node between the markers, we can just
// set its value, rather than replacing it.
// TODO(justinfagnani): Can we just check if this.value is primitive?
(node as Text).data = value as string;
(node as Text).data = valueAsString;
} else {
this.__commitNode(document.createTextNode(
typeof value === 'string' ? value : String(value)));
this.__commitNode(document.createTextNode(valueAsString));
}
this.value = value;
}
Expand Down
9 changes: 4 additions & 5 deletions src/lib/render.ts
Expand Up @@ -20,18 +20,17 @@ import {removeNodes} from './dom.js';
import {NodePart} from './parts.js';
import {RenderOptions} from './render-options.js';
import {templateFactory} from './template-factory.js';
import {TemplateResult} from './template-result.js';

export const parts = new WeakMap<Node, NodePart>();

/**
* Renders a template to a container.
* Renders a template result or other value to a container.
*
* To update a container with new values, reevaluate the template literal and
* call `render` with the new result.
*
* @param result a TemplateResult created by evaluating a template tag like
* `html` or `svg`.
* @param result Any value renderable by NodePart - typically a TemplateResult
* created by evaluating a template tag like `html` or `svg`.
* @param container A DOM parent to render to. The entire contents are either
* replaced, or efficiently updated if the same result type was previous
* rendered there.
Expand All @@ -40,7 +39,7 @@ export const parts = new WeakMap<Node, NodePart>();
* container, as those changes will not effect previously rendered DOM.
*/
export const render =
(result: TemplateResult,
(result: unknown,
container: Element|DocumentFragment,
options?: Partial<RenderOptions>) => {
let part = parts.get(container);
Expand Down
50 changes: 35 additions & 15 deletions src/lib/shady-render.ts
Expand Up @@ -125,8 +125,13 @@ const shadyRenderSet = new Set<string>();
* output.
*/
const prepareTemplateStyles =
(renderedDOM: DocumentFragment, template: Template, scopeName: string) => {
(scopeName: string, renderedDOM: DocumentFragment, template?: Template) => {
shadyRenderSet.add(scopeName);
// If `renderedDOM` is stamped from a Template, then we need to edit that
// 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');
// Move styles out of rendered DOM and store.
const styles = renderedDOM.querySelectorAll('style');
const {length} = styles;
Expand All @@ -135,7 +140,14 @@ const prepareTemplateStyles =
// Ensure prepareTemplateStyles is called to support adding
// styles via `prepareAdoptedCssText` since that requires that
// `prepareTemplateStyles` is called.
window.ShadyCSS!.prepareTemplateStyles(template.element, scopeName);
//
// ShadyCSS will only update styles containing @apply in the template
// given to `prepareTemplateStyles`. If no lit Template was given,
// ShadyCSS will not be able to update uses of @apply in any relevant
// template. However, this is not a problem because we only create the
// template for the purpose of supporting `prepareAdoptedCssText`,
// which doesn't support @apply at all.
window.ShadyCSS!.prepareTemplateStyles(templateElement, scopeName);
return;
}
const condensedStyle = document.createElement('style');
Expand All @@ -153,18 +165,22 @@ const prepareTemplateStyles =
removeStylesFromLitTemplates(scopeName);
// And then put the condensed style into the "root" template passed in as
// `template`.
const content = template.element.content;
insertNodeIntoTemplate(template, condensedStyle, content.firstChild);
const content = templateElement.content;
if (!!template) {
insertNodeIntoTemplate(template, condensedStyle, content.firstChild);
} else {
content.insertBefore(condensedStyle, content.firstChild);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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 added the comment we were working on in offline discussion.

}
// Note, it's important that ShadyCSS gets the template that `lit-html`
// will actually render so that it can update the style inside when
// needed (e.g. @apply native Shadow DOM case).
window.ShadyCSS!.prepareTemplateStyles(template.element, scopeName);
window.ShadyCSS!.prepareTemplateStyles(templateElement, scopeName);
const style = content.querySelector('style');
if (window.ShadyCSS!.nativeShadow && style !== null) {
// When in native Shadow DOM, ensure the style created by ShadyCSS is
// included in initially rendered output (`renderedDOM`).
renderedDOM.insertBefore(style.cloneNode(true), renderedDOM.firstChild);
} else {
} else if (!!template) {
// When no style is left in the template, parts will be broken as a
// result. To fix this, we put back the style node ShadyCSS removed
// and then tell lit to remove that node from the template.
Expand Down Expand Up @@ -241,7 +257,7 @@ export interface ShadyRenderOptions extends Partial<RenderOptions> {
* supported.
*/
export const render =
(result: TemplateResult,
(result: unknown,
container: Element|DocumentFragment|ShadowRoot,
options: ShadyRenderOptions) => {
if (!options || typeof options !== 'object' || !options.scopeName) {
Expand All @@ -251,7 +267,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 @@ -275,12 +291,16 @@ 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.
const template = part.value instanceof TemplateInstance ?
part.value.template :
undefined;
prepareTemplateStyles(
scopeName, renderContainer as DocumentFragment, template);
removeNodes(container, container.firstChild);
container.appendChild(renderContainer);
parts.set(container, part);
Expand All @@ -289,7 +309,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
47 changes: 47 additions & 0 deletions src/test/lib/parts_test.ts
Expand Up @@ -124,6 +124,53 @@ suite('Parts', () => {
assert.equal(stripExpressionMarkers(container.innerHTML), '');
});

test('accepts a symbol', () => {
const sym = Symbol();
part.setValue(sym);
part.commit();
assert.equal(stripExpressionMarkers(container.innerHTML), String(sym));
});

test('accepts a symbol with a description', () => {
const sym = Symbol('description!');
part.setValue(sym);
part.commit();
assert.equal(stripExpressionMarkers(container.innerHTML), String(sym));
});

test('accepts a symbol on subsequent renders', () => {
const sym1 = Symbol();
part.setValue(sym1);
part.commit();
assert.equal(stripExpressionMarkers(container.innerHTML), String(sym1));

// If the previously rendered value caused a single text node to be
// created, then subsequent renders will try to update the existing text
// node by setting `.data`. If the new value is a symbol and it isn't
// explicitly converted with `String`, then this would throw.
const sym2 = Symbol('description!');
part.setValue(sym2);
part.commit();
assert.equal(stripExpressionMarkers(container.innerHTML), String(sym2));
});

test('accepts an object', () => {
part.setValue({});
part.commit();
assert.equal(
stripExpressionMarkers(container.innerHTML), '[object Object]');
});

test('accepts an object with a `toString` method', () => {
part.setValue({
toString() {
return 'toString!';
}
});
part.commit();
assert.equal(stripExpressionMarkers(container.innerHTML), 'toString!');
});

test('accepts a function', () => {
const f = () => {
throw new Error();
Expand Down
48 changes: 48 additions & 0 deletions src/test/lib/render_test.ts
Expand Up @@ -1409,4 +1409,52 @@ suite('render()', () => {
assert(container.innerHTML, '<div></div>');
});
});

// `render` directly passes the given value to `NodePart#setValue`, so these
// 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', () => {
Copy link
Member

Choose a reason for hiding this comment

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

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

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 at the bottom.

test('accepts undefined', () => {
render(undefined, container);
assert.equal(stripExpressionMarkers(container.innerHTML), '');
});

test('accepts a string', () => {
render('test', container);
assert.equal(stripExpressionMarkers(container.innerHTML), 'test');
});

test('accepts an object', () => {
render({}, container);
assert.equal(
stripExpressionMarkers(container.innerHTML), '[object Object]');
});

test('accepts an object with `toString`', () => {
render(
{
toString() {
return 'toString!';
}
},
container);
assert.equal(stripExpressionMarkers(container.innerHTML), 'toString!');
});

test('accepts an symbol', () => {
const sym = Symbol('description!');
render(sym, container);
assert.equal(stripExpressionMarkers(container.innerHTML), String(sym));
});

test('accepts a node', () => {
const div = document.createElement('div');
div.appendChild(document.createTextNode('text in the div'));
render(div, container);
assert.equal(
stripExpressionMarkers(container.innerHTML),
'<div>text in the div</div>');
});
});
});
25 changes: 25 additions & 0 deletions src/test/lib/shady-render-apply_test.ts
Expand Up @@ -49,6 +49,31 @@ suite('shady-render @apply', () => {
document.body.removeChild(container);
});

test('styles with mixins that are not in a TemplateInstance', function() {
const container = document.createElement('scope-6');
document.body.appendChild(container);
const style = document.createElement('style');
style.innerHTML = `
:host {
--batch: {
border: 3px solid orange;
padding: 4px;
};
}
div {
@apply --batch;
}
`;
const result = [style, htmlWithApply`<div>Testing...</div>`];
renderShadowRoot(result, container);
const div = (container.shadowRoot!).querySelector('div');
const computedStyle = getComputedStyle(div!);
assert.equal(
computedStyle.getPropertyValue('border-top-width').trim(), '3px');
assert.equal(computedStyle.getPropertyValue('padding-top').trim(), '4px');
document.body.removeChild(container);
});

test(
'styles with css custom properties using @apply render in different contexts',
async () => {
Expand Down