Bug 105672 – REGRESSION: ChildrenAffectedBy flags lost between siblings which have cousin elements sharing style

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

[Philippe Wittenbergh]

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.

[Philippe Wittenbergh]

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.

[Philippe Wittenbergh]

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.

[Philippe Wittenbergh]

This works in r135981, but fails with r136017 (builds from http://nightly.webkit.org).

Possibly:
http://trac.webkit.org/changeset/136001

[Allan Sandfeld Jensen]

Confirmed. I will investigate.

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 180879 [details]
Patch

[Andreas Kling]

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?

[Allan Sandfeld Jensen]

(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.

[Antti Koivisto]

(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?

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

Created attachment 181156 [details]
Patch

[Allan Sandfeld Jensen]

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

[Allan Sandfeld Jensen]

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

[Andreas Kling]

Comment on attachment 185208 [details]
Patch

r=me

[Allan Sandfeld Jensen]

Committed r141093: <http://trac.webkit.org/changeset/141093>