Adds `attachShadow({shadyUpgradeFragment: documentFragment})` #316

sorvell · 2019-02-14T18:43:07Z

This API is being added as an optimization for creating and initially populating a ShadowRoot. Instead of:

element.attachShadow({mode: 'open'});
element.shadowRoot.appendChild(aDocFragment);

it's now faster to do:

element.attachShadow({mode: 'open', shadyUpgradeFragment: aDocFragment});
element.shadowRoot.appendChild(aDocFragment);

This removes the need to create an extra document fragment and optimizes the copying of nodes into the shadowRoot. Note that when this is used, the passed in documentFragment becomes the shadowRoot.

* As an optimization for creation and initial population of a ShadowRoot, adds the optional `upgrade` static method. Note, the `fragment` dom must already have any necessary style scoping applied. * Also removes initial default property sets from `ShadyData` and `ShadyRoot` which is empirically slightly faster. * slightly optimizes capturing logical data by not accessing `childNodes`

This replaces but is the same as `ShadowRoot.upgrade`. Prefer putting bespoke API on non-standard object. Also adds some slight type refinement.

Remove`.only` that was mistakenly skipping tests. Slight re-organization of a few tests

Since `appendChild` is not used, the custom elements patch that calls `connectedCallback` will not run. Therefore, do not use this when custom elements are being polyfilled.

Performing `appendChild` with a documentFragment with a patched prototype appears to blow up IE.

kevinpschaaf · 2019-02-20T08:08:47Z

src/attach-shadow.js

