LibWeb: Create clip and scroll frame trees separately for each navigable

While introducing clip and scroll frame trees, I made a mistake by assuming that the paintable tree includes boxes from nested navigables. Therefore, this comment in the code was incorrect, and clip/scroll frames were simply not assigned for iframes: // NOTE: We only need to refresh the scroll state for traversables // because they are responsible for tracking the state of all // nested navigables. As a result, anything with "overflow: scroll" is currently not scrollable inside an iframe This change fixes that by ensuring clip and scroll frames are assigned and refreshed for each navigable. To achieve this, I had to modify the display list building process to record a separate display list for each navigable. This is necessary because scroll frame ids are local to a navigable, making it impossible to call `DisplayList::apply_scroll_offsets()` on a display list that contains ids from multiple navigables.
Author: https://github.com/kalenikaliaksandr Commit: ea8d0304e9 Pull-request: https://github.com/LadybirdBrowser/ladybird/pull/1011
2025-07-30 20:59:16 +00:00 · 2024-08-08 14:17:54 +03:00 · 2024-08-08 14:17:54 +03:00 · ea8d0304e9 · 2024-08-10 08:40:33 +00:00
commit ea8d0304e9
parent 1b2f35c3af
15 changed files with 156 additions and 46 deletions
--- a/Tests/LibWeb/Ref/iframe-contains-scrollable-box.html
+++ b/Tests/LibWeb/Ref/iframe-contains-scrollable-box.html
@ -0,0 +1,46 @@
+<!DOCTYPE html>
+<link rel="match" href="reference/iframe-contains-scrollable-box-ref.html" />
+<style>
+    iframe {
+        width: 300px;
+        height: 200px;
+        border: 1px solid #ccc;
+    }
+</style>
+<iframe
+    srcdoc="
+    <!DOCTYPE html>
+    <style>
+        * {
+            scrollbar-width: none;
+        }
+        body {
+            margin: 0;
+        }
+        #scrollable-box {
+            width: 300px;
+            height: 200px;
+            overflow-y: scroll;
+            border: 1px solid #ccc;
+            box-sizing: border-box;
+        }
+        #scroll-space-filler {
+            height: 300px;
+        }
+        #box {
+            width: 50px;
+            height: 50px;
+            background-color: magenta;
+        }
+    </style>
+    <div id='scrollable-box'>
+        <div id='scroll-space-filler'></div>
+        <div id='box'></div>
+        <div id='scroll-space-filler'></div>
+    </div>
+    <script>
+        const scrollable = document.getElementById('scrollable-box');
+        scrollable.scrollTop = 300;
+    </script>
+"
+></iframe>
--- a/Tests/LibWeb/Ref/reference/iframe-contains-scrollable-box-ref.html
+++ b/Tests/LibWeb/Ref/reference/iframe-contains-scrollable-box-ref.html
@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<style>
+    iframe {
+        width: 300px;
+        height: 200px;
+        border: 1px solid #ccc;
+    }
+</style>
+<iframe
+    srcdoc="
+    <!DOCTYPE html>
+    <style>
+        * {
+            scrollbar-width: none;
+        }
+        body {
+            margin: 0;
+        }
+        #scrollable-box {
+            width: 300px;
+            height: 200px;
+            overflow-y: scroll;
+            border: 1px solid #ccc;
+            box-sizing: border-box;
+        }
+        #box {
+            width: 50px;
+            height: 50px;
+            background-color: magenta;
+        }
+    </style>
+    <div id='scrollable-box'>
+        <div id='box'></div>
+    </div>
+"
+></iframe>
--- a/Userland/Libraries/LibWeb/DOM/Document.cpp
+++ b/Userland/Libraries/LibWeb/DOM/Document.cpp
@ -1156,12 +1156,10 @@ void Document::update_layout()
    navigable->set_needs_display();
    set_needs_to_resolve_paint_only_properties();

