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

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Eric Seidel (no email)]

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

Hyatt is working on fixing this as we speak.

[Dave Hyatt]

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

[Dave Hyatt]

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 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)]

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

[Dave Hyatt]

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

[Dave Hyatt]

Comment on attachment 18858 [details]
Make sure to check m_collectRulesOnly to avoid crashes

New patch coming in a sec.

[Dave Hyatt]

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

[Oliver Hunt]

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]

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

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]

Fixed in r29932.