-  Object.defineProperty(window['CustomElementRegistry'].prototype, 'define', {
-    value: function(name, constructor) {
+  //Object.defineProperty(window['CustomElementRegistry'].prototype, 'define', {
+  window.customElements.define = function(name, constructor) {


If we're keeping this change, let's add a comment for why... did something else patch the instance rather than the prototype?

Yeah, ShadyCSS patches this directly. Changing is path of least resistance.

We could change this though, correct? https://github.com/webcomponents/shadycss/blob/f377017e50f4d472a57ac224973f88b63a92f17e/src/style-placeholder.js#L54

The "fast" (tearoff) native shim also patches the instance. So nevermind, let's just align on instance patching for now.

kevinpschaaf · 2019-02-20T08:10:50Z

src/link-nodes.js

-    let c$ = node[utils.SHADY_PREFIX + 'childNodes'];
-    for (let i=0; i < c$.length; i++) {
-      linkNode(c$[i], container, ref_node);
+    const first = node[utils.NATIVE_PREFIX + 'firstChild'] || null;


Is || null required? Native should always return null, no?

Yup, removing.

kevinpschaaf · 2019-02-20T08:22:12Z

src/link-nodes.js

+  if (!root && nodeData.firstChild !== undefined) {
+    return;
+  }
+  root = root || node;


Confusing to call this root here, it's really parent. L110 could just be sd.parentNode = root || node.

It might be good to call the root argument adoptedRoot or something like that?

Changed to adoptedParent

kevinpschaaf · 2019-02-20T08:27:34Z

src/patches/Node.js

@@ -307,7 +306,7 @@ export const NodePatches = utils.getOwnPropertyDescriptors({
      });
    }
    // if a slot is added, must render containing root.
-    if (this.localName === 'slot' || slotsAdded.length) {
+    if (slotsAdded.length) {


Why can this.localName === 'slot' be removed?

It was incorrect. This says "the node we're inserting into is a slot" and in this case a slot was not added. What it was meant to trap was node.localName but this is not needed because the treeVisitor checks node.

src/shady-data.js

kevinpschaaf · 2019-02-20T08:35:17Z

src/shadydom.js

@@ -82,9 +84,23 @@ if (utils.settings.inUse) {
    // access that requires Shadow DOM behavior to be proxied via `ShadyDOM.wrap`.
    'noPatch': utils.settings.noPatch,
    'nativeMethods': nativeMethods,
-    'nativeTree': nativeTree
+    'nativeTree': nativeTree,
+    'upgrade': (fragment, host, options) => {


upgrade just sounds too much like the CE concept... can we just call it attachDom?

src/link-nodes.js

kevinpschaaf · 2019-02-20T23:41:53Z

src/utils.js

+settings.IS_IE = IS_IE;
+
+let _canUpgrade;
+// do not use `upgrade` when custom elements is polyfilled or on IE.


Elminiate the bail-out on CE polyfill by detecting shadowRoot.appendChild(shadowRoot) and doing the attachDom work there.

IE blows up if we patch the documentFragment.

Optimizations were moved in situ: attachDOM puts the fragment on the host and attachShadow and appendChild use that.

This requires making sure `attachShadow` and `appendChild` are called. The code now special cases inside of those patches.

* ensures a shadowRoot itself does not maintain a `__noInsertionPoint` flag after initial creation when ShadyDOM.attachDom is used. * adds small optimization to prevent text nodes from being unscoped

…nt options.

kevinpschaaf · 2019-04-12T20:58:33Z

src/patch-events.js

@@ -440,7 +440,7 @@ export function addEventListener(type, fnOrObj, optionsOrCapture) {
  const wrapperFn = function(e) {
    // Support `once` option.
    if (once) {
-      this[utils.SHADY_PREFIX + 'removeEventListener'](type, fnOrObj, nativeEventOptions);
+      this[utils.SHADY_PREFIX + 'removeEventListener'](type, fnOrObj, optionsOrCapture);


Not following this change...

The nativeEventOptions should only be passed to native methods. It's important to pass the actual options to the removeEventListener so we can find the wrapper function to remove.

kevinpschaaf · 2019-04-13T00:13:30Z

src/shadydom.js

@@ -82,7 +84,16 @@ if (utils.settings.inUse) {
    // access that requires Shadow DOM behavior to be proxied via `ShadyDOM.wrap`.
    'noPatch': utils.settings.noPatch,
    'nativeMethods': nativeMethods,
-    'nativeTree': nativeTree
+    'nativeTree': nativeTree,
+    'attachDom': (fragment, host, options) => {


Suggestion: eliminate attachDom and instead have attachShadow take another option, such that this pattern triggers the same logic:

this.attachShadow({mode: 'open', shadyUseFragment: fragment}); this.shadowRoot.appendChild(fragment);

…agment})`

kevinpschaaf

LGTM

Steven Orvell added 11 commits January 22, 2019 17:46

[upgrade] Fix rendering with slots

71f9768

Avoid childNodes when possible.

96be5a4

More avoiding of childNodes when possible.

3091902

Remove superfluous check

3da0f04

[upgrade] Refine argument name

eb98d72

Merge branch 'more-flush' into shadowRoot-upgrade

a0ce84a

Merge branch 'master' into shadowRoot-upgrade

f542a57

Merge branch 'master' into shadowRoot-upgrade

a67d835

Adds ShadyDOM.upgrade(fragment, host, options)

d359957

This replaces but is the same as `ShadowRoot.upgrade`. Prefer putting bespoke API on non-standard object. Also adds some slight type refinement.

Re-add skipped tests

4342887

Remove`.only` that was mistakenly skipping tests. Slight re-organization of a few tests

sorvell requested a review from kevinpschaaf February 14, 2019 18:43

sorvell requested a review from dfreedm as a code owner February 14, 2019 18:43

upgarde: avoid optimal path when customElements polyfill is in use

003a148

Since `appendChild` is not used, the custom elements patch that calls `connectedCallback` will not run. Therefore, do not use this when custom elements are being polyfilled.

sorvell mentioned this pull request Feb 14, 2019

Use ShadyDOM.attachDom Polymer/polymer#5487

Merged

Avoid upgrade on IE

2b355c2

Performing `appendChild` with a documentFragment with a patched prototype appears to blow up IE.

kevinpschaaf suggested changes Feb 20, 2019

View reviewed changes

Address review feedback

59cff79

kevinpschaaf changed the title ~~Adds ShadyDOM.upgrade(fragment, host, options)~~ Adds ShadyDOM.attachShadow(fragment, host, options) Feb 20, 2019

kevinpschaaf changed the title ~~Adds ShadyDOM.attachShadow(fragment, host, options)~~ Adds ShadyDOM.attachDom(fragment, host, options) Feb 20, 2019

Slight simplification based on review

87c8e35

kevinpschaaf reviewed Feb 20, 2019

View reviewed changes

src/link-nodes.js Show resolved Hide resolved

kevinpschaaf reviewed Feb 20, 2019

View reviewed changes

Allow ShadyDOM.attachDOM to work in the customElements polyfill

f70e34e

This requires making sure `attachShadow` and `appendChild` are called. The code now special cases inside of those patches.

Steven Orvell added 3 commits April 1, 2019 14:15

Merge branch 'master' into shadowRoot-upgrade

ba1a238

Merge branch 'wrap-className' into shadowRoot-upgrade

4eb1c12

Merge branch 'master' into shadowRoot-upgrade

3fb6f83

Steven Orvell added 3 commits April 4, 2019 16:12

Ensure scoping updates correctly when ShadyDOM.attachDom is used.

9fa8f8a

* ensures a shadowRoot itself does not maintain a `__noInsertionPoint` flag after initial creation when ShadyDOM.attachDom is used. * adds small optimization to prevent text nodes from being unscoped

Fix event removal for platforms (e.g. old Chrome) that don't have eve…

5d9f909

…nt options.

Simplify native patching slightly and add className

9dd608d

kevinpschaaf reviewed Apr 12, 2019

View reviewed changes

kevinpschaaf reviewed Apr 13, 2019

View reviewed changes

Remove ShadyDOM.attachDom in favor of `attachShadow({shadyUpgradeFr…

eb6e08a

…agment})`

sorvell changed the title ~~Adds ShadyDOM.attachDom(fragment, host, options)~~ Adds attachShadow({shadyUpgradeFragment: documentFragment}) Apr 13, 2019

Steven Orvell added 2 commits April 12, 2019 17:41

Fix typo

d585fd4

Lint fix.

5a77147

kevinpschaaf approved these changes Apr 13, 2019

View reviewed changes

kevinpschaaf merged commit 4447ac7 into master Apr 13, 2019

kevinpschaaf deleted the shadowRoot-upgrade branch April 13, 2019 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `attachShadow({shadyUpgradeFragment: documentFragment})` #316

Adds `attachShadow({shadyUpgradeFragment: documentFragment})` #316

sorvell commented Feb 14, 2019 •

edited

Loading

kevinpschaaf Feb 20, 2019

sorvell Feb 20, 2019

kevinpschaaf Apr 12, 2019

kevinpschaaf Apr 12, 2019

kevinpschaaf Feb 20, 2019

sorvell Feb 20, 2019

kevinpschaaf Feb 20, 2019

sorvell Apr 10, 2019

kevinpschaaf Feb 20, 2019 •

edited

Loading

sorvell Apr 10, 2019

kevinpschaaf Feb 20, 2019

sorvell Apr 10, 2019

kevinpschaaf Feb 20, 2019

kevinpschaaf Feb 20, 2019

sorvell Apr 10, 2019

kevinpschaaf Apr 12, 2019

sorvell Apr 13, 2019

kevinpschaaf Apr 13, 2019

kevinpschaaf left a comment

Adds attachShadow({shadyUpgradeFragment: documentFragment}) #316

Adds attachShadow({shadyUpgradeFragment: documentFragment}) #316

Conversation

sorvell commented Feb 14, 2019 • 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

kevinpschaaf Feb 20, 2019 • 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

kevinpschaaf left a comment

Choose a reason for hiding this comment

Adds `attachShadow({shadyUpgradeFragment: documentFragment})` #316

Adds `attachShadow({shadyUpgradeFragment: documentFragment})` #316

sorvell commented Feb 14, 2019 •

edited

Loading

kevinpschaaf Feb 20, 2019 •

edited

Loading