Bug 105672 - REGRESSION: ChildrenAffectedBy flags lost between siblings which have cousin elements sharing style
Summary: REGRESSION: ChildrenAffectedBy flags lost between siblings which have cousin ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
: 108111 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-12-21 18:42 PST by Philippe Wittenbergh
Modified: 2013-01-29 03:38 PST (History)
9 users (show)

See Also:


Attachments
test case 1 (979 bytes, text/html)
2012-12-21 18:42 PST, Philippe Wittenbergh
no flags Details
test case 2 with :last-child (949 bytes, text/html)
2012-12-21 18:45 PST, Philippe Wittenbergh
no flags Details
Patch (3.34 KB, patch)
2012-12-28 08:06 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (10.33 KB, patch)
2012-12-28 08:47 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (10.45 KB, patch)
2013-01-03 01:27 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (11.98 KB, patch)
2013-01-29 02:47 PST, Allan Sandfeld Jensen
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Wittenbergh 2012-12-21 18:42:00 PST
Created attachment 180583 [details]
test case 1

Consider this style rule:
td+td+td { color: red;  background: yellow;}

with this markup (note the line-break after the opening <tr>):

<tr>
<td>cell></td><td>cell></td><td>cell></td></tr>
<tr>
<td>cell></td><td>cell></td><td>cell></td></tr>

After the 1st row in the table, _all_ cells will be painted based on the rule above.

Now for the funky (ahem) part: use the Inspector to analyse the issue then close the inspector: now all cell are painted correctly. Reload the page and the problem is back.

Seen with WebKit r138355 and Chrome Version 25.0.1364.5 dev / OS X Mountain Lion

I don’t know (yet) when this regressed; I’ve seen the issue on some forum for at least 2 weeks, but I haven’t had time to debug until now.
Comment 1 Philippe Wittenbergh 2012-12-21 18:43:29 PST
I forgot to note: in the test case 1, the first table has no line-breaks in the markup (and displays as expected), the second table shows the problem.
Comment 2 Philippe Wittenbergh 2012-12-21 18:45:26 PST
Created attachment 180584 [details]
test case 2 with :last-child

The :last-child pseudo-class has a similar problem, in that only the last cell of the 1st row is painted with the specified rule.
Comment 3 Philippe Wittenbergh 2012-12-21 19:36:24 PST
This works in r135981, but fails with r136017 (builds from http://nightly.webkit.org).

Possibly:
http://trac.webkit.org/changeset/136001
Comment 4 Allan Sandfeld Jensen 2012-12-28 02:55:34 PST
Confirmed. I will investigate.
Comment 5 Allan Sandfeld Jensen 2012-12-28 03:10:05 PST
I suspect this is not as much a regression but a change in when the issue is triggered. It is likely the same issue can be triggered in both before and after r136001 if you give each TR and id or class attribute.

If correct the cause is sharing of style with cousin elements (children elements of parent's siblings), which has the result that childrenAffectedBy are not observed for later TR elements when their table cell chilren share styles with cell children of previous TR elements. Previously this would work in very basic cases due to another bug that caused these bits to be incorrectly shared betwen the TR elements. This might be correct in this case, but was causing odd issues and inefficiencies in many other cases.

One way to solve the issue could be to copy the childrenAffectedBy bits between the uncles when sharing style between cousin elements.
Comment 6 Allan Sandfeld Jensen 2012-12-28 08:06:31 PST
Created attachment 180877 [details]
Patch

Fix by not sharing styles from elements prevented to do so by parents. Needs to have test-case added, and I still wish to investigate why a line-break makes a difference and RenderStyle unique does not solve the issue.
Comment 7 Allan Sandfeld Jensen 2012-12-28 08:47:02 PST
Created attachment 180879 [details]
Patch
Comment 8 Andreas Kling 2012-12-31 19:19:42 PST
Comment on attachment 180879 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1047
> -            if (currentNode->renderStyle() == parentStyle && currentNode->lastChild()) {
> +            if (currentNode->renderStyle() == parentStyle && currentNode->lastChild() && !parentElementPreventsSharing(toElement(currentNode))) {

Is it always safe to cast currentNode to an Element here?
Comment 9 Allan Sandfeld Jensen 2013-01-02 00:43:51 PST
(In reply to comment #8)
> (From update of attachment 180879 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180879&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:1047
> > -            if (currentNode->renderStyle() == parentStyle && currentNode->lastChild()) {
> > +            if (currentNode->renderStyle() == parentStyle && currentNode->lastChild() && !parentElementPreventsSharing(toElement(currentNode))) {
> 
> Is it always safe to cast currentNode to an Element here?

It should be, currentNode starts as a parentElement(), and is then iterated over by previousSibling, which can be non-element, but here it has been tested that it has a at least one child. Perhaps it would be cleaner if currentNode was always element, but that would take some refactoring, but it should at least be Element at this point.
Comment 10 Antti Koivisto 2013-01-02 03:40:28 PST
(In reply to comment #9)
> It should be, currentNode starts as a parentElement(), and is then iterated over by previousSibling, which can be non-element, but here it has been tested that it has a at least one child. Perhaps it would be cleaner if currentNode was always element, but that would take some refactoring, but it should at least be Element at this point.

ShadowRoots are non-Elements that can have Element children though this code probably can't reach any. Even so it would definitely be nicer if the code operated on Elements instead of Nodes so that would be clear.

Have you ran tests in debug to ensure the cast assert doesn't get hit?
Comment 11 Allan Sandfeld Jensen 2013-01-02 10:20:00 PST
(In reply to comment #10)
> (In reply to comment #9)
> > It should be, currentNode starts as a parentElement(), and is then iterated over by previousSibling, which can be non-element, but here it has been tested that it has a at least one child. Perhaps it would be cleaner if currentNode was always element, but that would take some refactoring, but it should at least be Element at this point.
> 
> ShadowRoots are non-Elements that can have Element children though this code probably can't reach any. Even so it would definitely be nicer if the code operated on Elements instead of Nodes so that would be clear.
> 
> Have you ran tests in debug to ensure the cast assert doesn't get hit?

No, and now that I think about it again; I think it really should do an isElement check here for clarity and code safety. I was making an assumption that even if true, is not guaranteed not to break in the future.
Comment 12 Allan Sandfeld Jensen 2013-01-03 01:27:02 PST
Created attachment 181156 [details]
Patch
Comment 13 Allan Sandfeld Jensen 2013-01-29 02:35:48 PST
*** Bug 108111 has been marked as a duplicate of this bug. ***
Comment 14 Allan Sandfeld Jensen 2013-01-29 02:47:03 PST
Created attachment 185208 [details]
Patch

Update the patch to also fix bug #108111. Also adds a method that makes it more clear why these flags prevent sharing
Comment 15 Andreas Kling 2013-01-29 02:58:13 PST
Comment on attachment 185208 [details]
Patch

r=me
Comment 16 Allan Sandfeld Jensen 2013-01-29 03:38:14 PST
Committed r141093: <http://trac.webkit.org/changeset/141093>