[layoutng] Handle overflow: auto more correctly

We need to relayout whenever scrollbars change. The change in
https://chromium-review.googlesource.com/1162657 was not quite correct.

The change to CopyFragmentDataToLayoutBox is necessary to not break
virtual/layout_ng_experimental/fragmentation/auto-overflow.html

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I589d6a1bd1b3f06224e6c4aeae3e7cdf163206cc
Reviewed-on: https://chromium-review.googlesource.com/1178804Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584316}

third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG

@@ -65,7 +65,6 @@ crbug.com/591099 compositing/iframes/floating-self-painting-frame.html [ Failure
 crbug.com/591099 compositing/layer-creation/overlap-animation.html [ Crash ]
 crbug.com/869265 compositing/layer-creation/stacking-context-overlap-nested.html [ Failure ]
 crbug.com/591099 css3/filters/composited-layer-child-bounds-after-composited-to-sw-shadow-change.html [ Failure ]
-crbug.com/591099 css3/flexbox/bug646288.html [ Failure ]
 crbug.com/591099 css3/flexbox/flex-flow-margins-auto-size.html [ Failure ]
 crbug.com/591099 css3/flexbox/intrinsic-width-orthogonal-writing-mode.html [ Failure ]
 crbug.com/591099 css3/flexbox/line-wrapping.html [ Failure ]

third_party/blink/renderer/core/layout/ng/geometry/ng_box_strut.cc

@@ -18,6 +18,10 @@ bool NGBoxStrut::operator==(const NGBoxStrut& other) const {
          std::tie(inline_start, inline_end, block_start, block_end);
 }
 
+bool NGBoxStrut::operator!=(const NGBoxStrut& other) const {
+  return !(*this == other);
+}
+
 NGPhysicalBoxStrut NGBoxStrut::ConvertToPhysical(
     WritingMode writing_mode,
     TextDirection direction) const {

third_party/blink/renderer/core/layout/ng/geometry/ng_box_strut.h

@@ -77,6 +77,7 @@ struct CORE_EXPORT NGBoxStrut {
   }
 
   bool operator==(const NGBoxStrut& other) const;
+  bool operator!=(const NGBoxStrut& other) const;
 
   String ToString() const;
 

third_party/blink/renderer/core/layout/ng/ng_block_node.cc

@@ -203,20 +203,40 @@ scoped_refptr<NGLayoutResult> NGBlockNode::Layout(
     box_->ComputePreferredLogicalWidths();
   }
 
-  bool preferred_logical_widths_were_dirty =
-      box_->PreferredLogicalWidthsDirty();
+  NGBoxStrut old_scrollbars = GetScrollbarSizes();
   layout_result = LayoutWithAlgorithm(*this, constraint_space, break_token,
                                       /* ignored */ nullptr);
-  if (!preferred_logical_widths_were_dirty &&
-      box_->PreferredLogicalWidthsDirty()) {
-    // The only thing that should dirty preferred widths at this point is the
-    // addition of overflow:auto scrollbars in a descendant. To avoid a
-    // potential infinite loop, run layout again with auto scrollbars frozen in
-    // their current state.
+
+  FinishLayout(constraint_space, break_token, layout_result);
+  if (old_scrollbars != GetScrollbarSizes()) {
+    // If our scrollbars have changed, we need to relayout because either:
+    // - Our size has changed (if shrinking to fit), or
+    // - Space available to our children has changed.
+    // This mirrors legacy code in PaintLayerScrollableArea::UpdateAfterLayout.
+    // TODO(cbiesinger): It seems that we should also check if
+    // PreferredLogicalWidthsDirty() has changed from false to true during
+    // layout, so that we correctly size ourselves when shrinking to fit
+    // and a child gained a vertical scrollbar. However, no test fails
+    // without that check.
     PaintLayerScrollableArea::FreezeScrollbarsScope freeze_scrollbars;
     layout_result = LayoutWithAlgorithm(*this, constraint_space, break_token,
                                         /* ignored */ nullptr);
+    FinishLayout(constraint_space, break_token, layout_result);
   }
+
+  return layout_result;
+}
+
+void NGBlockNode::FinishLayout(const NGConstraintSpace& constraint_space,
+                               NGBreakToken* break_token,
+                               scoped_refptr<NGLayoutResult> layout_result) {
+  if (!IsBlockLayoutComplete(constraint_space, *layout_result))
+    return;
+
+  // TODO(kojii): Even when we paint fragments, there seem to be some data we
+  // need to copy to LayoutBox. Review if we can minimize the copy.
+  LayoutBlockFlow* block_flow =
+      box_->IsLayoutNGMixin() ? ToLayoutBlockFlow(box_) : nullptr;
   if (block_flow) {
     block_flow->SetCachedLayoutResult(constraint_space, break_token,
                                       layout_result);
@@ -225,27 +245,24 @@ scoped_refptr<NGLayoutResult> NGBlockNode::Layout(
       block_flow->SetPaintFragment(break_token, nullptr);
   }
 
-  if (IsBlockLayoutComplete(constraint_space, *layout_result)) {
-    DCHECK(layout_result->PhysicalFragment());
-
-    if (block_flow && first_child && first_child.IsInline()) {
-      NGBoxStrut scrollbars = GetScrollbarSizes();
-      CopyFragmentDataToLayoutBoxForInlineChildren(
-          ToNGPhysicalBoxFragment(*layout_result->PhysicalFragment()),
-          layout_result->PhysicalFragment()->Size().width -
-              scrollbars.block_start,
-          Style().IsFlippedBlocksWritingMode());
-
-      block_flow->SetPaintFragment(break_token,
-                                   layout_result->PhysicalFragment());
-    }
+  DCHECK(layout_result->PhysicalFragment());
 
-    // TODO(kojii): Even when we paint fragments, there seem to be some data we
-    // need to copy to LayoutBox. Review if we can minimize the copy.
-    CopyFragmentDataToLayoutBox(constraint_space, *layout_result);
+  NGLayoutInputNode first_child = FirstChild();
+  if (block_flow && first_child && first_child.IsInline()) {
+    NGBoxStrut scrollbars = GetScrollbarSizes();
+    CopyFragmentDataToLayoutBoxForInlineChildren(
+        ToNGPhysicalBoxFragment(*layout_result->PhysicalFragment()),
+        layout_result->PhysicalFragment()->Size().width -
+            scrollbars.block_start,
+        Style().IsFlippedBlocksWritingMode());
+
+    block_flow->SetPaintFragment(break_token,
+                                 layout_result->PhysicalFragment());
   }
 
-  return layout_result;
+  // TODO(kojii): Even when we paint fragments, there seem to be some data we
+  // need to copy to LayoutBox. Review if we can minimize the copy.
+  CopyFragmentDataToLayoutBox(constraint_space, *layout_result);
 }
 
 MinMaxSize NGBlockNode::ComputeMinMaxSize(
@@ -415,7 +432,11 @@ void NGBlockNode::CopyFragmentDataToLayoutBox(
   } else {
     DCHECK_EQ(box_->LogicalWidth(), fragment_logical_size.inline_size)
         << "Variable fragment inline size not supported";
-    logical_height = box_->LogicalHeight();
+    logical_height =
+        PreviouslyUsedBlockSpace(constraint_space, physical_fragment);
+    // TODO(layout-ng): We should store this on the break token instead of
+    // relying on previously-stored data. Our relayout in NGBlockNode::Layout
+    // will otherwise lead to wrong data.
     intrinsic_content_logical_height = box_->IntrinsicContentLogicalHeight();
   }
   logical_height += fragment_logical_size.block_size;

third_party/blink/renderer/core/layout/ng/ng_block_node.h

@@ -97,6 +97,10 @@ class CORE_EXPORT NGBlockNode final : public NGLayoutInputNode {
   String ToString() const;
 
  private:
+  void FinishLayout(const NGConstraintSpace&,
+                    NGBreakToken*,
+                    scoped_refptr<NGLayoutResult>);
+
   // After we run the layout algorithm, this function copies back the geometry
   // data to the layout box.
   void CopyFragmentDataToLayoutBox(const NGConstraintSpace&,