Bug 133934 - Subtrees with :first-child and :last-child are not invalidated when siblings are added/removed
Summary: Subtrees with :first-child and :last-child are not invalidated when siblings ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-15 21:42 PDT by Benjamin Poulain
Modified: 2014-06-18 15:11 PDT (History)
8 users (show)

See Also:

Attachments
Patch (30.35 KB, patch)
2014-06-15 21:53 PDT, Benjamin Poulain 		no flags Details | Formatted Diff | Diff
Patch (30.39 KB, patch)
2014-06-16 12:19 PDT, Benjamin Poulain 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (600.24 KB, application/zip)
2014-06-16 13:48 PDT, Build Bot 		no flags Details
Patch (30.43 KB, patch)
2014-06-16 15:39 PDT, Benjamin Poulain 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.