فهرست منبع

LibWeb+Base: Fix row-height bug in Grid when there is a column gap

This fixes a bug in the CSS Grid when there is a column and/or row gap,
as previously it would take the index of the incorrect column when
finding the `AvailableSize`.

There is a mild complication in the GridFormattingContext as the
OccupationGrid does not take into account the gap columns and rows that
later appear in the `Vector<TemporaryTrack>` columns and rows. The
PositionedBoxes are kind of a connection between the two, and so it's
now more explicit whether you would like to refer to a column by its
position taking into the gap columns/rows or not.
Tom 2 سال پیش
والد
کامیت
52e45fb6fa

+ 14 - 0
Base/res/html/misc/display-grid.html

@@ -6,8 +6,13 @@
   .grid-item {
     background-color: lightblue;
   }
+  .with-border {
+    border: 1px solid black;
+  }
 </style>
 
+<body style="margin-bottom: 2rem;">
+
 <!-- A basic grid -->
 <p>Should render a 2x2 grid</p>
 <div 
@@ -588,3 +593,12 @@ class="grid-container"
   <div class="grid-item" style="grid-area: two;">max-content</div>
   <div class="grid-item" style="grid-area: three;">1fr</div>
 </div>
+
+<p>Bug with column gaps - grid items should have normal height</p>
+<div class="grid-container with-border" style="
+      grid-column-gap: 10px;
+      grid-template-columns: minmax(0, 1fr) minmax(0, 1fr);
+    ">
+    <div class="grid-item with-border">left side text</div>
+    <div class="grid-item with-border">right side text right side text right side text</div>
+</div>

+ 42 - 30
Userland/Libraries/LibWeb/Layout/GridFormattingContext.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, Martin Falisse <mfalisse@outlook.com>
+ * Copyright (c) 2022-2023, Martin Falisse <mfalisse@outlook.com>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -261,7 +261,7 @@ void GridFormattingContext::place_item_with_row_and_column_position(Box const& b
 
     // FIXME: If the placement contains only a span for a named line, replace it with a span of 1.
 
-    m_positioned_boxes.append({ child_box, row_start, row_span, column_start, column_span });
+    m_positioned_boxes.append(PositionedBox(child_box, row_start, row_span, column_start, column_span));
 
     m_occupation_grid.maybe_add_row(row_start + 1);
     m_occupation_grid.maybe_add_column(column_start + 1);
@@ -389,7 +389,7 @@ void GridFormattingContext::place_item_with_row_position(Box const& box, Box con
     }
     m_occupation_grid.set_occupied(column_start, column_start + column_span, row_start, row_start + row_span);
 
-    m_positioned_boxes.append({ child_box, row_start, row_span, column_start, column_span });
+    m_positioned_boxes.append(PositionedBox(child_box, row_start, row_span, column_start, column_span));
 }
 
 void GridFormattingContext::place_item_with_column_position(Box const& box, Box const& child_box, int& auto_placement_cursor_x, int& auto_placement_cursor_y)
@@ -512,7 +512,7 @@ void GridFormattingContext::place_item_with_column_position(Box const& box, Box
     // line according to its span from that position.
     m_occupation_grid.set_occupied(column_start, column_start + column_span, auto_placement_cursor_y, auto_placement_cursor_y + row_span);
 
-    m_positioned_boxes.append({ child_box, auto_placement_cursor_y, row_span, column_start, column_span });
+    m_positioned_boxes.append(PositionedBox(child_box, auto_placement_cursor_y, row_span, column_start, column_span));
 }
 
 void GridFormattingContext::place_item_with_no_declared_position(Box const& child_box, int& auto_placement_cursor_x, int& auto_placement_cursor_y)
@@ -571,7 +571,7 @@ finish:
     }
 
     m_occupation_grid.set_occupied(column_start, column_start + column_span, row_start, row_start + row_span);
-    m_positioned_boxes.append({ child_box, row_start, row_span, column_start, column_span });
+    m_positioned_boxes.append(PositionedBox(child_box, row_start, row_span, column_start, column_span));
 }
 
 void GridFormattingContext::initialize_grid_tracks(Box const& box, AvailableSpace const& available_space, int column_count, int row_count)
