Bug 68344 – tr:nth-child(even) is buggy for <tr>s that aren’t wrapped in <thead> and that contain only <th>s

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: CSS

[Mathias Bynens]

Created attachment 107821 [details]
Test case

Observed in nightly WebKit r95400.

[Alexey Proskuryakov]

I'm seeing the problem on the attached test case in Safari/WebKit 5.1, but not in r95422. Could you please double check that this is an issue in nightlies?

[Mathias Bynens]

(In reply to comment #1)
> I'm seeing the problem on the attached test case in Safari/WebKit 5.1, but not in r95422. Could you please double check that this is an issue in nightlies?

I’m still seeing the issue in r95481. At first page load after launching WebKit it seems to work fine indeed; but as soon as you refresh (⌘+R) the page, the issue occurs. Very weird.

[Shinya Kawanaka]

When resolving CSS selector matching (in SelectorChecker::checkOneSelector), the first <tr> and the second <tr> shares RenderStyle.

When checking the second <tr>, CSS selector sets childIndex (of RenderStyle). However, the RenderStyle is shared, it overwrites the RenderStyle of the first <tr>.

It causes this bug, I think.

[Shane Stephens]

Still happening in WebKit nightly @ r97109 on OS X

[Shinya Kawanaka]

Created attachment 114410 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #5)
> Created an attachment (id=114410) [details]
> Patch

Could somebody check this patch?

[David Barr]

This patch needs to be rebased. There is a conflict in SelectorChecker::checkOneSelector() when applied to ToT.

[Shinya Kawanaka]

Created attachment 135265 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #7)
> This patch needs to be rebased. There is a conflict in SelectorChecker::checkOneSelector() when applied to ToT.

rebased.

[Shinya Kawanaka]

Hey, no one can review this patch...?

[Kristóf Kosztyó]

Comment on attachment 135265 [details]
Patch

Attachment 135265 [details] did not pass qt-ews (qt):
Output: http://qt-ews.appspot.com/results/50005

[Kristóf Kosztyó]

(In reply to comment #11)
> (From update of attachment 135265 [details])
> Attachment 135265 [details] did not pass qt-ews (qt):
> Output: http://qt-ews.appspot.com/results/50005

Sorry, we just try some experiments with the ews, and this made some spam :(

[Eric Seidel (no email)]

Comment on attachment 135265 [details]
Patch

The test does not fail for me in Chrome Dev channel.  Suggesting either the fix or the test is wrong.

Could you please post a new test which fails on TOT?

[Shinya Kawanaka]

(In reply to comment #13)
> (From update of attachment 135265 [details])
> The test does not fail for me in Chrome Dev channel.  Suggesting either the fix or the test is wrong.
> 
> Could you please post a new test which fails on TOT?

Really? As far as I tried today (on ToT), the test fails!
If you tried the test in a browser, the test might not fail.
Please try the test on DumpRenderTree.

[David Barr]

Confirmed that the test fails in:

Safari Version 5.1.5 (6534.55.3, r117738) aka WebKit Nightly

Google Chrome: 21.0.1145.0 (Official Build 138079) canary
OS: Mac OS X
WebKit: 537.1 (@117602)

DumpRenderTree (Chrome Mac r138143, WebKit r117808)

Created attachment 150713 [details]
Simple test case

Simple test case, the test fails if PASS is not displayed.

This was reported in Chromium: http://code.google.com/p/chromium/issues/detail?id=111814

[Shinya Kawanaka]

Created attachment 150717 [details]
Patch

[Shinya Kawanaka]

rebased on ToT.

Comment on attachment 150717 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150717&action=review

> Source/WebCore/css/SelectorChecker.cpp:912
> +                    if (s && s != element->renderStyle()) {

Can't we still run into problems if a render style is shared with some other element?

Given three sibling nodes in sequence, A, B and C where A and B share a render style. If we enter this code for A, we will update the cached child index and it will appear that B has the same index as A. If we then enter this code to calculate the index of C, it will be miscalculated.

[Morten Stenshorne]

Works for me. Both test cases pass.

[Mathias Bynens]

(In reply to comment #20)
> Works for me. Both test cases pass.

Same here. This seems to have been fixed already.

[Anders Carlsson]

Comment on attachment 150717 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.