-    if (navigable->is_traversable()) {
-        // NOTE: The assignment of scroll frames only needs to occur for traversables because they take care of all
-        //       nested navigable documents.
-        paintable()->assign_scroll_frames();
-        paintable()->assign_clip_frames();
+    paintable()->assign_scroll_frames();
+    paintable()->assign_clip_frames();

+    if (navigable->is_traversable()) {
        page().client().page_did_layout();
    }

--- a/Userland/Libraries/LibWeb/HTML/Navigable.cpp
+++ b/Userland/Libraries/LibWeb/HTML/Navigable.cpp
@ -5,6 +5,7 @@
 * SPDX-License-Identifier: BSD-2-Clause
 */

+#include <LibWeb/CSS/SystemColor.h>
 #include <LibWeb/Crypto/Crypto.h>
 #include <LibWeb/DOM/Document.h>
 #include <LibWeb/DOM/DocumentLoading.h>
@ -2082,11 +2083,18 @@ void Navigable::inform_the_navigation_api_about_aborting_navigation()
    }));
 }

-void Navigable::record_display_list(Painting::DisplayListRecorder& display_list_recorder, PaintConfig config)
+RefPtr<Painting::DisplayList> Navigable::record_display_list(PaintConfig config)
 {
    auto document = active_document();
    if (!document)
-        return;
+        return {};
+
+    auto display_list = Painting::DisplayList::create();
+    Painting::DisplayListRecorder display_list_recorder(display_list);
+
+    if (config.canvas_fill_rect.has_value()) {
+        display_list_recorder.fill_rect(config.canvas_fill_rect.value(), CSS::SystemColor::canvas());
+    }

    auto const& page = traversable_navigable()->page();
    auto viewport_rect = page.css_to_device_rect(this->viewport_rect());
@ -2109,27 +2117,22 @@ void Navigable::record_display_list(Painting::DisplayListRecorder& display_list_

    auto& viewport_paintable = *document->paintable();

-    // NOTE: We only need to refresh the scroll state for traversables because they are responsible
-    //       for tracking the state of all nested navigables.
-    if (is_traversable()) {
-        viewport_paintable.refresh_scroll_state();
-        viewport_paintable.refresh_clip_state();
-    }
+    viewport_paintable.refresh_scroll_state();
+    viewport_paintable.refresh_clip_state();

    viewport_paintable.paint_all_phases(context);

-    // FIXME: Support scrollable frames inside iframes.
-    if (is_traversable()) {
-        Vector<Gfx::IntPoint> scroll_offsets_by_frame_id;
-        scroll_offsets_by_frame_id.resize(viewport_paintable.scroll_state.size());
-        for (auto [_, scrollable_frame] : viewport_paintable.scroll_state) {
-            auto scroll_offset = context.rounded_device_point(scrollable_frame->offset).to_type<int>();
-            scroll_offsets_by_frame_id[scrollable_frame->id] = scroll_offset;
-        }
-        display_list_recorder.display_list().apply_scroll_offsets(scroll_offsets_by_frame_id);
+    Vector<Gfx::IntPoint> scroll_offsets_by_frame_id;
+    scroll_offsets_by_frame_id.resize(viewport_paintable.scroll_state.size());
+    for (auto [_, scrollable_frame] : viewport_paintable.scroll_state) {
+        auto scroll_offset = context.rounded_device_point(scrollable_frame->offset).to_type<int>();
+        scroll_offsets_by_frame_id[scrollable_frame->id] = scroll_offset;
    }
+    display_list_recorder.display_list().apply_scroll_offsets(scroll_offsets_by_frame_id);

    m_needs_repaint = false;
+
+    return display_list;
 }

 // https://html.spec.whatwg.org/multipage/browsing-the-web.html#event-uni
--- a/Userland/Libraries/LibWeb/HTML/Navigable.h
+++ b/Userland/Libraries/LibWeb/HTML/Navigable.h
@ -185,8 +185,9 @@ public:
        bool paint_overlay { false };
        bool should_show_line_box_borders { false };
        bool has_focus { false };
+        Optional<Gfx::IntRect> canvas_fill_rect {};
    };
-    void record_display_list(Painting::DisplayListRecorder& display_list_recorder, PaintConfig);
+    RefPtr<Painting::DisplayList> record_display_list(PaintConfig);

    Page& page() { return m_page; }
    Page const& page() const { return m_page; }
--- a/Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp
+++ b/Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp
@ -6,7 +6,6 @@

 #include <AK/QuickSort.h>
 #include <LibWeb/Bindings/MainThreadVM.h>
-#include <LibWeb/CSS/SystemColor.h>
 #include <LibWeb/DOM/Document.h>
 #include <LibWeb/HTML/BrowsingContextGroup.h>
 #include <LibWeb/HTML/DocumentState.h>
@ -1190,17 +1189,15 @@ JS::GCPtr<DOM::Node> TraversableNavigable::currently_focused_area()

 void TraversableNavigable::paint(DevicePixelRect const& content_rect, Painting::BackingStore& target, PaintOptions paint_options)
 {
-    auto display_list = Painting::DisplayList::create();
-    Painting::DisplayListRecorder display_list_recorder(display_list);
-
-    Gfx::IntRect bitmap_rect { {}, content_rect.size().to_type<int>() };
-    display_list_recorder.fill_rect(bitmap_rect, CSS::SystemColor::canvas());
-
    HTML::Navigable::PaintConfig paint_config;
    paint_config.paint_overlay = paint_options.paint_overlay == PaintOptions::PaintOverlay::Yes;
    paint_config.should_show_line_box_borders = paint_options.should_show_line_box_borders;
    paint_config.has_focus = paint_options.has_focus;
-    record_display_list(display_list_recorder, paint_config);
+    paint_config.canvas_fill_rect = Gfx::IntRect { {}, content_rect.size() };
+    auto display_list = record_display_list(paint_config);
+    if (!display_list) {
+        return;
+    }

    switch (page().client().display_list_player_type()) {
    case DisplayListPlayerType::SkiaGPUIfAvailable: {
@ -1209,7 +1206,7 @@ void TraversableNavigable::paint(DevicePixelRect const& content_rect, Painting::
            auto& iosurface_backing_store = static_cast<Painting::IOSurfaceBackingStore&>(target);
            auto texture = m_metal_context->create_texture_from_iosurface(iosurface_backing_store.iosurface_handle());
            Painting::DisplayListPlayerSkia player(*m_skia_backend_context, *texture);
-            player.execute(display_list);
+            player.execute(*display_list);
            return;
        }
 #endif
@ -1217,19 +1214,19 @@ void TraversableNavigable::paint(DevicePixelRect const& content_rect, Painting::
 #ifdef USE_VULKAN
        if (m_skia_backend_context) {
            Painting::DisplayListPlayerSkia player(*m_skia_backend_context, target.bitmap());
-            player.execute(display_list);
+            player.execute(*display_list);
            return;
        }
 #endif

        // Fallback to CPU backend if GPU is not available
        Painting::DisplayListPlayerSkia player(target.bitmap());
-        player.execute(display_list);
+        player.execute(*display_list);
        break;
    }
    case DisplayListPlayerType::SkiaCPU: {
        Painting::DisplayListPlayerSkia player(target.bitmap());
-        player.execute(display_list);
+        player.execute(*display_list);
        break;
    }
    default:
--- a/Userland/Libraries/LibWeb/Painting/Command.h
+++ b/Userland/Libraries/LibWeb/Painting/Command.h
@ -360,6 +360,18 @@ struct AddMask {
    }
 };

+struct PaintNestedDisplayList {
+    RefPtr<DisplayList> display_list;
+    Gfx::IntRect rect;
+
+    [[nodiscard]] Gfx::IntRect bounding_rect() const { return rect; }
+
+    void translate_by(Gfx::IntPoint const& offset)
+    {
+        rect.translate_by(offset);
+    }
+};
+
 using Command = Variant<
    DrawGlyphRun,
    FillRect,
@ -389,6 +401,7 @@ using Command = Variant<
    DrawRect,
    DrawTriangleWave,
    AddRoundedRectClip,
-    AddMask>;
+    AddMask,
+    PaintNestedDisplayList>;

 }
--- a/Userland/Libraries/LibWeb/Painting/DisplayList.cpp
+++ b/Userland/Libraries/LibWeb/Painting/DisplayList.cpp
@ -86,6 +86,7 @@ void DisplayListPlayer::execute(DisplayList& display_list)
        else HANDLE_COMMAND(DrawTriangleWave, draw_triangle_wave)
        else HANDLE_COMMAND(AddRoundedRectClip, add_rounded_rect_clip)
        else HANDLE_COMMAND(AddMask, add_mask)
+        else HANDLE_COMMAND(PaintNestedDisplayList, paint_nested_display_list)
        else VERIFY_NOT_REACHED();
        // clang-format on
    }
--- a/Userland/Libraries/LibWeb/Painting/DisplayList.h
+++ b/Userland/Libraries/LibWeb/Painting/DisplayList.h
@ -70,6 +70,7 @@ private:
    virtual void draw_triangle_wave(DrawTriangleWave const&) = 0;
    virtual void add_rounded_rect_clip(AddRoundedRectClip const&) = 0;
    virtual void add_mask(AddMask const&) = 0;
+    virtual void paint_nested_display_list(PaintNestedDisplayList const&) = 0;
    virtual bool would_be_fully_clipped_by_painter(Gfx::IntRect) const = 0;
 };

