Summary: | PseudoElement is abusing parent node pointer | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, eoconnor, esprehn+autocc, glenn, kangil.han, kling, kondapallykalyan, rniwa | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Antti Koivisto
2013-08-16 02:54:40 PDT
Created attachment 208902 [details]
patch
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 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
Is this consistent with http://dev.w3.org/csswg/cssom/#pseudoelement ? Yeah. That interface does not even make PseudoElement an Element. Created attachment 208927 [details]
another
Now including fixes to the (messy) traversal functions used for resolving CSS counters. 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 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
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 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
Created attachment 208986 [details]
another
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? (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. This caused bug 119969. |