Bug 17143

Summary:

:first-child does not update dynamically when the DOM changes

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

CSS

Assignee:

Dave Hyatt <hyatt>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hyatt, oliver, webkit

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

11384

Attachments:

Description	Flags
Patch to make :first-child and :first-of-type properly dynamic	none
Make sure to check m_collectRulesOnly to avoid crashes	hyatt: review-
Patch that addresses Eric's comments.	oliver: review+

Eric Seidel (no email)

Reported 2008-02-01 14:19:22 PST

:first-child does not update dynamically when the DOM changes Hyatt is working on fixing this as we speak.

Attachments
Patch to make :first-child and :first-of-type properly dynamic (180.38 KB, patch) 2008-02-01 14:48 PST, Dave Hyatt	no flags	Details Formatted Diff Diff
Make sure to check m_collectRulesOnly to avoid crashes (181.29 KB, patch) 2008-02-01 16:05 PST, Dave Hyatt	hyatt: review-	Details Formatted Diff Diff
Patch that addresses Eric's comments. (181.54 KB, patch) 2008-02-01 16:51 PST, Dave Hyatt	oliver: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Dave Hyatt

Comment 1 2008-02-01 14:48:54 PST

Created attachment 18855 [details] Patch to make :first-child and :first-of-type properly dynamic

Dave Hyatt

Comment 2 2008-02-01 16:05:04 PST

Created attachment 18858 [details] Make sure to check m_collectRulesOnly to avoid crashes This patch fixes a crasher. It's the same one as reported in http://bugs.webkit.org/show_bug.cgi?id=17141. I went ahead and patched the empty state case as well as the first-child and first-of-type cases.

Eric Seidel (no email)

Comment 3 2008-02-01 16:16:38 PST

Comment on attachment 18855 [details] Patch to make :first-child and :first-of-type properly dynamic No need to check e &&. e is checked at the top of checkOneSelector So we end up setting the first child state (on the first child) when checking style on every sibling? That seems unnecessary... e->renderStyle()->setFirstChildState(result, e->parentNode()->renderStyle()); :sigh: hasPositionalChildren makes it sound like they have position: set on them (which is not what the variable name means). However any other name I come up with is 18 miles long (I realize that's a variable from a prior patch). I think ideally the: if (style->childrenAffectedByFirstChildRules()) { if blocks should be out in their own separate static functions: static bool needsStyleRecalcDueToFirstChildChange(Element* e); static bool needsStyleRecalcDueToLastChildChange(Element* e); (Or they could be private methods on Element) if (needsStyleRecalcDueToFirstChildChange() || needsStyleRecalcDueToLastChildChange()) setChanged(); Not required for review, but personally I prefer lots of statics over long methods. I'm not marking r=me only due to the (unnecessary?) setFirstChildState() calls. If you can explain why those are necessary, I'm happy to mark it r+.

Eric Seidel (no email)

Comment 4 2008-02-01 16:18:56 PST

*** Bug 17141 has been marked as a duplicate of this bug. ***

Dave Hyatt

Comment 5 2008-02-01 16:28:41 PST

You are right. They aren't necessary. I'll update the patch.

Dave Hyatt

Comment 6 2008-02-01 16:34:02 PST

Comment on attachment 18858 [details] Make sure to check m_collectRulesOnly to avoid crashes New patch coming in a sec.

Dave Hyatt

Comment 7 2008-02-01 16:51:39 PST

Created attachment 18859 [details] Patch that addresses Eric's comments.

Oliver Hunt

Comment 8 2008-02-01 19:45:51 PST

Comment on attachment 18859 [details] Patch that addresses Eric's comments. This appears to address eric's comments, and it also seems sane to me. r=me

Oliver Hunt

Comment 9 2008-02-01 19:47:08 PST

(whoops, forgot to mention change i suggested to dhyatt -- i suck :-O )

mitz

Comment 10 2008-02-01 23:39:09 PST

Comment on attachment 18859 [details] Patch that addresses Eric's comments. + else if (e && e->renderStyle() && (e->document()->usesSiblingRules() || e->renderStyle()->unique())) No need to null-check e, because of the early return at the beginning of the function.

Dave Hyatt

Comment 11 2008-02-02 02:09:13 PST

Fixed in r29932.

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