--- a/Userland/Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp
+++ b/Userland/Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp
@ -1299,6 +1299,13 @@ void DisplayListPlayerSkia::add_mask(AddMask const& command)
    surface().canvas().clipShader(shader);
 }

+void DisplayListPlayerSkia::paint_nested_display_list(PaintNestedDisplayList const& command)
+{
+    auto& canvas = surface().canvas();
+    canvas.translate(command.rect.x(), command.rect.y());
+    execute(*command.display_list);
+}
+
 bool DisplayListPlayerSkia::would_be_fully_clipped_by_painter(Gfx::IntRect rect) const
 {
    return surface().canvas().quickReject(to_skia_rect(rect));
--- a/Userland/Libraries/LibWeb/Painting/DisplayListPlayerSkia.h
+++ b/Userland/Libraries/LibWeb/Painting/DisplayListPlayerSkia.h
@ -77,6 +77,7 @@ private:
    void draw_triangle_wave(DrawTriangleWave const&) override;
    void add_rounded_rect_clip(AddRoundedRectClip const&) override;
    void add_mask(AddMask const&) override;
+    void paint_nested_display_list(PaintNestedDisplayList const&) override;

    bool would_be_fully_clipped_by_painter(Gfx::IntRect) const override;

--- a/Userland/Libraries/LibWeb/Painting/DisplayListRecorder.cpp
+++ b/Userland/Libraries/LibWeb/Painting/DisplayListRecorder.cpp
@ -22,6 +22,13 @@ void DisplayListRecorder::append(Command&& command)
    m_command_list.append(move(command), state().scroll_frame_id);
 }