@@ -740,8 +740,10 @@ void GridFormattingContext::calculate_sizes_of_columns(Box const& box, Available
     // not a flexible sizing function, consider the items in it with a span of 1:
     int index = 0;
     for (auto& grid_column : m_grid_columns) {
-        if (grid_column.is_gap)
+        if (grid_column.is_gap) {
+            ++index;
             continue;
+        }
         if (!grid_column.min_track_sizing_function.is_intrinsic_track_sizing()) {
             ++index;
             continue;
@@ -749,8 +751,8 @@ void GridFormattingContext::calculate_sizes_of_columns(Box const& box, Available
 
         Vector<Box const&> boxes_of_column;
         for (auto& positioned_box : m_positioned_boxes) {
-            if (positioned_box.column == index && positioned_box.column_span == 1)
-                boxes_of_column.append(positioned_box.box);
+            if (positioned_box.gap_adjusted_column(box) == index && positioned_box.raw_column_span() == 1)
+                boxes_of_column.append(positioned_box.box());
         }
 
         switch (grid_column.min_track_sizing_function.type()) {
@@ -1238,8 +1240,10 @@ void GridFormattingContext::calculate_sizes_of_rows(Box const& box)
     // not a flexible sizing function, consider the items in it with a span of 1:
     auto index = 0;
     for (auto& grid_row : m_grid_rows) {
-        if (grid_row.is_gap)
+        if (grid_row.is_gap) {
+            ++index;
             continue;
+        }
         if (!grid_row.min_track_sizing_function.is_intrinsic_track_sizing()) {
             ++index;
             continue;
@@ -1247,7 +1251,7 @@ void GridFormattingContext::calculate_sizes_of_rows(Box const& box)
 
         Vector<PositionedBox&> positioned_boxes_of_row;
         for (auto& positioned_box : m_positioned_boxes) {
-            if (positioned_box.row == index && positioned_box.row_span == 1)
+            if (positioned_box.gap_adjusted_row(box) == index && positioned_box.raw_row_span() == 1)
                 positioned_boxes_of_row.append(positioned_box);
         }
 
@@ -1258,7 +1262,7 @@ void GridFormattingContext::calculate_sizes_of_rows(Box const& box)
         case CSS::GridSize::Type::MinContent: {
             CSSPixels row_height = 0;
             for (auto& positioned_box : positioned_boxes_of_row)
-                row_height = max(row_height, calculate_min_content_height(positioned_box.box, AvailableSize::make_definite(m_grid_columns[positioned_box.column].base_size)));
+                row_height = max(row_height, calculate_min_content_height(positioned_box.box(), AvailableSize::make_definite(m_grid_columns[positioned_box.gap_adjusted_column(box)].base_size)));
             grid_row.base_size = row_height;
         } break;
         // - For max-content minimums:
@@ -1267,7 +1271,7 @@ void GridFormattingContext::calculate_sizes_of_rows(Box const& box)
         case CSS::GridSize::Type::MaxContent: {
             CSSPixels row_height = 0;
             for (auto& positioned_box : positioned_boxes_of_row)
-                row_height = max(row_height, calculate_max_content_height(positioned_box.box, AvailableSize::make_definite(m_grid_columns[positioned_box.column].base_size)));
+                row_height = max(row_height, calculate_max_content_height(positioned_box.box(), AvailableSize::make_definite(m_grid_columns[positioned_box.gap_adjusted_column(box)].base_size)));
             grid_row.base_size = row_height;
         } break;
         // - For auto minimums:
@@ -1291,7 +1295,7 @@ void GridFormattingContext::calculate_sizes_of_rows(Box const& box)
         case CSS::GridSize::Type::FlexibleLength: {
             CSSPixels grid_row_height = 0;
             for (auto& positioned_box : positioned_boxes_of_row)
-                grid_row_height = max(grid_row_height, calculate_min_content_height(positioned_box.box, AvailableSize::make_definite(m_grid_columns[positioned_box.column].base_size)));
+                grid_row_height = max(grid_row_height, calculate_min_content_height(positioned_box.box(), AvailableSize::make_definite(m_grid_columns[positioned_box.gap_adjusted_column(box)].base_size)));
             grid_row.base_size = grid_row_height;
         } break;
         default:
@@ -1305,7 +1309,7 @@ void GridFormattingContext::calculate_sizes_of_rows(Box const& box)
         case CSS::GridSize::Type::MinContent: {
             CSSPixels row_height = 0;
             for (auto& positioned_box : positioned_boxes_of_row)
-                row_height = max(row_height, calculate_max_content_height(positioned_box.box, AvailableSize::make_definite(m_grid_columns[positioned_box.column].base_size)));
+                row_height = max(row_height, calculate_max_content_height(positioned_box.box(), AvailableSize::make_definite(m_grid_columns[positioned_box.gap_adjusted_column(box)].base_size)));
             grid_row.base_size = row_height;
         } break;
         // - For max-content maximums:
@@ -1315,7 +1319,7 @@ void GridFormattingContext::calculate_sizes_of_rows(Box const& box)
         case CSS::GridSize::Type::MaxContent: {
             CSSPixels row_height = 0;
             for (auto& positioned_box : positioned_boxes_of_row)
-                row_height = max(row_height, calculate_max_content_height(positioned_box.box, AvailableSize::make_definite(m_grid_columns[positioned_box.column].base_size)));
+                row_height = max(row_height, calculate_max_content_height(positioned_box.box(), AvailableSize::make_definite(m_grid_columns[positioned_box.gap_adjusted_column(box)].base_size)));
             grid_row.base_size = row_height;
         } break;
         case CSS::GridSize::Type::Length:
@@ -1871,26 +1875,24 @@ void GridFormattingContext::run(Box const& box, LayoutMode, AvailableSpace const
     };
 
     for (auto& positioned_box : m_positioned_boxes) {
-        auto resolved_row_start = box.computed_values().row_gap().is_auto() ? positioned_box.row : positioned_box.row * 2;
-        auto resolved_row_span = box.computed_values().row_gap().is_auto() ? positioned_box.row_span : positioned_box.row_span * 2;
-        if (!box.computed_values().row_gap().is_auto() && resolved_row_start == 0)
+        auto resolved_row_span = box.computed_values().row_gap().is_auto() ? positioned_box.raw_row_span() : positioned_box.raw_row_span() * 2;
+        if (!box.computed_values().row_gap().is_auto() && positioned_box.gap_adjusted_row(box) == 0)
             resolved_row_span -= 1;
-        if (resolved_row_start + resolved_row_span > static_cast<int>(m_grid_rows.size()))
-            resolved_row_span = m_grid_rows.size() - resolved_row_start;
+        if (positioned_box.gap_adjusted_row(box) + resolved_row_span > static_cast<int>(m_grid_rows.size()))
+            resolved_row_span = m_grid_rows.size() - positioned_box.gap_adjusted_row(box);
 
-        auto resolved_column_start = box.computed_values().column_gap().is_auto() ? positioned_box.column : positioned_box.column * 2;
-        auto resolved_column_span = box.computed_values().column_gap().is_auto() ? positioned_box.column_span : positioned_box.column_span * 2;
-        if (!box.computed_values().column_gap().is_auto() && resolved_column_start == 0)
+        auto resolved_column_span = box.computed_values().column_gap().is_auto() ? positioned_box.raw_column_span() : positioned_box.raw_column_span() * 2;
+        if (!box.computed_values().column_gap().is_auto() && positioned_box.gap_adjusted_column(box) == 0)
             resolved_column_span -= 1;
-        if (resolved_column_start + resolved_column_span > static_cast<int>(m_grid_columns.size()))
-            resolved_column_span = m_grid_columns.size() - resolved_column_start;
+        if (positioned_box.gap_adjusted_column(box) + resolved_column_span > static_cast<int>(m_grid_columns.size()))
+            resolved_column_span = m_grid_columns.size() - positioned_box.gap_adjusted_column(box);
 
         layout_box(
-            resolved_row_start,
-            resolved_row_start + resolved_row_span,
-            resolved_column_start,
-            resolved_column_start + resolved_column_span,
-            positioned_box.box);
+            positioned_box.gap_adjusted_row(box),
+            positioned_box.gap_adjusted_row(box) + resolved_row_span,
+            positioned_box.gap_adjusted_column(box),
+            positioned_box.gap_adjusted_column(box) + resolved_column_span,
+            positioned_box.box());
     }
 
     CSSPixels total_y = 0;
@@ -2040,4 +2042,14 @@ bool OccupationGrid::is_occupied(int column_index, int row_index)
     return m_occupation_grid[row_index][column_index];
 }
 
+int PositionedBox::gap_adjusted_row(Box const& parent_box)
+{
+    return parent_box.computed_values().row_gap().is_auto() ? m_row : m_row * 2;
+}
+
+int PositionedBox::gap_adjusted_column(Box const& parent_box)
+{
+    return parent_box.computed_values().column_gap().is_auto() ? m_column : m_column * 2;
+}
+
 }

+ 28 - 9
Userland/Libraries/LibWeb/Layout/GridFormattingContext.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, Martin Falisse <mfalisse@outlook.com>
+ * Copyright (c) 2022-2023, Martin Falisse <mfalisse@outlook.com>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -29,6 +29,33 @@ private:
     Vector<Vector<bool>> m_occupation_grid;
 };
 
+class PositionedBox {
+public:
+    PositionedBox(Box const& box, int row, int row_span, int column, int column_span)
+        : m_box(box)
+        , m_row(row)
+        , m_row_span(row_span)
+        , m_column(column)
+        , m_column_span(column_span)
+    {
+    }
+
+    Box const& box() { return m_box; }
+
+    int raw_row_span() { return m_row_span; }
+    int raw_column_span() { return m_column_span; }
+
+    int gap_adjusted_row(Box const& parent_box);
+    int gap_adjusted_column(Box const& parent_box);
+
+private:
+    Box const& m_box;
+    int m_row { 0 };
+    int m_row_span { 1 };
+    int m_column { 0 };
+    int m_column_span { 1 };
+};
+
 class GridFormattingContext final : public FormattingContext {
 public:
     explicit GridFormattingContext(LayoutState&, Box const& grid_container, FormattingContext* parent);
@@ -43,14 +70,6 @@ private:
     bool is_auto_positioned_column(CSS::GridTrackPlacement const&, CSS::GridTrackPlacement const&) const;
     bool is_auto_positioned_track(CSS::GridTrackPlacement const&, CSS::GridTrackPlacement const&) const;
 
-    struct PositionedBox {
-        Box const& box;
-        int row { 0 };
-        int row_span { 1 };
-        int column { 0 };
-        int column_span { 1 };
-    };
-
     struct TemporaryTrack {
         CSS::GridSize min_track_sizing_function;
         CSS::GridSize max_track_sizing_function;