Incorrect computed style in pseudo-elements with display: contents

[Emilio Cobos Álvarez (:emilio)]

Bug 178513 gave them an anon renderer as the main renderer. Unfortunately, the style of that box is exposed via getComputedStyle, so that needs to be handled somehow.

[Emilio Cobos Álvarez (:emilio)]

Created attachment 327151 [details]
Patch

[EWS Watchlist]

Comment on attachment 327151 [details]
Patch

Attachment 327151 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5270189

New failing tests:
http/tests/appcache/resource-redirect-2.html

[EWS Watchlist]

Created attachment 327152 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

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

[Emilio Cobos Álvarez (:emilio)]

Those look flaky and completely unrelated to this.

Antti or Ryosuke, mind reviewing? Thanks!

Please don't land this directly yet, I'll ensure the test change is upstreamed first.

[Antti Koivisto]

Comment on attachment 327151 [details]
Patch

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

> Source/WebCore/dom/PseudoElement.h:48
> +    bool hasDisplayContents() const { return m_hasDisplayContents; }
> +    void setHasDisplayContents(bool has) { m_hasDisplayContents = has; }

Instead of adding this bit and the element special cases, could we just put the original display:contents style to m_computedStyle in pseudoelement rare data?

[Emilio Cobos Álvarez (:emilio)]

(In reply to Antti Koivisto from comment #5)
> Instead of adding this bit and the element special cases, could we just put
> the original display:contents style to m_computedStyle in pseudoelement rare
> data?

You'd still need the element special cases to avoid using the renderStyle() or the renderer() unfortunately. I'm not in love with this patch either, but seemed the less-bad approach.

We can also regigger the hasDisplayContents / existingComputedStyle logic a bit more using the pseudo rare data, I guess, but that didn't sound particularly preferable to me.

[Antti Koivisto]

> You'd still need the element special cases to avoid using the renderStyle()
> or the renderer() unfortunately. I'm not in love with this patch either, but
> seemed the less-bad approach.
> 
> We can also regigger the hasDisplayContents / existingComputedStyle logic a
> bit more using the pseudo rare data, I guess, but that didn't sound
> particularly preferable to me.

Ok, we can go with this for now but I think there is too much special casing here. 

Longer term we should formalise the idea that style used for rendering and style used for getComputedStyle are two different things and we need to keep both around.

[Antti Koivisto]

I do like that this moves the display:contents special case from style resolution to render tree building.

[Emilio Cobos Álvarez (:emilio)]

Created attachment 327199 [details]
Patch

[Emilio Cobos Álvarez (:emilio)]

(In reply to Antti Koivisto from comment #7)
> Ok, we can go with this for now but I think there is too much special casing
> here. 
> 
> Longer term we should formalise the idea that style used for rendering and
> style used for getComputedStyle are two different things and we need to keep
> both around.

Actually you're right, I felt bad about the patch. I uploaded one that should be simpler and nicer :)

[Antti Koivisto]

Comment on attachment 327199 [details]
Patch

Much better! r=me

[Emilio Cobos Álvarez (:emilio)]

Created attachment 327357 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 327357 [details]
Patch for landing

Clearing flags on attachment: 327357

Committed r225049: <https://trac.webkit.org/changeset/225049>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35644571>