+void DisplayListRecorder::paint_nested_display_list(RefPtr<DisplayList> display_list, Gfx::IntRect rect)
+{
+    append(PaintNestedDisplayList {
+        .display_list = move(display_list),
+        .rect = state().translation.map(rect) });
+}
+
 void DisplayListRecorder::add_rounded_rect_clip(CornerRadii corner_radii, Gfx::IntRect border_rect, CornerClip corner_clip)
 {
    append(AddRoundedRectClip {
--- a/Userland/Libraries/LibWeb/Painting/DisplayListRecorder.h
+++ b/Userland/Libraries/LibWeb/Painting/DisplayListRecorder.h
@ -121,6 +121,8 @@ public:
    void push_stacking_context(PushStackingContextParams params);
    void pop_stacking_context();

+    void paint_nested_display_list(RefPtr<DisplayList> display_list, Gfx::IntRect rect);
+
    void add_rounded_rect_clip(CornerRadii corner_radii, Gfx::IntRect border_rect, CornerClip corner_clip);
    void add_mask(RefPtr<DisplayList> display_list, Gfx::IntRect rect);

--- a/Userland/Libraries/LibWeb/Painting/NestedBrowsingContextPaintable.cpp
+++ b/Userland/Libraries/LibWeb/Painting/NestedBrowsingContextPaintable.cpp
@ -40,8 +40,7 @@ void NestedBrowsingContextPaintable::paint(PaintContext& context, PaintPhase pha

    if (phase == PaintPhase::Foreground) {
        auto absolute_rect = this->absolute_rect();
-        absolute_rect.translate_by(enclosing_scroll_frame_offset());
-        auto clip_rect = context.rounded_device_rect(absolute_rect);
+        auto clip_rect = context.rounded_device_rect(absolute_rect.translated(enclosing_scroll_frame_offset()));
        ScopedCornerRadiusClip corner_clip { context, clip_rect, normalized_border_radii_data(ShrinkRadiiForBorders::Yes) };

        auto const* hosted_document = layout_box().dom_node().content_document_without_origin_check();
@ -54,14 +53,13 @@ void NestedBrowsingContextPaintable::paint(PaintContext& context, PaintPhase pha
        context.display_list_recorder().save();

        context.display_list_recorder().add_clip_rect(clip_rect.to_type<int>());
-        auto absolute_device_rect = context.enclosing_device_rect(absolute_rect);
-        context.display_list_recorder().translate(absolute_device_rect.x().value(), absolute_device_rect.y().value());

        HTML::Navigable::PaintConfig paint_config;
        paint_config.paint_overlay = context.should_paint_overlay();
        paint_config.should_show_line_box_borders = context.should_show_line_box_borders();
        paint_config.has_focus = context.has_focus();
-        const_cast<DOM::Document*>(hosted_document)->navigable()->record_display_list(context.display_list_recorder(), paint_config);
+        auto display_list = const_cast<DOM::Document*>(hosted_document)->navigable()->record_display_list(paint_config);
+        context.display_list_recorder().paint_nested_display_list(display_list, context.enclosing_device_rect(absolute_rect).to_type<int>());

        context.display_list_recorder().restore();

--- a/Userland/Libraries/LibWeb/SVG/SVGDecodedImageData.cpp
+++ b/Userland/Libraries/LibWeb/SVG/SVGDecodedImageData.cpp
@ -92,17 +92,16 @@ RefPtr<Gfx::Bitmap> SVGDecodedImageData::render(Gfx::IntSize size) const
    m_document->navigable()->set_viewport_size(size.to_type<CSSPixels>());
    m_document->update_layout();

-    auto display_list = Painting::DisplayList::create();
-    Painting::DisplayListRecorder display_list_recorder(display_list);
-
-    m_document->navigable()->record_display_list(display_list_recorder, {});
+    auto display_list = m_document->navigable()->record_display_list({});
+    if (!display_list)
+        return {};

    auto painting_command_executor_type = m_page_client->display_list_player_type();
    switch (painting_command_executor_type) {
    case DisplayListPlayerType::SkiaGPUIfAvailable:
    case DisplayListPlayerType::SkiaCPU: {
        Painting::DisplayListPlayerSkia display_list_player { *bitmap };
-        display_list_player.execute(display_list);
+        display_list_player.execute(*display_list);
        break;
    }
    default: