Skip to content

Commit 8569124

Browse files
LibWeb: Fix scroll state refresh in cached display list for iframes
6507d23 introduced a bug when snapshot for iframe is saved in `PaintNestedDisplayList` and, since display lists are immutable, it's not possible to update before the next repaint. This change fixes the issue by moving `ScrollStateSnapshot` for nested display lists from `PaintNestedDisplayList` to `HashMap<NonnullRefPtr<DisplayList>, ScrollStateSnapshot>` that is placed into pending rendering task, making it possible to update snapshots for all display lists before the next repaint. This change doesn't have a test because it's really hard to make a ref test that will specifically check scenario when scroll offset of an iframe is advanced after display list is cached. We already have `Tests/LibWeb/Ref/input/scroll-iframe.html` but unfortunately it did not catch this bug. Fixes #5486
1 parent 124bdce commit 8569124

18 files changed

+63
-28
lines changed

Libraries/LibWeb/CSS/StyleValues/CursorStyleValue.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,7 @@ Optional<Gfx::ImageCursor> CursorStyleValue::make_image_cursor(Layout::NodeWithS
9494
case DisplayListPlayerType::SkiaCPU: {
9595
auto painting_surface = Gfx::PaintingSurface::wrap_bitmap(bitmap);
9696
Painting::DisplayListPlayerSkia display_list_player;
97-
Painting::ScrollStateSnapshot scroll_state_snapshot;
98-
display_list_player.execute(*display_list, scroll_state_snapshot, painting_surface);
97+
display_list_player.execute(*display_list, {}, painting_surface);
9998
break;
10099
}
101100
}

Libraries/LibWeb/DOM/Document.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6405,6 +6405,11 @@ void Document::invalidate_display_list()
64056405
}
64066406
}
64076407

6408+
RefPtr<Painting::DisplayList> Document::cached_display_list() const
6409+
{
6410+
return m_cached_display_list;
6411+
}
6412+
64086413
RefPtr<Painting::DisplayList> Document::record_display_list(HTML::PaintConfig config)
64096414
{
64106415
if (m_cached_display_list && m_cached_display_list_paint_config == config) {

Libraries/LibWeb/DOM/Document.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -831,6 +831,7 @@ class Document
831831
void set_needs_display(InvalidateDisplayList = InvalidateDisplayList::Yes);
832832
void set_needs_display(CSSPixelRect const&, InvalidateDisplayList = InvalidateDisplayList::Yes);
833833

834+
RefPtr<Painting::DisplayList> cached_display_list() const;
834835
RefPtr<Painting::DisplayList> record_display_list(HTML::PaintConfig);
835836

836837
void invalidate_display_list();

Libraries/LibWeb/Forward.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,9 @@ class DisplayList;
4040
class DisplayListPlayerSkia;
4141
class DisplayListRecorder;
4242
class SVGGradientPaintStyle;
43+
class ScrollStateSnapshot;
4344
using PaintStyle = RefPtr<SVGGradientPaintStyle>;
45+
using ScrollStateSnapshotByDisplayList = HashMap<NonnullRefPtr<DisplayList>, ScrollStateSnapshot>;
4446

4547
}
4648

Libraries/LibWeb/HTML/Navigable.cpp

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
#include <LibWeb/Loader/GeneratedPagesLoader.h>
4747
#include <LibWeb/Page/Page.h>
4848
#include <LibWeb/Painting/DisplayListPlayerSkia.h>
49+
#include <LibWeb/Painting/NavigableContainerViewportPaintable.h>
4950
#include <LibWeb/Painting/Paintable.h>
5051
#include <LibWeb/Painting/ViewportPaintable.h>
5152
#include <LibWeb/Platform/EventLoopPlugin.h>
@@ -2612,14 +2613,35 @@ void Navigable::start_display_list_rendering(Gfx::PaintingSurface& painting_surf
26122613
callback();
26132614
return;
26142615
}
2615-
document->paintable()->refresh_scroll_state();
26162616
auto display_list = document->record_display_list(paint_config);
26172617
if (!display_list) {
26182618
callback();
26192619
return;
26202620
}
2621-
auto scroll_state_snapshot = document->paintable()->scroll_state().snapshot();
2622-
m_rendering_thread.enqueue_rendering_task(*display_list, move(scroll_state_snapshot), painting_surface, move(callback));
2621+
2622+
auto& document_paintable = *document->paintable();
2623+
Painting::ScrollStateSnapshotByDisplayList scroll_state_snapshot_by_display_list;
2624+
document_paintable.refresh_scroll_state();
2625+
auto scroll_state_snapshot = document_paintable.scroll_state().snapshot();
2626+
scroll_state_snapshot_by_display_list.set(*display_list, move(scroll_state_snapshot));
2627+
// Collect scroll state snapshots for each nested navigable
2628+
document_paintable.for_each_in_inclusive_subtree_of_type<Painting::NavigableContainerViewportPaintable>([&scroll_state_snapshot_by_display_list](auto& navigable_container_paintable) {
2629+
auto const* hosted_document = navigable_container_paintable.layout_box().dom_node().content_document_without_origin_check();
2630+
if (!hosted_document || !hosted_document->paintable())
2631+
return TraversalDecision::Continue;
2632+
// We are only interested in collecting scroll state snapshots for visible nested navigables, which is
2633+
// detectable by checking if they have a cached display list that should've been populated by
2634+
// record_display_list() on top-level document.
2635+
auto navigable_display_list = hosted_document->cached_display_list();
2636+
if (!navigable_display_list)
2637+
return TraversalDecision::Continue;
2638+
const_cast<DOM::Document&>(*hosted_document).paintable()->refresh_scroll_state();
2639+
auto navigable_scroll_state_snapshot = hosted_document->paintable()->scroll_state().snapshot();
2640+
scroll_state_snapshot_by_display_list.set(*navigable_display_list, move(navigable_scroll_state_snapshot));
2641+
return TraversalDecision::Continue;
2642+
});
2643+
2644+
m_rendering_thread.enqueue_rendering_task(*display_list, move(scroll_state_snapshot_by_display_list), painting_surface, move(callback));
26232645
}
26242646

