Primitive shadow parts support #329

aomarks · 2020-05-19T03:54:27Z

Note: suggest reviewing the two commits separately, since the first one contains some reformatting.

This supports basic patterns of styling parts whenever a node is inserted into a shadow root. It also adds and tests a disableShadowParts ShadyCSS setting.

Support for more kinds of mutations, exportparts, CSS custom properties are coming in follow up PRs.

Part of #311 and #252. Fixes #337.

This supports basic patterns of styling parts whenever a node is inserted into a shadow root. It also adds and tests a disableShadowParts ShadyCSS setting. Support for more kinds of mutations, exportparts, CSS custom properties are coming in follow up PRs.

sorvell · 2020-05-26T20:49:22Z

packages/shadycss/src/scoping-shim.js

@@ -23,6 +23,8 @@ import templateMap from './template-map.js';
 import * as ApplyShimUtils from './apply-shim-utils.js';
 import {updateNativeProperties, detectMixin} from './common-utils.js';
 import {CustomStyleInterfaceInterface, CustomStyleProvider} from './custom-style-interface.js'; // eslint-disable-line no-unused-vars
+import * as shadowParts from './shadow-parts.js';


Did you consider whether shadow parts should be a part of the scoping shim or given separate opt-in support in ShadyCSS, like @apply or custom style interface?

Good thought, hadn't considered that. What are the main advantages you are thinking of?

The main one I can think of is that there would be more options for shipping smaller bundles to users who don't need shadow parts. We could create more permutations of the bundles, and update the loader to check the disableShadowParts flag, and only load shadow parts support when not true.

Does this suggestion imply you think shadow parts should be opt-in rather than opt-out? I've been thinking we should do opt-out, because 1) shadow parts are so widely supported now that it feels like they are part of the core set of web components APIs, and 2) I think the performance cost to applications that don't end up using shadow parts is very small (since we will do almost nothing until a ::part style is first detected in at least one template).

I think you've evaluated it correctly.

Filed #339, will think about this a little more and refactor in a separate PR.

sorvell · 2020-05-26T20:50:53Z

packages/shadycss/src/shadow-parts.js

