[ShadowDOM]: Filtered insertion points inside nested roots fail to distribute based on dom changes

[sorvell]

Here's a repro: http://jsbin.com/naboya/1/edit.

Clicking the Test button toggles an attribute on the #test node and should cause it to toggle between the insertion points contained in its parent's shadowRoot. This fails.

Clicking the Test2 button removes/adds the #test node. This appears to function correctly.

There's a clue in the fact that an incorrect distribution caused by clicking Test can be corrected by clicking Test2 2x to remove and re-add the node.

After some debugging, it appears that the renderer found for the x-host element does not properly report that it should invalidate based on the foo and bar attributes changing on children. Strangely, the x-outer element renderer does report that it should invalidate on foo and bar which is also incorrect .

[jmesserly]

fascinating. taking a look at it ... seems very mysterious

[jmesserly]

well, that was easy with your notes to start from! I think this is the fix:

diff --git a/src/ShadowDOM/ShadowRenderer.js b/src/ShadowDOM/ShadowRenderer.js
index cd3d22e..ace0b82 100644
--- a/src/ShadowDOM/ShadowRenderer.js
+++ b/src/ShadowDOM/ShadowRenderer.js
@@ -340,9 +340,10 @@
         var shadowTrees = getShadowTrees(shadowHost);

         // 1.2
+        var renderer = getRendererForHost(shadowHost);
         for (var i = 0; i < shadowTrees.length; i++) {
           // 1.2.1
-          this.poolDistribution(shadowTrees[i], pool);
+          renderer.poolDistribution(shadowTrees[i], pool);
         }

         // 1.3

tellingly, the one other place we check isShadowHost and mutate a field on renderer:

      if (isShadowHost(node)) {
        var renderer = getRendererForHost(node);
        renderer.dirty = false;
      }

In contrast, attributes were breaking the invariant that the current ShadowRenderer matches the host.

working on a test case, but that's probably the fix.

[jmesserly]

oops, and I should've noticed this was assigned. Sorry @garlicnation !

if you want to take this up and land it, feel free. Otherwise I was going to adapt @sorvell 's test case to be standalone (just setting up the shadow roots and such) so we can add to tests/ShadowDOM/...

[jmesserly]

But the fix above does seem to fix the problem without breaking any of the existing SD tests (tried chrome/safari/firefox).

[sorvell]

Pretty sure @garlicnation hasn't had a chance to look. Re-assigned so @jmesserly can get full credit!

[jmesserly]

update: either something's wrong with my test or I seem to have found another problem (not sure if related). Digging ...

[jmesserly]

It seems unrelated, even rolling back my fix shows some really busted behavior in the test. After setting things up like your jsbin, ShadowDOM's innerHTML reports (newlines mine):

<x-host id="host">
<child foo="">~Foo~</child>
<child foo="" bar="">~Foo+Bar~</child>
<child id="test" bar="">~Bar~</child>
</x-host>
<br>

Looks good right? Now after rendering (with or without a fix for the attribute issue):

<x-host id="host">
<child foo="">~Foo~</child>
<child foo="" bar="">~Foo+Bar~</child>
] Bar: [
<content select="[bar]"></content>
]
</x-host>
<br>

that's like totally busted. Strangely, the visual HTML tree is correct, but SD's internal tracking seems sufficiently off it can't re-render properly. It looks like the (virtual) nextSibling pointer from the Foo+Bar got set incorrectly (well, didn't get set, therefore falls back to the visual HTML nextSibling which is wrong). Going to need to dig more. I didn't see any of this in my manual HTML testing. Then again, the test environment uses detached DOM trees and forces rendering, so it can sometimes be a bit different.

[jmesserly]

Wow, figured it out. On a hunch, I added whitespace nodes where they would usually be inside the <template> (after each <child> tag) and the second bug goes away. So, we might have some obscure breakage with ShadowDOM polyfill if the HTML is missing whitespace between distributed elements. Anyway, other than tripping on that unrelated issue, the fix and test works. I'll file another bug for that issue, and send out a pull request that fixes this one

[sorvell]

u rox... thx!

[sorvell]

Reverted via f94ca5e.

This change broke some Polymer tests (https://github.com/Polymer/polymer/blob/master/test/unit/polymer-dom.js#L101 which can be run via https://github.com/Polymer/polymer/blob/master/test/unit/polymer-dom-shadow.html). It's not clear why this change broke these tests, but they also make changes to attributes to affect distribution.

@jmesserly would you mind taking a look?

[TimvdLippe]

I don't think this is an issue any longer.