Leave inflow position alone when resuming a parallel flow.

ComputeInflowPosition() would reset the margin strut, and thus we'd lose the MarginStrut::discard_margins flag. This flag is set to truncate margins at unforced breaks, and needs to remain true as long as we haven't added any in-flow content to the next fragmentainer. The bug was that the block-start margin of some child would be re-applied after an unforced break, because this flag was reset prematurely. The crash in crbug.com/1494668 was caused by this correctness issue. It caused inline elements to become non-contiguous unexpectedly. E.g. there's a fragment item for a SPAN in the first and the third columns, but not in the second, due to a large margin, that should have been ignored. Bug: 1494668, 1497404 Change-Id: I71f7223b2de703cc8df79d0ee6c7475d812d86b0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4994164 Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Cr-Commit-Position: refs/heads/main@{#1217689}
Denger-Network · Oct 31, 2023 · 9001e79 · 9001e79
1 parent 8c50dee
commit 9001e79
Show file tree

Hide file tree

Showing 8 changed files with 113 additions and 8 deletions.
diff --git 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
@@ -1609,10 +1609,12 @@ NGLayoutResult::EStatus NGBlockLayoutAlgorithm::HandleNewFormattingContext(
   container_builder_.AddResult(*layout_result, logical_offset,
                                resolved_margins);
 
-  *previous_inflow_position = ComputeInflowPosition(
-      *previous_inflow_position, child, child_data,
-      child_bfc_offset.block_offset, logical_offset, *layout_result, fragment,
-      /* self_collapsing_child_had_clearance */ false);
+  if (!child_break_token || !child_break_token->IsInParallelFlow()) {
+    *previous_inflow_position = ComputeInflowPosition(
+        *previous_inflow_position, child, child_data,
+        child_bfc_offset.block_offset, logical_offset, *layout_result, fragment,
+        /* self_collapsing_child_had_clearance */ false);
+  }
 
   if (ConstraintSpace().HasBlockFragmentation() &&
       !has_break_opportunity_before_next_child_) {
@@ -2236,10 +2238,12 @@ NGLayoutResult::EStatus NGBlockLayoutAlgorithm::FinishInflow(
     container_builder_.AddResult(*layout_result, logical_offset);
   }
 
-  *previous_inflow_position = ComputeInflowPosition(
-      *previous_inflow_position, child, *child_data, child_bfc_block_offset,
-      logical_offset, *layout_result, fragment,
-      self_collapsing_child_had_clearance);
+  if (!child_break_token || !child_break_token->IsInParallelFlow()) {
+    *previous_inflow_position = ComputeInflowPosition(
+        *previous_inflow_position, child, *child_data, child_bfc_block_offset,
+        logical_offset, *layout_result, fragment,
+        self_collapsing_child_had_clearance);
+  }
 
   if (child.IsInline()) {
     *previous_inline_break_token =

diff --git a/third_party/blink/renderer/core/layout/ng/ng_break_token.cc b/third_party/blink/renderer/core/layout/ng/ng_break_token.cc
@@ -22,6 +22,16 @@ ASSERT_SIZE(NGBreakToken, SameSizeAsNGBreakToken);
 
 }  // namespace
 
+bool NGBreakToken::IsInParallelFlow() const {
+  if (const auto* block_break_token = DynamicTo<NGBlockBreakToken>(this)) {
+    return block_break_token->IsAtBlockEnd();
+  }
+  if (const auto* inline_break_token = DynamicTo<InlineBreakToken>(this)) {
+    return inline_break_token->IsInParallelBlockFlow();
+  }
+  return false;
+}
+
 #if DCHECK_IS_ON()
 
 namespace {

diff --git a/third_party/blink/renderer/core/layout/ng/ng_break_token.h b/third_party/blink/renderer/core/layout/ng/ng_break_token.h
@@ -48,6 +48,10 @@ class CORE_EXPORT NGBreakToken : public GarbageCollected<NGBreakToken> {
         static_cast<NGLayoutInputNode::NGLayoutInputNodeType>(type_));
   }
 
+  // Return true if this break token is for a node that's being resumed in a
+  // parallel flow.
+  bool IsInParallelFlow() const;
+
 #if DCHECK_IS_ON()
   String ToString() const;
   void ShowBreakTokenTree() const;

diff --git a/third_party/blink/web_tests/external/wpt/css/css-break/margin-at-break-001.html b/third_party/blink/web_tests/external/wpt/css/css-break/margin-at-break-001.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<link rel="author" title="Morten Stenshorne" href="mailto:mstensho@chromium.org">
+<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1497404">
+<link rel="match" href="../reference/ref-filled-green-100px-square.xht">
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div style="columns:2; gap:0; column-fill:auto; width:100px; height:100px; background:red;">
+  <div style="height:70px;">
+    <div style="height:150px; background:green;"></div>
+  </div>
+  <div>
+    <div style="margin-top:60px;">
+      <div style="height:50px;"></div>
+      <div style="height:50px; background:green;"></div>
+    </div>
+  </div>
+</div>
diff --git a/third_party/blink/web_tests/external/wpt/css/css-break/margin-at-break-002.html b/third_party/blink/web_tests/external/wpt/css/css-break/margin-at-break-002.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<link rel="author" title="Morten Stenshorne" href="mailto:mstensho@chromium.org">
+<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1497404">
+<link rel="match" href="../reference/ref-filled-green-100px-square.xht">
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div style="columns:2; gap:0; column-fill:auto; width:100px; height:100px; background:red;">
+  <div style="display:flow-root; height:70px;">
+    <div style="height:150px; background:green;"></div>
+  </div>
+  <div>
+    <div style="margin-top:60px;">
+      <div style="height:50px;"></div>
+      <div style="height:50px; background:green;"></div>
+    </div>
+  </div>
+</div>
diff --git a/third_party/blink/web_tests/external/wpt/css/css-break/margin-at-break-003.html b/third_party/blink/web_tests/external/wpt/css/css-break/margin-at-break-003.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<link rel="author" title="Morten Stenshorne" href="mailto:mstensho@chromium.org">
+<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1497404">
+<link rel="match" href="../reference/ref-filled-green-100px-square.xht">
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div style="columns:2; gap:0; column-fill:auto; width:100px; height:100px; background:red;">
+  <div style="height:70px;">
+    <div style="height:150px; background:green;"></div>
+  </div>
+  <span>
+    <div style="margin-top:60px;">
+      <div style="height:50px;"></div>
+      <div style="height:50px; background:green;"></div>
+    </div>
+  </span>
+</div>
diff --git a/third_party/blink/web_tests/external/wpt/css/css-break/margin-at-break-004.html b/third_party/blink/web_tests/external/wpt/css/css-break/margin-at-break-004.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<link rel="author" title="Morten Stenshorne" href="mailto:mstensho@chromium.org">
+<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1497404">
+<link rel="match" href="../reference/ref-filled-green-100px-square.xht">
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div style="columns:2; gap:0; column-fill:auto; width:100px; height:100px; background:red;">
+  <span>
+    <div style="height:70px;">
+      <div style="height:150px; background:green;"></div>
+    </div>
+    <span>
+      <div style="margin-top:60px;">
+        <div style="height:50px;"></div>
+        <div style="height:50px; background:green;"></div>
+      </div>
+    </span>
+  </span>
+</div>
diff --git a/third_party/blink/web_tests/external/wpt/css/css-break/margin-at-break-005.html b/third_party/blink/web_tests/external/wpt/css/css-break/margin-at-break-005.html
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<link rel="author" title="Morten Stenshorne" href="mailto:mstensho@chromium.org">
+<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1494668">
+<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1497404">
+<link rel="match" href="../reference/ref-filled-green-100px-square.xht">
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div style="columns:2; gap:0; column-fill:auto; width:100px; height:100px; background:red;">
+  <span>
+    <div style="height:70px;">
+      <div style="height:150px; background:green;"></div>
+    </div>
+    <span>
+      <div style="margin-top:160px;">
+        <div style="float:left; width:100%;">
+          <div style="height:50px;"></div>
+          <div style="height:50px; background:green;"></div>
+        </div>
+      </div>
+    </span>
+  </span>
+</div>