[LayoutNG] Allow (more) movement of self-collapsing blocks.

This relaxes a constraint we had previously that we'd never re-use a
layout-result if our "forced" BFC block-offset was different.

For self-collapsing blocks we can somewhat ignore this rule for the
following cases:
 1. If the self-collapsing block has no adjoining-objects, it can be
    placed anywhere.
 2. If the self-collapsing block has adjoining-objects, it can be
    placed anywhere if its old and new expected position doesn't
    intersect with any floats.

Change-Id: Ib2a85455955ed5b207aed64c2d691584437eaa6b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1769158
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690875}

third_party/blink/renderer/core/layout/layout_box.cc

@@ -2363,21 +2363,26 @@ scoped_refptr<const NGLayoutResult> LayoutBox::CachedLayoutResult(
   if (size_cache_status == NGLayoutCacheStatus::kNeedsSimplifiedLayout)
     cache_status = NGLayoutCacheStatus::kNeedsSimplifiedLayout;
 
+  LayoutUnit bfc_line_offset = new_space.BfcOffset().line_offset;
   base::Optional<LayoutUnit> bfc_block_offset =
       cached_layout_result->BfcBlockOffset();
-  LayoutUnit bfc_line_offset = new_space.BfcOffset().line_offset;
+  LayoutUnit block_offset_delta;
 
   const NGConstraintSpace& old_space =
       cached_layout_result->GetConstraintSpaceForCaching();
 
   // Check the BFC offset. Even if they don't match, there're some cases we can
   // still reuse the fragment.
-  bool is_bfc_offset_equal = new_space.BfcOffset() == old_space.BfcOffset();
+  bool are_bfc_offsets_equal =
+      new_space.BfcOffset() == old_space.BfcOffset() &&
+      new_space.ExpectedBfcBlockOffset() ==
+          old_space.ExpectedBfcBlockOffset() &&
+      new_space.ForcedBfcBlockOffset() == old_space.ForcedBfcBlockOffset();
 
   // Even for the first fragment, when block fragmentation is enabled, block
   // offset changes should cause re-layout, since we will fragment at other
   // locations than before.
-  if (UNLIKELY(!is_bfc_offset_equal && new_space.HasBlockFragmentation())) {
+  if (UNLIKELY(!are_bfc_offsets_equal && new_space.HasBlockFragmentation())) {
     DCHECK(old_space.HasBlockFragmentation());
     return nullptr;
   }
@@ -2394,7 +2399,7 @@ scoped_refptr<const NGLayoutResult> LayoutBox::CachedLayoutResult(
   // need to perform a series of additional checks if we can still reuse this
   // layout result.
   if (!is_new_formatting_context &&
-      (!is_bfc_offset_equal || !is_exclusion_space_equal ||
+      (!are_bfc_offsets_equal || !is_exclusion_space_equal ||
        new_space.ClearanceOffset() != old_space.ClearanceOffset())) {
     DCHECK(!CreatesNewFormattingContext());
 
@@ -2410,8 +2415,9 @@ scoped_refptr<const NGLayoutResult> LayoutBox::CachedLayoutResult(
 
     DCHECK_EQ(cache_status, NGLayoutCacheStatus::kHit);
 
-    if (!MaySkipLayoutWithinBlockFormattingContext(
-            *cached_layout_result, new_space, &bfc_block_offset))
+    if (!MaySkipLayoutWithinBlockFormattingContext(*cached_layout_result,
+                                                   new_space, &bfc_block_offset,
+                                                   &block_offset_delta))
       return nullptr;
   }
 
@@ -2450,7 +2456,7 @@ scoped_refptr<const NGLayoutResult> LayoutBox::CachedLayoutResult(
 
   // We can safely reuse this result if our BFC and "input" exclusion spaces
   // were equal.
-  if (is_bfc_offset_equal && is_exclusion_space_equal &&
+  if (are_bfc_offsets_equal && is_exclusion_space_equal &&
       !needs_cached_result_update) {
     // In order not to rebuild the internal derived-geometry "cache" of float
     // data, we need to move this to the new "output" exclusion space.
@@ -2459,9 +2465,9 @@ scoped_refptr<const NGLayoutResult> LayoutBox::CachedLayoutResult(
     return cached_layout_result;
   }
 
-  scoped_refptr<const NGLayoutResult> new_result =
-      base::AdoptRef(new NGLayoutResult(*cached_layout_result, new_space,
-                                        bfc_line_offset, bfc_block_offset));
+  scoped_refptr<const NGLayoutResult> new_result = base::AdoptRef(
+      new NGLayoutResult(*cached_layout_result, new_space, bfc_line_offset,
+                         bfc_block_offset, block_offset_delta));
 
   if (needs_cached_result_update)
     SetCachedLayoutResult(*new_result, break_token);

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

@@ -1169,7 +1169,8 @@ scoped_refptr<const NGLayoutResult> NGBlockNode::RunLegacyLayout(
     if (needs_cached_result_update) {
       layout_result = base::AdoptRef(new NGLayoutResult(
           *layout_result, constraint_space, layout_result->BfcLineOffset(),
-          layout_result->BfcBlockOffset()));
+          layout_result->BfcBlockOffset(),
+          LayoutUnit() /* block_offset_delta */));
       box_->SetCachedLayoutResult(*layout_result, /* break_token */ nullptr);
     }
   }

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

@@ -579,7 +579,6 @@ class CORE_EXPORT NGConstraintSpace final {
 
     bool MaySkipLayout(const RareData& other) const {
       return margin_strut == other.margin_strut &&
-             forced_bfc_block_offset == other.forced_bfc_block_offset &&
              table_cell_borders == other.table_cell_borders &&
              table_cell_intrinsic_padding ==
                  other.table_cell_intrinsic_padding &&
@@ -594,7 +593,6 @@ class CORE_EXPORT NGConstraintSpace final {
     // Must be kept in sync with members checked within |MaySkipLayout|.
     bool IsInitialForMaySkipLayout() const {
       return margin_strut == NGMarginStrut() &&
-             forced_bfc_block_offset == base::nullopt &&
              table_cell_borders == NGBoxStrut() &&
              table_cell_intrinsic_padding == NGBoxStrut() &&
              fragmentainer_block_size == kIndefiniteSize &&

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

@@ -76,7 +76,8 @@ NGLayoutResult::NGLayoutResult(NGLayoutResultStatus status,
 NGLayoutResult::NGLayoutResult(const NGLayoutResult& other,
                                const NGConstraintSpace& new_space,
                                LayoutUnit bfc_line_offset,
-                               base::Optional<LayoutUnit> bfc_block_offset)
+                               base::Optional<LayoutUnit> bfc_block_offset,
+                               LayoutUnit block_offset_delta)
     : space_(new_space),
       physical_fragment_(other.physical_fragment_),
       intrinsic_block_size_(other.intrinsic_block_size_),
@@ -97,7 +98,7 @@ NGLayoutResult::NGLayoutResult(const NGLayoutResult& other,
   }
 
   NGExclusionSpace new_exclusion_space = MergeExclusionSpaces(
-      other, space_.ExclusionSpace(), bfc_line_offset, bfc_block_offset);
+      other, space_.ExclusionSpace(), bfc_line_offset, block_offset_delta);
 
   if (new_exclusion_space != space_.ExclusionSpace()) {
     bitfields_.has_rare_data_exclusion_space = true;
@@ -167,18 +168,9 @@ NGExclusionSpace NGLayoutResult::MergeExclusionSpaces(
     const NGLayoutResult& other,
     const NGExclusionSpace& new_input_exclusion_space,
     LayoutUnit bfc_line_offset,
-    base::Optional<LayoutUnit> bfc_block_offset) {
-  // If we are merging exclusion spaces we should be copying a previous layout
-  // result. It is impossible to reach a state where bfc_block_offset has a
-  // value, and the result which we are copying doesn't (or visa versa).
-  // This would imply the result has switched its "empty" state for margin
-  // collapsing, which would mean it isn't possible to reuse the result.
-  DCHECK_EQ(bfc_block_offset.has_value(), other.BfcBlockOffset().has_value());
-
+    LayoutUnit block_offset_delta) {
   NGBfcDelta offset_delta = {bfc_line_offset - other.BfcLineOffset(),
-                             bfc_block_offset && other.BfcBlockOffset()
-                                 ? *bfc_block_offset - *other.BfcBlockOffset()
-                                 : LayoutUnit()};
+                             block_offset_delta};
 
   return NGExclusionSpace::MergeExclusionSpaces(
       /* old_output */ other.ExclusionSpace(),

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

@@ -47,7 +47,8 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
   NGLayoutResult(const NGLayoutResult& other,
                  const NGConstraintSpace& new_space,
                  LayoutUnit bfc_line_offset,
-                 base::Optional<LayoutUnit> bfc_block_offset);
+                 base::Optional<LayoutUnit> bfc_block_offset,
+                 LayoutUnit block_offset_delta);
   ~NGLayoutResult();
 
   const NGPhysicalContainerFragment& PhysicalFragment() const {
@@ -266,7 +267,7 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
       const NGLayoutResult& other,
       const NGExclusionSpace& new_input_exclusion_space,
       LayoutUnit bfc_line_offset,
-      base::Optional<LayoutUnit> bfc_block_offset);
+      LayoutUnit block_offset_delta);
 
   struct RareData {
     USING_FAST_MALLOC(RareData);

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

@@ -916,5 +916,91 @@ TEST_F(NGLayoutResultCachingTest, OptimisticFloatPlacementNoRelayout) {
   EXPECT_EQ(space.ForcedBfcBlockOffset(), base::nullopt);
 }
 
+TEST_F(NGLayoutResultCachingTest, SelfCollapsingShifting) {
+  ScopedLayoutNGFragmentCachingForTest layout_ng_fragment_caching(true);
+
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      .bfc { display: flow-root; width: 300px; height: 300px; }
+      .float { float: left; width: 10px; height: 10px; }
+      .adjoining-oof { position: absolute; display: inline; }
+    </style>
+    <div class="bfc">
+      <div class="float"></div>
+      <div id="test1"></div>
+    </div>
+    <div class="bfc">
+      <div class="float" style="height; 20px;"></div>
+      <div id="src1"></div>
+    </div>
+    <div class="bfc">
+      <div class="float"></div>
+      <div id="test2">
+        <div class="adjoining-oof"></div>
+      </div>
+    </div>
+    <div class="bfc">
+      <div class="float" style="height; 20px;"></div>
+      <div id="src2">
+        <div class="adjoining-oof"></div>
+      </div>
+    </div>
+    <div class="bfc">
+      <div class="float"></div>
+      <div style="height: 30px;"></div>
+      <div id="test3">
+        <div class="adjoining-oof"></div>
+      </div>
+    </div>
+    <div class="bfc">
+      <div class="float" style="height; 20px;"></div>
+      <div style="height: 30px;"></div>
+      <div id="src3">
+        <div class="adjoining-oof"></div>
+      </div>
+    </div>
+  )HTML");
+
+  auto* test1 = To<LayoutBlockFlow>(GetLayoutObjectByElementId("test1"));
+  auto* test2 = To<LayoutBlockFlow>(GetLayoutObjectByElementId("test2"));
+  auto* test3 = To<LayoutBlockFlow>(GetLayoutObjectByElementId("test3"));
+  auto* src1 = To<LayoutBlockFlow>(GetLayoutObjectByElementId("src1"));
+  auto* src2 = To<LayoutBlockFlow>(GetLayoutObjectByElementId("src2"));
+  auto* src3 = To<LayoutBlockFlow>(GetLayoutObjectByElementId("src3"));
+
+  NGLayoutCacheStatus cache_status;
+  base::Optional<NGFragmentGeometry> fragment_geometry;
+
+  NGConstraintSpace space =
+      src1->GetCachedLayoutResult()->GetConstraintSpaceForCaching();
+  scoped_refptr<const NGLayoutResult> result = test1->CachedLayoutResult(
+      space, nullptr, &fragment_geometry, &cache_status);
+
+  // Case 1: We have a different set of constraints, but as the child has no
+  // adjoining descendants it can be shifted anywhere.
+  EXPECT_EQ(cache_status, NGLayoutCacheStatus::kHit);
+  EXPECT_NE(result.get(), nullptr);
+
+  fragment_geometry.reset();
+  space = src2->GetCachedLayoutResult()->GetConstraintSpaceForCaching();
+  result = test2->CachedLayoutResult(space, nullptr, &fragment_geometry,
+                                     &cache_status);
+
+  // Case 2: We have a different set of constraints, but the child has an
+  // adjoining object and isn't "past" the floats - it can't be reused.
+  EXPECT_EQ(cache_status, NGLayoutCacheStatus::kNeedsLayout);
+  EXPECT_EQ(result.get(), nullptr);
+
+  fragment_geometry.reset();
+  space = src3->GetCachedLayoutResult()->GetConstraintSpaceForCaching();
+  result = test3->CachedLayoutResult(space, nullptr, &fragment_geometry,
+                                     &cache_status);
+
+  // Case 3: We have a different set of constraints, and adjoining descendants,
+  // but have a position past where they might affect us.
+  EXPECT_EQ(cache_status, NGLayoutCacheStatus::kHit);
+  EXPECT_NE(result.get(), nullptr);
+}
+
 }  // namespace
 }  // namespace blink

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

@@ -368,10 +368,12 @@ bool MaySkipLegacyLayout(const NGBlockNode& node,
 bool MaySkipLayoutWithinBlockFormattingContext(
     const NGLayoutResult& cached_layout_result,
     const NGConstraintSpace& new_space,
-    base::Optional<LayoutUnit>* bfc_block_offset) {
+    base::Optional<LayoutUnit>* bfc_block_offset,
+    LayoutUnit* block_offset_delta) {
   DCHECK_EQ(cached_layout_result.Status(), NGLayoutResult::kSuccess);
   DCHECK(cached_layout_result.HasValidConstraintSpaceForCaching());
   DCHECK(bfc_block_offset);
+  DCHECK(block_offset_delta);
 
   const NGConstraintSpace& old_space =
       cached_layout_result.GetConstraintSpaceForCaching();
@@ -408,37 +410,81 @@ bool MaySkipLayoutWithinBlockFormattingContext(
     }
   }
 
+  const auto& physical_fragment = cached_layout_result.PhysicalFragment();
+
   // Check we have a descendant that *may* be positioned above the block-start
   // edge. We abort if either the old or new space has floats, as we don't keep
   // track of how far above the child could be. This case is relatively rare,
   // and only occurs with negative margins.
-  if (cached_layout_result.PhysicalFragment()
-          .MayHaveDescendantAboveBlockStart() &&
+  if (physical_fragment.MayHaveDescendantAboveBlockStart() &&
       (old_space.HasFloats() || new_space.HasFloats()))
     return false;
 
-  // We can now try to adjust the BFC block-offset.
-  if (*bfc_block_offset) {
-    // Check if the previous position may intersect with any floats.
-    if (**bfc_block_offset <
-        old_space.ExclusionSpace().ClearanceOffset(EClear::kBoth))
-      return false;
-
-    if (is_pushed_by_floats) {
-      DCHECK_EQ(**bfc_block_offset, old_clearance_offset);
-      *bfc_block_offset = new_clearance_offset;
-    } else {
-      *bfc_block_offset = **bfc_block_offset -
-                          old_space.BfcOffset().block_offset +
-                          new_space.BfcOffset().block_offset;
+  // Self collapsing blocks have different "shifting" rules applied to them.
+  if (cached_layout_result.IsSelfCollapsing()) {
+    DCHECK(!is_pushed_by_floats);
+
+    // The "expected" BFC block-offset is where adjoining objects will be
+    // placed (which may be wrong due to adjoining margins).
+    LayoutUnit old_expected = old_space.ExpectedBfcBlockOffset();
+    LayoutUnit new_expected = new_space.ExpectedBfcBlockOffset();
+
+    // If we have any adjoining object descendants (floats), we need to ensure
+    // that their position wouldn't be impacted by any preceding floats.
+    if (physical_fragment.HasAdjoiningObjectDescendants()) {
+      // Check if the previous position intersects with any floats.
+      if (old_expected <
+          old_space.ExclusionSpace().ClearanceOffset(EClear::kBoth))
+        return false;
+
+      // Check if the new position intersects with any floats.
+      if (new_expected <
+          new_space.ExclusionSpace().ClearanceOffset(EClear::kBoth))
+        return false;
     }
 
-    // Check if the new position may intersect with any floats.
-    if (**bfc_block_offset <
-        new_space.ExclusionSpace().ClearanceOffset(EClear::kBoth))
-      return false;
+    *block_offset_delta = new_expected - old_expected;
+
+    // Self-collapsing blocks with a "forced" BFC block-offset input receive a
+    // "resolved" BFC block-offset on their layout result.
+    *bfc_block_offset = new_space.ForcedBfcBlockOffset();
+
+    return true;
   }
 
+  // We can now try to adjust the BFC block-offset for regular blocks.
+  DCHECK(*bfc_block_offset);
+
+  // New formatting-contexts have (potentially) complex positioning logic. In
+  // some cases they will resolve a BFC block-offset twice (with their margins
+  // adjoining, and not adjoining), resulting in two different "forced" BFC
+  // block-offsets. We don't allow caching as we can't determine which pass a
+  // layout result belongs to for this case.
+  // TODO(ikilpatrick): We need to relax this restriction for fragments which
+  // have clearance past adjoining floats (which is quite common).
+  if (old_space.ForcedBfcBlockOffset() != new_space.ForcedBfcBlockOffset())
+    return false;
+
+  // Check if the previous position intersects with any floats.
+  if (**bfc_block_offset <
+      old_space.ExclusionSpace().ClearanceOffset(EClear::kBoth))
+    return false;
+
+  if (is_pushed_by_floats) {
+    DCHECK_EQ(**bfc_block_offset, old_clearance_offset);
+    *block_offset_delta = new_clearance_offset - old_clearance_offset;
+    *bfc_block_offset = new_clearance_offset;
+  } else {
+    *block_offset_delta =
+        new_space.BfcOffset().block_offset - old_space.BfcOffset().block_offset;
+    *bfc_block_offset = **bfc_block_offset + *block_offset_delta;
+  }
+
+  // Check if the new position intersects with any floats.
+  if (**bfc_block_offset <
+      new_space.ExclusionSpace().ClearanceOffset(EClear::kBoth))
+    return false;
+
   return true;
 }
 

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

@@ -48,12 +48,16 @@ bool MaySkipLegacyLayout(const NGBlockNode& node,
 // affected by clearance, or floats, and therefore might be able to skip
 // layout.
 // Additionally (if this function returns true) it will calculate the new
-// |bfc_block_offset| for the layout result. This may still be |base::nullopt|
-// if not previously set.
+// |bfc_block_offset|, and |block_offset_delta| for the layout result.
+// |bfc_block_offset| may still be |base::nullopt| if not previously set.
+//
+// If this function returns false, both |bfc_block_offset|, and
+// |block_offset_delta| are in an undefined state and should not be used.
 bool MaySkipLayoutWithinBlockFormattingContext(
     const NGLayoutResult& cached_layout_result,
     const NGConstraintSpace& new_space,
-    base::Optional<LayoutUnit>* bfc_block_offset);
+    base::Optional<LayoutUnit>* bfc_block_offset,
+    LayoutUnit* block_offset_delta);
 
 // Return true if layout is considered complete. In some cases we require more
 // than one layout pass.