[css-flex] New max-content width algorithm for column wrap flexboxes

min-content width algorithm is unchanged, which matches old behavior and what was suggested on w3c/csswg-drafts#6777, but I'm reserving the right to modify it to reduce the frequency of overflow. Bug: 240765 Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740999 Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Commit-Queue: David Grogan <dgrogan@chromium.org> Cr-Commit-Position: refs/heads/main@{#1033350} NOKEYCHECK=True GitOrigin-RevId: 3447e6fff1473a2f7b66d1c70eef9d1a195e8de5
mjfroman · Aug 10, 2022 · bc2bc96 · bc2bc96
1 parent b09511a
commit bc2bc96
Show file tree

Hide file tree

Showing 12 changed files with 408 additions and 20 deletions.
diff --git a/blink/renderer/core/layout/ng/flex/ng_flex_layout_algorithm.cc b/blink/renderer/core/layout/ng/flex/ng_flex_layout_algorithm.cc
@@ -349,6 +349,19 @@ bool NGFlexLayoutAlgorithm::WillChildCrossSizeBeContainerCrossSize(
          DoesItemStretch(child);
 }
 
+NGConstraintSpace NGFlexLayoutAlgorithm::BuildSpaceForIntrinsicInlineSize(
+    const NGBlockNode& child) const {
+  NGMinMaxConstraintSpaceBuilder builder(ConstraintSpace(), Style(), child,
+                                         /* is_new_fc */ true);
+  builder.SetAvailableBlockSize(ChildAvailableSize().block_size);
+  builder.SetPercentageResolutionBlockSize(child_percentage_size_.block_size);
+  builder.SetReplacedPercentageResolutionBlockSize(
+      child_percentage_size_.block_size);
+  if (!is_column_ && WillChildCrossSizeBeContainerCrossSize(child))
+    builder.SetBlockAutoBehavior(NGAutoBehavior::kStretchExplicit);
+  return builder.ToConstraintSpace();
+}
+
 NGConstraintSpace NGFlexLayoutAlgorithm::BuildSpaceForIntrinsicBlockSize(
     const NGBlockNode& flex_item) const {
   const ComputedStyle& child_style = flex_item.Style();
@@ -2129,16 +2142,8 @@ NGFlexLayoutAlgorithm::FindLargestFractions() const {
   FlexFractionParts max_content_largest_fraction(Style());
   for (const FlexItem& item : algorithm_.all_items_) {
     const NGBlockNode& child = item.ng_input_node_;
-    NGMinMaxConstraintSpaceBuilder builder(ConstraintSpace(), Style(), child,
-                                           /* is_new_fc */ true);
-    builder.SetAvailableBlockSize(ChildAvailableSize().block_size);
-    builder.SetPercentageResolutionBlockSize(child_percentage_size_.block_size);
-    builder.SetReplacedPercentageResolutionBlockSize(
-        child_percentage_size_.block_size);
-    if (!is_column_ && WillChildCrossSizeBeContainerCrossSize(child))
-      builder.SetBlockAutoBehavior(NGAutoBehavior::kStretchExplicit);
-    const NGConstraintSpace space = builder.ToConstraintSpace();
 
+    const NGConstraintSpace space = BuildSpaceForIntrinsicInlineSize(child);
     const MinMaxSizesResult min_max_content_contributions =
         ComputeItemContributions(space, item);
     depends_on_block_constraints |=
@@ -2156,6 +2161,54 @@ NGFlexLayoutAlgorithm::FindLargestFractions() const {
                     depends_on_block_constraints);
 }
 
+MinMaxSizesResult
+NGFlexLayoutAlgorithm::ComputeMinMaxSizeOfMultilineColumnContainer() {
+  NGFlexChildIterator iterator(Node());
+  MinMaxSizes largest_inline_size_contributions;
+  for (NGBlockNode child = iterator.NextChild(); child;
+       child = iterator.NextChild()) {
+    if (child.IsOutOfFlowPositioned())
+      continue;
+
+    auto space = BuildSpaceForIntrinsicInlineSize(child);
+    MinMaxSizesResult child_contributions =
+        ComputeMinAndMaxContentContribution(Style(), child, space);
+    NGBoxStrut child_margins =
+        ComputeMarginsFor(space, child.Style(), ConstraintSpace());
+    child_contributions.sizes += child_margins.InlineSum();
+
+    largest_inline_size_contributions.Encompass(child_contributions.sizes);
+  }
+
+  // The algorithm for determining the max-content width of a column-wrap
+  // container is simply: Run layout on the container but give the items an
+  // overridden available size, equal to the largest max-content width of any
+  // item, when they are laid out. The container's max-content width is then
+  // the farthest outer inline-end point of all the items.
+  item_inline_available_size_override_ =
+      largest_inline_size_contributions.max_size;
+  HeapVector<NGFlexLine> flex_line_outputs;
+  HeapVector<Member<LayoutBox>> dummy_oof_children;
+  PlaceFlexItems(&flex_line_outputs, &dummy_oof_children);
+  if (!flex_line_outputs.IsEmpty()) {
+    largest_inline_size_contributions.max_size =
+        flex_line_outputs.back().line_cross_size +
+        flex_line_outputs.back().cross_axis_offset -
+        flex_line_outputs.front().cross_axis_offset;
+  }
+
+  DCHECK_GE(largest_inline_size_contributions.min_size, 0);
+  DCHECK_LE(largest_inline_size_contributions.min_size,
+            largest_inline_size_contributions.max_size);
+
+  largest_inline_size_contributions += BorderScrollbarPadding().InlineSum();
+
+  // This always depends on block constraints because if block constraints
+  // change, this flexbox could get a different number of columns.
+  return {largest_inline_size_contributions,
+          /* depends_on_block_constraints */ true};
+}
+
 MinMaxSizesResult
 NGFlexLayoutAlgorithm::ComputeMinMaxSizeOfSingleLineRowContainer() {
   ConstructAndAppendFlexItems(/*is_computing_intrinsic_size*/ true);
@@ -2220,7 +2273,7 @@ MinMaxSizesResult NGFlexLayoutAlgorithm::ComputeMinMaxSizes(
     // TODO(crbug.com/240765): Implement all the cases here.
     if (is_column_) {
       if (algorithm_.IsMultiline()) {
-        // multiline column flexbox
+        return ComputeMinMaxSizeOfMultilineColumnContainer();
       } else {
         // singleline column flexbox
       }
@@ -2242,16 +2295,7 @@ MinMaxSizesResult NGFlexLayoutAlgorithm::ComputeMinMaxSizes(
       continue;
     number_of_items++;
 
-    NGMinMaxConstraintSpaceBuilder builder(ConstraintSpace(), Style(), child,
-                                           /* is_new_fc */ true);
-    builder.SetAvailableBlockSize(ChildAvailableSize().block_size);
-    builder.SetPercentageResolutionBlockSize(child_percentage_size_.block_size);
-    builder.SetReplacedPercentageResolutionBlockSize(
-        child_percentage_size_.block_size);
-    if (!is_column_ && WillChildCrossSizeBeContainerCrossSize(child))
-      builder.SetBlockAutoBehavior(NGAutoBehavior::kStretchExplicit);
-    const auto space = builder.ToConstraintSpace();
-
+    const NGConstraintSpace space = BuildSpaceForIntrinsicInlineSize(child);
     MinMaxSizesResult child_result =
         ComputeMinAndMaxContentContribution(Style(), child, space);
     NGBoxStrut child_margins =
@@ -2550,4 +2594,13 @@ void NGFlexLayoutAlgorithm::CheckFlexLines(
 }
 #endif
 
+LogicalSize NGFlexLayoutAlgorithm::ChildAvailableSize() const {
+  LogicalSize available_size = NGLayoutAlgorithm::ChildAvailableSize();
+  if (UNLIKELY(item_inline_available_size_override_.has_value())) {
+    DCHECK_EQ(container_builder_.InlineSize(), kIndefiniteSize);
+    available_size.inline_size = *item_inline_available_size_override_;
+  }
+  return available_size;
+}
+
 }  // namespace blink
diff --git a/blink/renderer/core/layout/ng/flex/ng_flex_layout_algorithm.h b/blink/renderer/core/layout/ng/flex/ng_flex_layout_algorithm.h
@@ -63,6 +63,8 @@ class CORE_EXPORT NGFlexLayoutAlgorithm
   bool IsColumnContainerMainSizeDefinite() const;
   bool IsContainerCrossSizeDefinite() const;
 
+  NGConstraintSpace BuildSpaceForIntrinsicInlineSize(
+      const NGBlockNode& flex_item) const;
   NGConstraintSpace BuildSpaceForFlexBasis(const NGBlockNode& flex_item) const;
   NGConstraintSpace BuildSpaceForIntrinsicBlockSize(
       const NGBlockNode& flex_item) const;
@@ -116,7 +118,9 @@ class CORE_EXPORT NGFlexLayoutAlgorithm
              NGFlexLayoutAlgorithm::FlexFractionParts,
              bool>
   FindLargestFractions() const;
+
   MinMaxSizesResult ComputeMinMaxSizeOfSingleLineRowContainer();
+  MinMaxSizesResult ComputeMinMaxSizeOfMultilineColumnContainer();
   // This implements 9.9.3. Flex Item Intrinsic Size Contributions, from
   // https://drafts.csswg.org/css-flexbox/#intrinsic-item-contributions.
   MinMaxSizesResult ComputeItemContributions(const NGConstraintSpace& space,
@@ -183,6 +187,13 @@ class CORE_EXPORT NGFlexLayoutAlgorithm
   void CheckFlexLines(HeapVector<NGFlexLine>& flex_line_outputs) const;
 #endif
 
+  // These are used when determining the max-content width of a column-wrap flex
+  // container. Note: |item_inline_available_size_override_| has to be
+  // initialized before is_cross_size_definite_, and probably before some other
+  // data members too.
+  LogicalSize ChildAvailableSize() const;
+  absl::optional<LayoutUnit> item_inline_available_size_override_;
+
   const bool is_column_;
   const bool is_horizontal_flow_;
   const bool is_cross_size_definite_;

diff --git a/blink/web_tests/TestExpectations b/blink/web_tests/TestExpectations
@@ -1478,6 +1478,14 @@ crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/row-004.html [ Fail
 crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/row-005.html [ Failure ]
 crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/row-007.html [ Failure ]
 crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/row-008.html [ Failure ]
+crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/col-wrap-001.html [ Failure ]
+crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/col-wrap-002.html [ Failure ]
+crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/col-wrap-003.html [ Failure ]
+crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/col-wrap-005.html [ Failure ]
+crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/col-wrap-006.html [ Failure ]
+crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/col-wrap-007.html [ Failure ]
+crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/col-wrap-008.html [ Failure ]
+crbug.com/240765 external/wpt/css/css-flexbox/intrinsic-size/col-wrap-009.html [ Failure ]
 
 # These require css-align-3 positional keywords that Blink doesn't yet support for flexbox.
 crbug.com/1011718 external/wpt/css/css-flexbox/flexbox-safe-overflow-position-001.html [ Failure ]

diff --git a/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-001.html b/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-001.html
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<link rel="author" title="David Grogan" href="mailto:dgrogan@chromium.org">
+<link rel="help" href="https://drafts.csswg.org/css-flexbox/#intrinsic-sizes">
+<link rel="match" href="../../reference/ref-filled-green-100px-square.xht">
+<meta name="assert"
+  content="column-wrap container's max-content width is big enough that items don't overflow when the container has fixed height and items have fixed width and height. Old algorithm gave max-content width of 50px." />
+
+<style>
+  #reference-overlapped-red {
+    position: absolute;
+    background-color: red;
+    width: 100px;
+    height: 100px;
+    z-index: -1;
+  }
+
+  .item {
+    /* Remove min-height so we don't have to think about it. */
+    min-height: 0px;
+    width: 50px;
+    flex: 0 0 100px
+  }
+</style>
+
+<p>Test passes if there is a filled green square and <strong>no red</strong>.
+</p>
+
+<div id=reference-overlapped-red></div>
+
+<div
+  style="display: flex; flex-flow: column wrap; height: 100px; width: max-content; background: green;"
+  class=flex>
+  <div class="item"></div>
+  <div class="item"></div>
+</div>
diff --git a/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-002.html b/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-002.html
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<link rel="author" title="David Grogan" href="mailto:dgrogan@chromium.org">
+<link rel="help" href="https://drafts.csswg.org/css-flexbox/#intrinsic-sizes">
+<link rel="match" href="../../reference/ref-filled-green-100px-square.xht">
+<meta name="assert"
+  content="column-wrap container's max-content width is big enough that items don't overflow when the container has indefinite height but fixed max-height and items have fixed width and height. Old algorithm gave max-content width of 50px." />
+
+<style>
+  #reference-overlapped-red {
+    position: absolute;
+    background-color: red;
+    width: 100px;
+    height: 100px;
+    z-index: -1;
+  }
+
+  .item {
+    /* Remove min-height so we don't have to think about it. */
+    min-height: 0px;
+    width: 50px;
+    flex: 0 0 100px
+  }
+</style>
+
+<p>Test passes if there is a filled green square and <strong>no red</strong>.
+</p>
+
+<div id=reference-overlapped-red></div>
+
+<div
+  style="display: flex; flex-flow: column wrap; max-height: 100px; width: max-content; background: green;"
+  class=flex>
+  <div class="item"></div>
+  <div class="item"></div>
+</div>
diff --git a/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-003.html b/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-003.html
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<link rel="author" title="David Grogan" href="mailto:dgrogan@chromium.org">
+<link rel="help" href="https://drafts.csswg.org/css-flexbox/#intrinsic-sizes">
+<link rel="match" href="../../reference/ref-filled-green-100px-square.xht">
+<meta name="assert"
+  content="Cross-axis borders are accounted for when determining inline content sizes of column-wrap flexboxes." />
+
+<style>
+  #reference-overlapped-red {
+    position: absolute;
+    background-color: red;
+    width: 100px;
+    height: 100px;
+    z-index: -1;
+  }
+
+  .item {
+    /* Remove min-height so we don't have to think about it. */
+    min-height: 0px;
+    width: 40px;
+    flex: 0 0 100px;
+  }
+</style>
+
+<p>Test passes if there is a filled green square and <strong>no red</strong>.
+</p>
+
+<div id=reference-overlapped-red></div>
+
+<div
+  style="display: flex; flex-flow: column wrap; max-height: 100px; width: max-content; background: green; padding-left: 5px; border-left: 9px solid green; border-right: 6px solid green;">
+  <div class="item"></div>
+  <div class="item"></div>
+</div>
diff --git a/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-004.html b/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-004.html
@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<link rel="author" title="David Grogan" href="mailto:dgrogan@chromium.org">
+<link rel="help" href="https://drafts.csswg.org/css-flexbox/#intrinsic-sizes">
+<link rel="match" href="../../reference/ref-filled-green-100px-square.xht">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/check-layout-th.js"></script>
+<meta name="assert"
+  content="Item with width:100% doesn't contribute to max-content size but does get 100px width after final layout. The item also overflows the container." />
+
+<style>
+  .item {
+    /* Remove min-height so we don't have to think about it. */
+    min-height: 0px;
+    flex: 0 0 100px;
+    outline: 1px solid;
+  }
+</style>
+
+<div
+  style="display: flex; flex-flow: column wrap; width: max-content; height: 100px; background: green; position: relative;"
+  data-expected-width="100">
+  <div class="item" style="width: 100px;"></div>
+  <div class="item" style="width: 100%;" data-expected-width="100"
+    data-offset-x="100"></div>
+</div>
+
+<script>
+  checkLayout('body > div');
+</script>
diff --git a/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-005.html b/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-005.html
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<link rel="author" title="David Grogan" href="mailto:dgrogan@chromium.org">
+<link rel="help" href="https://drafts.csswg.org/css-flexbox/#intrinsic-sizes">
+<link rel="match" href="../../reference/ref-filled-green-100px-square.xht">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/check-layout-th.js"></script>
+<meta name="assert"
+  content="The item still has sufficient available size at the time fit-content is resolved. (A bug in the code could cause the right-most item to have a width of 75px.)">
+
+<style>
+  .item {
+    /* Remove min-height so we don't have to think about it. */
+    min-height: 0px;
+    flex: 0 0 100px;
+    outline: 1px solid;
+  }
+
+  .grandchild {
+    width: 75px;
+    float: left;
+  }
+</style>
+
+<div
+  style="display: flex; flex-flow: column wrap; width: max-content; height: 100px; background: green; position: relative;"
+  data-expected-width="250">
+  <div class="item" style="width: 100px;"></div>
+  <div class="item" style="width: fit-content;" data-expected-width="150"
+    data-offset-x="100">
+    <!-- This item has min-content=75 and max-content=150. -->
+    <div class="grandchild"></div>
+    <div class="grandchild"></div>
+  </div>
+</div>
+
+<script>
+  checkLayout('body > div');
+</script>
diff --git a/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-006.html b/blink/web_tests/external/wpt/css/css-flexbox/intrinsic-size/col-wrap-006.html
@@ -0,0 +1,37 @@
+<!DOCTYPE html>
+<link rel="author" title="David Grogan" href="mailto:dgrogan@chromium.org">
+<link rel="help" href="https://drafts.csswg.org/css-flexbox/#intrinsic-sizes">
+<link rel="match" href="../../reference/ref-filled-green-100px-square.xht">
+<meta name="assert"
+  content="flex item order is accounted for when determining inline content sizes of column-wrap flexboxes.">
+
+<style>
+  #reference-overlapped-red {
+    position: absolute;
+    background-color: red;
+    width: 100px;
+    height: 100px;
+    z-index: -1;
+  }
+
+  .item {
+    /* Remove min-height so we don't have to think about it. */
+    min-height: 0px;
+    width: 50px;
+    flex: 0 0 auto;
+  }
+
+</style>
+
+<p>Test passes if there is a filled green square and <strong>no red</strong>.
+</p>
+
+<div id=reference-overlapped-red></div>
+
+<!--  An implementation that ignored order could make the max-content size of this flexbox 150px. -->
+<div
+  style="display: flex; flex-flow: column wrap; width: max-content; height: 100px; background: green;">
+  <div class="item" style="height: 90px; order: 0;"></div>
+  <div class="item" style="height: 100px; order: 2;"></div>
+  <div class="item" style="height: 10px; order: 1;"></div>
+</div>