Bug 26362

Summary:

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

Product:

WebKit

Reporter:

Simon Fraser (smfr) <simon.fraser>

Component:

CSS

Assignee:

Dave Hyatt <hyatt>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hyatt

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

URL:

http://www.quirksmode.org/css/nthchild.html

Bug Depends on:

Bug Blocks:

9610

Attachments:

Description	Flags
Patch, testcase, changelog	darin: review+
Patch	hyatt: review-
Better patch	simon.fraser: review+

Simon Fraser (smfr)

Reported 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.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2009-06-14 10:14:42 PDT

Created attachment 31265 [details] Patch, testcase, changelog

Darin Adler

Comment 2 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

Simon Fraser (smfr)

Comment 3 2009-06-14 14:27:35 PDT

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

Dave Hyatt

Comment 4 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.

Dave Hyatt

Comment 5 2009-06-14 14:31:51 PDT

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

Dave Hyatt

Comment 6 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.

Dave Hyatt

Comment 7 2009-06-14 15:35:36 PDT

Created attachment 31271 [details] Patch

Dave Hyatt

Comment 8 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.

Dave Hyatt

Comment 9 2009-06-14 16:42:05 PDT

Created attachment 31272 [details] Better patch

Simon Fraser (smfr)

Comment 10 2009-06-14 16:48:02 PDT

Comment on attachment 31272 [details] Better patch r=me

Dave Hyatt

Comment 11 2009-06-14 16:52:57 PDT

Fixed in r44673.

Note You need to log in before you can comment on or make changes to this bug.