RESOLVED FIXED 100451
[Shadow DOM][Meta] Changing attribute does not cause distribution. 
https://bugs.webkit.org/show_bug.cgi?id=100451
Summary [Shadow DOM][Meta] Changing attribute does not cause distribution.
Sergey G. Grekhov
Reported 2012-10-25 21:01:22 PDT
Found in Chrome 22.0.1229.94 m The following test (in fact rewritten Bob-developer example from Shadow DOM spec) fails: var iframe = document.createElement('iframe'); document.body.appendChild(iframe); iframe.contentDocument.body.innerHTML = "<ul class='stories'>" + "<li id='li1'><a href='#1'>Link1</a></li>" + "<li id='li2'><a href='#2'>Link 2</a></li>" + "<li id='li3' class='shadow'><a href='#3'>Link 3 Shadow</a></li>" + "<li id='li4'><a href='#4'>Link 4</a></li>" + "<li id='li5'><a href='#5'>Link 5</a></li>" + "<li id='li6' class='shadow'><a href='#5'>Link 6 Shadow</a></li>" + "</ul>"; var d = iframe.contentDocument; var ul = d.querySelector("ul.stories"); var SR = window.ShadowRoot || window.WebKitShadowRoot; var s = new SR(ul); //make shadow subtree var subdiv1 = document.createElement('div'); subdiv1.innerHTML = "<ul><content select='.shadow'></content></ul>"; s.appendChild(subdiv1); assert_true(d.querySelector('#li3').offsetTop < d.querySelector('#li6').offsetTop, 'Point 1: Elements that match insertion point criteria don\'t participate in distribution'); assert_true(d.querySelector('#li3').offsetTop > 0, 'Point 2: Elements that match insertion point criteria don\'t participate in distribution'); assert_equals(d.querySelector('#li1').offsetTop, 0, 'Point 3: Elements that don\'t match insertion point criteria participate in distribution'); assert_equals(d.querySelector('#li2').offsetTop, 0, 'Point 4: Elements that don\'t match insertion point criteria participate in distribution'); assert_equals(d.querySelector('#li4').offsetTop, 0, 'Point 5: Elements that don\'t match insertion point criteria participate in distribution'); assert_equals(d.querySelector('#li5').offsetTop, 0, 'Point 6: Elements that don\'t match insertion point criteria participate in distribution'); var li5 = d.querySelector('#li5'); li5.className = 'shadow'; assert_true(li5.offsetTop > 0, 'Distribution should reoccur if a variable affecting it is changed'); Class name of li5 was changed to the one that matches insertion point criteria, so the distribution should reoccur but it doesn't.
Attachments
Add attachment proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1 2012-10-29 18:49:58 PDT
I'll see it today. If it's found in chrome 22, we might have fixed it already though...
Shinya Kawanaka
Comment 2 2012-10-29 20:55:28 PDT
OK. I've confirmed this test is still failing in the current trunk.
Shinya Kawanaka
Comment 3 2012-10-30 02:26:02 PDT
Hmm... it seems changing attribute does not reflect immediately at all. Let's use this as a meta bug and fix them one by one.
Hajime Morrita
Comment 4 2012-10-30 03:42:11 PDT
(In reply to comment #3) > Hmm... it seems changing attribute does not reflect immediately at all. > Let's use this as a meta bug and fix them one by one. Right. we need to address this. On the other hand, it is bad idea to re-run distribution algorithm for each attribute change from a performance perspective. Can we do something more efficient like examining @select attribute values before invalidating the distribution?
Dimitri Glazkov (Google)
Comment 5 2012-10-30 10:54:20 PDT
(In reply to comment #4) > (In reply to comment #3) > > Hmm... it seems changing attribute does not reflect immediately at all. > > Let's use this as a meta bug and fix them one by one. > > Right. we need to address this. > On the other hand, it is bad idea to re-run distribution algorithm for each attribute change > from a performance perspective. It may sound pretty bad, but it's something CSS does anyway, right? > Can we do something more efficient like examining @select attribute values before > invalidating the distribution? This sounds like an interesting optimization: 1) determine effects of the select as some subset of DOM changes 2) act only on DOM changes that are in this subset. Also sounds: 1) incredibly complex 2) something that could be shared with general CSS rule matching
Shinya Kawanaka
Comment 6 2012-10-30 18:16:16 PDT
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Hmm... it seems changing attribute does not reflect immediately at all. > > > Let's use this as a meta bug and fix them one by one. > > > > Right. we need to address this. > > On the other hand, it is bad idea to re-run distribution algorithm for each attribute change > > from a performance perspective. > > It may sound pretty bad, but it's something CSS does anyway, right? > > > Can we do something more efficient like examining @select attribute values before > > invalidating the distribution? > > This sounds like an interesting optimization: > 1) determine effects of the select as some subset of DOM changes > 2) act only on DOM changes that are in this subset. > > Also sounds: > 1) incredibly complex > 2) something that could be shared with general CSS rule matching Seeing Element::attributeChanged, Element checks CSS Selector to decide whether we should recalculate style. So maybe we can do this? It's complex though...
Shinya Kawanaka
Comment 7 2012-10-30 21:14:31 PDT
I'm now thinking the following plan: 1. ShadowRoot should know the set of descendant InsertionPoint - Maybe we can hook insertedInto and removedFrom 2. ShadowRoot should also know the descendant elements having ElementShadow - We also track addShadowRoot() 3. ShadowRoot will have FeatureRuleSet or something like that - We update it when InsertionPoint / ElementShadow is added/removed 4. When attribute of the direct child (or non-direct child in fallback element case) is changed, we consult with the FeatureRuleSet whether the change is related or not. - If it's related, we invalidate the distribution. - If not, we recursively consult with ElementShadow.
Hajime Morrita
Comment 8 2012-10-30 23:12:38 PDT
(In reply to comment #7) > I'm now thinking the following plan: > > 1. ShadowRoot should know the set of descendant InsertionPoint > - Maybe we can hook insertedInto and removedFrom > 2. ShadowRoot should also know the descendant elements having ElementShadow > - We also track addShadowRoot() > 3. ShadowRoot will have FeatureRuleSet or something like that > - We update it when InsertionPoint / ElementShadow is added/removed > 4. When attribute of the direct child (or non-direct child in fallback element case) is changed, we consult with the FeatureRuleSet whether the change is related or not. > - If it's related, we invalidate the distribution. > - If not, we recursively consult with ElementShadow. Sounds like a plan. I hope we only need to track only the number of insertion points/shadows instead of the nodes themselves. Probably we can land the slow version fast, then introducing these optimisation.
Shinya Kawanaka
Comment 9 2012-10-30 23:47:04 PDT
If we track InsertionPoint, distribution will be fast, because we can only iterate such elements instead of all descendant nodes. (This is not directly related to this bug, though.) So we might want consider it in another bug.
Shinya Kawanaka
Comment 10 2012-10-30 23:49:49 PDT
I've created: https://bugs.webkit.org/show_bug.cgi?id=100820
Shinya Kawanaka
Comment 11 2012-11-11 23:17:58 PST
For pseudoClasses, please see this. http://www.whatwg.org/specs/web-apps/current-work/multipage/selectors.html#pseudo-classes
Shinya Kawanaka
Comment 12 2012-11-19 18:14:09 PST
All subbugs are closed. Let's close this.
Note You need to log in before you can comment on or make changes to this bug.