[table-fragmentation] Remove rowspan avoidance logic.

When a cell was rowspanned we'd try to avoid breaking inside it as this makes the logic circular. This however had the undesired effect of rowspanned cells which are larger than the current page looking undesirable, and creating large gaps when printing. This removes the avoidance logic, at the expense of the rowspanned cell not having the correct height if the table has grown due to fragmentation. (cherry picked from commit 631b6db) Bug: 1396218 Change-Id: I10c0afc7bbc5d3ed3323b33417ee2ee22d191404 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4136462 Reviewed-by: Morten Stenshorne <mstensho@chromium.org> Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1090941} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4166380 Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Auto-Submit: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/branch-heads/5481@{#283} Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
chromium · Jan 13, 2023 · ba2820f · ba2820f
1 parent b86e48d
commit ba2820f
Show file tree

Hide file tree

Showing 9 changed files with 34 additions and 55 deletions.
diff --git a/third_party/blink/renderer/core/layout/ng/ng_constraint_space.h b/third_party/blink/renderer/core/layout/ng/ng_constraint_space.h
@@ -329,11 +329,6 @@ class CORE_EXPORT NGConstraintSpace final {
                          : false;
   }
 
-  bool IsTableCellWithEffectiveRowspan() const {
-    return HasRareData() ? rare_data_->IsTableCellWithEffectiveRowspan()
-                         : false;
-  }
-
   const NGTableConstraintSpaceData* TableData() const {
     return HasRareData() ? rare_data_->TableData() : nullptr;
   }
@@ -1203,15 +1198,6 @@ class CORE_EXPORT NGConstraintSpace final {
       EnsureTableCellData()->has_collapsed_borders = has_collapsed_borders;
     }
 
-    bool IsTableCellWithEffectiveRowspan() const {
-      return GetDataUnionType() == DataUnionType::kTableCellData &&
-             table_cell_data_.has_effective_rowspan;
-    }
-
-    void SetIsTableCellWithEffectiveRowspan(bool has_effective_rowspan) {
-      EnsureTableCellData()->has_effective_rowspan = has_effective_rowspan;
-    }
-
     void SetTableRowData(
         scoped_refptr<const NGTableConstraintSpaceData> table_data,
         wtf_size_t row_index) {
@@ -1374,22 +1360,20 @@ class CORE_EXPORT NGConstraintSpace final {
         return table_cell_borders == other.table_cell_borders &&
                table_cell_column_index == other.table_cell_column_index &&
                is_hidden_for_paint == other.is_hidden_for_paint &&
-               has_collapsed_borders == other.has_collapsed_borders &&
-               has_effective_rowspan == other.has_effective_rowspan;
+               has_collapsed_borders == other.has_collapsed_borders;
       }
 
       bool IsInitialForMaySkipLayout() const {
         return table_cell_borders == NGBoxStrut() &&
                table_cell_column_index == kNotFound && !is_hidden_for_paint &&
-               !has_collapsed_borders && !has_effective_rowspan;
+               !has_collapsed_borders;
       }
 
       NGBoxStrut table_cell_borders;
       wtf_size_t table_cell_column_index = kNotFound;
       absl::optional<LayoutUnit> table_cell_alignment_baseline;
       bool is_hidden_for_paint = false;
       bool has_collapsed_borders = false;
-      bool has_effective_rowspan = false;
     };
 
     struct TableRowData {

diff --git a/third_party/blink/renderer/core/layout/ng/ng_constraint_space_builder.h b/third_party/blink/renderer/core/layout/ng/ng_constraint_space_builder.h
@@ -441,17 +441,6 @@ class CORE_EXPORT NGConstraintSpaceBuilder final {
     }
   }
 
-  void SetIsTableCellWithEffectiveRowspan(bool has_effective_rowspan) {
-#if DCHECK_IS_ON()
-    DCHECK(!is_table_cell_with_effective_rowspan_set_);
-    is_table_cell_with_effective_rowspan_set_ = true;
-#endif
-    if (has_effective_rowspan) {
-      space_.EnsureRareData()->SetIsTableCellWithEffectiveRowspan(
-          has_effective_rowspan);
-    }
-  }
-
   void SetIsTableCellChild(bool b) {
     space_.bitfields_.is_table_cell_child = b;
   }
