ShadowDOM renderer invalidated after insertion-point distribution

[jcmoore]

EDIT: fixed formatting/markdown

Thanks for your work on Polymer!

This problem exists in v0.2.4 but not v0.2.3 -- possible solution described below.

Given 3 web components A, B, and C:

( see example https://gist.github.com/jcmoore/3432b06621e3c3ccd39c#file-shadow_events-html )

if A has changed-listerner and a corresponding getter on its prototype that will trigger a call to ShadowDOMPolyfill.renderAllPending(),

when C has a B in its shadow DOM template and C’s content-tag is inside of that B, and B has its own content-tag in its shadow DOM template,

when an A is placed in the light DOM of a C, B will not get events that originate on A.

This seems to be because during C's upgrade, its ShadowRenderer is invalidated and made pending before A's (and B's). C will then (synchronously) begin upgrading its subtree (which includes A and B). During A's upgrade, its createPropertyObserver() will trigger ShadowDOMPolyfill.renderAllPending() which will cause C's already queued ShadowRenderer to distribute insertion points (including B's content-tag) to A before A's own createShadowRoot() is called -- which will in turn cause its own ShadowRenderer to be invalidated and queued, ultimately abandoning the previously stored insertion points (especially those from B) at the next execution of ShadowDOMPolyfill.renderAllPending().

The result can be seen in ShadowDOM/src/wrappers/events.js' getEventPath() method, which will return a path missing B's insertion point(s) -- so event listeners on B will not fire for events on A.

I'm not sure what the side-effects are, but the undesirable invalidation/distribution order might be prevented by delaying any execution of ShadowDOMPolyfill.renderAllPending() until after any "currently upgrading" nodes have finished upgrading -- perhaps by making the following changes to ShadowDOM/src/ShadowRenderer.js:

https://gist.github.com/jcmoore/3432b06621e3c3ccd39c#file-shadowdom-src-shadowrenderer-js-L132-L156

and the following changes to CustomElements/src/CustomElements.js:

https://gist.github.com/jcmoore/3432b06621e3c3ccd39c#file-customelements-src-customelements-js-L235-L254

[jmesserly]

wow, thanks for the detailed notes! I can confirm this problem is still happening. Going to look into it further; ideally we could fix this purely in the ShadowDOM polyfill.

[jmesserly]

simplified it slightly, you can reproduce also by calling offsetWidth (which calls renderAllPending) from the created callback of mm-list-item.

                Polymer('mm-list-item', {
                    created: function() {
                        this.offsetWidth;
                    }
                });

[dlasky]

we also ran into a similar issue where accessing measurement properties in ready will cause this (basically any of the lifecycle callbacks before domReady)

[jmesserly]

ah, nice, thanks for confirming that @dlasky

... what is really confusing me about this is renderAllPending is designed to be run at basically any time, and it doesn't seem to be doing anything wrong, at least as far as the visual tree is concerned. I think I haven't fully loaded @jcmoore 's observations into my brain yet. Going to step through getEventPath and see if it clicks

[jcmoore]

I'm happy to help explain my understanding of the situation directly if that helps (I'm sitting in #polymer irc at the moment)

[dlasky]

if some background helps at all, our team discovered this in the first place because events were missing nested shadowRoots in their dispatch path, so we had @jcmoore dig into it and he discovered it was the premature reset causing the event path to be calculated incorrectly. you can see this in action by breakpointing at:

https://github.com/Polymer/ShadowDOM/blob/master/src/wrappers/events.js#L87

then loading one of the 'broken' nested shadow cases and dispatching a click event from the topmost element which will result in the event only being dispatched at the top and bottom most shadowRoots in the chain (if you have say 4 of them nested)

[jmesserly]

Think I found the problem and a simple fix, and it passes the existing tests. Now trying to create a unit test for it.

p.s. this is one of the most helpful bug reports ever. thanks so much for the comments :)

[jcmoore]

Awesome! Glad we were able to help, and curious to see what the final solution looks like.

[jmesserly]

googlearchive/ShadowDOM#502 ... skip the first bit of resetAll, since we don't want to discard the host's destination insertion points.

I think the bug happened because the spec http://w3c.github.io/webcomponents/spec/shadow/#distribution-algorithms describes how to run distribution over the whole world, and here we're trying to be a bit lazier about rendering.

I could be wrong and that reset is actually needed, so it might need a different fix (@arv is the real expert here) but it does pass all of the existing tests.