Bug 119886 – PseudoElement is abusing parent node pointer

• Component: DOM
• Status: RESOLVED FIXED

[Antti Koivisto]

PseudoElement sets its host node as its parent. This is confusing and wrong as it breaks the basic tree consistency (a node is a child node of its parent node).

[Antti Koivisto]

Created attachment 208902 [details]
patch

[Build Bot]

Comment on attachment 208902 [details]
patch

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

New failing tests:
css2.1/t1202-counter-08-b.html
css2.1/t1202-counters-14-b.html
accessibility/render-counter-text.html
css2.1/t1202-counters-06-b.html
css2.1/t1202-counters-16-c.html
css2.1/t1202-counter-07-b.html
css2.1/t1202-counters-12-b.html
css2.1/t1202-counter-15-b.html
css2.1/t1202-counters-09-b.html
css2.1/t1202-counters-11-b.html
css2.1/t1202-counters-13-b.html
css2.1/t1202-counter-16-f.html
css2.1/t1202-counters-04-b.html
css2.1/t1202-counters-03-b.html
css2.1/t1202-counter-12-b.html
css2.1/t1202-counters-07-b.html
css2.1/t1202-counters-15-b.html
css2.1/t1202-counter-09-b.html
css2.1/t1202-counters-08-b.html
css2.1/t1202-counter-14-b.html
css2.1/t1202-counter-11-b.html
css2.1/t1202-counters-01-b.html
css2.1/t1202-counter-13-b.html
css2.1/t1202-counters-17-d.html
css2.1/t1202-counter-05-b.html
css2.1/t1202-counter-06-b.html
css2.1/t1202-counters-02-b.html
css2.1/t1202-counters-00-b.html
css2.1/t1202-counter-00-b.html
css2.1/t1202-counters-05-b.html

[Build Bot]

Created attachment 208911 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Ryosuke Niwa]

Is this consistent with http://dev.w3.org/csswg/cssom/#pseudoelement ?

[Antti Koivisto]

Yeah. That interface does not even make PseudoElement an Element.

[Antti Koivisto]

Created attachment 208927 [details]
another

[Antti Koivisto]

Now including fixes to the (messy) traversal functions used for resolving CSS counters.

[Build Bot]

Comment on attachment 208927 [details]
another

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

New failing tests:
fast/lists/anonymous-items.html

[Build Bot]

Created attachment 208933 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 208927 [details]
another

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

New failing tests:
fast/lists/anonymous-items.html

[Build Bot]

Created attachment 208947 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Antti Koivisto]

Created attachment 208986 [details]
another

[Darin Adler]

Comment on attachment 208986 [details]
another

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

The part of this patch I don’t understand is which calls to parentNode you decided to change to add checks for pseudo elements and shadow roots. I don’t understand why you had to do this in some places but not others and how you are sure you covered everything. The RenderCounter and RenderListItem cases are the puzzling ones. I don’t see how the code worked before. It was just calling parentNode like tons of other places that call parentNode. How did you know that these were the ones that needed to be changed?

> Source/WebCore/rendering/RenderCounter.cpp:72
> +    if (object->node()->isPseudoElement())
> +        return toPseudoElement(object->node())->hostElement();
> +    return toElement(object->node())->parentElement();

I would have written this with a local variable, but I guess we can count on the compiler to get this right?

> Source/WebCore/rendering/RenderListItem.cpp:108
> +    Node* parent = listItemNode->isPseudoElement() ? toPseudoElement(listItemNode)->hostElement() : listItemNode->parentNode();
>      // We use parentNode because the enclosing list could be a ShadowRoot that's not Element.
> -    for (Node* parent = listItemNode->parentNode(); parent; parent = parent->parentNode()) {
> +    for (; parent; parent = parent->parentNode()) {

Why does the first step of walking up the parent tree need to traverse host element pointers, but not any other steps? Can’t there be a transition to a host later on? Is this covered by tests?

[Antti Koivisto]

(In reply to comment #13)
> (From update of attachment 208986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208986&action=review
> 
> The part of this patch I don’t understand is which calls to parentNode you decided to change to add checks for pseudo elements and shadow roots. I don’t understand why you had to do this in some places but not others and how you are sure you covered everything. The RenderCounter and RenderListItem cases are the puzzling ones. I don’t see how the code worked before. It was just calling parentNode like tons of other places that call parentNode. How did you know that these were the ones that needed to be changed?

RenderCounter and RenderListItem do traversal where PseudoElements are treated as if they were real elements to compute counter values. The code for going to siblings and down the tree was already using explicit pseudo element tests to find the right node. Traversing up did not need the tests since the parentNode of the PseudoElement was set to point to the host (the asymmetry was a result of violating the "node is its parents child" consistency). 

I changed all call sites that are part of this special type of traversal. We have pretty good test coverage here, some that I had missed were spotted by the tests.

Making PseudoElements actual Elements was probably a mistake in the first place. Benefits are not obvious.
 
> > Source/WebCore/rendering/RenderCounter.cpp:72
> > +    if (object->node()->isPseudoElement())
> > +        return toPseudoElement(object->node())->hostElement();
> > +    return toElement(object->node())->parentElement();
> 
> I would have written this with a local variable, but I guess we can count on the compiler to get this right?

Everything that is being called is inlined so I think so. But yes, it would look nicer with variables.

> > Source/WebCore/rendering/RenderListItem.cpp:108
> > +    Node* parent = listItemNode->isPseudoElement() ? toPseudoElement(listItemNode)->hostElement() : listItemNode->parentNode();
> >      // We use parentNode because the enclosing list could be a ShadowRoot that's not Element.
> > -    for (Node* parent = listItemNode->parentNode(); parent; parent = parent->parentNode()) {
> > +    for (; parent; parent = parent->parentNode()) {
> 
> Why does the first step of walking up the parent tree need to traverse host element pointers, but not any other steps? Can’t there be a transition to a host later on? Is this covered by tests?

PseudoElements can only ever be leaf nodes (for purpose of this traversal case, they are really not in the tree at all). This sort of tightening of logic is an useful side-effect.

[Antti Koivisto]

http://trac.webkit.org/changeset/154232

[Alexey Proskuryakov]

This caused bug 119969.