Summary: | Wrong display with "tr:nth-child(even) td" and missing <tbody> | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robin Gloster <robin> | ||||||||||||||
Component: | Tables | Assignee: | Takashi Sakamoto <tasak> | ||||||||||||||
Status: | RESOLVED DUPLICATE | ||||||||||||||||
Severity: | Normal | CC: | allan.jensen, cmarcelo, darin, jchaffraix, kling, koivisto, macpherson, menard, tasak, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 420+ | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
URL: | http://robin-gloster.de/webkit.html | ||||||||||||||||
Attachments: |
|
Description
Robin Gloster
2012-10-01 05:39:04 PDT
Created attachment 166850 [details]
Patch
Hello, This bug is caused by sibling style cache. If some positional rules are applied to the given element, the element cannot use sibling style cache. To know whether there exists any positional rule or not, sibling style cache looks at the parent style of the element, i.e. childrenAffectedByPositinalRules. The value is set in SelectorChecker::checkSelector. So, <style> table tr:nth-child(even) td { background: #eee; } </style> <table> <tr><th></th></tr> <tr><td></td></tr> </table> while resolving styles for the first <tr><th></th></tr>, since <th> doesn't match any rules, <tr>'s style will be created but its parent's childrenAffectedByPositionalRules will be false. In the next line, i.e. <tr><td></td></tr>, the <tr>'s style is the same as the first <tr>'s style, because the <tr>'s parent's childenAffectedByPositionalRules is false. Since<td> matches the rightmost selector of "table tr:nth-child(even) td", checkSelector checks whether there exists any ancestor which matches "tr:nth-child(even)" or not. In checkSelector, case CSSSelector::PseudoNthChild: ... if (m_mode == ResolvingStyle) { RenderStyle* childStyle = context.elementStyle ? context.elementStyle : element->renderStyle(); ... childStyle->setChildIndex(count); ... to avoid calculating child position again and again, the <tr>'s style is updated to have the <tr>'s position. However, the <tr>'s style is the same as the previous sibling <tr>. Best regards, Takashi Sakamoto Created attachment 167684 [details]
Patch
Comment on attachment 167684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167684&action=review > Source/WebCore/css/SelectorChecker.cpp:1391 > +inline int SelectorChecker::DOMTraversalStrategy<SelectorChecker::SelectorCheckingContext>::countElementsBefore(const SelectorChecker::SelectorCheckingContext& context, Element* element, bool resolvingState) I think a better name for this “resolving state” boolean would be shouldSetChildIndex. Also, there’s an extra stray space here after the comma. > Source/WebCore/css/SelectorChecker.cpp:1410 > + if (resolvingState) { > + const Element* previousSibling = element->previousElementSibling(); > + RenderStyle* childStyle = context.elementStyle ? context.elementStyle : element->renderStyle(); > + // Don't setChildIndex if childStyle is a shared style. > + if (childStyle && !(previousSibling && previousSibling->renderStyle() == childStyle)) > + childStyle->setChildIndex(count + 1); > + } A better way to express the “don’t set the child index if the style is shared” is to make a named local variable. I’d write the code like this: if (shouldSetChildIndex) { if (RenderStyle* childStyle = context.elementStyle ? context.elementStyle : element->renderStyle()) { const Element* previousSibling = element->previousElementSibling(); bool isSharedStyle = previousSibling && previousSibling->renderStyle() == childStyle; if (!isSharedStyle) childStyle->setIndex(count + 1); } } This way, the code comments itself so you don’t need that comment. Will doing these additional previousElementSibling linked list walks have a measurable performance impact? I think it’s really unfortunate here that the “count + 1” rule is hardcoded here as well as at the call site. I don’t understand why this code has to be moved inside the countElementsBefore function. The block of code here doesn’t seem to take advantage of the state at all. It seems like we just added the previousElementSibling logic, but this logic could have been added in checkOneSelector without moving the code. > Source/WebCore/css/SelectorChecker.h:91 > + static int countElementsBefore(const Context&, Element*, bool); This bool needs an argument name. The rule is that arguments with meanings that are obvious given the type can be omitted, but this bool does not qualify. Frankly the qualified name arguments for types in the surrounding functions also don’t, and need an argument name, but this is an even more extreme example. Created attachment 167896 [details]
Patch
Created attachment 167904 [details]
Patch
Comment on attachment 167684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167684&action=review Thank you for reviewing. >> Source/WebCore/css/SelectorChecker.cpp:1391 >> +inline int SelectorChecker::DOMTraversalStrategy<SelectorChecker::SelectorCheckingContext>::countElementsBefore(const SelectorChecker::SelectorCheckingContext& context, Element* element, bool resolvingState) > > I think a better name for this “resolving state” boolean would be shouldSetChildIndex. > > Also, there’s an extra stray space here after the comma. I reverted the code. >> Source/WebCore/css/SelectorChecker.cpp:1410 >> + } > > A better way to express the “don’t set the child index if the style is shared” is to make a named local variable. I’d write the code like this: > > if (shouldSetChildIndex) { > if (RenderStyle* childStyle = context.elementStyle ? context.elementStyle : element->renderStyle()) { > const Element* previousSibling = element->previousElementSibling(); > bool isSharedStyle = previousSibling && previousSibling->renderStyle() == childStyle; > if (!isSharedStyle) > childStyle->setIndex(count + 1); > } > } > > This way, the code comments itself so you don’t need that comment. > > Will doing these additional previousElementSibling linked list walks have a measurable performance impact? > > I think it’s really unfortunate here that the “count + 1” rule is hardcoded here as well as at the call site. > > I don’t understand why this code has to be moved inside the countElementsBefore function. The block of code here doesn’t seem to take advantage of the state at all. It seems like we just added the previousElementSibling logic, but this logic could have been added in checkOneSelector without moving the code. > I don’t understand why this code has to be moved inside the countElementsBefore function. I agree with you. I'm afraid that some new checkSelector has its own "previousElementSibling" and my patch will break styles. So I talked with a guy who are working on another checkSelector. He was planning to use only CollectingStyle in his new checkSelector. So I reverted the code, i.e. checkSelector does setChildIndex. >> Source/WebCore/css/SelectorChecker.h:91 >> + static int countElementsBefore(const Context&, Element*, bool); > > This bool needs an argument name. The rule is that arguments with meanings that are obvious given the type can be omitted, but this bool does not qualify. Frankly the qualified name arguments for types in the surrounding functions also don’t, and need an argument name, but this is an even more extreme example. I reverted the code. Created attachment 168620 [details]
Patch
CC'ing some good reviewers for a CSS change. Created attachment 172279 [details]
Patch
Why is the style shared at this point, and wouldn't it be better to trigger it to unshare? Btw, it would be great if we could more directly tell if a style is shared. With all the RefPtr everywhere looking at the refCount is probably not the best solution, but it could be flag on the style. If we had that, it would be great thing to assert on in several other places we assume non shared styles. Comment on attachment 172279 [details]
Patch
This feels like wrong approach. The style shouldn't get shared in the first place in cases where nth-child rules might apply.
(In reply to comment #11) > Why is the style shared at this point, and wouldn't it be better to trigger it to unshare? I agree, if possible, we should disable sharing the style in StyleResolver::locatedShareStyle. But from the viewpoint of current WebKit implementation, it is very hard. Because WebKit always tries to check css selectors from right to left. I will show an example: <style> table tr:nth-child(even) td { background: #eee; } </style> <table> <tr><th></th></tr> <tr><td></td></tr> </table> The css selectors "table tr:nth-child(even) td" does't match any element defined in the line: "<tr><th></th></tr>", because <tr> doesn't match "td" and <th> doesn't match "td". No checkSelector for "nth-child" is invoked. So there is no way to know whether <tr> has nth-child(even) or not. When we see the next line "<tr><td></td></tr>", we can reach checkSelector for "nth-child", because <td> matches "table tr:nth-child(even) td"'s rightmost selector. Now we find that <tr> has nth-child(even) and the style should not be shared. But it is too late. (In reply to comment #13) > When we see the next line "<tr><td></td></tr>", we can reach checkSelector for "nth-child", because <td> matches "table tr:nth-child(even) td"'s rightmost selector. Now we find that <tr> has nth-child(even) and the style should not be shared. But it is too late. It shouldn't be too late. What I don't understand here is that sharing also skips the selector matching. In this case we are in the process of selector matching, so why is the style already shared? During the slow path the base style should be a completely new one, and if the fundamentals of the this system has changed, then you would need to have a copy-on-write system, and it would need to be triggered at this point. (In reply to comment #14) > (In reply to comment #13) > > When we see the next line "<tr><td></td></tr>", we can reach checkSelector for "nth-child", because <td> matches "table tr:nth-child(even) td"'s rightmost selector. Now we find that <tr> has nth-child(even) and the style should not be shared. But it is too late. > > It shouldn't be too late. What I don't understand here is that sharing also skips the selector matching. In this case we are in the process of selector matching, so why is the style already shared? During the slow path the base style should be a completely new one, and if the fundamentals of the this system has changed, then you would need to have a copy-on-write system, and it would need to be triggered at this point. No wait, strike that. It is not the style being created that is shared, it is the parent style. I guess that leaves three options: 1) Detect nth selectors earlier and disable sharing, we do this for some other tricky selectors. 2) A system to change an already shared style to become unshared. We have copy-on-write for subparts of the renderstyle, perhaps renderstyle itself could work similarly. 3) We could move child-index out of style and into Elements, probably in ElementRareData. We could move other dynamic settings with it, most of the bitfield flags that is set on parent elements would be good to move. This would also save us copying them to new styles in recalcStyle. (In reply to comment #15) > 3) We could move child-index out of style and into Elements, probably in ElementRareData. We could move other dynamic settings with it, most of the bitfield flags that is set on parent elements would be good to move. This would also save us copying them to new styles in recalcStyle. I said that because I have previosly been working on a patch that moved some of these flags out, though not childIndex. I have updated my old patch and added childIndex to the flags moved, and uploaded the patch to bug #101448 (In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > When we see the next line "<tr><td></td></tr>", we can reach checkSelector for "nth-child", because <td> matches "table tr:nth-child(even) td"'s rightmost selector. Now we find that <tr> has nth-child(even) and the style should not be shared. But it is too late. > > > > It shouldn't be too late. What I don't understand here is that sharing also skips the selector matching. In this case we are in the process of selector matching, so why is the style already shared? During the slow path the base style should be a completely new one, and if the fundamentals of the this system has changed, then you would need to have a copy-on-write system, and it would need to be triggered at this point. > > No wait, strike that. It is not the style being created that is shared, it is the parent style. Yes, it is the parent style. I think, updating the parent style in checkSelector causes this issue. > 3) We could move child-index out of style and into Elements, probably in ElementRareData. We could move other dynamic settings with it, most of the bitfield flags that is set on parent elements would be good to move. This would also save us copying them to new styles in recalcStyle. I see. This can make us avoid updating the parent style in checkSelector. It looks better solution than my way. I will cancel my patch's review. Comment on attachment 172279 [details] Patch I will cancel this review, because the patch for bug #101448 will fix this issue and the solution looks better than this. *** This bug has been marked as a duplicate of bug 10448 *** *** This bug has been marked as a duplicate of bug 101448 *** |