[Webkit-unassigned] [Bug 32856] querySelectorAll with "nth-child(3n+1), nth-child(3n)"

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32856


Yoshiki Hayashi <yhayashi at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yhayashi at google.com




--- Comment #3 from Yoshiki Hayashi <yhayashi at google.com>  2010-05-18 01:31:23 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.