Bug 32856 – querySelectorAll with "nth-child(3n+1),nth-child(3n)"

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: CSS

[caii]

See the attachment;
querySelectorAll find wrong elements。

[caii]

Created attachment 45371 [details]
a(document.querySelectorAll("ul li:nth-child(3n+1),ul li:nth-child(3n)")) // FF:8   ;webkit：0；

test.html

[Julien Chaffraix]

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.

[Yoshiki Hayashi]

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.

[Yoshiki Hayashi]

Created attachment 56335 [details]
Proposed Patch

[Nikolas Zimmermann]

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.

[Brent Fulgham]

Safari, Chrome, and Firefox show the same rendering behavior for this test case. I do not believe any further compatibility issue remains.