Seed SVG partial relayout root from fragments tree

Libraries/LibWeb/DOM/Document.cpp

@@ -1764,18 +1764,19 @@ void Document::end_style_stabilization_epoch()
     m_animations_created_in_stabilization_epoch.clear();
 }
 
-static void relayout_subtree(Layout::Box& subtree_root, Painting::Paintable& old_paintable)
+static void relayout_subtree(Layout::Box& subtree_root)
 {
     Layout::LayoutRustBridge bridge;
     // Absolutely positioned boundaries re-resolve their own size and position by replaying
-    // their layout from saved inputs; SVG root boundaries keep the frozen geometry from the
-    // previous layout. Rust reads the old paintable before replacing it in either path.
+    // their layout from saved inputs; SVG root boundaries keep the frozen geometry saved at
+    // the previous commit. The commit sink resolves the paintable to splice out in either
+    // path.
     if (subtree_root.is_absolutely_positioned()) {
         VERIFY(subtree_root.containing_block());
         VERIFY(subtree_root.has_saved_abspos_layout_inputs());
-        bridge.replay_saved_abspos_layout(subtree_root, old_paintable);
+        bridge.replay_saved_abspos_layout(subtree_root);
     } else {
-        bridge.compute_subtree_layout(subtree_root, old_paintable);
+        bridge.compute_subtree_layout(subtree_root);
     }
 
     subtree_root.for_each_in_inclusive_subtree([](auto& node) {
@@ -2042,35 +2043,19 @@ Document::PartialRelayoutResult Document::try_partial_relayout(HashTable<WeakPtr
     // survived the build, plus the nearest boundary containing each rebuilt subtree - which
     // re-discovers a boundary whose own box the build replaced, since the saved layout inputs
     // carried over to the replacement.
-    struct PartialRelayoutRoot {
-        Layout::Box* box { nullptr };
-        RefPtr<Painting::Paintable> old_paintable;
-    };
-    Vector<PartialRelayoutRoot> partial_relayout_roots;
+    Vector<Layout::Box*> partial_relayout_roots;
     HashTable<Layout::Box*> collected_boundaries;
     auto collect_boundary = [&](Layout::Box& box, bool box_was_replaced) {
         if (collected_boundaries.set(&box) != AK::HashSetResult::InsertedNewEntry)
             return true;
 
-        RefPtr<Painting::Paintable> old_paintable = box.paintable_box();
-        if (!old_paintable && box_was_replaced && box.dom_node()) {
-            // A replaced box has no paintable yet; the previous one stays referenced by the
-            // DOM node until the next commit replaces it there.
-            old_paintable = box.dom_node()->unsafe_paintable();
-        }
-        if (!old_paintable)
-            return false;
-
         // A replaced box applies the saved-inputs validity check unconditionally: the change
         // that drove the replacement cannot be classified anymore.
         bool saved_inputs_may_be_style_stale = box.needs_own_geometry_update() || box_was_replaced;
         if (saved_inputs_may_be_style_stale && box.is_absolutely_positioned() && !Layout::can_replay_saved_abspos_layout_inputs_after_style_change(box))
             return false;
 
-        partial_relayout_roots.append({
-            .box = &box,
-            .old_paintable = old_paintable,
-        });
+        partial_relayout_roots.append(&box);
         return true;
     };
 
@@ -2091,7 +2076,7 @@ Document::PartialRelayoutResult Document::try_partial_relayout(HashTable<WeakPtr
         // The rebuilt box itself may qualify with its paintable still pending; boundaries
         // above it were not replaced and must have one.
         Layout::Box* containing_boundary = nullptr;
-        if (auto* rebuilt_box = as_if<Layout::Box>(*rebuilt_root); rebuilt_box && rebuilt_box->is_partial_relayout_boundary(Layout::RequireExistingPaintable::No))
+        if (auto* rebuilt_box = as_if<Layout::Box>(*rebuilt_root); rebuilt_box && rebuilt_box->is_partial_relayout_boundary())
             containing_boundary = rebuilt_box;
         for (auto* ancestor = rebuilt_root->parent(); !containing_boundary && ancestor; ancestor = ancestor->parent()) {
             if (auto* ancestor_box = as_if<Layout::Box>(*ancestor); ancestor_box && ancestor_box->is_partial_relayout_boundary())
@@ -2102,8 +2087,8 @@ Document::PartialRelayoutResult Document::try_partial_relayout(HashTable<WeakPtr
     }
 
     // A root nested inside another root is relaid out as part of the ancestor's subtree.
-    partial_relayout_roots.remove_all_matching([&](auto const& root) {
-        for (auto* ancestor = root.box->parent(); ancestor; ancestor = ancestor->parent()) {
+    partial_relayout_roots.remove_all_matching([&](auto* root) {
+        for (auto* ancestor = root->parent(); ancestor; ancestor = ancestor->parent()) {
             if (auto* ancestor_box = as_if<Layout::Box>(*ancestor); ancestor_box && collected_boundaries.contains(ancestor_box))
                 return true;
         }
@@ -2114,11 +2099,11 @@ Document::PartialRelayoutResult Document::try_partial_relayout(HashTable<WeakPtr
         return PartialRelayoutResult::NotEligible;
 
     layout_node_arena().sync_enrolled_content_for_layout();
-    for (auto const& root : partial_relayout_roots) {
-        relayout_subtree(*root.box, *root.old_paintable);
+    for (auto* root : partial_relayout_roots) {
+        relayout_subtree(*root);
         // NB: The subtree commit reset the root's descendant paintables, and the subtree's
         //     new size may change ancestor scrollable overflow; scheduling the root covers both.
-        schedule_scrollable_overflow_recalculation(*root.box);
+        schedule_scrollable_overflow_recalculation(*root);
     }
 
     ++m_partial_layout_count;

Libraries/LibWeb/Layout/Box.cpp

@@ -26,32 +26,37 @@ Box::~Box()
 {
 }
 
-bool Box::is_partial_relayout_boundary(RequireExistingPaintable require_existing_paintable) const
+bool Box::is_partial_relayout_boundary() const
 {
     // An absolutely or fixed positioned descendant whose containing block is outside this
     // box's subtree is laid out by a formatting context outside it, which makes subtree
     // isolation impossible for any kind of boundary.
     if (abspos_descendant_escapes())
         return false;
 
+    // Committing a subtree splices the new paint subtree into the old paintable's paint-tree
+    // position, so a boundary must still have one - either on the box or, for a box the tree
+    // builder just rebuilt, held by the DOM node until the next commit replaces it there.
+    if (!paintable_box() && !(dom_node() && dom_node()->unsafe_paintable()))
+        return false;
+
     // A nested <svg> never qualifies: its subtree is laid out in the outer SVG's
     // viewBox-transformed coordinate system, which a relayout rooted at the inner <svg> cannot
     // reproduce.
     bool is_outermost_svg_root = is_svg_svg_box() && !(parent() && (parent()->is_svg_box() || parent()->is_svg_svg_box()));
 
     // An in-flow SVG root's used size is determined solely by its own attributes and outer
     // context, never by its children, so its size and position from the previous layout can be
-    // reused. An absolutely positioned SVG root's placement is not frozen, so it must qualify
-    // through the saved-inputs replay path below instead.
+    // reused - provided a commit has actually saved them. An absolutely positioned SVG root's
+    // placement is not frozen, so it must qualify through the saved-inputs replay path below
+    // instead.
     if (is_svg_svg_box() && !is_absolutely_positioned())
-        return is_outermost_svg_root;
+        return is_outermost_svg_root && has_saved_committed_geometry();
 
     if (!is_absolutely_positioned())
         return false;
     if (is_anonymous())
         return false;
-    if (require_existing_paintable == RequireExistingPaintable::Yes && !paintable_box())
-        return false;
     if (dom_node() == document().document_element())
         return false;
     if (!has_saved_abspos_layout_inputs())

Libraries/LibWeb/Layout/Box.h

@@ -14,11 +14,6 @@
 
 namespace Web::Layout {
 
-enum class RequireExistingPaintable : u8 {
-    No,
-    Yes,
-};
-
 struct LineBoxFragmentCoordinate {
     size_t line_box_index { 0 };
     size_t fragment_index { 0 };
@@ -34,7 +29,7 @@ class WEB_API Box : public NodeWithStyle {
     // A partial relayout boundary is a box whose subtree can be re-laid out in
     // isolation: its own used size and position are guaranteed not to change
     // when layout is invalidated somewhere inside its subtree.
-    bool is_partial_relayout_boundary(RequireExistingPaintable = RequireExistingPaintable::Yes) const;
+    bool is_partial_relayout_boundary() const;
 
     // https://www.w3.org/TR/css-images-3/#natural-dimensions
     virtual CSS::SizeWithAspectRatio natural_size() const { return {}; }
@@ -59,6 +54,7 @@ class WEB_API Box : public NodeWithStyle {
     virtual RefPtr<Painting::Paintable> create_paintable() const override;
 
     bool has_saved_abspos_layout_inputs() const { return has_flag(RustFFI::NodeFlag::HasSavedAbsposLayoutInputs); }
+    bool has_saved_committed_geometry() const { return has_flag(RustFFI::NodeFlag::HasSavedCommittedGeometry); }
     bool saved_abspos_cb_derives_from_own_computed_values() const { return has_flag(RustFFI::NodeFlag::SavedAbsposCbDerivesFromOwnComputedValues); }
     bool saved_abspos_alignment_derives_from_own_computed_values() const { return has_flag(RustFFI::NodeFlag::SavedAbsposAlignmentDerivesFromOwnComputedValues); }
 

Libraries/LibWeb/Layout/LayoutRustBridge.cpp

@@ -684,7 +684,7 @@ void LayoutRustBridge::run_root_layout(Box& viewport, CSSPixels viewport_inline_
     VERIFY(!m_line_commit_context);
 }
 
-void LayoutRustBridge::compute_subtree_layout(Box& root, Painting::Paintable& paintable_to_replace)
+void LayoutRustBridge::compute_subtree_layout(Box& root)
 {
     VERIFY(!m_commit_root);
     m_commit_root = &root;
@@ -702,7 +702,6 @@ void LayoutRustBridge::compute_subtree_layout(Box& root, Painting::Paintable& pa
         RustFFI::rust_layout_compute_subtree_layout(
             Node::slot_id(&root),
             Node::slot_id(&root.root()),
-            &paintable_to_replace,
             viewport_rect.width().raw_value(),
             viewport_rect.height().raw_value(),
             &callbacks,
@@ -711,7 +710,7 @@ void LayoutRustBridge::compute_subtree_layout(Box& root, Painting::Paintable& pa
     VERIFY(!m_line_commit_context);
 }
 
-void LayoutRustBridge::replay_saved_abspos_layout(Box& box, Painting::Paintable& paintable_to_replace)
+void LayoutRustBridge::replay_saved_abspos_layout(Box& box)
 {
     VERIFY(!m_commit_root);
     m_commit_root = &box;
@@ -725,7 +724,7 @@ void LayoutRustBridge::replay_saved_abspos_layout(Box& box, Painting::Paintable&
     auto sink = commit_sink();
     {
         ActiveLayoutPassScope active_pass;
-        RustFFI::rust_layout_replay_saved_abspos_layout(Node::slot_id(&box), &paintable_to_replace, &callbacks, &sink);
+        RustFFI::rust_layout_replay_saved_abspos_layout(Node::slot_id(&box), &callbacks, &sink);
     }
     VERIFY(!m_line_commit_context);
 }
@@ -755,17 +754,20 @@ RustFFI::FfiCommitSink LayoutRustBridge::commit_sink()
 {
     return {
         .context = this,
-        .begin_commit = [](void* context, void* root_pointer, void* paintable_to_replace_pointer) {
+        .begin_commit = [](void* context, void* root_pointer) {
             auto& bridge = *static_cast<LayoutRustBridge*>(context);
             auto& root = *static_cast<Box*>(root_pointer);
             VERIFY(!bridge.m_replaced_paintable);
             VERIFY(!bridge.m_commit_parent_paintable);
             VERIFY(!bridge.m_commit_insert_before_paintable);
 
-            if (paintable_to_replace_pointer) {
-                bridge.m_replaced_paintable = *static_cast<Painting::Paintable*>(paintable_to_replace_pointer);
-            } else if (!root.is_viewport()) {
+            if (!root.is_viewport()) {
                 bridge.m_replaced_paintable = root.paintable();
+                if (!bridge.m_replaced_paintable && root.dom_node()) {
+                    // A rebuilt box has no paintable yet; the previous one stays referenced by
+                    // the DOM node (and alive in the paint tree) until this commit replaces it.
+                    bridge.m_replaced_paintable = root.dom_node()->unsafe_paintable();
+                }
             }
 
             if (bridge.m_replaced_paintable) {
@@ -1112,47 +1114,6 @@ RustFFI::FfiLayoutFcCallbacks LayoutRustBridge::formatting_context_callbacks()
             auto const* node_with_style = as_if<NodeWithStyle>(*static_cast<Node const*>(node));
             VERIFY(node_with_style);
             return build_svg_element_facts(*node_with_style); },
-        .read_paintable_geometry = [](void*, void* node, void* paintable_pointer, RustFFI::FfiPaintableGeometry* out) {
-            VERIFY(out);
-            VERIFY(paintable_pointer);
-            auto const* paintable = static_cast<Painting::Paintable const*>(paintable_pointer);
-            auto const& box_model = paintable->box_model();
-            *out = {
-                .content_inline_size = paintable->content_width().raw_value(),
-                .content_block_size = paintable->content_height().raw_value(),
-                .content_offset = {
-                    .x = paintable->offset().x().raw_value(),
-                    .y = paintable->offset().y().raw_value(),
-                },
-                .svg_viewport_size = {},
-                .margin_left = box_model.margin.left.raw_value(),
-                .margin_right = box_model.margin.right.raw_value(),
-                .margin_top = box_model.margin.top.raw_value(),
-                .margin_bottom = box_model.margin.bottom.raw_value(),
-                .border_left = box_model.border.left.raw_value(),
-                .border_right = box_model.border.right.raw_value(),
-                .border_top = box_model.border.top.raw_value(),
-                .border_bottom = box_model.border.bottom.raw_value(),
-                .padding_left = box_model.padding.left.raw_value(),
-                .padding_right = box_model.padding.right.raw_value(),
-                .padding_top = box_model.padding.top.raw_value(),
-                .padding_bottom = box_model.padding.bottom.raw_value(),
-                .inset_left = box_model.inset.left.raw_value(),
-                .inset_right = box_model.inset.right.raw_value(),
-                .inset_top = box_model.inset.top.raw_value(),
-                .inset_bottom = box_model.inset.bottom.raw_value(),
-            };
-
-            // NB: We check the node type rather than the paintable type to mirror the rust-side logic.
-            if (is<SVGSVGBox>(*static_cast<Node const*>(node))) {
-                auto const* svg_svg_paintable = as_if<Painting::SVGSVGPaintable>(paintable);
-                VERIFY(svg_svg_paintable);
-                out->svg_viewport_size = {
-                    .width = svg_svg_paintable->svg_viewport_size().width().raw_value(),
-                    .height = svg_svg_paintable->svg_viewport_size().height().raw_value(),
-                };
-            }
-            return true; },
         .compute_svg_path = [](void*, void* node, RustFFI::FfiSvgPathRequest request) {
             auto const* node_with_style = as_if<NodeWithStyle>(*static_cast<Node const*>(node));
             VERIFY(node_with_style);

Libraries/LibWeb/Layout/LayoutRustBridge.h

@@ -33,8 +33,8 @@ class LayoutRustBridge {
     ~LayoutRustBridge();
 
     void run_root_layout(Box& viewport, CSSPixels viewport_inline_size, CSSPixels viewport_block_size, bool should_collect_devtools_layout_data);
-    void compute_subtree_layout(Box&, Painting::Paintable& paintable_to_replace);
-    void replay_saved_abspos_layout(Box&, Painting::Paintable& paintable_to_replace);
+    void compute_subtree_layout(Box&);
+    void replay_saved_abspos_layout(Box&);
 
 private:
     [[nodiscard]] RustFFI::FfiLayoutFcCallbacks formatting_context_callbacks();

Libraries/LibWeb/Rust/src/layout/commit.rs

@@ -82,7 +82,7 @@ pub struct FfiPaintableGeometry {
 #[repr(C)]
 pub struct FfiCommitSink {
     pub context: *mut c_void,
-    pub begin_commit: unsafe extern "C" fn(*mut c_void, *mut c_void, *mut c_void) -> FfiCommitPosition,
+    pub begin_commit: unsafe extern "C" fn(*mut c_void, *mut c_void) -> FfiCommitPosition,
     pub finish_commit: unsafe extern "C" fn(*mut c_void),
     pub prepare_node: unsafe extern "C" fn(*mut c_void, *mut c_void, bool) -> *mut c_void,
     pub set_box_metrics: unsafe extern "C" fn(*mut c_void, *mut c_void, FfiCommittedBoxMetrics),
@@ -117,6 +117,40 @@ fn commit_subtree(
     let entry = scopes.link_for_slot(slot_index);
     if let Some(link) = entry {
         callbacks.set_saved_abspos_layout_inputs(node, link.abspos_layout_inputs);
+        // SVG roots are the only non-abspos partial relayout boundaries; save their committed
+        // geometry so a later subtree pass can seed itself without reading the old paintable.
+        if callbacks.node_data(node).kind == NodeKind::SVGSVGBox {
+            let fragment = &link.fragment;
+            debug_assert!(
+                fragment.svg_viewport_size.is_some(),
+                "committed SVG root fragment carries no viewport size"
+            );
+            callbacks.set_saved_committed_geometry(
+                node,
+                FfiPaintableGeometry {
+                    content_inline_size: fragment.content_inline_size,
+                    content_block_size: fragment.content_block_size,
+                    content_offset: link.committed_offset,
+                    svg_viewport_size: fragment.svg_viewport_size.unwrap_or_default(),
+                    margin_left: fragment.margin_left,
+                    margin_right: fragment.margin_right,
+                    margin_top: fragment.margin_top,
+                    margin_bottom: fragment.margin_bottom,
+                    border_left: fragment.border_left,
+                    border_right: fragment.border_right,
+                    border_top: fragment.border_top,
+                    border_bottom: fragment.border_bottom,
+                    padding_left: fragment.padding_left,
+                    padding_right: fragment.padding_right,
+                    padding_top: fragment.padding_top,
+                    padding_bottom: fragment.padding_bottom,
+                    inset_left: link.inset_left,
+                    inset_right: link.inset_right,
+                    inset_top: link.inset_top,
+                    inset_bottom: link.inset_bottom,
+                },
+            );
+        }
     }
     // SAFETY: The C++ sink owns paintables and copies every plain-data
     // input synchronously.
@@ -263,16 +297,15 @@ fn commit_subtree(
 
 pub(crate) fn commit_replacing(
     root: Node,
-    paintable_to_replace: *mut c_void,
     callbacks: &FfiLayoutFcCallbacks,
     sink: &FfiCommitSink,
     pass_fragments: &crate::layout::CompletedPassFragments,
 ) {
     let mut scopes = crate::layout::CommitScopes::for_pass(pass_fragments);
-    // SAFETY: The sink retains the replaced paintable, detaches it, and
-    // returns borrowed insertion pointers that stay live until
-    // finish_commit().
-    let position = unsafe { (sink.begin_commit)(sink.context, callbacks.shell(root), paintable_to_replace) };
+    // SAFETY: The sink resolves and retains the paintable to replace from the
+    // root, detaches it, and returns borrowed insertion pointers that stay
+    // live until finish_commit().
+    let position = unsafe { (sink.begin_commit)(sink.context, callbacks.shell(root)) };
     commit_subtree(
         root,
         position.parent_paintable,