From cfcc459140d45009acf5cc0365142a76d1ed0bd0 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Fri, 5 Jan 2024 04:24:36 +0100 Subject: [PATCH] LibWeb: Fix grid line name placement when repeat() is used Before this change, parsed grid-template-columns/grid-template-rows were represented as two lists: line names and track sizes. The problem with this approach is that it erases the relationship between tracks and their names, which results in unnecessarily complicated code that restores this data (incorrectly if repeat() is involved) during layout. This change solves that by representing line definitions as a list of sizes and names in the order they were defined. Visual progression https://genius.com/ --- .../grid/line-placement-with-repeat.txt | 18 +++ .../grid/line-placement-with-repeat.html | 14 +++ .../Libraries/LibWeb/CSS/GridTrackSize.cpp | 64 ++++++---- Userland/Libraries/LibWeb/CSS/GridTrackSize.h | 21 ++-- .../Libraries/LibWeb/CSS/Parser/Parser.cpp | 26 ++-- .../LibWeb/Layout/GridFormattingContext.cpp | 119 ++++++++++-------- .../LibWeb/Layout/GridFormattingContext.h | 10 +- 7 files changed, 168 insertions(+), 104 deletions(-) create mode 100644 Tests/LibWeb/Layout/expected/grid/line-placement-with-repeat.txt create mode 100644 Tests/LibWeb/Layout/input/grid/line-placement-with-repeat.html diff --git a/Tests/LibWeb/Layout/expected/grid/line-placement-with-repeat.txt b/Tests/LibWeb/Layout/expected/grid/line-placement-with-repeat.txt new file mode 100644 index 0000000000..1505ae3df9 --- /dev/null +++ b/Tests/LibWeb/Layout/expected/grid/line-placement-with-repeat.txt @@ -0,0 +1,18 @@ +Viewport <#document> at (0,0) content-size 800x600 children: not-inline + BlockContainer at (0,0) content-size 800x33.46875 [BFC] children: not-inline + BlockContainer at (8,8) content-size 784x17.46875 children: not-inline + Box at (8,8) content-size 784x17.46875 [GFC] children: not-inline + BlockContainer at (8,8) content-size 480x17.46875 [BFC] children: not-inline + BlockContainer at (8,8) content-size 480x17.46875 children: inline + line 0 width: 6.34375, height: 17.46875, bottom: 17.46875, baseline: 13.53125 + frag 0 from TextNode start: 0, length: 1, rect: [8,8 6.34375x17.46875] + "1" + TextNode <#text> + +ViewportPaintable (Viewport<#document>) [0,0 800x600] + PaintableWithLines (BlockContainer) [0,0 800x33.46875] + PaintableWithLines (BlockContainer) [8,8 784x17.46875] + PaintableBox (Box
.container) [8,8 784x17.46875] + PaintableWithLines (BlockContainer
.item) [8,8 480x17.46875] + PaintableWithLines (BlockContainer
.box) [8,8 480x17.46875] + TextPaintable (TextNode<#text>) diff --git a/Tests/LibWeb/Layout/input/grid/line-placement-with-repeat.html b/Tests/LibWeb/Layout/input/grid/line-placement-with-repeat.html new file mode 100644 index 0000000000..5c6a3041f7 --- /dev/null +++ b/Tests/LibWeb/Layout/input/grid/line-placement-with-repeat.html @@ -0,0 +1,14 @@ +
1
\ No newline at end of file diff --git a/Userland/Libraries/LibWeb/CSS/GridTrackSize.cpp b/Userland/Libraries/LibWeb/CSS/GridTrackSize.cpp index bd68e4661f..c84775dc7b 100644 --- a/Userland/Libraries/LibWeb/CSS/GridTrackSize.cpp +++ b/Userland/Libraries/LibWeb/CSS/GridTrackSize.cpp @@ -188,15 +188,21 @@ String ExplicitGridTrack::to_string() const } } -GridTrackSizeList::GridTrackSizeList(Vector track_list, Vector> line_names) - : m_track_list(track_list) - , m_line_names(line_names) +String GridLineNames::to_string() const +{ + StringBuilder builder; + builder.append("["sv); + builder.join(' ', names); + builder.append("]"sv); + return MUST(builder.to_string()); +} + +GridTrackSizeList::GridTrackSizeList(Vector>&& list) + : m_list(move(list)) { } GridTrackSizeList::GridTrackSizeList() - : m_track_list({}) - , m_line_names({}) { } @@ -208,30 +214,38 @@ GridTrackSizeList GridTrackSizeList::make_none() String GridTrackSizeList::to_string() const { StringBuilder builder; - auto print_line_names = [&](size_t index) -> void { - builder.append("["sv); - for (size_t y = 0; y < m_line_names[index].size(); ++y) { - builder.append(m_line_names[index][y]); - if (y != m_line_names[index].size() - 1) - builder.append(" "sv); - } - builder.append("]"sv); - }; - - for (size_t i = 0; i < m_track_list.size(); ++i) { - if (m_line_names.size() > 0 && m_line_names[i].size() > 0) { - print_line_names(i); + for (auto const& line_definition_or_name : m_list) { + if (!builder.is_empty()) builder.append(" "sv); + if (line_definition_or_name.has()) { + builder.append(line_definition_or_name.get().to_string()); + } else if (line_definition_or_name.has()) { + auto const& line_names = line_definition_or_name.get(); + builder.append(line_names.to_string()); } - builder.append(m_track_list[i].to_string()); - if (i < m_track_list.size() - 1) - builder.append(" "sv); - } - if (m_line_names.size() > 0 && m_line_names[m_track_list.size()].size() > 0) { - builder.append(" "sv); - print_line_names(m_track_list.size()); } return MUST(builder.to_string()); } +Vector GridTrackSizeList::track_list() const +{ + Vector track_list; + for (auto const& line_definition_or_name : m_list) { + if (line_definition_or_name.has()) + track_list.append(line_definition_or_name.get()); + } + return track_list; +} + +bool GridTrackSizeList::operator==(GridTrackSizeList const& other) const +{ + if (m_list.size() != other.m_list.size()) + return false; + for (size_t i = 0; i < m_list.size(); ++i) { + if (m_list[i] != other.m_list[i]) + return false; + } + return true; +} + } diff --git a/Userland/Libraries/LibWeb/CSS/GridTrackSize.h b/Userland/Libraries/LibWeb/CSS/GridTrackSize.h index d0bee2ed9b..0e23027ca9 100644 --- a/Userland/Libraries/LibWeb/CSS/GridTrackSize.h +++ b/Userland/Libraries/LibWeb/CSS/GridTrackSize.h @@ -84,25 +84,28 @@ private: GridSize m_max_grid_size; }; +struct GridLineNames { + Vector names; + + String to_string() const; + bool operator==(GridLineNames const& other) const { return names == other.names; } +}; + class GridTrackSizeList { public: - GridTrackSizeList(Vector track_list, Vector> line_names); + GridTrackSizeList(Vector>&& list); GridTrackSizeList(); static GridTrackSizeList make_none(); - Vector track_list() const { return m_track_list; } - Vector> line_names() const { return m_line_names; } + Vector track_list() const; + Vector> list() const { return m_list; } String to_string() const; - bool operator==(GridTrackSizeList const& other) const - { - return m_line_names == other.line_names() && m_track_list == other.track_list(); - } + bool operator==(GridTrackSizeList const& other) const; private: - Vector m_track_list; - Vector> m_line_names; + Vector> m_list; }; class GridRepeat { diff --git a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp index da9ad26042..35ae5f5a42 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp +++ b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp @@ -5372,8 +5372,7 @@ Optional Parser::parse_repeat(Vector const& com if (!part_two_tokens.has_next_token()) return {}; - Vector repeat_params; - Vector> line_names_list; + Vector> repeat_params; auto last_object_was_line_names = false; while (part_two_tokens.has_next_token()) { auto token = part_two_tokens.next_token(); @@ -5390,7 +5389,7 @@ Optional Parser::parse_repeat(Vector const& com line_names.append(current_block_token.token().ident().to_string()); block_tokens.skip_whitespace(); } - line_names_list.append(line_names); + repeat_params.append(GridLineNames { move(line_names) }); part_two_tokens.skip_whitespace(); } else { last_object_was_line_names = false; @@ -5408,8 +5407,6 @@ Optional Parser::parse_repeat(Vector const& com part_two_tokens.skip_whitespace(); } } - while (line_names_list.size() <= repeat_params.size()) - line_names_list.append({}); // Thus the precise syntax of the repeat() notation has several forms: // = repeat( [ ] , [ ? ]+ ? ) @@ -5431,11 +5428,11 @@ Optional Parser::parse_repeat(Vector const& com // each other, the name lists are merged. For example, repeat(2, [a] 1fr [b]) is equivalent to [a] // 1fr [b a] 1fr [b]. if (is_auto_fill) - return CSS::GridRepeat(CSS::GridTrackSizeList(repeat_params, line_names_list), CSS::GridRepeat::Type::AutoFill); + return GridRepeat(GridTrackSizeList(move(repeat_params)), GridRepeat::Type::AutoFill); else if (is_auto_fit) - return CSS::GridRepeat(CSS::GridTrackSizeList(repeat_params, line_names_list), CSS::GridRepeat::Type::AutoFit); + return GridRepeat(GridTrackSizeList(move(repeat_params)), GridRepeat::Type::AutoFit); else - return CSS::GridRepeat(CSS::GridTrackSizeList(repeat_params, line_names_list), repeat_count); + return GridRepeat(GridTrackSizeList(move(repeat_params)), repeat_count); } Optional Parser::parse_track_sizing_function(ComponentValue const& token) @@ -5480,8 +5477,7 @@ RefPtr Parser::parse_grid_track_size_list(TokenStream track_list; - Vector> line_names_list; + Vector> track_list; auto last_object_was_line_names = false; while (tokens.has_next_token()) { auto token = tokens.next_token(); @@ -5503,7 +5499,7 @@ RefPtr Parser::parse_grid_track_size_list(TokenStream Parser::parse_grid_track_size_list(TokenStream Parser::parse_grid_auto_track_sizes(TokenStream+ - Vector track_list; + Vector> track_list; auto transaction = tokens.begin_transaction(); while (tokens.has_next_token()) { auto token = tokens.next_token(); @@ -5594,7 +5588,7 @@ RefPtr Parser::parse_grid_auto_track_sizes(TokenStream Parser::parse_grid_track_placement(TokenStream& tokens) diff --git a/Userland/Libraries/LibWeb/Layout/GridFormattingContext.cpp b/Userland/Libraries/LibWeb/Layout/GridFormattingContext.cpp index 5cc4a04027..4d950972ed 100644 --- a/Userland/Libraries/LibWeb/Layout/GridFormattingContext.cpp +++ b/Userland/Libraries/LibWeb/Layout/GridFormattingContext.cpp @@ -207,8 +207,8 @@ void GridFormattingContext::place_item_with_row_and_column_position(Box const& c if (grid_column_end.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_column_end.identifier()); maybe_grid_area.has_value()) column_end = maybe_grid_area->column_end; - else if (auto line_name_index = get_line_index_by_line_name(grid_column_end.identifier(), grid_container().computed_values().grid_template_columns()); line_name_index > -1) - column_end = line_name_index; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Column, grid_column_end.identifier()); line_name_index.has_value()) + column_end = line_name_index.value(); else column_end = 1; column_start = column_end - 1; @@ -217,8 +217,8 @@ void GridFormattingContext::place_item_with_row_and_column_position(Box const& c if (grid_column_start.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_column_start.identifier()); maybe_grid_area.has_value()) column_start = maybe_grid_area->column_start; - else if (auto line_name_index = get_line_index_by_line_name(grid_column_start.identifier(), grid_container().computed_values().grid_template_columns()); line_name_index > -1) - column_start = line_name_index; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Column, grid_column_start.identifier()); line_name_index.has_value()) + column_start = line_name_index.value(); else column_start = 0; } @@ -226,8 +226,8 @@ void GridFormattingContext::place_item_with_row_and_column_position(Box const& c if (grid_row_end.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_row_end.identifier()); maybe_grid_area.has_value()) row_end = maybe_grid_area->row_end; - else if (auto line_name_index = get_line_index_by_line_name(grid_row_end.identifier(), grid_container().computed_values().grid_template_rows()); line_name_index > -1) - row_end = line_name_index; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Row, grid_row_end.identifier()); line_name_index.has_value()) + row_end = line_name_index.value(); else row_end = 1; row_start = row_end - 1; @@ -236,8 +236,8 @@ void GridFormattingContext::place_item_with_row_and_column_position(Box const& c if (grid_row_start.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_row_start.identifier()); maybe_grid_area.has_value()) row_start = maybe_grid_area->row_start; - else if (auto line_name_index = get_line_index_by_line_name(grid_row_start.identifier(), grid_container().computed_values().grid_template_rows()); line_name_index > -1) - row_start = line_name_index; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Row, grid_row_start.identifier()); line_name_index.has_value()) + row_start = line_name_index.value(); else row_start = 0; } @@ -347,8 +347,8 @@ void GridFormattingContext::place_item_with_row_position(Box const& child_box) if (grid_row_end.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_row_end.identifier()); maybe_grid_area.has_value()) row_end = maybe_grid_area->row_end; - else if (auto line_name_index = get_line_index_by_line_name(grid_row_end.identifier(), grid_container().computed_values().grid_template_rows()); line_name_index > -1) - row_end = line_name_index - 1; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Row, grid_row_end.identifier()); line_name_index.has_value()) + row_end = line_name_index.value() - 1; else row_end = 1; row_start = row_end - 1; @@ -357,8 +357,8 @@ void GridFormattingContext::place_item_with_row_position(Box const& child_box) if (grid_row_start.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_row_start.identifier()); maybe_grid_area.has_value()) row_start = maybe_grid_area->row_start; - else if (auto line_name_index = get_line_index_by_line_name(grid_row_start.identifier(), grid_container().computed_values().grid_template_rows()); line_name_index > -1) - row_start = line_name_index; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Row, grid_row_start.identifier()); line_name_index.has_value()) + row_start = line_name_index.value(); else row_start = 0; } @@ -485,9 +485,9 @@ void GridFormattingContext::place_item_with_column_position(Box const& child_box if (grid_column_end.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_column_end.identifier()); maybe_grid_area.has_value()) column_end = maybe_grid_area->column_end; - else if (auto line_name_index = get_line_index_by_line_name(grid_column_end.identifier(), grid_container().computed_values().grid_template_columns()); line_name_index > -1) - column_end = line_name_index; - else + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Column, grid_column_end.identifier()); line_name_index.has_value()) { + column_end = line_name_index.value(); + } else column_end = 1; column_start = column_end - 1; } @@ -495,8 +495,8 @@ void GridFormattingContext::place_item_with_column_position(Box const& child_box if (grid_column_start.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_column_start.identifier()); maybe_grid_area.has_value()) column_start = maybe_grid_area->column_start; - else if (auto line_name_index = get_line_index_by_line_name(grid_column_start.identifier(), grid_container().computed_values().grid_template_columns()); line_name_index > -1) { - column_start = line_name_index; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Column, grid_column_start.identifier()); line_name_index.has_value()) { + column_start = line_name_index.value(); } else { column_start = 0; } @@ -1925,6 +1925,9 @@ void GridFormattingContext::run(Box const&, LayoutMode, AvailableSpace const& av { m_available_space = available_space; + init_grid_lines(GridDimension::Column); + init_grid_lines(GridDimension::Row); + auto const& grid_computed_values = grid_container().computed_values(); // NOTE: We store explicit grid sizes to later use in determining the position of items with negative index. @@ -2039,8 +2042,8 @@ void GridFormattingContext::layout_absolutely_positioned_element(Box const& box, if (grid_column_end.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_column_end.identifier()); maybe_grid_area.has_value()) column_end = maybe_grid_area->column_end; - else if (auto line_name_index = get_line_index_by_line_name(grid_column_end.identifier(), grid_container().computed_values().grid_template_columns()); line_name_index > -1) - column_end = line_name_index; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Column, grid_column_end.identifier()); line_name_index.has_value()) + column_end = line_name_index.value(); else column_end = 1; column_start = column_end - 1; @@ -2049,8 +2052,8 @@ void GridFormattingContext::layout_absolutely_positioned_element(Box const& box, if (grid_column_start.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_column_start.identifier()); maybe_grid_area.has_value()) column_start = maybe_grid_area->column_start; - else if (auto line_name_index = get_line_index_by_line_name(grid_column_start.identifier(), grid_container().computed_values().grid_template_columns()); line_name_index > -1) - column_start = line_name_index; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Column, grid_column_start.identifier()); line_name_index.has_value()) + column_start = line_name_index.value(); else column_start = 0; } @@ -2058,8 +2061,8 @@ void GridFormattingContext::layout_absolutely_positioned_element(Box const& box, if (grid_row_end.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_row_end.identifier()); maybe_grid_area.has_value()) row_end = maybe_grid_area->row_end; - else if (auto line_name_index = get_line_index_by_line_name(grid_row_end.identifier(), grid_container().computed_values().grid_template_rows()); line_name_index > -1) - row_end = line_name_index; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Row, grid_row_end.identifier()); line_name_index.has_value()) + row_end = line_name_index.value(); else row_end = 1; row_start = row_end - 1; @@ -2068,8 +2071,8 @@ void GridFormattingContext::layout_absolutely_positioned_element(Box const& box, if (grid_row_start.has_identifier()) { if (auto maybe_grid_area = m_grid_areas.get(grid_row_start.identifier()); maybe_grid_area.has_value()) row_start = maybe_grid_area->row_start; - else if (auto line_name_index = get_line_index_by_line_name(grid_row_start.identifier(), grid_container().computed_values().grid_template_rows()); line_name_index > -1) - row_start = line_name_index; + else if (auto line_name_index = get_line_index_by_line_name(GridDimension::Row, grid_row_start.identifier()); line_name_index.has_value()) + row_start = line_name_index.value(); else row_start = 0; } @@ -2245,37 +2248,47 @@ AvailableSize GridFormattingContext::get_free_space(AvailableSpace const& availa return available_size; } -int GridFormattingContext::get_line_index_by_line_name(String const& needle, CSS::GridTrackSizeList grid_track_size_list) +Optional GridFormattingContext::get_line_index_by_line_name(GridDimension dimension, String const& line_name) { - if (grid_track_size_list.track_list().size() == 0) - return -1; - - auto repeated_tracks_count = 0; - for (size_t x = 0; x < grid_track_size_list.track_list().size(); x++) { - if (grid_track_size_list.track_list()[x].is_repeat()) { - // FIXME: Calculate amount of columns/rows if auto-fill/fit - if (!grid_track_size_list.track_list()[x].repeat().is_default()) - return -1; - auto repeat = grid_track_size_list.track_list()[x].repeat().grid_track_size_list(); - for (size_t y = 0; y < repeat.track_list().size(); y++) { - for (size_t z = 0; z < repeat.line_names()[y].size(); z++) { - if (repeat.line_names()[y][z] == needle) - return x + repeated_tracks_count; - repeated_tracks_count++; - } - } - } else { - for (size_t y = 0; y < grid_track_size_list.line_names()[x].size(); y++) { - if (grid_track_size_list.line_names()[x][y] == needle) - return x + repeated_tracks_count; - } + auto const& lines = dimension == GridDimension::Column ? m_column_lines : m_row_lines; + for (size_t line_index = 0; line_index < lines.size(); line_index++) { + for (auto const& name : lines[line_index].names) { + if (name == line_name) + return static_cast(line_index); } } - for (size_t y = 0; y < grid_track_size_list.line_names()[grid_track_size_list.track_list().size()].size(); y++) { - if (grid_track_size_list.line_names()[grid_track_size_list.track_list().size()][y] == needle) - return grid_track_size_list.track_list().size() + repeated_tracks_count; - } - return -1; + return {}; +} + +void GridFormattingContext::init_grid_lines(GridDimension dimension) +{ + auto const& grid_computed_values = grid_container().computed_values(); + auto const& lines_definition = dimension == GridDimension::Column ? grid_computed_values.grid_template_columns() : grid_computed_values.grid_template_rows(); + auto& lines = dimension == GridDimension::Column ? m_column_lines : m_row_lines; + + Vector line_names; + Function expand_lines_definition = [&](CSS::GridTrackSizeList const& lines_definition) { + for (auto const& item : lines_definition.list()) { + if (item.has()) { + line_names.extend(item.get().names); + } else if (item.has()) { + auto const& explicit_track = item.get(); + if (explicit_track.is_default() || explicit_track.is_minmax()) { + lines.append({ .names = line_names }); + line_names.clear(); + } else if (explicit_track.is_repeat() && explicit_track.repeat().is_default()) { + auto const& repeat_track = explicit_track.repeat(); + for (int i = 0; i < repeat_track.repeat_count(); i++) { + expand_lines_definition(repeat_track.grid_track_size_list()); + } + } + } + } + }; + + expand_lines_definition(lines_definition); + if (line_names.size() > 0) + lines.append({ .names = line_names }); } void OccupationGrid::set_occupied(int column_start, int column_end, int row_start, int row_end) diff --git a/Userland/Libraries/LibWeb/Layout/GridFormattingContext.h b/Userland/Libraries/LibWeb/Layout/GridFormattingContext.h index da92f83d05..6c93403bd4 100644 --- a/Userland/Libraries/LibWeb/Layout/GridFormattingContext.h +++ b/Userland/Libraries/LibWeb/Layout/GridFormattingContext.h @@ -147,6 +147,14 @@ private: bool invalid { false }; /* FIXME: Ignore ignore invalid areas during layout */ }; + struct GridLine { + Vector names; + }; + Vector m_row_lines; + Vector m_column_lines; + + void init_grid_lines(GridDimension); + HashMap m_grid_areas; Vector m_grid_rows; @@ -234,7 +242,7 @@ private: AvailableSize get_free_space(AvailableSpace const&, GridDimension const) const; - int get_line_index_by_line_name(String const& line_name, CSS::GridTrackSizeList); + Optional get_line_index_by_line_name(GridDimension dimension, String const&); CSSPixels resolve_definite_track_size(CSS::GridSize const&, AvailableSpace const&); int count_of_repeated_auto_fill_or_fit_tracks(Vector const& track_list, AvailableSpace const&); int get_count_of_tracks(Vector const&, AvailableSpace const&);