26252647
RefPtr<Gfx::SkiaBackendContext> Navigable::skia_backend_context() const

Libraries/LibWeb/HTML/RenderingThread.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ void RenderingThread::rendering_thread_loop()
6767
break;
6868
}
6969

70-
m_skia_player->execute(*task->display_list, task->scroll_state_snapshot, task->painting_surface);
70+
m_skia_player->execute(*task->display_list, move(task->scroll_state_snapshot_by_display_list), task->painting_surface);
7171
if (m_exit)
7272
break;
7373
m_main_thread_event_loop.deferred_invoke([callback = move(task->callback)] {
@@ -76,10 +76,10 @@ void RenderingThread::rendering_thread_loop()
7676
}
7777
}
7878

79-
void RenderingThread::enqueue_rendering_task(NonnullRefPtr<Painting::DisplayList> display_list, Painting::ScrollStateSnapshot&& scroll_state_snapshot, NonnullRefPtr<Gfx::PaintingSurface> painting_surface, Function<void()>&& callback)
79+
void RenderingThread::enqueue_rendering_task(NonnullRefPtr<Painting::DisplayList> display_list, Painting::ScrollStateSnapshotByDisplayList&& scroll_state_snapshot_by_display_list, NonnullRefPtr<Gfx::PaintingSurface> painting_surface, Function<void()>&& callback)
8080
{
8181
Threading::MutexLocker const locker { m_rendering_task_mutex };
82-
m_rendering_tasks.enqueue(Task { move(display_list), move(scroll_state_snapshot), move(painting_surface), move(callback) });
82+
m_rendering_tasks.enqueue(Task { move(display_list), move(scroll_state_snapshot_by_display_list), move(painting_surface), move(callback) });
8383
m_rendering_task_ready_wake_condition.signal();
8484
}
8585

Libraries/LibWeb/HTML/RenderingThread.h

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
#include <LibThreading/Thread.h>
1515
#include <LibWeb/Forward.h>
1616
#include <LibWeb/Page/Page.h>
17-
#include <LibWeb/Painting/ScrollState.h>
1817

