Bug 32856 - querySelectorAll with "nth-child(3n+1),nth-child(3n)"
Summary: querySelectorAll with "nth-child(3n+1),nth-child(3n)"
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2009-12-22 00:47 PST by caii
Modified: 2011-12-03 11:03 PST (History)
7 users (show)

See Also:


Attachments
a(document.querySelectorAll("ul li:nth-child(3n+1),ul li:nth-child(3n)")) // FF:8 ;webkit:0; (815 bytes, text/html)
2009-12-22 00:49 PST, caii
no flags Details
Proposed Patch (5.52 KB, patch)
2010-05-18 01:34 PDT, Yoshiki Hayashi
zimmermann: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description caii 2009-12-22 00:47:41 PST
See the attachment;
querySelectorAll find wrong elements。
Comment 1 caii 2009-12-22 00:49:28 PST
Created attachment 45371 [details]
a(document.querySelectorAll("ul li:nth-child(3n+1),ul li:nth-child(3n)")) // FF:8   ;webkit:0;

test.html
Comment 2 Julien Chaffraix 2010-03-31 09:34:46 PDT
Confirmed on ToT.

I tracked down this issue to CSSStyleSelector::SelectorChecker::checkOneSelector.

In the CSSSelector::PseudoNthChild case (line 2240), we cache the child count in the RenderStyle associated with the current Element. When we apply the 2nd selector (ul li:nth-child(3n)), we actually increase the count a second time. This is enough to make us match every elements.
Comment 3 Yoshiki Hayashi 2010-05-18 01:31:23 PDT
The bug is as what Julien said, the counter gets wrong value.

In the test case, all "li" nodes share the same RenderStyle object because they have the same styles and nothing else to distinguish them.  So in case CSSSelector::PseudoNthChild: code in CSSStyleSelector::SelectorChecker::checkOneSelector in css/CSSStyleSelector.cpp, both RenderStyle*s and RenderStyle* childStyle pointers have exact same value for all "li"s.  So the result is, every time this code is called, the counter is increased by 1 from whatever the value it was before regardless of the position of the "li" node.

The reason this doesn't happen with the <style> like below is because in that code path, in styleForElement, if the li could share the RenderStyle is checked and rejected because of nth-child selector and new RenderStyle is created and set to m_style.  In querySelectorAll path, styleForElement is not called but instead WebCore::CSSStyleSelector::SelectorChecker::checkSelector is called directly from  WebCore::createSelectorNodeList.  In querySelectorAll, elementStyle argument to checkOneSelector is NULL while in the <style> case, m_style (non-NULL) is passed.

<style>
ul li:nth-child(3n+1),ul li:nth-child(3n+2) {
  background: red;
}

I can see a few different way to fix this.

1. Don't cache childIndex when the RenderStyle is shared between different nodes
2. Cache childIndex in Element instead of RenderStyle
3. Create new different RenderStyle when it is shared before caching

For the patch I'm going to attach, I chose option 1 because it was the least intrusive change.  I couldn't find a way to easily check whether RenderStyle object is shared from multiple nodes so instead I changed the code to stop caching when elementStyle is not given, i.e. from selector API.  This makes nth-child in querySelectorAll a little bit more costly but it's no worse than other nth- type queries so I hope it's OK.

I'm open to implement different approaches if that seems more desirable.
Comment 4 Yoshiki Hayashi 2010-05-18 01:34:29 PDT
Created attachment 56335 [details]
Proposed Patch
Comment 5 Nikolas Zimmermann 2010-07-30 23:07:31 PDT
Comment on attachment 56335 [details]
Proposed Patch

LayoutTests/fast/dom/SelectorAPI/nth-child.html:16
 +    debug(i);
This should be removed, the expected.txt looks confusing.

LayoutTests/fast/dom/SelectorAPI/nth-child-expected.txt:27
 +  blue
You might want to load the js-test-post.js file delayed (there are numerous examples in existing tests how to do this), as the test output looks confusing:
blue
blue
white
..

Please fix these issues and resubmit.