WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
172596
Crash on display-contents-replaced-001.html
https://bugs.webkit.org/show_bug.cgi?id=172596
Summary
Crash on display-contents-replaced-001.html
Emilio Cobos Álvarez
Reported
2017-05-25 09:35:01 PDT
This crash is different from
bug 172503
. There's no backtrace when running that one, so it'll need a bit more investigation.
Attachments
patch
(5.13 KB, patch)
2017-05-29 12:25 PDT
,
Antti Koivisto
kling
: review+
Details
Formatted Diff
Diff
patch
(5.20 KB, patch)
2017-05-29 12:45 PDT
,
Antti Koivisto
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Antti Koivisto
Comment 1
2017-05-29 12:25:03 PDT
Created
attachment 311481
[details]
patch
Andreas Kling
Comment 2
2017-05-29 12:34:51 PDT
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
Comment 3
2017-05-29 12:45:36 PDT
Created
attachment 311482
[details]
patch
Emilio Cobos Álvarez
Comment 4
2017-05-29 18:12:13 PDT
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
Comment 5
2017-05-29 22:45:21 PDT
(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
Comment 6
2017-05-29 22:48:32 PDT
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
Comment 7
2017-05-30 01:36:10 PDT
https://trac.webkit.org/r217549
Emilio Cobos Álvarez
Comment 8
2017-05-30 01:50:13 PDT
(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
Comment 9
2017-05-30 04:05:32 PDT
> 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
Comment 10
2017-05-30 20:24:26 PDT
<
rdar://problem/32479783
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug