From 5ff7448feed0f79b18a8d6807de195ae935e9366 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Wed, 23 Aug 2023 18:58:42 +0200 Subject: [PATCH] LibWeb: Move viewport subscriptions from BrowsingContext to Document With this change, elements that want to receive viewport rect updates will need to register on document instead of the browsing context. This change solves the problem where a browsing context for a document is guaranteed to exist only while the document is active so browsing context might not exit by the time DOM node that want to register is constructed. This is a part of preparation work before switching to navigables where this issue becomes more visible. --- Userland/Libraries/LibWeb/DOM/Document.cpp | 20 ++++++++++++- Userland/Libraries/LibWeb/DOM/Document.h | 11 ++++++++ .../Libraries/LibWeb/HTML/BrowsingContext.cpp | 28 ++++--------------- .../Libraries/LibWeb/HTML/BrowsingContext.h | 12 -------- .../LibWeb/HTML/HTMLImageElement.cpp | 14 ++++++---- .../Libraries/LibWeb/HTML/HTMLImageElement.h | 7 +++-- Userland/Libraries/LibWeb/Layout/VideoBox.cpp | 10 +++---- Userland/Libraries/LibWeb/Layout/VideoBox.h | 8 +++--- .../LibWeb/Painting/ImagePaintable.cpp | 10 +++---- .../LibWeb/Painting/ImagePaintable.h | 7 ++--- 10 files changed, 66 insertions(+), 61 deletions(-) diff --git a/Userland/Libraries/LibWeb/DOM/Document.cpp b/Userland/Libraries/LibWeb/DOM/Document.cpp index cdda74a11e..ce30647700 100644 --- a/Userland/Libraries/LibWeb/DOM/Document.cpp +++ b/Userland/Libraries/LibWeb/DOM/Document.cpp @@ -1007,7 +1007,7 @@ void Document::update_layout() layout_state.commit(*m_layout_root); // Broadcast the current viewport rect to any new paintables, so they know whether they're visible or not. - browsing_context()->inform_all_viewport_clients_about_the_current_viewport_rect(); + inform_all_viewport_clients_about_the_current_viewport_rect(); browsing_context()->set_needs_display(); @@ -3023,6 +3023,24 @@ JS::NonnullGCPtr Document::visual_viewport() return *m_visual_viewport; } +void Document::register_viewport_client(ViewportClient& client) +{ + auto result = m_viewport_clients.set(&client); + VERIFY(result == AK::HashSetResult::InsertedNewEntry); +} + +void Document::unregister_viewport_client(ViewportClient& client) +{ + bool was_removed = m_viewport_clients.remove(&client); + VERIFY(was_removed); +} + +void Document::inform_all_viewport_clients_about_the_current_viewport_rect() +{ + for (auto* client : m_viewport_clients) + client->did_set_viewport_rect(viewport_rect()); +} + void Document::register_intersection_observer(Badge, IntersectionObserver::IntersectionObserver& observer) { auto result = m_intersection_observers.set(observer); diff --git a/Userland/Libraries/LibWeb/DOM/Document.h b/Userland/Libraries/LibWeb/DOM/Document.h index a283172fdb..a268ede06a 100644 --- a/Userland/Libraries/LibWeb/DOM/Document.h +++ b/Userland/Libraries/LibWeb/DOM/Document.h @@ -392,6 +392,15 @@ public: JS::NonnullGCPtr visual_viewport(); [[nodiscard]] CSSPixelRect viewport_rect() const; + class ViewportClient { + public: + virtual ~ViewportClient() = default; + virtual void did_set_viewport_rect(CSSPixelRect const&) = 0; + }; + void register_viewport_client(ViewportClient&); + void unregister_viewport_client(ViewportClient&); + void inform_all_viewport_clients_about_the_current_viewport_rect(); + bool has_focus() const; void set_parser(Badge, HTML::HTMLParser&); @@ -624,6 +633,8 @@ private: // Used by run_the_resize_steps(). Gfx::IntSize m_last_viewport_size; + HashTable m_viewport_clients; + // https://w3c.github.io/csswg-drafts/cssom-view-1/#document-pending-scroll-event-targets Vector> m_pending_scroll_event_targets; diff --git a/Userland/Libraries/LibWeb/HTML/BrowsingContext.cpp b/Userland/Libraries/LibWeb/HTML/BrowsingContext.cpp index 26866de0f9..14cb47f463 100644 --- a/Userland/Libraries/LibWeb/HTML/BrowsingContext.cpp +++ b/Userland/Libraries/LibWeb/HTML/BrowsingContext.cpp @@ -539,12 +539,6 @@ void BrowsingContext::set_active_document(JS::NonnullGCPtr docume previously_active_document->did_stop_being_active_document_in_browsing_context({}); } -void BrowsingContext::inform_all_viewport_clients_about_the_current_viewport_rect() -{ - for (auto* client : m_viewport_clients) - client->browsing_context_did_set_viewport_rect(viewport_rect()); -} - void BrowsingContext::set_viewport_rect(CSSPixelRect const& rect) { bool did_change = false; @@ -565,9 +559,8 @@ void BrowsingContext::set_viewport_rect(CSSPixelRect const& rect) did_change = true; } - if (did_change) { - for (auto* client : m_viewport_clients) - client->browsing_context_did_set_viewport_rect(rect); + if (did_change && active_document()) { + active_document()->inform_all_viewport_clients_about_the_current_viewport_rect(); } // Schedule the HTML event loop to ensure that a `resize` event gets fired. @@ -585,8 +578,9 @@ void BrowsingContext::set_size(CSSPixelSize size) document->set_needs_layout(); } - for (auto* client : m_viewport_clients) - client->browsing_context_did_set_viewport_rect(viewport_rect()); + if (auto* document = active_document()) { + document->inform_all_viewport_clients_about_the_current_viewport_rect(); + } // Schedule the HTML event loop to ensure that a `resize` event gets fired. HTML::main_thread_event_loop().schedule(); @@ -752,18 +746,6 @@ void BrowsingContext::select_all() (void)selection->select_all_children(*document->body()); } -void BrowsingContext::register_viewport_client(ViewportClient& client) -{ - auto result = m_viewport_clients.set(&client); - VERIFY(result == AK::HashSetResult::InsertedNewEntry); -} - -void BrowsingContext::unregister_viewport_client(ViewportClient& client) -{ - bool was_removed = m_viewport_clients.remove(&client); - VERIFY(was_removed); -} - void BrowsingContext::register_frame_nesting(AK::URL const& url) { m_frame_nesting_levels.ensure(url)++; diff --git a/Userland/Libraries/LibWeb/HTML/BrowsingContext.h b/Userland/Libraries/LibWeb/HTML/BrowsingContext.h index a58e1c4dad..12ba6c726c 100644 --- a/Userland/Libraries/LibWeb/HTML/BrowsingContext.h +++ b/Userland/Libraries/LibWeb/HTML/BrowsingContext.h @@ -118,14 +118,6 @@ public: return IterationDecision::Continue; } - class ViewportClient { - public: - virtual ~ViewportClient() = default; - virtual void browsing_context_did_set_viewport_rect(CSSPixelRect const&) = 0; - }; - void register_viewport_client(ViewportClient&); - void unregister_viewport_client(ViewportClient&); - bool is_top_level() const; bool is_focused_context() const; @@ -277,8 +269,6 @@ public: virtual String const& window_handle() const override { return m_window_handle; } virtual void set_window_handle(String handle) override { m_window_handle = move(handle); } - void inform_all_viewport_clients_about_the_current_viewport_rect(); - private: explicit BrowsingContext(Page&, HTML::NavigableContainer*); @@ -324,8 +314,6 @@ private: RefPtr m_cursor_blink_timer; bool m_cursor_blink_state { false }; - HashTable m_viewport_clients; - HashMap m_frame_nesting_levels; DeprecatedString m_name; diff --git a/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp index 08310a5821..22aab62187 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp @@ -39,8 +39,7 @@ HTMLImageElement::HTMLImageElement(DOM::Document& document, DOM::QualifiedName q m_animation_timer = Core::Timer::try_create().release_value_but_fixme_should_propagate_errors(); m_animation_timer->on_timeout = [this] { animate(); }; - if (auto* browsing_context = document.browsing_context()) - browsing_context->register_viewport_client(*this); + document.register_viewport_client(*this); } HTMLImageElement::~HTMLImageElement() = default; @@ -48,8 +47,7 @@ HTMLImageElement::~HTMLImageElement() = default; void HTMLImageElement::finalize() { Base::finalize(); - if (auto* browsing_context = document().browsing_context()) - browsing_context->unregister_viewport_client(*this); + document().unregister_viewport_client(*this); } void HTMLImageElement::initialize(JS::Realm& realm) @@ -60,6 +58,12 @@ void HTMLImageElement::initialize(JS::Realm& realm) m_current_request = ImageRequest::create(realm, *document().page()); } +void HTMLImageElement::adopted_from(DOM::Document& old_document) +{ + old_document.unregister_viewport_client(*this); + document().register_viewport_client(*this); +} + void HTMLImageElement::visit_edges(Cell::Visitor& visitor) { Base::visit_edges(visitor); @@ -630,7 +634,7 @@ void HTMLImageElement::add_callbacks_to_image_request(JS::NonnullGCPtr #include #include +#include #include #include #include @@ -23,7 +24,7 @@ class HTMLImageElement final : public HTMLElement , public FormAssociatedElement , public Layout::ImageProvider - , public BrowsingContext::ViewportClient { + , public DOM::Document::ViewportClient { WEB_PLATFORM_OBJECT(HTMLImageElement, HTMLElement); FORM_ASSOCIATED_ELEMENT(HTMLElement, HTMLImageElement) @@ -99,11 +100,13 @@ private: virtual void initialize(JS::Realm&) override; virtual void finalize() override; + virtual void adopted_from(DOM::Document&) override; + virtual void apply_presentational_hints(CSS::StyleProperties&) const override; virtual JS::GCPtr create_layout_node(NonnullRefPtr) override; - virtual void browsing_context_did_set_viewport_rect(CSSPixelRect const&) override; + virtual void did_set_viewport_rect(CSSPixelRect const&) override; void handle_successful_fetch(AK::URL const&, StringView mime_type, ImageRequest&, ByteBuffer, bool maybe_omit_events, AK::URL const& previous_url); void handle_failed_fetch(); diff --git a/Userland/Libraries/LibWeb/Layout/VideoBox.cpp b/Userland/Libraries/LibWeb/Layout/VideoBox.cpp index 48bc8673b0..ae827f9526 100644 --- a/Userland/Libraries/LibWeb/Layout/VideoBox.cpp +++ b/Userland/Libraries/LibWeb/Layout/VideoBox.cpp @@ -13,16 +13,16 @@ namespace Web::Layout { VideoBox::VideoBox(DOM::Document& document, DOM::Element& element, NonnullRefPtr style) : ReplacedBox(document, element, move(style)) { - browsing_context().register_viewport_client(*this); + document.register_viewport_client(*this); } void VideoBox::finalize() { Base::finalize(); - // NOTE: We unregister from the browsing context in finalize() to avoid trouble - // in the scenario where our BrowsingContext has already been swept by GC. - browsing_context().unregister_viewport_client(*this); + // NOTE: We unregister from the document in finalize() to avoid trouble + // in the scenario where our Document has already been swept by GC. + document().unregister_viewport_client(*this); } HTML::HTMLVideoElement& VideoBox::dom_node() @@ -49,7 +49,7 @@ void VideoBox::prepare_for_replaced_layout() set_natural_aspect_ratio({}); } -void VideoBox::browsing_context_did_set_viewport_rect(CSSPixelRect const&) +void VideoBox::did_set_viewport_rect(CSSPixelRect const&) { // FIXME: Several steps in HTMLMediaElement indicate we may optionally handle whether the media object // is in view. Implement those steps. diff --git a/Userland/Libraries/LibWeb/Layout/VideoBox.h b/Userland/Libraries/LibWeb/Layout/VideoBox.h index 0f8fdda409..d9ce706714 100644 --- a/Userland/Libraries/LibWeb/Layout/VideoBox.h +++ b/Userland/Libraries/LibWeb/Layout/VideoBox.h @@ -6,15 +6,15 @@ #pragma once +#include #include -#include #include namespace Web::Layout { class VideoBox final : public ReplacedBox - , public HTML::BrowsingContext::ViewportClient { + , public DOM::Document::ViewportClient { JS_CELL(VideoBox, ReplacedBox); public: @@ -28,8 +28,8 @@ public: private: VideoBox(DOM::Document&, DOM::Element&, NonnullRefPtr); - // ^BrowsingContext::ViewportClient - virtual void browsing_context_did_set_viewport_rect(CSSPixelRect const&) final; + // ^Document::ViewportClient + virtual void did_set_viewport_rect(CSSPixelRect const&) final; // ^JS::Cell virtual void finalize() override; diff --git a/Userland/Libraries/LibWeb/Painting/ImagePaintable.cpp b/Userland/Libraries/LibWeb/Painting/ImagePaintable.cpp index a19917acb6..aba446e7a3 100644 --- a/Userland/Libraries/LibWeb/Painting/ImagePaintable.cpp +++ b/Userland/Libraries/LibWeb/Painting/ImagePaintable.cpp @@ -23,16 +23,16 @@ JS::NonnullGCPtr ImagePaintable::create(Layout::ImageBox const& ImagePaintable::ImagePaintable(Layout::ImageBox const& layout_box) : PaintableBox(layout_box) { - browsing_context().register_viewport_client(*this); + const_cast(layout_box.document()).register_viewport_client(*this); } void ImagePaintable::finalize() { Base::finalize(); - // NOTE: We unregister from the browsing context in finalize() to avoid trouble - // in the scenario where our BrowsingContext has already been swept by GC. - browsing_context().unregister_viewport_client(*this); + // NOTE: We unregister from the document in finalize() to avoid trouble + // in the scenario where our Document has already been swept by GC. + document().unregister_viewport_client(*this); } Layout::ImageBox const& ImagePaintable::layout_box() const @@ -133,7 +133,7 @@ void ImagePaintable::paint(PaintContext& context, PaintPhase phase) const } } -void ImagePaintable::browsing_context_did_set_viewport_rect(CSSPixelRect const& viewport_rect) +void ImagePaintable::did_set_viewport_rect(CSSPixelRect const& viewport_rect) { const_cast(layout_box().image_provider()).set_visible_in_viewport(viewport_rect.intersects(absolute_rect())); } diff --git a/Userland/Libraries/LibWeb/Painting/ImagePaintable.h b/Userland/Libraries/LibWeb/Painting/ImagePaintable.h index 480f71b8fe..41569c2203 100644 --- a/Userland/Libraries/LibWeb/Painting/ImagePaintable.h +++ b/Userland/Libraries/LibWeb/Painting/ImagePaintable.h @@ -6,7 +6,6 @@ #pragma once -#include #include #include @@ -14,7 +13,7 @@ namespace Web::Painting { class ImagePaintable final : public PaintableBox - , public HTML::BrowsingContext::ViewportClient { + , public DOM::Document::ViewportClient { JS_CELL(ImagePaintable, PaintableBox); public: @@ -28,8 +27,8 @@ private: // ^JS::Cell virtual void finalize() override; - // ^BrowsingContext::ViewportClient - virtual void browsing_context_did_set_viewport_rect(CSSPixelRect const&) final; + // ^Document::ViewportClient + virtual void did_set_viewport_rect(CSSPixelRect const&) final; ImagePaintable(Layout::ImageBox const&); };