[LayoutNG] Don't update a block's intrinsic block size prematurely.

Assuming that every child block end offset contribute to intrinsic block size of the container is wrong. There may be negative margins. Calculate it after child layout, rather than during child layout for each in-flow child. Some extra care was required for NG block fragmentation, because there was code that updated (and checked) the intrinsic size rather than logical block offset. Using intrinsic size will no longer get us anywhere, since it's now only updated at the end of container layout, not during child layout. Move the initialization of intrinsic block size (set to content edge) after layout instead of before layout, to reduce variable span. We may eventually want to change it to a local variable instead of a member. Fixes 9 tests, 7 of which were not marked as failing, but rather had incorrect NG-specific baselines. Also wrote 2 new tests that test this fix a bit more directly. Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng Change-Id: Id57ba18b70f0cc8569deeff63fa5547cb8c44c98 Reviewed-on: https://chromium-review.googlesource.com/1100473Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Cr-Commit-Position: refs/heads/master@{#567429}

[LayoutNG] Don't update a block's intrinsic block size prematurely.
Assuming that every child block end offset contribute to intrinsic block size of the container is wrong. There may be negative margins. Calculate it after child layout, rather than during child layout for each in-flow child. Some extra care was required for NG block fragmentation, because there was code that updated (and checked) the intrinsic size rather than logical block offset. Using intrinsic size will no longer get us anywhere, since it's now only updated at the end of container layout, not during child layout. Move the initialization of intrinsic block size (set to content edge) after layout instead of before layout, to reduce variable span. We may eventually want to change it to a local variable instead of a member. Fixes 9 tests, 7 of which were not marked as failing, but rather had incorrect NG-specific baselines. Also wrote 2 new tests that test this fix a bit more directly. Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng Change-Id: Id57ba18b70f0cc8569deeff63fa5547cb8c44c98 Reviewed-on: https://chromium-review.googlesource.com/1100473Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Cr-Commit-Position: refs/heads/master@{#567429}
103877dd · Morten Stenshorne · Commit Bot · 385b5a51 · 103877dd · 103877dd
Commit 103877dd authored Jun 14, 2018 by Morten Stenshorne Committed by Commit Bot Jun 14, 2018
13 changed files
--- a/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
+++ b/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
@@ -108,7 +108,6 @@ crbug.com/591099 compositing/squashing/add-remove-squashed-layers.html [ Failure
 crbug.com/591099 css1/box_properties/float_on_text_elements.html [ Failure ]
 crbug.com/591099 css1/classification/list_style_image.html [ Failure ]
 crbug.com/591099 css2.1/20110323/margin-applies-to-008.htm [ Failure ]
-crbug.com/591099 css2.1/20110323/table-caption-margins-001.htm [ Failure ]
 crbug.com/591099 css2.1/t0510-c25-pseudo-elmnt-00-c.html [ Failure ]
 crbug.com/591099 css2.1/t0905-c414-flt-wrap-01-d-g.html [ Failure ]
 crbug.com/591099 css2.1/t100801-c544-valgn-00-a-ag.html [ Failure ]
@@ -152,7 +151,6 @@ crbug.com/591099 external/wpt/css/CSS2/normal-flow/block-in-inline-insert-001h.x
 crbug.com/591099 external/wpt/css/CSS2/normal-flow/block-in-inline-insert-002e.xht [ Pass ]
 crbug.com/591099 external/wpt/css/CSS2/normal-flow/block-in-inline-nested-002.xht [ Pass ]
 crbug.com/591099 external/wpt/css/CSS2/normal-flow/block-in-inline-remove-006.xht [ Pass ]
-crbug.com/591099 external/wpt/css/CSS2/normal-flow/root-box-001.xht [ Failure ]
 crbug.com/591099 external/wpt/css/CSS2/text/white-space-mixed-003.xht [ Pass ]
 crbug.com/591099 external/wpt/css/css-backgrounds/box-shadow-syntax-001.xht [ Failure ]
 crbug.com/591099 external/wpt/css/css-filter/filtered-inline-is-container.html [ Crash ]

--- a/third_party/WebKit/LayoutTests/TestExpectations
+++ b/third_party/WebKit/LayoutTests/TestExpectations
@@ -303,9 +303,6 @@ crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/normal-flow/block-in-in
 crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/normal-flow/block-in-inline-insert-001f.xht [ Failure ]
 crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/normal-flow/block-in-inline-insert-002f.xht [ Failure ]
-# Block: margin collapsing.
-crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/normal-flow/root-box-001.xht [ Failure ]
 # Tables.
 crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/normal-flow/max-width-applies-to-005.xht [ Failure ]
 crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/normal-flow/max-width-applies-to-006.xht [ Failure ]

--- a/third_party/WebKit/LayoutTests/external/wpt/css/CSS2/normal-flow/negative-margin-shrinking-container-size-001.html
+++ b/third_party/WebKit/LayoutTests/external/wpt/css/CSS2/normal-flow/negative-margin-shrinking-container-size-001.html
+<!DOCTYPE html>
+<title>Last block with negative in container that reduces container height</title>
+<link rel="author" title="Morten Stenshorne" href="mstensho@chromium.org">
+<link rel="help" href="https://www.w3.org/TR/CSS22/box.html#collapsing-margins" title="8.3.1 Collapsing margins">
+<link rel="match" href="../../reference/ref-filled-green-200px-square.html">
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div style="width:200px; background:red;">
+  <div style="height:200px;">
+    <div style="height:80px; background:green;"></div>
+  </div>
+  <div style="margin-top:-120px; height:20px; background:green;"></div>
+</div>
+<div style="width:200px; height:100px; background:green;"></div>
--- a/third_party/WebKit/LayoutTests/external/wpt/css/CSS2/normal-flow/negative-margin-shrinking-container-size-002.html
+++ b/third_party/WebKit/LayoutTests/external/wpt/css/CSS2/normal-flow/negative-margin-shrinking-container-size-002.html
+<!DOCTYPE html>
+<title>Last block with negative in container that reduces container height</title>
+<link rel="author" title="Morten Stenshorne" href="mstensho@chromium.org">
+<link rel="help" href="https://www.w3.org/TR/CSS22/box.html#collapsing-margins" title="8.3.1 Collapsing margins">
+<link rel="match" href="../../reference/ref-filled-green-200px-square.html">
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div style="overflow:hidden; width:200px; background:red;">
+  <div style="height:200px; background:green;"></div>
+  <div style="height:200px; margin-bottom:-200px;"></div>
+</div>
--- a/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/css2.1/t0803-c5505-mrgn-02-c-expected.txt
+++ b/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/css2.1/t0803-c5505-mrgn-02-c-expected.txt
 layer at (0,0) size 800x600 clip at (0,0) size 800x585 scrollWidth 802
  LayoutView at (0,0) size 800x600
-layer at (0,0) size 800x46
+layer at (0,0) size 800x36
-  LayoutNGBlockFlow {HTML} at (0,0) size 800x46
+  LayoutNGBlockFlow {HTML} at (0,0) size 800x36
    LayoutNGBlockFlow {BODY} at (8,-2) size 784x40
      LayoutNGBlockFlow {P} at (-10,0) size 804x40 [color=#000080]
        LayoutBR {BR} at (0,0) size 0x0

--- a/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/background-repeat-space-padding-box-expected.txt
+++ b/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/background-repeat-space-padding-box-expected.txt
 layer at (0,0) size 800x600
  LayoutView at (0,0) size 800x600
 layer at (0,0) size 800x438
-  LayoutNGBlockFlow {HTML} at (0,0) size 800x438.39
+  LayoutNGBlockFlow {HTML} at (0,0) size 800x437.78
-    LayoutNGBlockFlow {BODY} at (8,16) size 784x414.39
+    LayoutNGBlockFlow {BODY} at (8,16) size 784x413.78
      LayoutNGBlockFlow {P} at (0,0) size 784x20
        LayoutText {#text} at (0,0) size 302x19
          text run at (0,0) width 302: "Test passes if there is no red visible on the page."

--- a/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/background-size-002-expected.txt
+++ b/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/background-size-002-expected.txt
 layer at (0,0) size 800x600
  LayoutView at (0,0) size 800x600
-layer at (0,0) size 800x160
+layer at (0,0) size 800x110
-  LayoutNGBlockFlow {HTML} at (0,0) size 800x160
+  LayoutNGBlockFlow {HTML} at (0,0) size 800x110
-    LayoutNGBlockFlow {BODY} at (8,16) size 784x136
+    LayoutNGBlockFlow {BODY} at (8,16) size 784x86
      LayoutNGBlockFlow {P} at (0,0) size 784x20
        LayoutText {#text} at (0,0) size 302x19
          text run at (0,0) width 302: "Test passes if there is no red visible on the page."

--- a/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/border-radius-clip-002-expected.txt
+++ b/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/border-radius-clip-002-expected.txt
 layer at (0,0) size 800x600
  LayoutView at (0,0) size 800x600
-layer at (0,0) size 800x200
+layer at (0,0) size 800x180
-  LayoutNGBlockFlow {HTML} at (0,0) size 800x200
+  LayoutNGBlockFlow {HTML} at (0,0) size 800x180
-    LayoutNGBlockFlow {BODY} at (8,16) size 784x176
+    LayoutNGBlockFlow {BODY} at (8,16) size 784x156
      LayoutNGBlockFlow {P} at (0,0) size 784x20
        LayoutText {#text} at (0,0) size 302x19
          text run at (0,0) width 302: "Test passes if there is no red visible on the page."

--- a/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/border-radius-with-three-values-001-expected.txt
+++ b/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/border-radius-with-three-values-001-expected.txt
 layer at (0,0) size 800x600
  LayoutView at (0,0) size 800x600
 layer at (0,0) size 800x290
-  LayoutNGBlockFlow {HTML} at (0,0) size 800x290.38
+  LayoutNGBlockFlow {HTML} at (0,0) size 800x290.36
-    LayoutNGBlockFlow {BODY} at (8,16) size 784x266.38
+    LayoutNGBlockFlow {BODY} at (8,16) size 784x266.36
      LayoutNGBlockFlow {P} at (0,0) size 784x20
        LayoutText {#text} at (0,0) size 511x19
          text run at (0,0) width 511: "Test passes if the box has rounded corners and there is no red visible on the page."

--- a/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/border-radius-with-two-values-001-expected.txt
+++ b/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/border-radius-with-two-values-001-expected.txt
 layer at (0,0) size 800x600
  LayoutView at (0,0) size 800x600
 layer at (0,0) size 800x290
-  LayoutNGBlockFlow {HTML} at (0,0) size 800x290.38
+  LayoutNGBlockFlow {HTML} at (0,0) size 800x290.36
-    LayoutNGBlockFlow {BODY} at (8,16) size 784x266.38
+    LayoutNGBlockFlow {BODY} at (8,16) size 784x266.36
      LayoutNGBlockFlow {P} at (0,0) size 784x20
        LayoutText {#text} at (0,0) size 511x19
          text run at (0,0) width 511: "Test passes if the box has rounded corners and there is no red visible on the page."

--- a/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/box-shadow-002-expected.txt
+++ b/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/ietestcenter/css3/bordersbackgrounds/box-shadow-002-expected.txt
 layer at (0,0) size 800x600
  LayoutView at (0,0) size 800x600
-layer at (0,0) size 800x180
+layer at (0,0) size 800x160
-  LayoutNGBlockFlow {HTML} at (0,0) size 800x180
+  LayoutNGBlockFlow {HTML} at (0,0) size 800x160
-    LayoutNGBlockFlow {BODY} at (8,16) size 784x156
+    LayoutNGBlockFlow {BODY} at (8,16) size 784x136
      LayoutNGBlockFlow {P} at (0,0) size 784x20
        LayoutText {#text} at (0,0) size 302x19
          text run at (0,0) width 302: "Test passes if there is no red visible on the page."

--- a/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.cc
+++ b/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.cc
@@ -437,8 +437,6 @@ scoped_refptr<NGLayoutResult> NGBlockLayoutAlgorithm::Layout() {
  if (node_.IsQuirkyContainer())
    previous_inflow_position.margin_strut.is_quirky_container_start = true;
-  intrinsic_block_size_ = content_edge;
  // Before we descend into children (but after we have determined our inline
  // size), give the autosizer an opportunity to adjust the font size on the
  // children.
@@ -483,7 +481,9 @@ scoped_refptr<NGLayoutResult> NGBlockLayoutAlgorithm::Layout() {
        // We need to abort the layout, as our BFC offset was resolved.
        return container_builder_.Abort(NGLayoutResult::kBfcOffsetResolved);
      }
-      if (container_builder_.DidBreak() && IsFragmentainerOutOfSpace())
+      if (container_builder_.DidBreak() &&
+          IsFragmentainerOutOfSpace(
+              previous_inflow_position.logical_block_offset))
        break;
      has_processed_first_child_ = true;
    }
@@ -491,6 +491,10 @@ scoped_refptr<NGLayoutResult> NGBlockLayoutAlgorithm::Layout() {
  NGMarginStrut end_margin_strut = previous_inflow_position.margin_strut;
+  // The intrinsic block size is not allowed to be less than the content edge
+  // offset, as that could give us a negative content box size.
+  intrinsic_block_size_ = content_edge;
  // If the current layout is a new formatting context, we need to encapsulate
  // all of our floats.
  if (ConstraintSpace().IsNewFormattingContext()) {
@@ -543,6 +547,12 @@ scoped_refptr<NGLayoutResult> NGBlockLayoutAlgorithm::Layout() {
    intrinsic_block_size_ += border_scrollbar_padding_.block_end;
    end_margin_strut = NGMarginStrut();
+  } else {
+    // Update our intrinsic block size to be just past the block-end border edge
+    // of the last in-flow child. The pending margin is to be propagated to our
+    // container, so ignore it.
+    intrinsic_block_size_ = std::max(
+        intrinsic_block_size_, previous_inflow_position.logical_block_offset);
  }
  // Recompute the block-axis size now that we know our content size.
@@ -839,8 +849,8 @@ bool NGBlockLayoutAlgorithm::HandleNewFormattingContext(
        child_margin_got_separated ||
        child_bfc_offset.block_offset > child_bfc_offset_estimate ||
        layout_result->IsPushedByFloats();
-    if (BreakBeforeChild(child, *layout_result, logical_offset.block_offset,
+    if (BreakBeforeChild(child, *layout_result, previous_inflow_position,
-                         is_pushed_by_floats))
+                         logical_offset.block_offset, is_pushed_by_floats))
      return true;
    EBreakBetween break_after = JoinFragmentainerBreakValues(
        layout_result->FinalBreakAfter(), child.Style().BreakAfter());
@@ -849,10 +859,6 @@ bool NGBlockLayoutAlgorithm::HandleNewFormattingContext(
  PositionOrPropagateListMarker(*layout_result, &logical_offset);
-  intrinsic_block_size_ =
-      std::max(intrinsic_block_size_,
-               logical_offset.block_offset + fragment.BlockSize());
  container_builder_.AddChild(layout_result, logical_offset);
  container_builder_.PropagateBreak(layout_result);
@@ -1177,7 +1183,8 @@ bool NGBlockLayoutAlgorithm::HandleInflow(
      child, fragment, child_data.margins, child_bfc_offset);
  if (ConstraintSpace().HasBlockFragmentation()) {
-    if (BreakBeforeChild(child, *layout_result, logical_offset.block_offset,
+    if (BreakBeforeChild(child, *layout_result, previous_inflow_position,
+                         logical_offset.block_offset,
                         layout_result->IsPushedByFloats()))
      return true;
    EBreakBetween break_after = JoinFragmentainerBreakValues(
@@ -1187,20 +1194,7 @@ bool NGBlockLayoutAlgorithm::HandleInflow(
  PositionOrPropagateListMarker(*layout_result, &logical_offset);
-  // Only modify intrinsic_block_size_ if the fragment is non-empty block.
+  if (is_empty_block && !container_builder_.BfcOffset()) {
-  //
-  // Empty blocks don't immediately contribute to our size, instead we wait to
-  // see what the final margin produced, e.g.
-  // <div style="display: flow-root">
-  //   <div style="margin-top: -8px"></div>
-  //   <div style="margin-top: 10px"></div>
-  // </div>
-  if (!is_empty_block) {
-    DCHECK(container_builder_.BfcOffset());
-    intrinsic_block_size_ =
-        std::max(intrinsic_block_size_,
-                 logical_offset.block_offset + fragment.BlockSize());
-  } else if (!container_builder_.BfcOffset()) {
    container_builder_.AddAdjoiningFloatTypes(
        layout_result->AdjoiningFloatTypes());
  }
@@ -1376,12 +1370,13 @@ LayoutUnit NGBlockLayoutAlgorithm::FragmentainerSpaceAvailable() const {
         container_builder_.BfcOffset()->block_offset;
 }
-bool NGBlockLayoutAlgorithm::IsFragmentainerOutOfSpace() const {
+bool NGBlockLayoutAlgorithm::IsFragmentainerOutOfSpace(
+    LayoutUnit block_offset) const {
  if (!ConstraintSpace().HasBlockFragmentation())
    return false;
  if (!container_builder_.BfcOffset().has_value())
    return false;
-  return intrinsic_block_size_ >= FragmentainerSpaceAvailable();
+  return block_offset >= FragmentainerSpaceAvailable();
 }
 void NGBlockLayoutAlgorithm::FinalizeForFragmentation() {
@@ -1464,6 +1459,7 @@ void NGBlockLayoutAlgorithm::FinalizeForFragmentation() {
 bool NGBlockLayoutAlgorithm::BreakBeforeChild(
    NGLayoutInputNode child,
    const NGLayoutResult& layout_result,
+    NGPreviousInflowPosition* previous_inflow_position,
    LayoutUnit block_offset,
    bool is_pushed_by_floats) {
  DCHECK(ConstraintSpace().HasBlockFragmentation());
@@ -1571,7 +1567,8 @@ bool NGBlockLayoutAlgorithm::BreakBeforeChild(
  // content, due to the break) should still be occupied by this container.
  // TODO(mstensho): Figure out if we really need to <0 here. It doesn't seem
  // right to have negative available space.
-  intrinsic_block_size_ = space_available.ClampNegativeToZero();
+  previous_inflow_position->logical_block_offset =
+      space_available.ClampNegativeToZero();
  // Drop the fragment on the floor and retry at the start of the next
  // fragmentainer.
  container_builder_.AddBreakBeforeChild(child);

--- a/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.h
+++ b/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.h
@@ -167,12 +167,16 @@ class CORE_EXPORT NGBlockLayoutAlgorithm
  // Return true if the node being laid out by this fragmentainer has used all
  // the available space in the current fragmentainer.
-  bool IsFragmentainerOutOfSpace() const;
+  // |block_offset| is the border-edge relative block offset we want to check
+  // whether fits within the fragmentainer or not.
+  bool IsFragmentainerOutOfSpace(LayoutUnit block_offset) const;
  // Insert a fragmentainer break before the child if necessary.
-  // Return true if a break was inserted, false otherwise.
+  // Update previous in-flow position and return true if a break was inserted.
+  // Otherwise return false.
  bool BreakBeforeChild(NGLayoutInputNode child,
                        const NGLayoutResult&,
+                        NGPreviousInflowPosition*,
                        LayoutUnit block_offset,
                        bool is_pushed_by_floats);