1918
namespace Web::HTML {
2019

@@ -28,7 +27,7 @@ class RenderingThread {
2827

2928
void start(DisplayListPlayerType);
3029
void set_skia_player(OwnPtr<Painting::DisplayListPlayerSkia>&& player);
31-
void enqueue_rendering_task(NonnullRefPtr<Painting::DisplayList>, Painting::ScrollStateSnapshot&&, NonnullRefPtr<Gfx::PaintingSurface>, Function<void()>&& callback);
30+
void enqueue_rendering_task(NonnullRefPtr<Painting::DisplayList>, Painting::ScrollStateSnapshotByDisplayList&&, NonnullRefPtr<Gfx::PaintingSurface>, Function<void()>&& callback);
3231

3332
private:
3433
void rendering_thread_loop();
@@ -44,7 +43,7 @@ class RenderingThread {
4443

4544
struct Task {
4645
NonnullRefPtr<Painting::DisplayList> display_list;
47-
Painting::ScrollStateSnapshot scroll_state_snapshot;
46+
Painting::ScrollStateSnapshotByDisplayList scroll_state_snapshot_by_display_list;
4847
NonnullRefPtr<Gfx::PaintingSurface> painting_surface;
4948
Function<void()> callback;
5049
};

Libraries/LibWeb/Painting/Command.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -416,7 +416,6 @@ struct AddMask {
416416

417417
struct PaintNestedDisplayList {
418418
RefPtr<DisplayList> display_list;
419-
ScrollStateSnapshot scroll_state_snapshot;
420419
Gfx::IntRect rect;
421420

422421
[[nodiscard]] Gfx::IntRect bounding_rect() const { return rect; }

Libraries/LibWeb/Painting/DisplayList.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
* SPDX-License-Identifier: BSD-2-Clause
55
*/
66

7+
#include <AK/TemporaryChange.h>
78
#include <LibWeb/Painting/DevicePixelConverter.h>
89
#include <LibWeb/Painting/DisplayList.h>
910

@@ -46,12 +47,14 @@ static bool command_is_clip_or_mask(Command const& command)
4647
});
4748
}
4849

49-
void DisplayListPlayer::execute(DisplayList& display_list, ScrollStateSnapshot const& scroll_state, RefPtr<Gfx::PaintingSurface> surface)
50+
void DisplayListPlayer::execute(DisplayList& display_list, ScrollStateSnapshotByDisplayList&& scroll_state_snapshot_by_display_list, RefPtr<Gfx::PaintingSurface> surface)
5051
{
52+
TemporaryChange change { m_scroll_state_snapshots_by_display_list, move(scroll_state_snapshot_by_display_list) };
5153
if (surface) {
5254
surface->lock_context();
5355
}
54-
execute_impl(display_list, scroll_state, surface);
56+
auto scroll_state_snapshot = m_scroll_state_snapshots_by_display_list.get(display_list).value_or({});
57+
execute_impl(display_list, scroll_state_snapshot, surface);
5558
if (surface) {
5659
surface->unlock_context();
5760
}

Libraries/LibWeb/Painting/DisplayList.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,24 +15,25 @@
1515
#include <LibGfx/ImmutableBitmap.h>
1616
#include <LibGfx/PaintStyle.h>
1717
#include <LibWeb/CSS/Enums.h>
18+
#include <LibWeb/Forward.h>
1819
#include <LibWeb/Painting/ClipFrame.h>
1920
#include <LibWeb/Painting/Command.h>
2021
#include <LibWeb/Painting/ScrollState.h>
2122

2223
namespace Web::Painting {
2324

24-
class DisplayList;
25-
2625
class DisplayListPlayer {
2726
public:
2827
virtual ~DisplayListPlayer() = default;
2928

30-
void execute(DisplayList&, ScrollStateSnapshot const&, RefPtr<Gfx::PaintingSurface>);
29+
void execute(DisplayList&, ScrollStateSnapshotByDisplayList&&, RefPtr<Gfx::PaintingSurface>);
3130

3231
protected:
3332
Gfx::PaintingSurface& surface() const { return m_surfaces.last(); }
3433
void execute_impl(DisplayList&, ScrollStateSnapshot const& scroll_state, RefPtr<Gfx::PaintingSurface>);
3534

35+
ScrollStateSnapshotByDisplayList m_scroll_state_snapshots_by_display_list;
36+
3637
private:
3738
virtual void flush() = 0;
3839
virtual void draw_glyph_run(DrawGlyphRun const&) = 0;

0 commit comments

Comments
 (0)