Bug 172596 – Crash on display-contents-replaced-001.html

• Component: CSS
• Status: RESOLVED FIXED

[Emilio Cobos Álvarez]

This crash is different from bug 172503.

There's no backtrace when running that one, so it'll need a bit more investigation.

[Antti Koivisto]

Created attachment 311481 [details]
patch

[Andreas Kling]

Comment on attachment 311481 [details]
patch

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

r=me

> Source/WebCore/css/StyleResolver.cpp:817
> +    // FIXME: <g>, <use> and <tspan> have special behaviors for display:contents in the current draft spec.

I would include a link to the relevant part of the spec here.

[Antti Koivisto]

Created attachment 311482 [details]
patch

[Emilio Cobos Álvarez]

Note that the spec requires contents to still compute to contents, which I don't think will be true with this patch.

i.e., a test like:

var svg = document.querySelector('svg');
svg.style.display = "contents";
getComputedStyle(svg).display; // "none" with your patch, "contents" per spec.

I had a WIP patch that did this[1]. Pending thing was to address the special-cases you mention in your FIXME.

Let me know if you want me to clean that patch up and submit it for review in a separate bug.

[1]: https://gist.github.com/emilio/9faec4ab5c6dd16ad6ccba4e098999ac

[Antti Koivisto]

(In reply to Emilio Cobos Álvarez from comment #4)
> Note that the spec requires contents to still compute to contents, which I
> don't think will be true with this patch.
> 
> i.e., a test like:
> 
> var svg = document.querySelector('svg');
> svg.style.display = "contents";
> getComputedStyle(svg).display; // "none" with your patch, "contents" per
> spec.

Is the spec correct though? We generally return the effective display value from getComputedStyle rather than the original. We already save the original in RenderStyle so if we want we could add a bit that says "return original instead of effective" to fix this.
 
> I had a WIP patch that did this[1]. Pending thing was to address the
> special-cases you mention in your FIXME.

I'd like to use a blacklist like in my patch instead of virtuals. It is much easier to compare the code to the spec. A bigger problem with your approach is that we have a bunch of sites that explicitly test for display() ==/!= NONE (and some for CONTENTS) and there is no way to know they should behave like 'none' in the effective case.

Does this make sense to you?

[Antti Koivisto]

Though it is possible that all those display() == NONE tests should be replaces with display() == NONE || display() == CONTENTS tests in which case this doesn't matter...

[Antti Koivisto]

https://trac.webkit.org/r217549

[Emilio Cobos Álvarez]

(In reply to Antti Koivisto from comment #5)
> (In reply to Emilio Cobos Álvarez from comment #4)
> > Note that the spec requires contents to still compute to contents, which I
> > don't think will be true with this patch.
> > 
> > i.e., a test like:
> > 
> > var svg = document.querySelector('svg');
> > svg.style.display = "contents";
> > getComputedStyle(svg).display; // "none" with your patch, "contents" per
> > spec.
> 
> Is the spec correct though? We generally return the effective display value
> from getComputedStyle rather than the original. We already save the original
> in RenderStyle so if we want we could add a bit that says "return original
> instead of effective" to fix this.

Hmmm... Yeah, perhaps it's just easier to lie in getComputedStyle if needed.

If you feel very strongly about it feel free to write something in https://github.com/w3c/csswg-drafts/issues/540 (or raise another issue). Otherwise I'll take care of the tests and the getComputedStyle stuff if needed.

> > I had a WIP patch that did this[1]. Pending thing was to address the
> > special-cases you mention in your FIXME.
> 
> I'd like to use a blacklist like in my patch instead of virtuals. It is much
> easier to compare the code to the spec. A bigger problem with your approach
> is that we have a bunch of sites that explicitly test for display() ==/!=
> NONE (and some for CONTENTS) and there is no way to know they should behave
> like 'none' in the effective case.
>
> Does this make sense to you?

Yeah, that makes sense, thanks for looking into it :)

[Antti Koivisto]

> Hmmm... Yeah, perhaps it's just easier to lie in getComputedStyle if needed.

RenderStyle::display() is really the "effective display"  

    void setDisplay(EDisplay v) { m_nonInheritedFlags.setEffectiveDisplay(v); }

which is often different from the specified display (as you see if you look through adjustStyle). Currently getComputedStyle always return the effective value not the specified one so this patch is consistent with the usual WebKit behaviour

I'm not sure if other browsers do this differently.

[Radar WebKit Bug Importer]

<rdar://problem/32479783>