Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684833003:
Disable display: contents in SVG. (Closed)
|
Created:
3 years, 10 months ago by emilio Modified:
3 years, 10 months ago Reviewers:
rune CC:
chromium-reviews, blink-reviews-style_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, Manuel Rego Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable display: contents in SVG.
BUG=657748
TEST=svg/css/display-computed.html
Review-Url: https://codereview.chromium.org/2684833003
Cr-Commit-Position: refs/heads/master@{#449304}
Committed: https://chromium.googlesource.com/chromium/src/+/ba3e8dd8fae979960278a5e604717ef9658ee3ac
Patch Set 1 #Patch Set 2 : Disable display: contents in SVG. #
Total comments: 3
Patch Set 3 : Make display: contents act as an inline when it has a layout box, and ignore display: contents in S… #
Total comments: 2
Patch Set 4 : Make display: contents compute to inline in SVG subtrees. #
Messages
Total messages: 25 (12 generated)
ecobos@igalia.com changed reviewers: + rune@opera.com
Hey, rune, could you take a look at this? I preferred this approach instead of modifying layoutObjectIsNeeded for SVG elements because it felt weird that display still computed to contents when doing that.
The CQ bit was checked by ecobos@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/02/08 13:13:48, emilio wrote: > Hey, rune, could you take a look at this? > > I preferred this approach instead of modifying layoutObjectIsNeeded for SVG > elements because it felt weird that display still computed to contents when > doing that. But it computes to specified values for other display types, e.g. table. Also, Gecko computes display to contents when specified on svg elements. I don't think we should adjust the computed value.
On 2017/02/08 19:07:16, rune wrote: > On 2017/02/08 13:13:48, emilio wrote: > > Hey, rune, could you take a look at this? > > > > I preferred this approach instead of modifying layoutObjectIsNeeded for SVG > > elements because it felt weird that display still computed to contents when > > doing that. > > But it computes to specified values for other display types, e.g. table. Also, > Gecko computes display to contents when specified on svg elements. I don't think > we should adjust the computed value. Fair enough, I think the updated patch would look better to you :)
https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/css/display-expected.html (right): https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/css/display-expected.html:17: #t7 { display: block; } I did some brief testing and this is not what Gecko does. Also, does block and inline display differently for replaced content for your test? Having inline content before or after the svg/image would have made it clearer. https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:185: if (LIKELY(!element->isSVGElement())) Shouldn't this be testing isSVGSVGElement? Also, it looks to me Gecko makes this inline, not block. https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:187: // else fallthrough Just "// fallthrough".
Hi rune, First of all, thanks for being more patient while reviewing than me while writing code :) On 2017/02/08 22:28:18, rune wrote: > https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/svg/css/display-expected.html (right): > > https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/svg/css/display-expected.html:17: #t7 { display: > block; } > I did some brief testing and this is not what Gecko does. Also, does block and > inline display differently for replaced content for your test? Having inline > content before or after the svg/image would have made it clearer. Ok, so I went directly to see what Gecko does for SVG stuff. They ignore display: contents completely for SVG stuff[1], and they only check for display: none[2] before constructing an SVG frame. > https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): > > https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutObject.cpp:185: if > (LIKELY(!element->isSVGElement())) > Shouldn't this be testing isSVGSVGElement? Also, it looks to me Gecko makes this > inline, not block. I was checking for general SVG elements because I believed you wanted to ignore also display: contents for those. In any case, all the display-affected SVG elements override their createLayoutObject method, so I think this can't be reached from those. For those who are not affected by display, we probably care about giving them a layout object per my comment above, but I think they all override createLayoutObject too, so I'll just DCHECK that the element is not a SVG element. The reason why it was behaving as block when given a LayoutObject is because, unlike Gecko[3], we weren't treating display: contents as an inline display kind. I've checked the places we call isReplacedDisplayType, and I believe we want to treat it as a plain inline. I finally also took the time too clean up a dirty check that was on dom/LayoutTreeBuilder.cpp [1]: http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d... [2]: http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d... [3]: http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d...
The CQ bit was checked by ecobos@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2684833003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2684833003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3713: isDisplayReplacedType(display); display:contents is neither an inline, nor block type display, so this looks wrong even if it works. How did this work for other replaced content like input elements in your other CL? (or was the csswg imported replaced content test skipped?)
On 2017/02/09 00:57:38, emilio wrote: > Hi rune, > > First of all, thanks for being more patient while reviewing than me while > writing code :) np. > https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): > > > > > https://codereview.chromium.org/2684833003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutObject.cpp:185: if > > (LIKELY(!element->isSVGElement())) > > Shouldn't this be testing isSVGSVGElement? Also, it looks to me Gecko makes > this > > inline, not block. > > I was checking for general SVG elements because I believed you wanted to ignore > also display: contents for those. In any case, all the display-affected SVG > elements override their createLayoutObject method, so I think this can't be > reached from those. For those who are not affected by display, we probably care > about giving them a layout object per my comment above, but I think they all > override createLayoutObject too, so I'll just DCHECK that the element is not a > SVG element. Yes, I thought only the SVG root element would end up here, but since that won't either, a DCHECK is good. > The reason why it was behaving as block when given a LayoutObject is because, > unlike Gecko[3], we weren't treating display: contents as an inline display > kind. I've checked the places we call isReplacedDisplayType, and I believe we > want to treat it as a plain inline. See separate comment. display:contents behaves as display-outside:inline for replaced content, but it could cause problems in the future checking for inline display without checking for a layout object. Is it possible to make this specific to replaced content?
https://codereview.chromium.org/2684833003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2684833003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3713: isDisplayReplacedType(display); On 2017/02/09 09:41:10, rune wrote: > display:contents is neither an inline, nor block type display, so this looks > wrong even if it works. How did this work for other replaced content like input > elements in your other CL? (or was the csswg imported replaced content test > skipped?) Replaced content was ignored in that CL, but actually I've been going through the spec and I need to make it compute to inline for those (this was recently changed[1], in Gecko's implementation display: contents is just ignored). Would you be fine landing this as-is a temporary measure and I'll make that change either with all the other style adjustment changes in that CL, or in a followup CL? [1]: https://github.com/w3c/csswg-drafts/issues/540
On 2017/02/09 11:49:37, emilio wrote: > https://codereview.chromium.org/2684833003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/style/ComputedStyle.h (right): > > https://codereview.chromium.org/2684833003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/style/ComputedStyle.h:3713: > isDisplayReplacedType(display); > On 2017/02/09 09:41:10, rune wrote: > > display:contents is neither an inline, nor block type display, so this looks > > wrong even if it works. How did this work for other replaced content like > input > > elements in your other CL? (or was the csswg imported replaced content test > > skipped?) > > Replaced content was ignored in that CL, but actually I've been going through > the spec and I need to make it compute to inline for those (this was recently > changed[1], in Gecko's implementation display: contents is just ignored). > > Would you be fine landing this as-is a temporary measure and I'll make that > change either with all the other style adjustment changes in that CL, or in a > followup CL? > > [1]: https://github.com/w3c/csswg-drafts/issues/540 Oh, sorry, forgot about that one. Then we might as well go back to PS1, but use inline instead of block. We can fix non-root-SVG elements later when it gets specified. Sorry about the extra work :-(
On 2017/02/09 12:15:56, rune wrote: > On 2017/02/09 11:49:37, emilio wrote: > > > https://codereview.chromium.org/2684833003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/style/ComputedStyle.h (right): > > > > > https://codereview.chromium.org/2684833003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/style/ComputedStyle.h:3713: > > isDisplayReplacedType(display); > > On 2017/02/09 09:41:10, rune wrote: > > > display:contents is neither an inline, nor block type display, so this looks > > > wrong even if it works. How did this work for other replaced content like > > input > > > elements in your other CL? (or was the csswg imported replaced content test > > > skipped?) > > > > Replaced content was ignored in that CL, but actually I've been going through > > the spec and I need to make it compute to inline for those (this was recently > > changed[1], in Gecko's implementation display: contents is just ignored). > > > > Would you be fine landing this as-is a temporary measure and I'll make that > > change either with all the other style adjustment changes in that CL, or in a > > followup CL? > > > > [1]: https://github.com/w3c/csswg-drafts/issues/540 > > Oh, sorry, forgot about that one. Then we might as well go back to PS1, but use > inline instead of block. We can fix non-root-SVG elements later when it gets > specified. Sorry about the extra work :-( Done, no worries! Good to see you're human too ;) I spawned off the layoutObjectIsNeeded cleanup from this, see https://codereview.chromium.org/2685113002/.
lgtm
The CQ bit was checked by ecobos@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486650689129790,
"parent_rev": "0e9764ee9211235cb379f45df8bf983af2b0b0aa", "commit_rev":
"ba3e8dd8fae979960278a5e604717ef9658ee3ac"}
Message was sent while issue was closed.
Description was changed from ========== Disable display: contents in SVG. BUG=657748 TEST=svg/css/display-computed.html ========== to ========== Disable display: contents in SVG. BUG=657748 TEST=svg/css/display-computed.html Review-Url: https://codereview.chromium.org/2684833003 Cr-Commit-Position: refs/heads/master@{#449304} Committed: https://chromium.googlesource.com/chromium/src/+/ba3e8dd8fae979960278a5e60471... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ba3e8dd8fae979960278a5e60471... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
