New issue
Advanced search Search tips

Issue 916387 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[LayoutNG] Containing block for replaced elements incorrect.

Project Member Reported by ikilpatrick@chromium.org, Dec 19

Issue description

See: https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=6472
The containing block for the 2nd SVG is reporting the "default" replaced size, which seems wrong.

Seen on loading spinner on vimeo.com e.g.
https://vimeo.com/76311230
 
Status: Available (was: Untriaged)
Summary: [LayoutNG] Containing block for replaced elements incorrect. (was: [LayoutNG] Containing block for replacced elements incorrect.)
Interesting. The second SVG thinks its containing block is 300x150 (fallback intrinsic size?)
Looks like legacy layout has some problems here as well. Compare the attached test with how it renders in Firefox.
tc.html
502 bytes View Download
Cc: f...@opera.com
The intrinsic size of the SVG is set when creating the layout object - this is the same in legacy and NG (300x150):
#0  blink::LayoutReplaced::SetIntrinsicSize (this=0x38c1dd434010, intrinsic_size=LayoutSize(300px, 150px)) at ../../third_party/blink/renderer/core/layout/layout_replaced.h:129
#1  0x00007fffe2ae6786 in blink::LayoutSVGRoot::LayoutSVGRoot (this=0x38c1dd434010, node=0x17c4cdb45808) at ../../third_party/blink/renderer/core/layout/svg/layout_svg_root.cc:65
#2  0x00007fffe2eff0af in blink::SVGSVGElement::CreateLayoutObject (this=0x17c4cdb45808) at ../../third_party/blink/renderer/core/svg/svg_svg_element.cc:505
#3  0x00007fffe1eb9b47 in blink::LayoutTreeBuilderForElement::CreateLayoutObject (this=0x7fff9e8d46f0) at ../../third_party/blink/renderer/core/dom/layout_tree_builder.cc:142

The intrinsic size bleeds into NG here, where box_->IntrinsicSize() is called:
#1  0x00007fffe2a79e1c in blink::NGLayoutInputNode::IntrinsicSize (this=0x2909024c3f58, default_intrinsic_size=0x7fff9e8cc2e8, computed_inline_size=0x7fff9e8cc2e0, computed_block_size=0x7fff9e8cc2d8, aspect_ratio=0x7fff9e8cc2d0) at ../../third_party/blink/renderer/core/layout/ng/ng_layout_input_node.cc:89
#2  0x00007fffe2a7e9c2 in blink::ComputeReplacedSize (node=..., space=..., child_minmax=...) at ../../third_party/blink/renderer/core/layout/ng/ng_length_utils.cc:601
#3  0x00007fffe2a83942 in blink::NGOutOfFlowLayoutPart::LayoutDescendant (this=0x7fff9e8ccb38, descendant=..., offset=0x7fff9e8cc8b8) at ../../third_party/blink/renderer/core/layout/ng/ng_out_of_flow_layout_part.cc:310
#4  0x00007fffe2a82858 in blink::NGOutOfFlowLayoutPart::Run (this=0x7fff9e8ccb38, only_layout=0x0) at ../../third_party/blink/renderer/core/layout/ng/ng_out_of_flow_layout_part.cc:75
#5  0x00007fffe2a50bfc in blink::NGBlockLayoutAlgorithm::Layout (this=0x7fff9e8ce178) at ../../third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.cc:668
#6  0x00007fffe2a609a9 in blink::(anonymous namespace)::LayoutWithAlgorithm (node=..., space=..., break_token=0x0) at ../../third_party/blink/renderer/core/layout/ng/ng_block_node.cc:75

Legacy layout, on the other hand, seems to disregard this fake/bogus intrinsic size. That seems reasonable, since all 4 insets have been specified, and this SVG doesn't actually have an intrinsic size anyway.

Legacy layout gets it slightly more correct in LayoutReplaced::ComputeReplacedLogicalWidth(), by honoring IntrinsicSizingInfo, which in our case says that there's no intrinsic width, nor height, only an intrinsic ratio.