+export function onInsertBefore(parentNode, newNode, referenceNode) {
+  /* eslint-enable no-unused-vars */
+  if (!parentNode.getRootNode) {
+    // TODO(aomarks) Why is it in noPatch mode on Chrome 41 and other older


In noPatch mode, the DOM is not patched so getRootNode is not brought into existence because of the polyfill. On browsers that don't require the polyfill, getRootNode already exists.

To include support for noPatch mode, ShadyDOM.wrap must be called.

I'm having trouble getting tests to work with noPatch enabled -- it looks like there are no tests under shadycss with noPatch enabled yet. I'll need to spend some more time on it, so I've updated the TODO and filed #343 and will do noPatch support as a separate PR in next sprint, if that's ok.

sorvell · 2020-05-26T20:52:49Z

packages/shadycss/src/shadow-parts.js

+    // we don't waste time querying it.
+    return;
+  }
+  const parts = parentNode.querySelectorAll('[part]');


Again, to support noPatch mode any DOM API must use ShadyDOM.wrap.

Does this do a querySelectorAll on every call to insertBefore? If so, have you measured the performance impact of this? I'd expect it to be noticeable. ShadyDOM already conditionally does a walk during insertBefore (for example, to find slots); maybe consider a tighter integration to be able to participate in that walk.

Also consider doing this in prepareTemplateDom so that only dynamic parts need to be changed here.

This should be searching newNode and newNode.querySelectorAll, not parentNode.querySelectorAll.

Also consider doing this in prepareTemplateDom so that only dynamic parts need to be changed here.

Good idea. Probably missing something obvious here and I can look into it myself, but how can I avoid not querying the template again to look for parts when the template is stamped? Would I need to add some new tracing to understand when an insertBefore is happening as a result of the element's shadycss-registered template stamping, or is there already a way I could figure that out?

Also, it seems like this could cut down on the cost of the querySelectorAll since I guess I could store some kind of index of the part locations (or at the very least, a bit to tell me if it is even worth querying for parts), but I couldn't actually fully pre-process the template to add the shady-part attribute, since the value of that attribute depends on knowing the super-scope, which we don't know until we're connected. Does that sound right?

You'd need to add support for knowing when to not support the querySelector, but there's precedent for this: https://github.com./webcomponents/polyfills/blob/master/packages/shadydom/src/patches/Node.js#L309.

This should be searching newNode and newNode.querySelectorAll, not parentNode.querySelectorAll.

Done. I also needed some other changes to make this happen (since newNode can sometimes be a DocumentFragment or Text), and added tests for various kinds of inserts.

Again, to support noPatch mode any DOM API must use ShadyDOM.wrap.

As mentioned in other comment, will do in follow up (#343).

Does this do a querySelectorAll on every call to insertBefore? If so, have you measured the performance impact of this? I'd expect it to be noticeable. ShadyDOM already conditionally does a walk during insertBefore (for example, to find slots); maybe consider a tighter integration to be able to participate in that walk.

No benchmarks yet. I will add them in an upcoming task: #344

Will look into replacing the querySelectorAll with an integration into the ShadyDOM walk as an upcoming task: #345

Also, it seems like this could cut down on the cost of the querySelectorAll since I guess I could store some kind of index of the part locations (or at the very least, a bit to tell me if it is even worth querying for parts), but I couldn't actually fully pre-process the template to add the shady-part attribute, since the value of that attribute depends on knowing the super-scope, which we don't know until we're connected. Does that sound right?

@sorvell any thoughts on this? I've filed #346 for future investigation.

sorvell · 2020-05-26T20:56:54Z

packages/shadycss/src/shadow-parts.js

+
+/*  eslint-disable no-unused-vars */
+/**
+ * Perform any needed Shadow Parts updates after a ShadyDOM insertBefore call


Can you describe in more detail what this does and why it's necessary?

It would be much less stress if we didn't have to alter the part attribute. Can you describe how this would limit the fidelity of the shim?

In particular, naively, a rule like x-super x-host [part~=foo] is at least somewhat similar to what you have. This doesn't do lower bound encapsulation at all, but ShadyCSS already addresses this, so could it be x-super .x-host[part~=foo] ?

Yeah, interesting. I thought of an example that seemed sort of realistic where your suggestion would have caused an error, but then I thought of a modification that resolves it:

(1)

<style> /* error, the <div> grand-child of <x-other> shouldn't match */ x-super .x-host[part~=foo] { color: red; } /* but is OK with this modification */ x-super x-host.x-super .x-host[part~=foo] { color: red; } </style> <x-super> <x-host class="x-super"> <div class="x-host" part="foo"> <x-other class="x-super"> <x-host class="x-other"> <div class="x-host" part="foo">

However, you can still construct some patterns that fail:

(2)

<style> x-super x-host.x-super .x-host[part~=foo] { color: red; } </style> <x-super> <x-host class="x-super"> <div class="x-host" part="foo"> <x-other class="x-super"> <x-host class="x-other">  <div class="x-host" part="foo">

Whereas with the shady-parts attribute, the selectors are much tighter:

(3)

<style> x-super x-host [shady-part~="x-super:x-host:foo"] { color: red; } </style> <x-super> <x-host> <div class="x-host" part="foo" shady-part="x-super:x-host:foo"> <x-other> <x-host>  <div class="x-host" part="foo" shady-part="x-other:x-host:foo">

This is not to say that the shady-parts approach is perfect -- once you start repeating the same elements in a nesting hierarchy then you get similar problems (as described at https://github.com./webcomponents/polyfills/blob/shady-parts-basic/packages/shadycss/shadow-parts.md#3-recursion-is-crude). But it is at least a more limited set of patterns.

I guess the next question is, are examples like (2) above acceptable gaps in fidelity? It seems a little tricky to workaround the (2) example unless you can control the part names. WDYT? Can you think of any other selector tricks that would work even better?

The other consideration here is exported parts. In the implementation in #331, the way we support export parts is by adding additional shady-parts attributes to each part node based on the exportparts attributes found when we walk up the tree of shadow hosts. This allows one transformed style rule to match parts in all possible export paths.

I guess the way to do this if we used this other approach would be: each time we discover that an exported part needs to receive some style, we clone that style rule (or update its selector) with the new scope. We'd need to do per-instance styling here, though, in cases where two instances of the same element have different exportparts attributes. I already interact with per-instance styling in #333 to support custom properties within part rules, so it wouldn't be that wild. The difference here, though, would be that we'd be doing per-instance styling for more cases, even in Edge when native custom property support is available.

WDYT, am I thinking about this right?

Thanks for considering this so carefully. To summarize, nesting is a problem with any of our approaches, but with the current approach nesting of (A->B) inside an (A->B) is problematic whereas with the proposed approach nesting an A inside an A is problematic.

I was considering this without exportparts. I think considering that argues for how you've done it.

Acknowledged.

sorvell · 2020-05-26T21:02:01Z

packages/shadycss/src/style-transformer.js

+    if (!disableShadowParts && PART.test(selector)) {
+      // Hacky transform "::part(foo bar)" to "::part(foo,bar)" so that
+      // SIMPLE_SELECTOR_SEP isn't confused by the spaces.
+      // TODO(aomarks) Can we make SIMPLE_SELECTOR_SEP smarter instead?


It would be nice to work this out, but acknowledge that it may be non-trivial.

Acknowledged. Leaving for now with TODO.

sorvell · 2020-05-26T21:03:08Z

packages/shadycss/src/style-transformer.js

@@ -396,6 +405,20 @@ class StyleTransformer {
    return output.join('');
  }

+  _transformPartSelector(selector, scope) {


Doc with an example transformation.

sorvell · 2020-05-26T21:07:21Z

packages/shadydom/src/patches/Node.js

@@ -375,6 +375,12 @@ export const NodePatches = utils.getOwnPropertyDescriptors({
    } else if (node.ownerDocument !== this.ownerDocument) {
      this.ownerDocument.adoptNode(node);
    }
+    if (!utils.disableShadowParts) {


See previous comment about performance concerns and consider instead a tighter integration at https://github.com./webcomponents/polyfills/blob/master/packages/shadydom/src/patches/Node.js#L311-L333.

Acknowledged. Added TODO and filed #345 for upcoming task.

sorvell · 2020-05-26T21:16:06Z

packages/shadydom/src/utils.js

@@ -12,6 +12,10 @@ import {shadyDataForNode} from './shady-data.js';
 /** @type {!Object} */
 export const settings = window['ShadyDOM'] || {};

+/** @type {boolean} */
+export const disableShadowParts =
+    Boolean(window['ShadyCSS'] && window['ShadyCSS']['disableShadowParts']);


Shouldn't this be true if ShadyCSS does not exist?

I don't think so, should it? If there's no ShadyCSS, then the user hasn't set any initialization flags, so the default should hold, which would be disableShadowParts set to false.

aomarks

Thanks for review. Addressed some comments, but the others I've filed as follow up tasks, in the interest of incremental progress and getting the 3 PRs landed (note I'm merging all these PRs into the shady-parts dev branch). Hope that's ok!

Use ShadyDOM walk instead of querySelectorAll: Shady Parts: integrate with ShadyDOM walk instead of querySelectorAll #345
noPatch support: ShadyParts: noPatch mode support #343
Add benchmarks: Benchmarks for Shady Parts #344
Move work into prepareTemplateDom where possible: Shady Parts: move some work to prepareTemplateDom if possible #346

aomarks · 2020-06-02T01:56:23Z

packages/shadycss/src/scoping-shim.js

@@ -23,6 +23,8 @@ import templateMap from './template-map.js';
 import * as ApplyShimUtils from './apply-shim-utils.js';
 import {updateNativeProperties, detectMixin} from './common-utils.js';
 import {CustomStyleInterfaceInterface, CustomStyleProvider} from './custom-style-interface.js'; // eslint-disable-line no-unused-vars
+import * as shadowParts from './shadow-parts.js';


Filed #339, will think about this a little more and refactor in a separate PR.

aomarks · 2020-06-02T01:56:44Z

packages/shadycss/src/shadow-parts.js

+
+/*  eslint-disable no-unused-vars */
+/**
+ * Perform any needed Shadow Parts updates after a ShadyDOM insertBefore call


Acknowledged.

aomarks · 2020-06-04T00:00:53Z

packages/shadycss/src/shadow-parts.js

+export function onInsertBefore(parentNode, newNode, referenceNode) {
+  /* eslint-enable no-unused-vars */
+  if (!parentNode.getRootNode) {
+    // TODO(aomarks) Why is it in noPatch mode on Chrome 41 and other older


I'm having trouble getting tests to work with noPatch enabled -- it looks like there are no tests under shadycss with noPatch enabled yet. I'll need to spend some more time on it, so I've updated the TODO and filed #343 and will do noPatch support as a separate PR in next sprint, if that's ok.

aomarks · 2020-06-04T00:06:30Z

packages/shadycss/src/shadow-parts.js

+    // we don't waste time querying it.
+    return;
+  }
+  const parts = parentNode.querySelectorAll('[part]');


This should be searching newNode and newNode.querySelectorAll, not parentNode.querySelectorAll.

Done. I also needed some other changes to make this happen (since newNode can sometimes be a DocumentFragment or Text), and added tests for various kinds of inserts.

aomarks · 2020-06-04T00:08:40Z

packages/shadydom/src/utils.js

@@ -12,6 +12,10 @@ import {shadyDataForNode} from './shady-data.js';
 /** @type {!Object} */
 export const settings = window['ShadyDOM'] || {};

+/** @type {boolean} */
+export const disableShadowParts =
+    Boolean(window['ShadyCSS'] && window['ShadyCSS']['disableShadowParts']);


I don't think so, should it? If there's no ShadyCSS, then the user hasn't set any initialization flags, so the default should hold, which would be disableShadowParts set to false.

aomarks · 2020-06-04T21:05:38Z

packages/shadycss/src/shadow-parts.js

+    // we don't waste time querying it.
+    return;
+  }
+  const parts = parentNode.querySelectorAll('[part]');


Does this do a querySelectorAll on every call to insertBefore? If so, have you measured the performance impact of this? I'd expect it to be noticeable. ShadyDOM already conditionally does a walk during insertBefore (for example, to find slots); maybe consider a tighter integration to be able to participate in that walk.

No benchmarks yet. I will add them in an upcoming task: #344

Will look into replacing the querySelectorAll with an integration into the ShadyDOM walk as an upcoming task: #345

aomarks · 2020-06-04T21:08:28Z

packages/shadycss/src/shadow-parts.js

+    // we don't waste time querying it.
+    return;
+  }
+  const parts = parentNode.querySelectorAll('[part]');


Also, it seems like this could cut down on the cost of the querySelectorAll since I guess I could store some kind of index of the part locations (or at the very least, a bit to tell me if it is even worth querying for parts), but I couldn't actually fully pre-process the template to add the shady-part attribute, since the value of that attribute depends on knowing the super-scope, which we don't know until we're connected. Does that sound right?

@sorvell any thoughts on this? I've filed #346 for future investigation.

aomarks · 2020-06-04T21:10:35Z

packages/shadycss/src/style-transformer.js

+    if (!disableShadowParts && PART.test(selector)) {
+      // Hacky transform "::part(foo bar)" to "::part(foo,bar)" so that
+      // SIMPLE_SELECTOR_SEP isn't confused by the spaces.
+      // TODO(aomarks) Can we make SIMPLE_SELECTOR_SEP smarter instead?


Acknowledged. Leaving for now with TODO.

aomarks · 2020-06-04T21:17:28Z

packages/shadycss/src/style-transformer.js

@@ -396,6 +405,20 @@ class StyleTransformer {
    return output.join('');
  }

+  _transformPartSelector(selector, scope) {


aomarks · 2020-06-04T21:17:43Z

packages/shadydom/src/patches/Node.js

@@ -375,6 +375,12 @@ export const NodePatches = utils.getOwnPropertyDescriptors({
    } else if (node.ownerDocument !== this.ownerDocument) {
      this.ownerDocument.adoptNode(node);
    }
+    if (!utils.disableShadowParts) {


Acknowledged. Added TODO and filed #345 for upcoming task.

sorvell · 2020-06-04T23:58:38Z

packages/shadycss/src/shadow-parts.js

+    parts = newNode.querySelectorAll('[part]');
+  } else if (newNode instanceof Text) {
+    // E.g. an innerHTML assignment.
+    parts = parentNode.querySelectorAll('[part]');


Why is this here?

Good question, I got muddled in some debugging and was thinking the innerHTML call would show up as an insert with text, but obviously that's wrong. Removed.

sorvell · 2020-06-05T00:02:50Z

packages/tests/shadycss/shadow-parts/insertions.html

+      document.body.removeChild(xa);
+    });
+
+    test('appendChild: part', () => {


Add test for appending/inserting a Text node?

Good call, added test and an early return so that we don't crash in this case.

sorvell

One little nit but otherwise looks good.

packages/tests/shadycss/shadow-parts/insertions.html

Co-authored-by: Steve Orvell <[email protected]>

aomarks requested a review from sorvell May 19, 2020 03:54

aomarks requested a review from bicknellr as a code owner May 19, 2020 03:54

googlebot added the cla: yes label May 19, 2020

aomarks added 2 commits May 19, 2020 12:36

Fix Shady Parts testing, format, other misc tweaks

3922e3e

aomarks force-pushed the shady-parts-basic branch from bc6557e to 4e29618 Compare May 19, 2020 19:37

sorvell requested changes May 26, 2020

View reviewed changes

aomarks added 2 commits June 4, 2020 13:24

Query new node for parts, not parent, on insert

441c0de

Update shady parts comments

64b9e6b

aomarks commented Jun 4, 2020

View reviewed changes

aomarks requested a review from sorvell June 4, 2020 22:00

sorvell requested changes Jun 5, 2020

View reviewed changes

Fix shadow parts text handling

50f934a

aomarks requested a review from sorvell June 5, 2020 00:21

sorvell requested changes Jun 5, 2020

View reviewed changes

packages/tests/shadycss/shadow-parts/insertions.html Outdated Show resolved Hide resolved

Update packages/tests/shadycss/shadow-parts/insertions.html

afae643

Co-authored-by: Steve Orvell <[email protected]>

aomarks requested a review from sorvell June 5, 2020 02:14

sorvell approved these changes Jun 8, 2020

View reviewed changes

aomarks merged commit 33afe7d into shady-parts Jun 8, 2020

aomarks deleted the shady-parts-basic branch June 8, 2020 18:42

Primitive shadow parts support #329

Primitive shadow parts support #329

Conversation

aomarks commented May 19, 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

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

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

sorvell May 26, 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

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

aomarks 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

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

sorvell left a comment

Choose a reason for hiding this comment

aomarks commented May 19, 2020 •

edited

Loading

sorvell May 26, 2020 •

edited

Loading