Bug 98021 - Wrong display with "tr:nth-child(even) td" and missing <tbody>
Summary: Wrong display with "tr:nth-child(even) td" and missing <tbody>
Status: RESOLVED DUPLICATE of bug 101448
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 420+
Hardware: PC Linux
: P2 Normal
Assignee: Takashi Sakamoto
URL: http://robin-gloster.de/webkit.html
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-01 05:39 PDT by Robin Gloster
Modified: 2012-11-27 12:02 PST (History)
10 users (show)

See Also:


Attachments
Patch (5.56 KB, patch)
2012-10-03 03:52 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (7.53 KB, patch)
2012-10-08 21:36 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (7.77 KB, patch)
2012-10-09 19:33 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (5.61 KB, patch)
2012-10-09 20:30 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (5.49 KB, patch)
2012-10-14 21:23 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (5.46 KB, patch)
2012-11-05 00:13 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Gloster 2012-10-01 05:39:04 PDT
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
Comment 1 Takashi Sakamoto 2012-10-03 03:52:23 PDT
Created attachment 166850 [details]
Patch
Comment 2 Takashi Sakamoto 2012-10-03 04:10:40 PDT
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
Comment 3 Takashi Sakamoto 2012-10-08 21:36:19 PDT
Created attachment 167684 [details]
Patch
Comment 4 Darin Adler 2012-10-09 09:51:14 PDT
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.
Comment 5 Takashi Sakamoto 2012-10-09 19:33:13 PDT
Created attachment 167896 [details]
Patch
Comment 6 Takashi Sakamoto 2012-10-09 20:30:44 PDT
Created attachment 167904 [details]
Patch
Comment 7 Takashi Sakamoto 2012-10-09 20:39:32 PDT
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.
Comment 8 Takashi Sakamoto 2012-10-14 21:23:15 PDT
Created attachment 168620 [details]
Patch
Comment 9 Julien Chaffraix 2012-10-31 09:46:00 PDT
CC'ing some good reviewers for a CSS change.
Comment 10 Takashi Sakamoto 2012-11-05 00:13:56 PST
Created attachment 172279 [details]
Patch
Comment 11 Allan Sandfeld Jensen 2012-11-06 06:15:30 PST
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 12 Antti Koivisto 2012-11-06 10:48:29 PST
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.
Comment 13 Takashi Sakamoto 2012-11-07 00:47:40 PST
(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.
Comment 14 Allan Sandfeld Jensen 2012-11-07 01:35:42 PST
(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.
Comment 15 Allan Sandfeld Jensen 2012-11-07 01:40:52 PST
(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.
Comment 16 Allan Sandfeld Jensen 2012-11-07 05:00:04 PST
(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
Comment 17 Takashi Sakamoto 2012-11-07 22:36:35 PST
(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 18 Takashi Sakamoto 2012-11-07 22:38:35 PST
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.
Comment 19 Allan Sandfeld Jensen 2012-11-27 12:02:13 PST

*** This bug has been marked as a duplicate of bug 10448 ***
Comment 20 Allan Sandfeld Jensen 2012-11-27 12:02:26 PST

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