Disallow nested repeated content in NG block fragmentation.

Don't allow repeated table headers / footers nested inside another piece
of repeatable content. It just gets too complicated, and there's
probably no need to support this anyway. This is a partial revert of
CL:3710528.

One test starts failing because of this change. Tracked by
crbug.com/1352931

Rename IsRepeatable to ShouldRepeat, to make it clearer what it does
(it's false when we know that we're at the last fragment), and introduce
IsInsideRepeatableContent, which is always true when inside any type of
content that's eligible for repetition.

Bug: 1348774, 1348725, 1352931
Change-Id: I94e2295e31b4206bbf2a729a992f49df65c87af0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3829224
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Alison Maher <almaher@microsoft.com>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1035182}

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

@@ -710,7 +710,7 @@ const NGLayoutResult* NGBlockNode::LayoutRepeatableRoot(
   // numbers right, which is important when adding the result to the LayoutBox,
   // and it's also needed by pre-paint / paint.
   const NGBlockBreakToken* outgoing_break_token = nullptr;
-  if (constraint_space.IsRepeatable())
+  if (constraint_space.ShouldRepeat())
     outgoing_break_token = NGBlockBreakToken::CreateRepeated(*this, index);
   auto mutator = fragment.GetMutableForCloning();
   mutator.SetBreakToken(outgoing_break_token);
@@ -724,7 +724,7 @@ const NGLayoutResult* NGBlockNode::LayoutRepeatableRoot(
     box_->SetLayoutResult(result, index);
   }
 
-  if (!constraint_space.IsRepeatable()) {
+  if (!constraint_space.ShouldRepeat()) {
     // This is the last fragment. It won't be repeated again. We have already
     // created fragments for the repeated nodes, but the cloning was shallow.
     // We're now ready to deep-clone the entire subtree for each repeated

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

@@ -54,7 +54,7 @@ class CORE_EXPORT NGBlockNode : public NGLayoutInputNode {
   // element during printing, or table header / footer). To be called once for
   // each container fragment in which it repeats.
   //
-  // NGConstraintSpace::IsRepeatable() will tell whether the node is
+  // NGConstraintSpace::ShouldRepeat() will tell whether the node is
   // (potentially [1]) going to repeat again (in which case an outgoing "repeat"
   // break token will be created, or if this is the last time (no outgoing break
   // token will be created).

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

@@ -682,7 +682,7 @@ void NGBoxFragmentBuilder::CheckNoBlockFragmentation() const {
   DCHECK(!HasInflowChildBreakInside());
   DCHECK(!DidBreakSelf());
   DCHECK(!has_forced_break_);
-  DCHECK(ConstraintSpace().IsRepeatable() || !HasBreakTokenData());
+  DCHECK(ConstraintSpace().ShouldRepeat() || !HasBreakTokenData());
   DCHECK_EQ(minimal_space_shortage_, kIndefiniteSize);
   if (!ConstraintSpace().ShouldPropagateChildBreakValues()) {
     DCHECK(!initial_break_before_);

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

@@ -399,11 +399,20 @@ class CORE_EXPORT NGConstraintSpace final {
     return HasRareData() && rare_data_->is_at_fragmentainer_start;
   }
 
-  // Return true if the content is repeatable inside block fragmentation, which
-  // is the case when an element is fixed positioned (printing only), or a
+  // Return true if the content will be repeated in the next fragmentainer.
+  // This is the case when an element is fixed positioned (printing only), or a
+  // repeatable table header / footer. Will return false even for repeatable
+  // content, if we can tell for sure that this is the last time that the node
+  // will repeat.
+  bool ShouldRepeat() const {
+    return HasRareData() && rare_data_->should_repeat;
+  }
+
+  // Return true if we're inside repeatable content inside block fragmentation,
+  // which is the case when an element is fixed positioned (printing only), or a
   // repeatable table header / footer.
-  bool IsRepeatable() const {
-    return HasRareData() && rare_data_->is_repeatable;
+  bool IsInsideRepeatableContent() const {
+    return HasRareData() && rare_data_->is_inside_repeatable_content;
   }
 
   // Whether the current constraint space is for the newly established
@@ -856,7 +865,8 @@ class CORE_EXPORT NGConstraintSpace final {
           min_break_appeal(kBreakAppealLastResort),
           propagate_child_break_values(false),
           is_at_fragmentainer_start(false),
-          is_repeatable(false) {}
+          should_repeat(false),
+          is_inside_repeatable_content(false) {}
     RareData(const RareData& other)
         : percentage_resolution_size(other.percentage_resolution_size),
           replaced_percentage_resolution_block_size(
@@ -885,7 +895,8 @@ class CORE_EXPORT NGConstraintSpace final {
           min_break_appeal(other.min_break_appeal),
           propagate_child_break_values(other.propagate_child_break_values),
           is_at_fragmentainer_start(other.is_at_fragmentainer_start),
-          is_repeatable(other.is_repeatable) {
+          should_repeat(other.should_repeat),
+          is_inside_repeatable_content(other.is_inside_repeatable_content) {
       switch (GetDataUnionType()) {
         case DataUnionType::kNone:
           break;
@@ -960,7 +971,8 @@ class CORE_EXPORT NGConstraintSpace final {
           is_in_column_bfc != other.is_in_column_bfc ||
           min_break_appeal != other.min_break_appeal ||
           propagate_child_break_values != other.propagate_child_break_values ||
-          is_repeatable != other.is_repeatable)
+          should_repeat != other.should_repeat ||
+          is_inside_repeatable_content != other.is_inside_repeatable_content)
         return false;
 
       switch (GetDataUnionType()) {
@@ -995,7 +1007,7 @@ class CORE_EXPORT NGConstraintSpace final {
           should_ignore_forced_breaks || is_in_column_bfc ||
           min_break_appeal != kBreakAppealLastResort ||
           propagate_child_break_values || is_at_fragmentainer_start ||
-          is_repeatable)
+          should_repeat || is_inside_repeatable_content)
         return false;
 
       switch (GetDataUnionType()) {
@@ -1276,7 +1288,8 @@ class CORE_EXPORT NGConstraintSpace final {
     unsigned min_break_appeal : kNGBreakAppealBitsNeeded;
     unsigned propagate_child_break_values : 1;
     unsigned is_at_fragmentainer_start : 1;
-    unsigned is_repeatable : 1;
+    unsigned should_repeat : 1;
+    unsigned is_inside_repeatable_content : 1;
 
    private:
     struct BlockData {

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

@@ -38,8 +38,9 @@ class CORE_EXPORT NGConstraintSpaceBuilder final {
                                  adjust_inline_size_if_needed) {
     if (parent_space.ShouldPropagateChildBreakValues())
       SetShouldPropagateChildBreakValues();
-    if (parent_space.IsRepeatable())
-      SetIsRepeatable(/*is_repeatable*/ true);
+    if (parent_space.ShouldRepeat())
+      SetShouldRepeat(true);
+    SetIsInsideRepeatableContent(parent_space.IsInsideRepeatableContent());
   }
 
   // The setters on this builder are in the writing mode of parent_writing_mode.
@@ -149,8 +150,10 @@ class CORE_EXPORT NGConstraintSpaceBuilder final {
     space_.EnsureRareData()->is_at_fragmentainer_start = true;
   }
 
-  void SetIsRepeatable(bool is_repeatable) {
-    space_.EnsureRareData()->is_repeatable = is_repeatable;
+  void SetShouldRepeat(bool b) { space_.EnsureRareData()->should_repeat = b; }
+
+  void SetIsInsideRepeatableContent(bool b) {
+    space_.EnsureRareData()->is_inside_repeatable_content = b;
   }
 
   void SetIsFixedInlineSize(bool b) {

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

@@ -126,10 +126,7 @@ void NGFragmentRepeater::CloneChildFragments(
         child.fragment = &child_result->PhysicalFragment();
       } else if (child_box->IsFragmentainerBox()) {
         child_box = NGPhysicalBoxFragment::Clone(*child_box);
-        NGFragmentRepeater child_repeater(
-            is_first_clone_, is_last_fragment_,
-            /* is_inside_nested_fragmentainer */ true);
-        child_repeater.CloneChildFragments(*child_box);
+        CloneChildFragments(*child_box);
         child.fragment = child_box;
       }
     } else if (child->IsLineBox()) {
@@ -211,8 +208,7 @@ const NGLayoutResult* NGFragmentRepeater::GetClonableLayoutResult(
   for (const NGLayoutResult* result : layout_box.GetLayoutResults()) {
     const NGBlockBreakToken* break_token =
         To<NGPhysicalBoxFragment>(result->PhysicalFragment()).BreakToken();
-    if (!break_token ||
-        (break_token->IsRepeated() && !is_inside_nested_fragmentainer_))
+    if (!break_token || break_token->IsRepeated())
       return result;
   }
   NOTREACHED();

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

@@ -26,12 +26,8 @@ class NGFragmentRepeater {
   STACK_ALLOCATED();
 
  public:
-  explicit NGFragmentRepeater(bool is_first_clone,
-                              bool is_last_fragment,
-                              bool is_inside_nested_fragmentainer = false)
-      : is_first_clone_(is_first_clone),
-        is_last_fragment_(is_last_fragment),
-        is_inside_nested_fragmentainer_(is_inside_nested_fragmentainer) {}
+  NGFragmentRepeater(bool is_first_clone, bool is_last_fragment)
+      : is_first_clone_(is_first_clone), is_last_fragment_(is_last_fragment) {}
 
   // Deep-clone the subtree of an already shallowly cloned fragment. This will
   // also create new break tokens inside, in order to set unique sequence
@@ -52,10 +48,6 @@ class NGFragmentRepeater {
   // True when at the last container fragment. No outgoing "repeat" break tokens
   // should be created then.
   bool is_last_fragment_;
-
-  // True when we are cloning a subset of the tree in which an inner
-  // fragmentainer was found.
-  bool is_inside_nested_fragmentainer_;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/layout/ng/table/ng_table_layout_algorithm.cc

@@ -990,7 +990,8 @@ const NGLayoutResult* NGTableLayoutAlgorithm::GenerateFragment(
                                               section_index);
 
     if (repeat_mode != kNotRepeated) {
-      section_space_builder.SetIsRepeatable(repeat_mode == kMayRepeatAgain);
+      section_space_builder.SetShouldRepeat(repeat_mode == kMayRepeatAgain);
+      section_space_builder.SetIsInsideRepeatableContent(true);
     } else if (ConstraintSpace().HasBlockFragmentation()) {
       SetupSpaceBuilderForFragmentation(
           ConstraintSpace(), section, block_offset, &section_space_builder,
@@ -1024,12 +1025,15 @@ const NGLayoutResult* NGTableLayoutAlgorithm::GenerateFragment(
   bool has_repeated_header = false;
   absl::optional<LayoutUnit> pending_repeated_footer_block_size;
 
+  // Before fragmented layout we need to go through the table's children, to
+  // look for repeatable headers and footers. This is especially important for
+  // footers, since we need to reserve space for it after any preceding
+  // non-repeated sections (typically tbody). We'll only repeat headers /
+  // footers if we're not already inside repeatable content, though.
+  // See crbug.com/1352931
   if (ConstraintSpace().HasKnownFragmentainerBlockSize() &&
+      !ConstraintSpace().IsInsideRepeatableContent() &&
       (grouped_children.header || grouped_children.footer)) {
-    // Before layout, we need to go through the table's children, to look for
-    // repeatable headers and footers. This is especially important for footers,
-    // since we need to reserve space for it after any preceding non-repeated
-    // sections (typically tbody).
     LayoutUnit max_section_block_size =
         ConstraintSpace().FragmentainerBlockSize() / 4;
     NGTableChildIterator child_iterator(grouped_children, BreakToken());

third_party/blink/web_tests/TestExpectations

@@ -1761,6 +1761,7 @@ crbug.com/1078927 fragmentation/border-spacing-break-before-unbreakable-row.html
 crbug.com/1078927 fragmentation/fragmented-rowspan-alignment.html [ Failure ]
 crbug.com/1078927 fragmentation/fragmented-rowspan.html [ Failure ]
 crbug.com/1078927 fragmentation/no-repeating-table-header-after-sections.html [ Failure ]
+crbug.com/1352931 fragmentation/repeating-thead-under-repeating-thead.html [ Failure ]
 crbug.com/1335870 fragmentation/single-line-cells-multiple-tables-repeating-thead-with-border-spacing-at-top-of-row.html [ Failure ]
 crbug.com/1078927 fragmentation/table-overlapping-rowspan.html [ Failure ]
 

third_party/blink/web_tests/external/wpt/css/css-multicol/crashtests/repeated-section-in-nested-table-nested-multicol.html

@@ -0,0 +1,15 @@
+<!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=1348774">
+<div style="columns:2; column-fill:auto; height:160px; background:yellow;">
+  <div style="display:table;">
+    <div style="display:table;">
+      <div style="contain:size; height:150px;"></div>
+      <div style="display:table-footer-group; break-inside:avoid;">
+        <div style="columns:2;">
+          x AAAAAAAAAAAAAAAAAAAA x
+        </div>
+      </div>
+    </div>
+  </div>
+</div>