Miss the cache when breaking before a fragment with low break appeal.

If a fragment doesn't fit in the fragmentainer, we should of course break before it. This normally happens automatically, but in some cases the machinery needs an extra nudge. FinishFragmentation() will in some cases clamp the break (inside) appeal down to kBreakAppealLastResort. We need to miss the cache if we find such layout results in the cache, since the break appeal may no longer be valid, when we have moved over to a new fragmentainer. Otherwise we will never let the node fit in the previous fragmentainer, not even after making the fragmentainers taller. This leads to correctness issues, and also freezes during column balancing, since no fragmentainer stretch amount will be enough to pull the node back to the first fragmentainer. (cherry picked from commit 870ba6c) Bug: 1395408, 1399449 Change-Id: Ic0fc032b0b7022dfb9e5c35af75e09d8b3f50316 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4085506 Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: Alison Maher <almaher@microsoft.com> Cr-Original-Commit-Position: refs/heads/main@{#1081158} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4092856 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Owners-Override: Srinivas Sista <srinivassista@chromium.org> Commit-Queue: Srinivas Sista <srinivassista@chromium.org> Cr-Commit-Position: refs/branch-heads/5359@{#1147} Cr-Branched-From: 27d3765-refs/heads/main@{#1058933}
chromium · Dec 9, 2022 · d25a109 · d25a109
1 parent 06be376
commit d25a109
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 0 deletions.
diff --git a/third_party/blink/renderer/core/layout/layout_box_hot.cc b/third_party/blink/renderer/core/layout/layout_box_hot.cc
@@ -312,6 +312,19 @@ const NGLayoutResult* LayoutBox::CachedLayoutResult(
       if (column_spanner_path || cached_layout_result->ColumnSpannerPath())
         return nullptr;
 
+      // Break appeal may have been reduced because the fragment crosses the
+      // fragmentation line, to send a strong signal to break before it
+      // instead. If we actually ended up breaking before it, this break appeal
+      // may no longer be valid, since there could be more room in the next
+      // fragmentainer. Miss the cache.
+      //
+      // TODO(mstensho): Maybe this shouldn't be necessary. Look into how
+      // FinishFragmentation() clamps break appeal down to
+      // kBreakAppealLastResort. Maybe there are better ways.
+      if (break_token && break_token->IsBreakBefore() &&
+          cached_layout_result->BreakAppeal() < kBreakAppealPerfect)
+        return nullptr;
+
       // If the node didn't break into multiple fragments, we might be able to
       // re-use the result. If the fragmentainer block-size has changed, or if
       // the fragment's block-offset within the fragmentainer has changed, we

diff --git a/...rnal/wpt/css/css-break/flexbox/increase-fragmentainer-size-flex-item-trailing-margin.html b/...rnal/wpt/css/css-break/flexbox/increase-fragmentainer-size-flex-item-trailing-margin.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=1395408">
+<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1399449">
+<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 id="mc" style="columns:1; width:100px; column-fill:auto; height:90px; background:red;">
+  <div style="height:40px; background:green;"></div>
+  <div style="display:flex; background:green;">
+    <div style="margin-bottom:10px; height:50px; width:100px;"></div>
+  </div>
+</div>
+<script>
+  document.body.offsetTop;
+  mc.style.height = "100px";
+</script>
diff --git a/...y/blink/web_tests/external/wpt/css/css-break/increase-fragmentainer-size-tall-border.html b/...y/blink/web_tests/external/wpt/css/css-break/increase-fragmentainer-size-tall-border.html
@@ -0,0 +1,13 @@
+<!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=1395408">
+<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 id="mc" style="columns:1; width:100px; column-fill:auto; height:60px; background:red;">
+  <div style="height:50px; background:green;"></div>
+  <div style="display:flow-root; border-top:50px solid green;"></div>
+</div>
+<script>
+  document.body.offsetTop;
+  mc.style.height = "100px";
+</script>
diff --git a/.../external/wpt/css/css-multicol/crashtests/balancing-flex-item-trailing-margin-freeze.html b/.../external/wpt/css/css-multicol/crashtests/balancing-flex-item-trailing-margin-freeze.html
@@ -0,0 +1,13 @@
+<!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=1395408">
+<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1399449">
+<div style="columns:2;">
+  <div style="contain:size; height:40px;">PASS if no freeze.</div>
+  <div style="display:flex;">
+    <div style="contain:size; margin-bottom:2px; height:30px; width:100px;"></div>
+  </div>
+  <div style="display:flex;">
+    <div style="contain:size; margin-bottom:2px; height:30px; width:100px;"></div>
+  </div>
+</div>
diff --git a/...ink/web_tests/external/wpt/css/css-multicol/crashtests/balancing-tall-borders-freeze.html b/...ink/web_tests/external/wpt/css/css-multicol/crashtests/balancing-tall-borders-freeze.html
@@ -0,0 +1,8 @@
+<!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=1395408">
+<div style="columns:2;">
+  <div style="contain:size; height:40px;">PASS if no freeze.</div>
+  <div style="display:flow-root; border-top:32px solid;"></div>
+  <div style="display:flow-root; border-top:32px solid;"></div>
+</div>