Bug 137032 - Chaining multiple :nth-child() does not work properly
Summary: Chaining multiple :nth-child() does not work properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Major
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-23 11:12 PDT by Mikael Jergefelt
Modified: 2014-09-27 11:49 PDT (History)
4 users (show)

See Also:


Attachments
Patch (228.52 KB, patch)
2014-09-25 17:18 PDT, Benjamin Poulain
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikael Jergefelt 2014-09-23 11:12:43 PDT
nth-child ranges does not work properly in Safari on iOS8 and Mac OSX Yosemite (DP8).

Examples can be viewed at:
http://nthmaster.com

nth-child(n+4):nth-child(-n+8) and most other ranges doesn't work as they should, and select way more elements.
Comment 1 Benjamin Poulain 2014-09-24 22:10:30 PDT
This is a really really dumb bug, it is sad nobody reported it earlier. The register that keeps the element's position is reused, making its value useless when evaluating a following :nth-child().

That only happen in certain cases. It looks like we were very unlucky with our tests, only hitting the case that do not destroy the counter.
Comment 2 Benjamin Poulain 2014-09-25 17:18:49 PDT
Created attachment 238685 [details]
Patch
Comment 3 Chris Dumez 2014-09-25 17:45:28 PDT
Comment on attachment 238685 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        1) When A and B are possitive, the counter would be the destination of "counter - B".

nit: "positive"
Comment 4 Gavin Barraclough 2014-09-26 16:12:45 PDT
Comment on attachment 238685 [details]
Patch

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

> Source/WebCore/cssjit/SelectorCompiler.cpp:1768
> +            if (m_registerAllocator.availableRegisterCount() > 1) {

Is this (plus twice below) "> 1" because we need to allocate more registers below (e.g. "LocalRegister divisorRegister(m_registerAllocator);")
If so, perhaps it might simplify to just hoist the further register allocation above this point?
Comment 5 Benjamin Poulain 2014-09-27 11:45:04 PDT
(In reply to comment #4)
> (From update of attachment 238685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238685&action=review
> 
> > Source/WebCore/cssjit/SelectorCompiler.cpp:1768
> > +            if (m_registerAllocator.availableRegisterCount() > 1) {
> 
> Is this (plus twice below) "> 1" because we need to allocate more registers below (e.g. "LocalRegister divisorRegister(m_registerAllocator);")
> If so, perhaps it might simplify to just hoist the further register allocation above this point?

It is indeed the reason, but moving the allocation of divisorRegister above creates more problems. Since idiv only works on RAX and RDX, I need to get those two in priority. If I were to allocate divisorRegister first, it may get RAX and RDX.
Comment 6 Benjamin Poulain 2014-09-27 11:49:26 PDT
Committed r174035: <http://trac.webkit.org/changeset/174035>