Bug 133934

Summary: Subtrees with :first-child and :last-child are not invalidated when siblings are added/removed
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cmarcelo, commit-queue, esprehn+autocc, kangil.han, koivisto, rniwa, ysuzuki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Patch none

Description Benjamin Poulain 2014-06-15 21:42:59 PDT
Subtrees with :first-child and :last-child are not invalidated when siblings are added/removed
Comment 1 Benjamin Poulain 2014-06-15 21:53:46 PDT
Created attachment 233150 [details]
Patch
Comment 2 Antti Koivisto 2014-06-16 04:55:21 PDT
Comment on attachment 233150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233150&action=review

> Source/WebCore/dom/Element.cpp:1552
> +    RenderStyle* style = element.renderStyle();
> +    if (!style || (element.styleAffectedByEmpty() && (!style->emptyState() || element.hasChildNodes())))
> +        element.setNeedsStyleRecalc();
>  }

Isn't this a bit blunt? Any element with display:none is going to have it's style recomputed on child change whether or not there are any :empty rules around.
Comment 3 Benjamin Poulain 2014-06-16 12:19:05 PDT
Created attachment 233170 [details]
Patch
Comment 4 Build Bot 2014-06-16 13:48:00 PDT
Comment on attachment 233170 [details]
Patch

Attachment 233170 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6303961720029184

New failing tests:
http/tests/misc/acid3.html
Comment 5 Build Bot 2014-06-16 13:48:04 PDT
Created attachment 233175 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 6 Benjamin Poulain 2014-06-16 14:46:22 PDT
Comment on attachment 233170 [details]
Patch

Damn, Acid 3 does not like the :empty change...
Comment 7 Benjamin Poulain 2014-06-16 14:57:00 PDT
Arg, of course, my last change is non-sense. There can be non-element nodes as children, that still qualify as :empty.
Comment 8 Benjamin Poulain 2014-06-16 15:39:49 PDT
Created attachment 233191 [details]
Patch
Comment 9 Antti Koivisto 2014-06-18 14:46:38 PDT
Comment on attachment 233191 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233191&action=review

> Source/WebCore/dom/Element.cpp:1597
> +            if (!style || style->lastChildState())

I'm still slightly worried we are invalidating more in display:none case. However maybe this doesn't have much real world significance.
Comment 10 Benjamin Poulain 2014-06-18 15:11:00 PDT
Comment on attachment 233191 [details]
Patch

Clearing flags on attachment: 233191

Committed r170121: <http://trac.webkit.org/changeset/170121>
Comment 11 Benjamin Poulain 2014-06-18 15:11:08 PDT
All reviewed patches have been landed.  Closing bug.