Bug 26362 – :nth-child() is not re-evaluated when adding or removing children

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 31265 [details]
Patch, testcase, changelog

[Darin Adler]

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

[Simon Fraser (smfr)]

Landed with a little extra const cleanup:
http://trac.webkit.org/changeset/44671

[Dave Hyatt]

I'd like to talk about this bug further.  I don't think this was the right approach.

[Dave Hyatt]

Reopening, as I'd like this to be revisited.

[Dave Hyatt]

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.

[Dave Hyatt]

Created attachment 31271 [details]
Patch

[Dave Hyatt]

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.

[Dave Hyatt]

Created attachment 31272 [details]
Better patch

[Simon Fraser (smfr)]

Comment on attachment 31272 [details]
Better patch

r=me

[Dave Hyatt]

Fixed in r44673.