Skip to content

Commit

Permalink
🐛 Upgrade shadow root contents when defining new CEs (ampproject#25337)
Browse files Browse the repository at this point in the history
* Upgrade shadow root contents when defining new CEs

When we've already initialized the the custom element registry, it used to drop the list of shadow roots. When a new CE was defined, we would then only query the host `document` (since we dropped all the shadow roots from the array). Obviously, that query selector won't find CEs inside the shadow tree.

So, we need to keep the list of shadow roots, that way we can query inside them when defining new custom elements.

Note, this will mean leaking every shadow permenantly. Even if the shadow's host is removed from the document, we'll still have to keep its reference. We could setup a long running interval to find disconnected trees, but it wouldn't be able to tell the difference between a temporarily disconneded shadow and a destoyed shadow.

* Fix comment

* Remove unused private
  • Loading branch information
jridgewell authored Oct 30, 2019
1 parent 82e6a1f commit 476b89d
Showing 1 changed file with 10 additions and 13 deletions.
23 changes: 10 additions & 13 deletions src/polyfills/custom-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,11 +227,6 @@ class Registry {
*/
this.win_ = win;

/**
* @private @const
*/
this.doc_ = win.document;

/**
* @type {!Object<string, !CustomElementDef>}
* @private
Expand Down Expand Up @@ -260,12 +255,11 @@ class Registry {
this.mutationObserver_ = null;

/**
* All the observed DOM trees, including shadow trees. This is cleared out
* when the mutation observer is created.
* All the observed DOM trees, including shadow trees.
*
* @private @const {!Array<!Node>}
*/
this.observed_ = [win.document];
this.roots_ = [win.document];
}

/**
Expand Down Expand Up @@ -346,7 +340,9 @@ class Registry {
};

this.observe_(name);
this.upgrade(this.doc_, name);
this.roots_.forEach(tree => {
this.upgrade(tree, name);
});
}

/**
Expand Down Expand Up @@ -510,10 +506,12 @@ class Registry {
});
this.mutationObserver_ = mo;

this.observed_.forEach(tree => {
// I would love to not have to hold onto all of the roots, since it's a
// memory leak. Unfortunately, there's no way to iterate a list and hold
// onto its contents weakly.
this.roots_.forEach(tree => {
mo.observe(tree, TRACK_SUBTREE);
});
this.observed_.length = 0;

installPatches(this.win_, this);
}
Expand All @@ -524,10 +522,9 @@ class Registry {
* @param {!Node} tree
*/
observe(tree) {
this.roots_.push(tree);
if (this.mutationObserver_) {
this.mutationObserver_.observe(tree, TRACK_SUBTREE);
} else {
this.observed_.push(tree);
}
}

Expand Down

0 comments on commit 476b89d

Please sign in to comment.