Bug 68344 - tr:nth-child(even) is buggy for <tr>s that aren’t wrapped in <thead> and that contain only <th>s
Summary: tr:nth-child(even) is buggy for <tr>s that aren’t wrapped in <thead> and that...
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-19 02:32 PDT by Mathias Bynens
Modified: 2014-02-05 10:57 PST (History)
17 users (show)

See Also:


Attachments
Test case (719 bytes, text/html)
2011-09-19 02:32 PDT, Mathias Bynens
no flags Details
Patch (6.14 KB, patch)
2011-11-09 17:50 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (6.19 KB, patch)
2012-04-02 20:14 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Simple test case (285 bytes, text/html)
2012-07-03 21:58 PDT, dstockwell
no flags Details
Patch (6.26 KB, patch)
2012-07-03 23:18 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mathias Bynens 2011-09-19 02:32:07 PDT
Created attachment 107821 [details]
Test case

Observed in nightly WebKit r95400.
Comment 1 Alexey Proskuryakov 2011-09-19 10:09:05 PDT
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?
Comment 2 Mathias Bynens 2011-09-19 21:10:30 PDT
(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.
Comment 3 Shinya Kawanaka 2011-10-02 22:45:07 PDT
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.
Comment 4 Shane Stephens 2011-10-10 20:41:00 PDT
Still happening in WebKit nightly @ r97109 on OS X
Comment 5 Shinya Kawanaka 2011-11-09 17:50:13 PST
Created attachment 114410 [details]
Patch
Comment 6 Shinya Kawanaka 2011-11-10 18:31:56 PST
(In reply to comment #5)
> Created an attachment (id=114410) [details]
> Patch

Could somebody check this patch?
Comment 7 David Barr 2012-04-01 23:44:06 PDT
This patch needs to be rebased. There is a conflict in SelectorChecker::checkOneSelector() when applied to ToT.
Comment 8 Shinya Kawanaka 2012-04-02 20:14:35 PDT
Created attachment 135265 [details]
Patch
Comment 9 Shinya Kawanaka 2012-04-02 20:15:14 PDT
(In reply to comment #7)
> This patch needs to be rebased. There is a conflict in SelectorChecker::checkOneSelector() when applied to ToT.

rebased.
Comment 10 Shinya Kawanaka 2012-04-06 05:24:42 PDT
Hey, no one can review this patch...?
Comment 11 Kristóf Kosztyó 2012-04-10 00:40:00 PDT
Comment on attachment 135265 [details]
Patch

Attachment 135265 [details] did not pass qt-ews (qt):
Output: http://qt-ews.appspot.com/results/50005
Comment 12 Kristóf Kosztyó 2012-04-10 04:34:34 PDT
(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 :(
Comment 13 Eric Seidel (no email) 2012-05-21 14:33:54 PDT
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?
Comment 14 Shinya Kawanaka 2012-05-21 19:07:56 PDT
(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.
Comment 15 David Barr 2012-05-21 20:10:50 PDT
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)
Comment 16 dstockwell 2012-07-03 21:58:54 PDT
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
Comment 17 Shinya Kawanaka 2012-07-03 23:18:49 PDT
Created attachment 150717 [details]
Patch
Comment 18 Shinya Kawanaka 2012-07-03 23:19:07 PDT
rebased on ToT.
Comment 19 dstockwell 2012-07-03 23:41:25 PDT
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.
Comment 20 Morten Stenshorne 2014-01-23 05:18:08 PST
Works for me. Both test cases pass.
Comment 21 Mathias Bynens 2014-01-23 05:24:02 PST
(In reply to comment #20)
> Works for me. Both test cases pass.

Same here. This seems to have been fixed already.
Comment 22 Anders Carlsson 2014-02-05 10:57:30 PST
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.