Bug 98021 – Wrong display with "tr:nth-child(even) td" and missing <tbody>

• Component: Tables
• Status: RESOLVED DUPLICATE

[Robin Gloster]

I have a table and want to color the background of alternating rows using:

table tr:nth-child(even) td {
  background: #eee;
}

This works if tbody is present, otherwise the first colored row gets mixed up.

See http://robin-gloster.de/webkit.html for page without tbody
See http://robin-gloster.de/webkit2.html for working page with tbody
See http://robin-gloster.de/webkit.css for the css

[Takashi Sakamoto]

Created attachment 166850 [details]
Patch

[Takashi Sakamoto]

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

[Takashi Sakamoto]

Created attachment 167684 [details]
Patch

[Darin Adler]

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.

[Takashi Sakamoto]

Created attachment 167896 [details]
Patch

[Takashi Sakamoto]

Created attachment 167904 [details]
Patch

[Takashi Sakamoto]

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.

[Takashi Sakamoto]

Created attachment 168620 [details]
Patch

[Julien Chaffraix]

CC'ing some good reviewers for a CSS change.

[Takashi Sakamoto]

Created attachment 172279 [details]
Patch

[Allan Sandfeld Jensen]

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.

[Antti Koivisto]

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.

[Takashi Sakamoto]

(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.

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

(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

[Takashi Sakamoto]

(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.

[Takashi Sakamoto]

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.

[Allan Sandfeld Jensen]

*** This bug has been marked as a duplicate of bug 10448 ***

[Allan Sandfeld Jensen]

*** This bug has been marked as a duplicate of bug 101448 ***