Bug 26362 - :nth-child() is not re-evaluated when adding or removing children
Summary: :nth-child() is not re-evaluated when adding or removing children
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Dave Hyatt
URL: http://www.quirksmode.org/css/nthchil...
Keywords:
Depends on:
Blocks: 9610
  Show dependency treegraph
 
Reported: 2009-06-12 17:44 PDT by Simon Fraser (smfr)
Modified: 2009-06-16 16:18 PDT (History)
1 user (show)

See Also:


Attachments
Patch, testcase, changelog (12.60 KB, patch)
2009-06-14 10:14 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff
Patch (4.43 KB, patch)
2009-06-14 15:35 PDT, Dave Hyatt
hyatt: review-
Details | Formatted Diff | Diff
Better patch (3.39 KB, patch)
2009-06-14 16:42 PDT, Dave Hyatt
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2009-06-12 17:44:53 PDT
The testcase at http://www.quirksmode.org/css/nthchild.html shows that WebKit fails to re-evaluate nth-child when siblings are added or removed.
Comment 1 Simon Fraser (smfr) 2009-06-14 10:14:42 PDT
Created attachment 31265 [details]
Patch, testcase, changelog
Comment 2 Darin Adler 2009-06-14 11:43:23 PDT
Comment on attachment 31265 [details]
Patch, testcase, changelog

> -Node::StyleChange Node::diff( RenderStyle *s1, RenderStyle *s2 )
> +Node::StyleChange Node::diff(RenderStyle *s1, RenderStyle *s2)

Since you're touching this line to tweak style, please move the "*" characters next to the type name too.

> +    bool pseudoClassStateEquivalent(const RenderStyle* style)
> +    {
> +        return m_affectedByEmpty == style->affectedByEmpty() &&
> +               (!m_affectedByEmpty || m_emptyState == style->emptyState()) &&
> +               m_firstChildState == style->firstChildState() &&
> +               m_lastChildState == style->lastChildState() &&
> +               m_childIndex == style->childIndex();
> +    }

The WebKit coding style (and my person preference) put the operators on the starts of lines, not the ends of lines. Like this:

    return m_affectedByEmpty == style->affectedByEmpty()
        && (!m_affectedByEmpty || m_emptyState == style->emptyState())
        && m_firstChildState == style->firstChildState()
        && m_lastChildState == style->lastChildState()
        && m_childIndex == style->childIndex();

r=me
Comment 3 Simon Fraser (smfr) 2009-06-14 14:27:35 PDT
Landed with a little extra const cleanup:
http://trac.webkit.org/changeset/44671
Comment 4 Dave Hyatt 2009-06-14 14:30:41 PDT
I'd like to talk about this bug further.  I don't think this was the right approach.
Comment 5 Dave Hyatt 2009-06-14 14:31:51 PDT
Reopening, as I'd like this to be revisited.
Comment 6 Dave Hyatt 2009-06-14 15:03:06 PDT
See checkForSiblingStyleChanges in Element.cpp, which should have handled this situation without resorting to destroying the render tree and recreating it.  I assume there is a bug in that method that needs to be investigated.  Your solution, though, is needlessly destroying the render objects now on child changes, which should not be necessary and is bad for performance.
Comment 7 Dave Hyatt 2009-06-14 15:35:36 PDT
Created attachment 31271 [details]
Patch
Comment 8 Dave Hyatt 2009-06-14 16:10:23 PDT
Minusing this as it will cause inefficiency when nested positional rules are used, e.g., nth-child inside of nth-child.  Rather than dirtying for style recalc, I should just patch the check that does the internal update.

Comment 9 Dave Hyatt 2009-06-14 16:42:05 PDT
Created attachment 31272 [details]
Better patch
Comment 10 Simon Fraser (smfr) 2009-06-14 16:48:02 PDT
Comment on attachment 31272 [details]
Better patch

r=me
Comment 11 Dave Hyatt 2009-06-14 16:52:57 PDT
Fixed in r44673.