@@ -600,7 +589,6 @@ class CORE_EXPORT NGConstraintSpaceBuilder final {
   bool is_table_cell_column_index_set_ = false;
   bool is_table_cell_hidden_for_paint_set_ = false;
   bool is_table_cell_with_collapsed_borders_set_ = false;
-  bool is_table_cell_with_effective_rowspan_set_ = false;
   bool is_custom_layout_data_set_ = false;
   bool is_lines_until_clamp_set_ = false;
   bool is_table_row_data_set_ = false;

diff --git a/third_party/blink/renderer/core/layout/ng/table/ng_table_layout_algorithm_utils.cc b/third_party/blink/renderer/core/layout/ng/table/ng_table_layout_algorithm_utils.cc
@@ -206,7 +206,6 @@ NGTableTypes::Row ComputeMinimumRowBlockSize(
           row_count_func() - (row_index - start_row_index);
       effective_rowspan = std::min(max_rows, effective_rowspan);
     }
-    bool has_effective_rowspan = effective_rowspan > 1;
 
     NGConstraintSpaceBuilder space_builder(
         table_writing_direction.GetWritingMode(), cell_writing_direction,
@@ -220,7 +219,7 @@ NGTableTypes::Row ComputeMinimumRowBlockSize(
         colspan_cell_tabulator->CurrentColumn(),
         /* is_initial_block_size_indefinite */ true,
         is_table_block_size_specified, has_collapsed_borders,
-        has_effective_rowspan, NGCacheSlot::kMeasure, &space_builder);
+        NGCacheSlot::kMeasure, &space_builder);
 
     const auto cell_space = space_builder.ToConstraintSpace();
     const NGLayoutResult* layout_result = cell.Layout(cell_space);
@@ -236,6 +235,7 @@ NGTableTypes::Row ComputeMinimumRowBlockSize(
 
     bool has_descendant_that_depends_on_percentage_block_size =
         layout_result->HasDescendantThatDependsOnPercentageBlockSize();
+    bool has_effective_rowspan = effective_rowspan > 1;
 
     NGTableTypes::CellBlockConstraint cell_block_constraint = {
         fragment.BlockSize(),
@@ -497,7 +497,6 @@ void NGTableAlgorithmUtils::SetupTableCellConstraintSpaceBuilder(
     bool is_initial_block_size_indefinite,
     bool is_table_block_size_specified,
     bool has_collapsed_borders,
-    bool has_effective_rowspan,
     NGCacheSlot cache_slot,
     NGConstraintSpaceBuilder* builder) {
   const auto& cell_style = cell.Style();
@@ -545,7 +544,6 @@ void NGTableAlgorithmUtils::SetupTableCellConstraintSpaceBuilder(
       is_table_block_size_specified || cell_style.LogicalHeight().IsFixed());
   builder->SetIsTableCellHiddenForPaint(is_hidden_for_paint);
   builder->SetIsTableCellWithCollapsedBorders(has_collapsed_borders);
-  builder->SetIsTableCellWithEffectiveRowspan(has_effective_rowspan);
   builder->SetHideTableCellIfEmpty(
       !has_collapsed_borders && cell_style.EmptyCells() == EEmptyCells::kHide);
   builder->SetCacheSlot(cache_slot);
@@ -718,16 +716,6 @@ void NGTableAlgorithmUtils::FinalizeTableCellLayout(
   builder->SetIsTableNGPart();
   builder->SetTableCellColumnIndex(space.TableCellColumnIndex());
 
-  // When fragmentation is present, table-cells with rowspan get complicated.
-  // These cells have layout performed in their originating (first) row. As a
-  // result a subsequent row may cause the rowspan-cell to grow, and we'd need
-  // to backtrack to expand the rowspan-cell. This may then cause a row that it
-  // spans to grow!
-  // As such - we make the rowspan cell unappealing to break within, and always
-  // leave it at the original measured block-size.
-  if (space.IsTableCellWithEffectiveRowspan())
-    builder->ClampBreakAppeal(kBreakAppealViolatingBreakAvoid);
-
   // If we're resuming after a break, there'll be no alignment, since the
   // fragment will start at the block-start edge of the fragmentainer then.
   if (IsBreakInside(builder->PreviousBreakToken()))

diff --git a/third_party/blink/renderer/core/layout/ng/table/ng_table_layout_algorithm_utils.h b/third_party/blink/renderer/core/layout/ng/table/ng_table_layout_algorithm_utils.h
@@ -58,7 +58,6 @@ class NGTableAlgorithmUtils {
       bool is_initial_block_size_indefinite,
       bool is_restricted_block_size_table,
       bool has_collapsed_borders,
-      bool has_effective_rowspan,
       NGCacheSlot,
       NGConstraintSpaceBuilder*);
 

diff --git a/third_party/blink/renderer/core/layout/ng/table/ng_table_row_layout_algorithm.cc b/third_party/blink/renderer/core/layout/ng/table/ng_table_row_layout_algorithm.cc
@@ -43,11 +43,10 @@ const NGLayoutResult* NGTableRowLayoutAlgorithm::Layout() {
           NGBlockNode cell, const NGTableConstraintSpaceData::Cell& cell_data,
           LayoutUnit row_block_size, absl::optional<LayoutUnit> row_baseline,
           bool min_block_size_should_encompass_intrinsic_size) {
-        bool has_effective_rowspan =
-            cell_data.rowspan_block_size != kIndefiniteSize;
-        const LayoutUnit cell_block_size = has_effective_rowspan
-                                               ? cell_data.rowspan_block_size
-                                               : row_block_size;
+        const LayoutUnit cell_block_size =
+            cell_data.rowspan_block_size != kIndefiniteSize
+                ? cell_data.rowspan_block_size
+                : row_block_size;
 
         DCHECK_EQ(table_data.table_writing_direction.GetWritingMode(),
                   ConstraintSpace().GetWritingMode());
@@ -62,8 +61,7 @@ const NGLayoutResult* NGTableRowLayoutAlgorithm::Layout() {
             container_builder_.InlineSize(), row_baseline,
             cell_data.start_column, cell_data.is_initial_block_size_indefinite,
             table_data.is_table_block_size_specified,
-            table_data.has_collapsed_borders, has_effective_rowspan,
-            NGCacheSlot::kLayout, &builder);
+            table_data.has_collapsed_borders, NGCacheSlot::kLayout, &builder);
 
         if (ConstraintSpace().HasBlockFragmentation()) {
           SetupSpaceBuilderForFragmentation(

diff --git a/third_party/blink/renderer/core/layout/ng/table/ng_table_section_layout_algorithm.cc b/third_party/blink/renderer/core/layout/ng/table/ng_table_section_layout_algorithm.cc
@@ -126,9 +126,6 @@ const NGLayoutResult* NGTableSectionLayoutAlgorithm::Layout() {
         actual_start_row_index = row_index;
       row_offsets.emplace_back(offset.block_offset);
     }
-
-    if (container_builder_.HasInflowChildBreakInside())
-      break;
   }
 
   if (!child_iterator.NextChild().node)

diff --git a/third_party/blink/web_tests/FlagExpectations/disable-layout-ng b/third_party/blink/web_tests/FlagExpectations/disable-layout-ng
@@ -408,6 +408,7 @@ crbug.com/1078927 external/wpt/css/css-break/table/table-collapsed-borders-paint
 crbug.com/1078927 external/wpt/css/css-break/table/table-parts-offsetheight.html [ Failure ]
 crbug.com/1078927 external/wpt/css/css-break/table/table-row-paint-vlr-rtl.html [ Failure ]
 crbug.com/1078927 external/wpt/css/css-break/table/table-row-paint-vrl-rtl.html [ Failure ]
+crbug.com/1078927 external/wpt/css/css-break/table/table-rowspan-001.html [ Failure ]
 crbug.com/1078927 external/wpt/css/css-break/table/table-section-paint-vrl-rtl.html [ Failure ]
 crbug.com/829028 external/wpt/css/css-break/tall-content-inside-constrained-block-000.tentative.html [ Failure ]
 crbug.com/829028 external/wpt/css/css-break/tall-content-inside-constrained-block-001.tentative.html [ Failure ]

diff --git a/third_party/blink/web_tests/TestExpectations b/third_party/blink/web_tests/TestExpectations
@@ -1549,6 +1549,11 @@ crbug.com/1347984 fragmentation/table-row-dimensions-break-freely.html [ Failure
 crbug.com/1347984 fragmentation/table-row-dimensions-with-thead.html [ Failure ]
 crbug.com/1347984 fragmentation/table-row-dimensions.html [ Failure ]
 
+# Rowspan cells need to grow for fragmentation expansion.
+crbug.com/1396218 fragmentation/fragmented-rowspan-alignment.html [ Failure ]
+crbug.com/1396218 fragmentation/fragmented-rowspan.html [ Failure ]
+crbug.com/1396218 fragmentation/table-overlapping-rowspan.html [ Failure ]
+
 ### Tests failing with LayoutNGFlexFragmentation enabled:
 crbug.com/1367912 external/wpt/css/css-break/flexbox/multi-line-row-flex-fragmentation-058.html [ Failure ]
 crbug.com/1367912 external/wpt/css/css-break/flexbox/single-line-column-flex-fragmentation-043.html [ Failure ]

diff --git a/third_party/blink/web_tests/external/wpt/css/css-break/table/table-rowspan-001.html b/third_party/blink/web_tests/external/wpt/css/css-break/table/table-rowspan-001.html
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1396218">
+<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; width:100px; gap:0; column-fill:auto; height:100px; background:red;">
+  <table cellpadding="0" cellspacing="0">
+    <tr>
+      <td style="width:25px; height:20px; background:green;"></td>
+      <td style="width:25px; height:20px; background:green;"></td>
+    </tr>
+    <tr>
+      <td rowspan="2" style="background:green;"></td>
+      <td style="height:20px; background:green;"></td>
+    </tr>
+    <tr>
+      <td style="height:160px; background:green;"></td>
+    </tr>
+  </table>
+</div>