From 152ce889846563ce7bca80a6123e1f0819b7b2e3 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Tue, 25 Jul 2023 22:18:08 +0200 Subject: [PATCH] LibWeb: Avoid leaking infinite remaining_free_space in FFC calculation After switching to fixed-point arithmetic in CSSPixels, it no longer supports representing infinite values, which was previously the case for remaining_free_space in FFC. Using Optional that is not empty only when value is finite to store remaining_free_space ensures that infinity is avoided in layout calculations. --- .../inf-available-space-with-auto-margins.txt | 8 +++++ ...inf-available-space-with-auto-margins.html | 10 ++++++ .../LibWeb/Layout/FlexFormattingContext.cpp | 33 ++++++++++--------- .../LibWeb/Layout/FlexFormattingContext.h | 2 +- 4 files changed, 36 insertions(+), 17 deletions(-) create mode 100644 Tests/LibWeb/Layout/expected/flex/inf-available-space-with-auto-margins.txt create mode 100644 Tests/LibWeb/Layout/input/flex/inf-available-space-with-auto-margins.html diff --git a/Tests/LibWeb/Layout/expected/flex/inf-available-space-with-auto-margins.txt b/Tests/LibWeb/Layout/expected/flex/inf-available-space-with-auto-margins.txt new file mode 100644 index 0000000000..3929cab693 --- /dev/null +++ b/Tests/LibWeb/Layout/expected/flex/inf-available-space-with-auto-margins.txt @@ -0,0 +1,8 @@ +Viewport <#document> at (0,0) content-size 800x600 children: not-inline + BlockContainer at (0,0) content-size 800x33.46875 [BFC] children: not-inline + Box at (8,8) content-size 784x17.46875 flex-container(column) [FFC] children: not-inline + BlockContainer
at (8,8) content-size 784x17.46875 flex-item [BFC] children: inline + line 0 width: 153.984375, height: 17.46875, bottom: 17.46875, baseline: 13.53125 + frag 0 from TextNode start: 0, length: 13, rect: [8,8 153.984375x17.46875] + "hmmMMMMmmmmmm" + TextNode <#text> diff --git a/Tests/LibWeb/Layout/input/flex/inf-available-space-with-auto-margins.html b/Tests/LibWeb/Layout/input/flex/inf-available-space-with-auto-margins.html new file mode 100644 index 0000000000..e728264ce7 --- /dev/null +++ b/Tests/LibWeb/Layout/input/flex/inf-available-space-with-auto-margins.html @@ -0,0 +1,10 @@ +
hmmMMMMmmmmmm \ No newline at end of file diff --git a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp index cd7d058b02..4fd48a79c8 100644 --- a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp +++ b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp @@ -949,7 +949,11 @@ void FlexFormattingContext::resolve_flexible_lengths_for_line(FlexLine& line) // Sum the outer sizes of all items on the line, and subtract this from the flex container’s inner main size. // For frozen items, use their outer target main size; for other items, use their outer flex base size. - auto calculate_remaining_free_space = [&]() -> CSSPixels { + auto calculate_remaining_free_space = [&]() -> Optional { + // AD-HOC: If the container is sized under max-content constraints, then remaining_free_space won't have + // a value to avoid leaking an infinite value into layout calculations. + if (available_main_size.might_be_saturated()) + return {}; CSSPixels sum = 0; for (auto const& item : line.items) { if (item.frozen) @@ -978,16 +982,16 @@ void FlexFormattingContext::resolve_flexible_lengths_for_line(FlexLine& line) line.remaining_free_space = calculate_remaining_free_space(); // If the sum of the unfrozen flex items’ flex factors is less than one, multiply the initial free space by this sum. - if (auto sum_of_flex_factor_of_unfrozen_items = line.sum_of_flex_factor_of_unfrozen_items(); sum_of_flex_factor_of_unfrozen_items < 1) { - auto value = initial_free_space * sum_of_flex_factor_of_unfrozen_items; + if (auto sum_of_flex_factor_of_unfrozen_items = line.sum_of_flex_factor_of_unfrozen_items(); sum_of_flex_factor_of_unfrozen_items < 1 && initial_free_space.has_value()) { + auto value = initial_free_space.value() * sum_of_flex_factor_of_unfrozen_items; // If the magnitude of this value is less than the magnitude of the remaining free space, use this as the remaining free space. - if (abs(value) < abs(line.remaining_free_space)) + if (abs(value) < abs(line.remaining_free_space.value())) line.remaining_free_space = value; } // AD-HOC: We allow the remaining free space to be infinite, but we can't let infinity // leak into the layout geometry, so we treat infinity as zero when used in arithmetic. - auto remaining_free_space_or_zero_if_infinite = !line.remaining_free_space.might_be_saturated() ? line.remaining_free_space : 0; + auto remaining_free_space_or_zero_if_infinite = line.remaining_free_space.has_value() ? line.remaining_free_space.value() : 0; // c. If the remaining free space is non-zero, distribute it proportional to the flex factors: if (line.remaining_free_space != 0) { @@ -1094,11 +1098,6 @@ void FlexFormattingContext::resolve_flexible_lengths_for_line(FlexLine& line) // NOTE: Calculate the remaining free space once again here, since it's needed later when aligning items. line.remaining_free_space = calculate_remaining_free_space(); - // AD-HOC: Due to the way we calculate the remaining free space, it can be infinite when sizing - // under a max-content constraint. In that case, we can simply set it to zero here. - if (line.remaining_free_space.might_be_saturated()) - line.remaining_free_space = 0; - // 6. Set each item’s used main size to its target main size. for (auto& item : line.items) { item.main_size = item.target_main_size; @@ -1278,8 +1277,8 @@ void FlexFormattingContext::distribute_any_remaining_free_space() // CSS-FLEXBOX-2: Account for gap between flex items. used_main_space += main_gap() * (flex_line.items.size() - 1); - if (flex_line.remaining_free_space > 0) { - CSSPixels size_per_auto_margin = flex_line.remaining_free_space / static_cast(auto_margins); + if (flex_line.remaining_free_space.has_value() && flex_line.remaining_free_space.value() > 0 && auto_margins > 0) { + CSSPixels size_per_auto_margin = flex_line.remaining_free_space.value() / auto_margins; for (auto& item : flex_line.items) { if (item.margins.main_before_is_auto) set_main_axis_first_margin(item, size_per_auto_margin); @@ -1331,11 +1330,12 @@ void FlexFormattingContext::distribute_any_remaining_free_space() } else { initial_offset = 0; } - if (number_of_items > 1) - space_between_items = flex_line.remaining_free_space / (number_of_items - 1); + if (flex_line.remaining_free_space.has_value() && number_of_items > 1) + space_between_items = flex_line.remaining_free_space.value() / (number_of_items - 1); break; case CSS::JustifyContent::SpaceAround: - space_between_items = flex_line.remaining_free_space / number_of_items; + if (flex_line.remaining_free_space.has_value()) + space_between_items = flex_line.remaining_free_space.value() / number_of_items; if (is_direction_reverse()) { initial_offset = inner_main_size(flex_container()) - space_between_items / 2.0; } else { @@ -1343,7 +1343,8 @@ void FlexFormattingContext::distribute_any_remaining_free_space() } break; case CSS::JustifyContent::SpaceEvenly: - space_between_items = flex_line.remaining_free_space / (number_of_items + 1); + if (flex_line.remaining_free_space.has_value()) + space_between_items = flex_line.remaining_free_space.value() / (number_of_items + 1); if (is_direction_reverse()) { initial_offset = inner_main_size(flex_container()) - space_between_items; } else { diff --git a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.h b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.h index 5a887b8c58..d96b02d776 100644 --- a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.h +++ b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.h @@ -109,7 +109,7 @@ private: struct FlexLine { Vector items; CSSPixels cross_size { 0 }; - CSSPixels remaining_free_space { 0 }; + Optional remaining_free_space; double chosen_flex_fraction { 0 }; double sum_of_flex_factor_of_unfrozen_items() const;