[LayoutNG] Containing block for replaced elements incorrect. |
||||
Issue descriptionSee: 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
,
Dec 19
Looks like legacy layout has some problems here as well. Compare the attached test with how it renders in Firefox.
,
Dec 19
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.
,
Dec 19
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.
,
Dec 19
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?)
,
Dec 19
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.
,
Dec 28
,
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
,
Dec 31
|
||||
►
Sign in to add a comment |
||||
Comment 1 by mstensho@chromium.org
, Dec 19Summary: [LayoutNG] Containing block for replaced elements incorrect. (was: [LayoutNG] Containing block for replacced elements incorrect.)