#0  blink::LayoutReplaced::ComputeReplacedLogicalWidth (this=0x2ae12cc34010, should_compute_preferred=blink::kComputeActual) at ../../third_party/blink/renderer/core/layout/layout_replaced.cc:717
#1  0x00007fffe2ae6cb7 in blink::LayoutSVGRoot::ComputeReplacedLogicalWidth (this=0x2ae12cc34010, should_compute_preferred=blink::kComputeActual) at ../../third_party/blink/renderer/core/layout/svg/layout_svg_root.cc:136
#2  0x00007fffe29202f7 in blink::LayoutReplaced::ComputePositionedLogicalWidth (this=0x2ae12cc34010, computed_values=...) at ../../third_party/blink/renderer/core/layout/layout_replaced.cc:245
#3  0x00007fffe28766ca in blink::LayoutBox::ComputeLogicalWidth (this=0x2ae12cc34010, computed_values=...) at ../../third_party/blink/renderer/core/layout/layout_box.cc:2720
#4  0x00007fffe2876461 in blink::LayoutBox::UpdateLogicalWidth (this=0x2ae12cc34010) at ../../third_party/blink/renderer/core/layout/layout_box.cc:2662
#5  0x00007fffe2ae6ef5 in blink::LayoutSVGRoot::UpdateLayout (this=0x2ae12cc34010) at ../../third_party/blink/renderer/core/layout/svg/layout_svg_root.cc:166
#6  0x00007fffe27f49f6 in blink::LayoutObject::LayoutIfNeeded (this=0x2ae12cc34010) at ../../third_party/blink/renderer/core/layout/layout_object.h:1305
#7  0x00007fffe281c6a7 in blink::LayoutBlock::LayoutPositionedObject (this=0x2ae12cc24270, positioned_object=0x2ae12cc34010, relayout_children=true, info=blink::LayoutBlock::kDefaultLayout) at ../../third_party/blink/renderer/core/layout/layout_block.cc:928
#8  0x00007fffe281c34f in blink::LayoutBlock::LayoutPositionedObjects (this=0x2ae12cc24270, relayout_children=true, info=blink::LayoutBlock::kDefaultLayout) at ../../third_party/blink/renderer/core/layout/layout_block.cc:850
#9  0x00007fffe2830632 in blink::LayoutBlockFlow::UpdateBlockLayout (this=0x2ae12cc24270, relayout_children=true) at ../../third_party/blink/renderer/core/layout/layout_block_flow.cc:525

I think this bug is SVG-specific, since, to my knowledge, that's the only type of replaced content that doesn't always have an intrinsic size (but may only have an intrinsic ratio, or not even that). Cc'ing fs, who might have some input. First of all - is my attached test case valid? Fails in both legacy and NG, albeit differently so.
I guess this is the "both width and height compute to auto and there's only an intrinsic ratio" case? (Which used to undefined.) The Gecko rendering looks like what I'd expect given that.

As for the "early" SetIntrinsicSize, I think that was added to workaround some issue at the time (with computing preferred widths before having updated the intrinsic dimensions? or something like that), and maybe it can be removed nowadays.
Looking at this some more, I'm starting to form the opinion that Legacy is correct. (For replaced element you'd never end solving for 'width' - which is what Gecko appears to be doing?)
Those CSS specs, there is always more to learn.

SVG differs from everything else wrt intrinsic sizing. It can have 
no intrinsic size, but will have intrinsic aspect ratio.

I remember seeing separation of intrinsic size and aspect ratio in Legacy code,
and deciding not to implement it because why would I ever need this?
Now I know why.

https://svgwg.org/svg2-draft/coords.html#SizingSVGInCSS


To fix this, replaced size needs to evolve from NGLogicalSize, to 
NGReplacedSizingInfo, which is counterpart to Legacy's 
IntrinsicSizingInfo.

NGReplacedSizingInfo could be:
  Optional<NGLogicalSize> replaced_size;
  Optional<NGLogicalSize> aspect_ratio;

Then abspos code needs to be modified to use this, instead of 
current logical size.

I can do it after printing bug is fixed.

Owner: atotic@chromium.org
Status: Started (was: Available)
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3d282d2073e6f2c781ead975072082dfa8d2e3d5

commit 3d282d2073e6f2c781ead975072082dfa8d2e3d5
Author: Aleks Totic <atotic@chromium.org>
Date: Mon Dec 31 17:46:20 2018

[LayoutNG] Fix SVG replaced element sizes

The old code made an incorrect assumption that if Legacy's
IntrinsicSizingInfo did not compute width/height, the fallback
should be LayoutBox::IntrinsicSize.

I thought I was being clever, and was trying to keep sizing computation
logic out of LayoutBox. Unfortunately, not enough information
is exposed by LayoutBox to compute ReplacedSize from raw
IntrinsicSize, and IntrinsicSizingInfo must be used.

The correct fix is to use ConstraintSpace::AvailableSize if
IntrinsicSizingInfo is unavailable.

Bug:  916387 
Change-Id: Ib58bbd4775aa9fe1129bc5613d1652123b12670e
Reviewed-on: https://chromium-review.googlesource.com/c/1392367
Commit-Queue: Aleks Totic <atotic@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619312}
[modify] https://crrev.com/3d282d2073e6f2c781ead975072082dfa8d2e3d5/third_party/blink/renderer/core/layout/ng/ng_layout_input_node.cc
[modify] https://crrev.com/3d282d2073e6f2c781ead975072082dfa8d2e3d5/third_party/blink/renderer/core/layout/ng/ng_layout_input_node.h
[modify] https://crrev.com/3d282d2073e6f2c781ead975072082dfa8d2e3d5/third_party/blink/renderer/core/layout/ng/ng_length_utils.cc
[add] https://crrev.com/3d282d2073e6f2c781ead975072082dfa8d2e3d5/third_party/blink/web_tests/external/wpt/svg/coordinate-systems/abspos.html
[add] https://crrev.com/3d282d2073e6f2c781ead975072082dfa8d2e3d5/third_party/blink/web_tests/external/wpt/svg/coordinate-systems/support/abspos-ref.html

Status: Fixed (was: Started)

Sign in to add a comment