Bug 17143 - :first-child does not update dynamically when the DOM changes
Summary: :first-child does not update dynamically when the DOM changes
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:
Keywords:
: 17141 (view as bug list)
Depends on:
Blocks: 11384
  Show dependency treegraph
 
Reported: 2008-02-01 14:19 PST by Eric Seidel (no email)
Modified: 2008-02-02 02:09 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 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.
Comment 1 Dave Hyatt 2008-02-01 14:48:54 PST
Created attachment 18855 [details]
Patch to make :first-child and :first-of-type properly dynamic
Comment 2 Dave Hyatt 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.
Comment 3 Eric Seidel (no email) 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+.
Comment 4 Eric Seidel (no email) 2008-02-01 16:18:56 PST
*** Bug 17141 has been marked as a duplicate of this bug. ***
Comment 5 Dave Hyatt 2008-02-01 16:28:41 PST
You are right.  They aren't necessary.  I'll update the patch.

Comment 6 Dave Hyatt 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.
Comment 7 Dave Hyatt 2008-02-01 16:51:39 PST
Created attachment 18859 [details]
Patch that addresses Eric's comments.
Comment 8 Oliver Hunt 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
Comment 9 Oliver Hunt 2008-02-01 19:47:08 PST
(whoops, forgot to mention change i suggested to dhyatt -- i suck :-O )
Comment 10 mitz 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.
Comment 11 Dave Hyatt 2008-02-02 02:09:13 PST
Fixed in r29932.