From 64f18a93c22fa5a245bfe5af4c71564565455ca5 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 27 Oct 2024 22:30:45 +0100 Subject: [PATCH] LibWeb: Make align-content on flex container behave more correctly In particular, this property now interacts correctly when the flex container has flex-wrap: wrap-reverse. This caused some "regressions" in WPT tests for negative overflow in flex containers, but the previous behavior wasn't correct either, it just happened to give false positives on tests. --- .../css-flexbox/align-content-horiz-002.txt | 38 ++-- .../css-flexbox/align-content-vert-002.txt | 38 ++-- .../css/css-flexbox/negative-overflow.txt | 6 +- .../LibWeb/Layout/FlexFormattingContext.cpp | 184 +++++++++++------- 4 files changed, 159 insertions(+), 107 deletions(-) diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/align-content-horiz-002.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/align-content-horiz-002.txt index 480dc05bcbf..d51c2ec6124 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/align-content-horiz-002.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/align-content-horiz-002.txt @@ -6,8 +6,8 @@ Rerun Found 72 tests -54 Pass -18 Fail +71 Pass +1 Fail Details Result Test Name MessagePass .flexbox div 1 Pass .flexbox div 2 @@ -17,22 +17,22 @@ Pass .flexbox div 5 Pass .flexbox div 6 Pass .flexbox div 7 Pass .flexbox div 8 -Fail .flexbox div 9 -Fail .flexbox div 10 -Fail .flexbox div 11 -Fail .flexbox div 12 -Fail .flexbox div 13 -Fail .flexbox div 14 -Fail .flexbox div 15 -Fail .flexbox div 16 -Fail .flexbox div 17 -Fail .flexbox div 18 -Fail .flexbox div 19 -Fail .flexbox div 20 -Fail .flexbox div 21 -Fail .flexbox div 22 -Fail .flexbox div 23 -Fail .flexbox div 24 +Pass .flexbox div 9 +Pass .flexbox div 10 +Pass .flexbox div 11 +Pass .flexbox div 12 +Pass .flexbox div 13 +Pass .flexbox div 14 +Pass .flexbox div 15 +Pass .flexbox div 16 +Pass .flexbox div 17 +Pass .flexbox div 18 +Pass .flexbox div 19 +Pass .flexbox div 20 +Pass .flexbox div 21 +Pass .flexbox div 22 +Pass .flexbox div 23 +Pass .flexbox div 24 Pass .flexbox div 25 Pass .flexbox div 26 Pass .flexbox div 27 @@ -41,7 +41,7 @@ Pass .flexbox div 29 Pass .flexbox div 30 Pass .flexbox div 31 Pass .flexbox div 32 -Fail .flexbox div 33 +Pass .flexbox div 33 Pass .flexbox div 34 Pass .flexbox div 35 Pass .flexbox div 36 diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/align-content-vert-002.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/align-content-vert-002.txt index 480dc05bcbf..d51c2ec6124 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/align-content-vert-002.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/align-content-vert-002.txt @@ -6,8 +6,8 @@ Rerun Found 72 tests -54 Pass -18 Fail +71 Pass +1 Fail Details Result Test Name MessagePass .flexbox div 1 Pass .flexbox div 2 @@ -17,22 +17,22 @@ Pass .flexbox div 5 Pass .flexbox div 6 Pass .flexbox div 7 Pass .flexbox div 8 -Fail .flexbox div 9 -Fail .flexbox div 10 -Fail .flexbox div 11 -Fail .flexbox div 12 -Fail .flexbox div 13 -Fail .flexbox div 14 -Fail .flexbox div 15 -Fail .flexbox div 16 -Fail .flexbox div 17 -Fail .flexbox div 18 -Fail .flexbox div 19 -Fail .flexbox div 20 -Fail .flexbox div 21 -Fail .flexbox div 22 -Fail .flexbox div 23 -Fail .flexbox div 24 +Pass .flexbox div 9 +Pass .flexbox div 10 +Pass .flexbox div 11 +Pass .flexbox div 12 +Pass .flexbox div 13 +Pass .flexbox div 14 +Pass .flexbox div 15 +Pass .flexbox div 16 +Pass .flexbox div 17 +Pass .flexbox div 18 +Pass .flexbox div 19 +Pass .flexbox div 20 +Pass .flexbox div 21 +Pass .flexbox div 22 +Pass .flexbox div 23 +Pass .flexbox div 24 Pass .flexbox div 25 Pass .flexbox div 26 Pass .flexbox div 27 @@ -41,7 +41,7 @@ Pass .flexbox div 29 Pass .flexbox div 30 Pass .flexbox div 31 Pass .flexbox div 32 -Fail .flexbox div 33 +Pass .flexbox div 33 Pass .flexbox div 34 Pass .flexbox div 35 Pass .flexbox div 36 diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/negative-overflow.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/negative-overflow.txt index 8a0ccc37e20..fca45316555 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/negative-overflow.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/css/css-flexbox/negative-overflow.txt @@ -6,8 +6,8 @@ Rerun Found 11 tests -5 Pass -6 Fail +4 Pass +7 Fail Details Result Test Name MessageFail .flexbox 1 Fail .flexbox 2 @@ -19,4 +19,4 @@ Fail .flexbox 7 Fail .flexbox 8 Pass .flexbox 9 Pass .flexbox 10 -Pass .flexbox 11 \ No newline at end of file +Fail .flexbox 11 \ No newline at end of file diff --git a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp index d966639f60b..71dbd42ddd1 100644 --- a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp +++ b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp @@ -828,9 +828,6 @@ void FlexFormattingContext::collect_flex_items_into_flex_lines() line_main_size += main_gap(); } m_flex_lines.append(move(line)); - - if (flex_container().computed_values().flex_wrap() == CSS::FlexWrap::WrapReverse) - m_flex_lines.reverse(); } // https://drafts.csswg.org/css-flexbox-1/#resolve-flexible-lengths @@ -1517,13 +1514,17 @@ void FlexFormattingContext::align_all_flex_items_along_the_cross_axis() } } -// https://www.w3.org/TR/css-flexbox-1/#algo-line-align +// https://drafts.csswg.org/css-flexbox-1/#algo-line-align void FlexFormattingContext::align_all_flex_lines() { + // Align all flex lines per align-content. + if (m_flex_lines.is_empty()) return; - // FIXME: Support reverse + bool wrap_reverse = flex_container().computed_values().flex_wrap() == CSS::FlexWrap::WrapReverse; + bool place_items_backwards = false; + bool iterate_lines_backwards = false; CSSPixels cross_size_of_flex_container = inner_cross_size(m_flex_container_state); @@ -1534,76 +1535,117 @@ void FlexFormattingContext::align_all_flex_lines() for (auto& item : flex_line.items) { item.cross_offset += center_of_line; } - } else { + return; + } - CSSPixels sum_of_flex_line_cross_sizes = 0; - for (auto& line : m_flex_lines) - sum_of_flex_line_cross_sizes += line.cross_size; + CSSPixels sum_of_flex_line_cross_sizes = 0; + for (auto& line : m_flex_lines) + sum_of_flex_line_cross_sizes += line.cross_size; - // CSS-FLEXBOX-2: Account for gap between flex lines. - sum_of_flex_line_cross_sizes += cross_gap() * (m_flex_lines.size() - 1); + // CSS-FLEXBOX-2: Account for gap between flex lines. + sum_of_flex_line_cross_sizes += cross_gap() * (m_flex_lines.size() - 1); - CSSPixels start_of_current_line = 0; - CSSPixels gap_size = 0; - switch (flex_container().computed_values().align_content()) { - case CSS::AlignContent::FlexStart: - case CSS::AlignContent::Start: + CSSPixels start_of_current_line = 0; + CSSPixels gap_size = 0; + switch (flex_container().computed_values().align_content()) { + case CSS::AlignContent::Start: + start_of_current_line = 0; + iterate_lines_backwards = wrap_reverse; + break; + case CSS::AlignContent::End: + start_of_current_line = cross_size_of_flex_container; + place_items_backwards = true; + iterate_lines_backwards = !wrap_reverse; + break; + case CSS::AlignContent::FlexStart: + if (wrap_reverse) { + start_of_current_line = cross_size_of_flex_container; + place_items_backwards = true; + } else { start_of_current_line = 0; - break; - case CSS::AlignContent::FlexEnd: - case CSS::AlignContent::End: - start_of_current_line = cross_size_of_flex_container - sum_of_flex_line_cross_sizes; - break; - case CSS::AlignContent::Center: + } + break; + case CSS::AlignContent::FlexEnd: + iterate_lines_backwards = true; + if (wrap_reverse) { + start_of_current_line = 0; + } else { + start_of_current_line = cross_size_of_flex_container; + place_items_backwards = true; + } + break; + case CSS::AlignContent::Center: + iterate_lines_backwards = wrap_reverse; + start_of_current_line = (cross_size_of_flex_container / 2) - (sum_of_flex_line_cross_sizes / 2); + break; + case CSS::AlignContent::SpaceBetween: { + if (wrap_reverse) { + start_of_current_line = cross_size_of_flex_container; + place_items_backwards = true; + } else { + start_of_current_line = 0; + } + + auto leftover_free_space = cross_size_of_flex_container - sum_of_flex_line_cross_sizes; + auto leftover_flex_lines_size = m_flex_lines.size(); + if (leftover_free_space >= 0 && leftover_flex_lines_size > 1) { + int gap_count = leftover_flex_lines_size - 1; + gap_size = leftover_free_space / gap_count; + } + break; + } + case CSS::AlignContent::SpaceAround: { + iterate_lines_backwards = wrap_reverse; + auto leftover_free_space = cross_size_of_flex_container - sum_of_flex_line_cross_sizes; + if (leftover_free_space < 0) { + // If the leftover free-space is negative this value is identical to center. start_of_current_line = (cross_size_of_flex_container / 2) - (sum_of_flex_line_cross_sizes / 2); break; - case CSS::AlignContent::SpaceBetween: { + } + + gap_size = leftover_free_space / m_flex_lines.size(); + + // The spacing between the first/last lines and the flex container edges is half the size of the spacing between flex lines. + start_of_current_line = gap_size / 2; + break; + } + case CSS::AlignContent::SpaceEvenly: { + iterate_lines_backwards = wrap_reverse; + auto leftover_free_space = cross_size_of_flex_container - sum_of_flex_line_cross_sizes; + if (leftover_free_space < 0) { + // If the leftover free-space is negative this value is identical to center. + start_of_current_line = (cross_size_of_flex_container / 2) - (sum_of_flex_line_cross_sizes / 2); + break; + } + + gap_size = leftover_free_space / (m_flex_lines.size() + 1); + + // The spacing between the first/last lines and the flex container edges is the size of the spacing between flex lines. + start_of_current_line = gap_size; + break; + } + + case CSS::AlignContent::Normal: + case CSS::AlignContent::Stretch: + if (wrap_reverse) { + start_of_current_line = cross_size_of_flex_container; + place_items_backwards = true; + } else { start_of_current_line = 0; + } + break; + } - auto leftover_free_space = cross_size_of_flex_container - sum_of_flex_line_cross_sizes; - auto leftover_flex_lines_size = m_flex_lines.size(); - if (leftover_free_space >= 0 && leftover_flex_lines_size > 1) { - int gap_count = leftover_flex_lines_size - 1; - gap_size = leftover_free_space / gap_count; + auto place_items = [&](FlexLine& flex_line) { + if (place_items_backwards) { + CSSPixels center_of_current_line = start_of_current_line - (flex_line.cross_size / 2); + for (auto& item : flex_line.items) { + item.cross_offset += center_of_current_line; } - break; - } - case CSS::AlignContent::SpaceAround: { - auto leftover_free_space = cross_size_of_flex_container - sum_of_flex_line_cross_sizes; - if (leftover_free_space < 0) { - // If the leftover free-space is negative this value is identical to center. - start_of_current_line = (cross_size_of_flex_container / 2) - (sum_of_flex_line_cross_sizes / 2); - break; - } - - gap_size = leftover_free_space / m_flex_lines.size(); - - // The spacing between the first/last lines and the flex container edges is half the size of the spacing between flex lines. - start_of_current_line = gap_size / 2; - break; - } - case CSS::AlignContent::SpaceEvenly: { - auto leftover_free_space = cross_size_of_flex_container - sum_of_flex_line_cross_sizes; - if (leftover_free_space < 0) { - // If the leftover free-space is negative this value is identical to center. - start_of_current_line = (cross_size_of_flex_container / 2) - (sum_of_flex_line_cross_sizes / 2); - break; - } - - gap_size = leftover_free_space / (m_flex_lines.size() + 1); - - // The spacing between the first/last lines and the flex container edges is the size of the spacing between flex lines. - start_of_current_line = gap_size; - break; - } - - case CSS::AlignContent::Normal: - case CSS::AlignContent::Stretch: - start_of_current_line = 0; - break; - } - - for (auto& flex_line : m_flex_lines) { + start_of_current_line -= flex_line.cross_size + gap_size; + // CSS-FLEXBOX-2: Account for gap between flex lines. + start_of_current_line -= cross_gap(); + } else { CSSPixels center_of_current_line = start_of_current_line + (flex_line.cross_size / 2); for (auto& item : flex_line.items) { item.cross_offset += center_of_current_line; @@ -1612,6 +1654,16 @@ void FlexFormattingContext::align_all_flex_lines() // CSS-FLEXBOX-2: Account for gap between flex lines. start_of_current_line += cross_gap(); } + }; + + if (iterate_lines_backwards) { + for (auto& flex_line : m_flex_lines.in_reverse()) { + place_items(flex_line); + } + } else { + for (auto& flex_line : m_flex_lines) { + place_items(flex_line); + } } }