From f25203f245e56f4c6aa734db5930b3220907917f Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 14 Sep 2022 11:43:57 +0200 Subject: [PATCH] LibWeb: Don't re-resolve "auto" flex item sizes after definitizing them This is rather subtle and points to our architecture around definite sizes not being exactly right, but... At some points during flexbox layout, the spec tells us that the sizes of certain flex items are considered definite from this point on. We implement this by marking each item's associated UsedValues as "has-definite-width/height". However, this breaks code that tries to resolve computed "auto" sizes by taking the corresponding size from the containing block. The end result was that the 1st sizing pass in flexbox would find the right size for an "auto" sized item, but the 2nd pass would override the correct size with the containing block's content size in that axis instead. To work around the issue, FFC now remembers when it "definitizes" an item, and future attempts to resolve an "auto" computed size for that value will bypass the computed-auto-is-resolved-against-containing-block step of the algorithm. It's not perfect, and we'll need to think more about how to really represent these intermediate states relating to box sizes being definite.. --- .../LibWeb/Layout/FlexFormattingContext.cpp | 37 ++++++++----------- .../LibWeb/Layout/FlexFormattingContext.h | 6 ++- .../Libraries/LibWeb/Layout/LayoutState.cpp | 22 +++++++++++ .../Libraries/LibWeb/Layout/LayoutState.h | 3 ++ 4 files changed, 45 insertions(+), 23 deletions(-) diff --git a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp index 7dfb2642ae..9ef89e1e8a 100644 --- a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp +++ b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.cpp @@ -77,6 +77,7 @@ void FlexFormattingContext::run(Box const& run_box, LayoutMode layout_mode) auto item_inner_cross_size = item_preferred_outer_cross_size - item.margins.cross_before - item.margins.cross_after - item.padding.cross_before - item.padding.cross_after - item.borders.cross_before - item.borders.cross_after; set_cross_size(item.box, item_inner_cross_size); set_has_definite_cross_size(item.box, true); + item.has_assigned_definite_cross_size = true; } } } @@ -172,6 +173,7 @@ void FlexFormattingContext::run(Box const& run_box, LayoutMode layout_mode) for (auto& item : m_flex_items) { set_cross_size(item.box, item.cross_size); set_has_definite_cross_size(item.box, true); + item.has_assigned_definite_cross_size = true; } } } @@ -344,26 +346,18 @@ float FlexFormattingContext::specified_cross_size(Box const& box) const return is_row_layout() ? box_state.content_height() : box_state.content_width(); } -float FlexFormattingContext::resolved_definite_cross_size(Box const& box) const +float FlexFormattingContext::resolved_definite_cross_size(FlexItem const& item) const { - auto const& cross_value = is_row_layout() ? box.computed_values().height() : box.computed_values().width(); - if (cross_value.is_auto()) - return specified_cross_size(flex_container()); - if (cross_value.is_length()) - return specified_cross_size(box); - auto containing_block_size = !is_row_layout() ? containing_block_width_for(box) : containing_block_height_for(box); - return cross_value.resolved(box, CSS::Length::make_px(containing_block_size)).to_px(box); + if (item.has_assigned_definite_cross_size) + return specified_cross_size(item.box); + return !is_row_layout() ? m_state.resolved_definite_width(item.box) : m_state.resolved_definite_height(item.box); } -float FlexFormattingContext::resolved_definite_main_size(Box const& box) const +float FlexFormattingContext::resolved_definite_main_size(FlexItem const& item) const { - auto const& main_value = is_row_layout() ? box.computed_values().width() : box.computed_values().height(); - if (main_value.is_auto()) - return specified_main_size(flex_container()); - if (main_value.is_length()) - return specified_main_size(box); - auto containing_block_size = is_row_layout() ? containing_block_width_for(box) : containing_block_height_for(box); - return main_value.resolved(box, CSS::Length::make_px(containing_block_size)).to_px(box); + if (item.has_assigned_definite_main_size) + return specified_main_size(item.box); + return is_row_layout() ? m_state.resolved_definite_width(item.box) : m_state.resolved_definite_height(item.box); } bool FlexFormattingContext::has_main_min_size(Box const& box) const @@ -524,7 +518,7 @@ void FlexFormattingContext::determine_available_main_and_cross_space(bool& main_ auto containing_block_effective_main_size = [&](Box const& box) -> Optional { auto& containing_block = *box.containing_block(); if (has_definite_main_size(containing_block)) - return resolved_definite_main_size(containing_block); + return is_row_layout() ? m_state.resolved_definite_width(box) : m_state.resolved_definite_height(box); return {}; }; @@ -688,7 +682,7 @@ void FlexFormattingContext::determine_flex_base_size_and_hypothetical_main_size( && flex_item.used_flex_basis.type == CSS::FlexBasis::Content && has_definite_cross_size(flex_item.box)) { // flex_base_size is calculated from definite cross size and intrinsic aspect ratio - return resolved_definite_cross_size(flex_item.box) * flex_item.box.intrinsic_aspect_ratio().value(); + return resolved_definite_cross_size(flex_item) * flex_item.box.intrinsic_aspect_ratio().value(); } // C. If the used flex basis is content or depends on its available space, @@ -721,7 +715,7 @@ void FlexFormattingContext::determine_flex_base_size_and_hypothetical_main_size( // FIXME: This is probably too naive. // FIXME: Care about FlexBasis::Auto if (has_definite_main_size(child_box)) - return resolved_definite_main_size(child_box); + return resolved_definite_main_size(flex_item); // NOTE: To avoid repeated layout work, we keep a cache of flex item main sizes on the // root LayoutState object. It's available through a full layout cycle. @@ -774,7 +768,7 @@ Optional FlexFormattingContext::transferred_size_suggestion(FlexItem cons if (item.box.has_intrinsic_aspect_ratio() && has_definite_cross_size(item.box)) { auto aspect_ratio = item.box.intrinsic_aspect_ratio().value(); // FIXME: Clamp cross size to min/max cross size before this conversion. - return resolved_definite_cross_size(item.box) * aspect_ratio; + return resolved_definite_cross_size(item) * aspect_ratio; } // It is otherwise undefined. @@ -1033,6 +1027,7 @@ void FlexFormattingContext::resolve_flexible_lengths() // 2. If a flex-item’s flex basis is definite, then its post-flexing main size is also definite. if (has_definite_main_size(flex_container()) || flex_item->used_flex_basis_is_definite) { set_has_definite_main_size(flex_item->box, true); + flex_item->has_assigned_definite_main_size = true; } } @@ -1055,7 +1050,7 @@ void FlexFormattingContext::determine_hypothetical_cross_size_of_item(FlexItem& // If we have a definite cross size, this is easy! No need to perform layout, we can just use it as-is. if (has_definite_cross_size(item.box)) { - item.hypothetical_cross_size = css_clamp(resolved_definite_cross_size(item.box), clamp_min, clamp_max); + item.hypothetical_cross_size = css_clamp(resolved_definite_cross_size(item), clamp_min, clamp_max); return; } diff --git a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.h b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.h index 585057b75d..5b53852a76 100644 --- a/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.h +++ b/Userland/Libraries/LibWeb/Layout/FlexFormattingContext.h @@ -62,6 +62,8 @@ private: DirectionAgnosticMargins padding {}; bool is_min_violation { false }; bool is_max_violation { false }; + bool has_assigned_definite_main_size { false }; + bool has_assigned_definite_cross_size { false }; float add_main_margin_box_sizes(float content_size) const { @@ -85,8 +87,8 @@ private: bool has_definite_cross_size(Box const&) const; float specified_main_size(Box const&) const; float specified_cross_size(Box const&) const; - float resolved_definite_main_size(Box const&) const; - float resolved_definite_cross_size(Box const&) const; + float resolved_definite_main_size(FlexItem const&) const; + float resolved_definite_cross_size(FlexItem const&) const; bool has_main_min_size(Box const&) const; bool has_cross_min_size(Box const&) const; float specified_main_max_size(Box const&) const; diff --git a/Userland/Libraries/LibWeb/Layout/LayoutState.cpp b/Userland/Libraries/LibWeb/Layout/LayoutState.cpp index 29911b212f..ccd54fbe78 100644 --- a/Userland/Libraries/LibWeb/Layout/LayoutState.cpp +++ b/Userland/Libraries/LibWeb/Layout/LayoutState.cpp @@ -232,4 +232,26 @@ void LayoutState::UsedValues::set_content_height(float height) m_content_height = height; } +float LayoutState::resolved_definite_width(Box const& box) const +{ + auto const& computed_value = box.computed_values().width(); + if (computed_value.is_auto()) + return get(*box.containing_block()).content_width(); + if (computed_value.is_length()) + return get(box).content_width(); + auto containing_block_size = get(*box.containing_block()).content_width(); + return computed_value.resolved(box, CSS::Length::make_px(containing_block_size)).to_px(box); +} + +float LayoutState::resolved_definite_height(Box const& box) const +{ + auto const& computed_value = box.computed_values().height(); + if (computed_value.is_auto()) + return get(*box.containing_block()).content_height(); + if (computed_value.is_length()) + return get(box).content_height(); + auto containing_block_size = get(*box.containing_block()).content_height(); + return computed_value.resolved(box, CSS::Length::make_px(containing_block_size)).to_px(box); +} + } diff --git a/Userland/Libraries/LibWeb/Layout/LayoutState.h b/Userland/Libraries/LibWeb/Layout/LayoutState.h index f4c7b01ba7..d7b3ec01a1 100644 --- a/Userland/Libraries/LibWeb/Layout/LayoutState.h +++ b/Userland/Libraries/LibWeb/Layout/LayoutState.h @@ -124,6 +124,9 @@ struct LayoutState { HashTable m_floating_descendants; }; + float resolved_definite_width(Box const&) const; + float resolved_definite_height(Box const&) const; + void commit(); // NOTE: get_mutable() will CoW the UsedValues if it's inherited from an ancestor state;