From ab4cf7c57de3b11e4a621bdc85daefe754800d85 Mon Sep 17 00:00:00 2001 From: Mathis Wiehl Date: Fri, 10 Mar 2023 21:46:18 +0100 Subject: [PATCH] LibWeb: Don't overflow flex containers on margin auto In case flex items had `margin: auto` on the primary flex axis, we were still also distributing remaining space according to `justify-content` rules. This lead to duplicated spacing in various places and overflows. It looks like this issue was observed previously but missidentified because there was logic to ignore margins at the start and end which would partially paper over the root cause. However this created other bugs (like for example not having a margin at beginning and end ;-)) and I can find nothing in the spec or other browser behaviour that indicates that this is something that should be done. Now we skip justify-content space distribution alltogether if it has already been distributed to auto margins. --- .../flex-margin-auto-justify-content.txt | 21 +++++ .../flex-margin-auto-justify-content.html | 27 ++++++ .../LibWeb/Layout/FlexFormattingContext.cpp | 87 ++++++++++--------- 3 files changed, 92 insertions(+), 43 deletions(-) create mode 100644 Tests/LibWeb/Layout/expected/flex-margin-auto-justify-content.txt create mode 100644 Tests/LibWeb/Layout/input/flex-margin-auto-justify-content.html diff --git a/Tests/LibWeb/Layout/expected/flex-margin-auto-justify-content.txt b/Tests/LibWeb/Layout/expected/flex-margin-auto-justify-content.txt new file mode 100644 index 0000000000..f1d4d357c4 --- /dev/null +++ b/Tests/LibWeb/Layout/expected/flex-margin-auto-justify-content.txt @@ -0,0 +1,21 @@ +Viewport <#document> at (0,0) content-size 800x600 children: not-inline + BlockContainer at (0,0) content-size 800x70 children: not-inline + BlockContainer at (8,8) content-size 784x54 children: not-inline + Box at (9,9) content-size 600x52 flex-container(row) children: not-inline + BlockContainer at (20,10) content-size 150x50 flex-item children: inline + line 0 width: 86.359375, height: 17.46875, bottom: 17.46875, baseline: 13.53125 + frag 0 from TextNode start: 0, length: 11, rect: [20,10 86.359375x17.46875] + "left margin" + TextNode <#text> + BlockContainer at (172,10) content-size 150x50 flex-item children: inline + line 0 width: 141.28125, height: 17.46875, bottom: 17.46875, baseline: 13.53125 + frag 0 from TextNode start: 0, length: 18, rect: [172,10 141.28125x17.46875] + "follow immediately" + TextNode <#text> + BlockContainer at (458,10) content-size 150x50 flex-item children: inline + line 0 width: 138.296875, height: 17.46875, bottom: 17.46875, baseline: 13.53125 + frag 0 from TextNode start: 0, length: 17, rect: [458,10 138.296875x17.46875] + "over at the right" + TextNode <#text> + BlockContainer <(anonymous)> at (8,62) content-size 784x0 children: inline + TextNode <#text> diff --git a/Tests/LibWeb/Layout/input/flex-margin-auto-justify-content.html b/Tests/LibWeb/Layout/input/flex-margin-auto-justify-content.html new file mode 100644 index 0000000000..ca9d09d255 --- /dev/null +++ b/Tests/LibWeb/Layout/input/flex-margin-auto-justify-content.html @@ -0,0 +1,27 @@ + +
left margin
follow immediately
over at the right
diff --git a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp index 2dd053c07c..95abfb3bad 100644 --- a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp +++ b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp @@ -1255,82 +1255,83 @@ void FlexFormattingContext::distribute_any_remaining_free_space() CSSPixels initial_offset = 0; auto number_of_items = flex_line.items.size(); + if (auto_margins == 0) { + switch (flex_container().computed_values().justify_content()) { + case CSS::JustifyContent::Start: + case CSS::JustifyContent::FlexStart: + if (is_direction_reverse()) { + initial_offset = inner_main_size(flex_container()); + } else { + initial_offset = 0; + } + break; + case CSS::JustifyContent::End: + case CSS::JustifyContent::FlexEnd: + if (is_direction_reverse()) { + initial_offset = 0; + } else { + initial_offset = inner_main_size(flex_container()); + } + break; + case CSS::JustifyContent::Center: + initial_offset = (inner_main_size(flex_container()) - used_main_space) / 2.0f; + break; + case CSS::JustifyContent::SpaceBetween: + space_between_items = flex_line.remaining_free_space / (number_of_items - 1); + break; + case CSS::JustifyContent::SpaceAround: + space_between_items = flex_line.remaining_free_space / number_of_items; + initial_offset = space_between_items / 2.0f; + break; + } + } + + // For reverse, we use FlexRegionRenderCursor::Right + // to indicate the cursor offset is the end and render backwards + // Otherwise the cursor offset is the 'start' of the region or initial offset enum class FlexRegionRenderCursor { Left, Right }; - auto flex_region_render_cursor = FlexRegionRenderCursor::Left; - bool justification_is_centered = false; switch (flex_container().computed_values().justify_content()) { case CSS::JustifyContent::Start: case CSS::JustifyContent::FlexStart: if (is_direction_reverse()) { flex_region_render_cursor = FlexRegionRenderCursor::Right; - initial_offset = inner_main_size(flex_container()); - } else { - initial_offset = 0; } break; case CSS::JustifyContent::End: case CSS::JustifyContent::FlexEnd: - if (is_direction_reverse()) { - initial_offset = 0; - } else { + if (!is_direction_reverse()) { flex_region_render_cursor = FlexRegionRenderCursor::Right; - initial_offset = inner_main_size(flex_container()); } break; - case CSS::JustifyContent::Center: - initial_offset = (inner_main_size(flex_container()) - used_main_space) / 2.0f; - justification_is_centered = true; - break; - case CSS::JustifyContent::SpaceBetween: - space_between_items = flex_line.remaining_free_space / (number_of_items - 1); - break; - case CSS::JustifyContent::SpaceAround: - space_between_items = flex_line.remaining_free_space / number_of_items; - initial_offset = space_between_items / 2.0f; - justification_is_centered = true; + default: break; } - // For reverse, we use FlexRegionRenderCursor::Right - // to indicate the cursor offset is the end and render backwards - // Otherwise the cursor offset is the 'start' of the region or initial offset CSSPixels cursor_offset = initial_offset; - auto place_item = [&](FlexItem& item, bool is_first_item, bool is_last_item) { - // NOTE: For centered justifications (`center` and `space-around`) we ignore any margin - // before the first item, and after the last item. - - auto item_margin_before = item.margins.main_before; - auto item_margin_after = item.margins.main_after; - if (justification_is_centered) { - if (is_first_item) - item_margin_before = 0; - if (is_last_item) - item_margin_after = 0; - } - + auto place_item = [&](FlexItem& item) { auto amount_of_main_size_used = item.main_size.value() - + item_margin_before + + item.margins.main_before + item.borders.main_before + item.padding.main_before - + item_margin_after + + item.margins.main_after + item.borders.main_after + item.padding.main_after + space_between_items; if (is_direction_reverse()) { - item.main_offset = cursor_offset - item.main_size.value() - item_margin_after - item.borders.main_after - item.padding.main_after; + item.main_offset = cursor_offset - item.main_size.value() - item.margins.main_after - item.borders.main_after - item.padding.main_after; cursor_offset -= amount_of_main_size_used; } else if (flex_region_render_cursor == FlexRegionRenderCursor::Right) { cursor_offset -= amount_of_main_size_used; - item.main_offset = cursor_offset + item_margin_before + item.borders.main_before + item.padding.main_before; + item.main_offset = cursor_offset + item.margins.main_before + item.borders.main_before + item.padding.main_before; } else { - item.main_offset = cursor_offset + item_margin_before + item.borders.main_before + item.padding.main_before; + item.main_offset = cursor_offset + item.margins.main_before + item.borders.main_before + item.padding.main_before; cursor_offset += amount_of_main_size_used; } }; @@ -1338,12 +1339,12 @@ void FlexFormattingContext::distribute_any_remaining_free_space() if (is_direction_reverse() || flex_region_render_cursor == FlexRegionRenderCursor::Right) { for (ssize_t i = flex_line.items.size() - 1; i >= 0; --i) { auto& item = flex_line.items[i]; - place_item(item, i == static_cast(flex_line.items.size()) - 1, i == 0); + place_item(item); } } else { for (size_t i = 0; i < flex_line.items.size(); ++i) { auto& item = flex_line.items[i]; - place_item(item, i == 0, i == flex_line.items.size() - 1); + place_item(item); } } }