Bug 100451
| Summary: | [Shadow DOM][Meta] Changing attribute does not cause distribution. | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Sergey G. Grekhov <sgrekhov> |
| Component: | DOM | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | dglazkov, morrita, shinyak, webcomponents-bugzilla |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Windows 7 | ||
| Bug Depends on: | 100738, 100921, 100922, 101180, 101697, 101698, 101699, 101700, 101891, 101900, 101901, 101902, 101903, 102160 | ||
| Bug Blocks: | 63606 | ||
Sergey G. Grekhov
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
I'll see it today. If it's found in chrome 22, we might have fixed it already though...
Shinya Kawanaka
OK. I've confirmed this test is still failing in the current trunk.
Shinya Kawanaka
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
(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)
(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
(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
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
(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
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
I've created: https://bugs.webkit.org/show_bug.cgi?id=100820
Shinya Kawanaka
For pseudoClasses, please see this.
http://www.whatwg.org/specs/web-apps/current-work/multipage/selectors.html#pseudo-classes
Shinya Kawanaka
All subbugs are closed. Let's close this.