From 4c1b69421acda26885fab1271007ed442312bf08 Mon Sep 17 00:00:00 2001 From: stevennovaryo Date: Thu, 5 Jun 2025 11:34:48 +0800 Subject: [PATCH] Fix bug Signed-off-by: stevennovaryo --- components/compositing/compositor.rs | 4 ++-- components/compositing/webview_renderer.rs | 4 ++-- components/constellation/constellation.rs | 7 +++---- .../layout/display_list/stacking_context.rs | 10 +++++++--- components/layout/layout_impl.rs | 18 ++++++++++++------ components/script/dom/window.rs | 3 +-- components/script/script_thread.rs | 16 ++++++++++------ components/shared/compositing/display_list.rs | 9 ++++----- components/shared/constellation/lib.rs | 1 - components/shared/script_layout/lib.rs | 2 +- 10 files changed, 42 insertions(+), 32 deletions(-) diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index 72d1645124a..bb8ba52d7c9 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -257,7 +257,7 @@ impl PipelineDetails { } } - // MYNOTES: we also do this in the display list now + /// Install a new scroll tree, but maintaining the scroll offsets it has from the old ones. fn install_new_scroll_tree(&mut self, new_scroll_tree: ScrollTree) { let old_scroll_offsets: FnvHashMap = self .scroll_tree @@ -744,7 +744,7 @@ impl IOCompositor { txn.set_scroll_offsets( external_scroll_id, vec![SampledScrollOffset { - offset, + offset: -offset, generation: 0, }], ); diff --git a/components/compositing/webview_renderer.rs b/components/compositing/webview_renderer.rs index 512ead58336..9901b02b70e 100644 --- a/components/compositing/webview_renderer.rs +++ b/components/compositing/webview_renderer.rs @@ -183,7 +183,6 @@ impl WebViewRenderer { self.pipelines.remove(&pipeline_id); } - // MYNOTES: I suppose this is not needed now? If changing composition's frame tree does trigger reflow? pub(crate) fn set_frame_tree(&mut self, frame_tree: &SendableFrameTree) { let pipeline_id = frame_tree.pipeline.id; let old_pipeline_id = std::mem::replace(&mut self.root_pipeline_id, Some(pipeline_id)); @@ -876,6 +875,8 @@ impl WebViewRenderer { combined_event.scroll_location, ) }); + // If something is scrolled, we are sending the scroll information to layout in + // order to sync the scroll offsets for queries and scripts. if let Some(scroll_result) = scroll_result { self.send_scroll_result_to_layout(scroll_result); } @@ -894,7 +895,6 @@ impl WebViewRenderer { /// scrolling to the applicable scroll node under that point. If a scroll was /// performed, returns the [`PipelineId`] of the node scrolled, the id, and the final /// scroll delta. - // MYNOTES: this could be changed to we only store the diff fn scroll_node_at_device_point( &mut self, cursor: DevicePoint, diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index 20e617cf17b..9d34557b2af 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -6022,14 +6022,13 @@ where } } - // MYNOTES: this shouldn't be necessary anymore since, we have scroll offsets tree at layout impl #[cfg_attr( feature = "tracing", tracing::instrument(skip_all, fields(servo_profiling = true), level = "trace") )] fn handle_set_scroll_states(&self, pipeline_id: PipelineId, scroll_states: Vec) { let Some(pipeline) = self.pipelines.get(&pipeline_id) else { - warn!("Discarding scroll offset update for unknown pipeline"); + warn!("Discarding scroll offsets update for unknown pipeline"); return; }; if let Err(error) = pipeline @@ -6049,7 +6048,7 @@ where )] fn handle_update_scroll_state(&self, pipeline_id: PipelineId, scroll_state: ScrollState) { let Some(pipeline) = self.pipelines.get(&pipeline_id) else { - warn!("Discarding scroll offset update for unknown pipeline"); + warn!("Discarding a scroll result update for unknown pipeline"); return; }; if let Err(error) = pipeline @@ -6059,7 +6058,7 @@ where scroll_state, )) { - warn!("Could not send scroll offsets to pipeline: {pipeline_id:?}: {error:?}"); + warn!("Could not send scroll result update to pipeline: {pipeline_id:?}: {error:?}"); } } diff --git a/components/layout/display_list/stacking_context.rs b/components/layout/display_list/stacking_context.rs index 7bddc2fe207..07eed78a2bf 100644 --- a/components/layout/display_list/stacking_context.rs +++ b/components/layout/display_list/stacking_context.rs @@ -118,7 +118,7 @@ pub(crate) struct StackingContextTree { impl StackingContextTree { /// Create a new [DisplayList] given the dimensions of the layout and the WebRender /// pipeline id. - // MYNOTES: fix this + // FIXME(stevennovaryo): refactor the stacking context parameters #[allow(clippy::too_many_arguments)] pub fn new( fragment_tree: &FragmentTree, @@ -979,6 +979,8 @@ impl BoxFragment { StackingContextSection::DescendantBackgroundsAndBorders } + // FIXME(stevennovaryo): refactor the stacking context parameters + #[allow(clippy::too_many_arguments)] fn build_stacking_context_tree( &self, fragment: Fragment, @@ -1000,6 +1002,8 @@ impl BoxFragment { ); } + // FIXME(stevennovaryo): refactor the stacking context parameters + #[allow(clippy::too_many_arguments)] fn build_stacking_context_tree_maybe_creating_reference_frame( &self, fragment: Fragment, @@ -1076,7 +1080,7 @@ impl BoxFragment { ); } - // MYNOTES: fix this + // FIXME(stevennovaryo): refactor the stacking context parameters #[allow(clippy::too_many_arguments)] fn build_stacking_context_tree_maybe_creating_stacking_context( &self, @@ -1172,7 +1176,7 @@ impl BoxFragment { .append(&mut stolen_children); } - // MYNOTES: fix this + // FIXME(stevennovaryo): refactor the stacking context parameters #[allow(clippy::too_many_arguments)] fn build_stacking_context_tree_for_children( &self, diff --git a/components/layout/layout_impl.rs b/components/layout/layout_impl.rs index c842732f60d..5ac920ae4f4 100644 --- a/components/layout/layout_impl.rs +++ b/components/layout/layout_impl.rs @@ -496,6 +496,9 @@ impl Layout for LayoutThread { ) { } + /// Sets the scroll offsets of the cached [`ScrollTree`] based on the scroll states. + /// + /// This is done whenever the embedder ask layout to do so. fn set_scroll_offsets(&mut self, scroll_states: &[ScrollState]) { if let Some(mut tree) = self.cached_scroll_tree_mut() { for ScrollState { @@ -508,7 +511,9 @@ impl Layout for LayoutThread { } } - // + /// Update a node's scroll offsets in the cached [`ScrollTree`]. + /// + /// This is done whenever the embedder ask layout to do so. fn update_scroll_offset(&self, scroll_id: ExternalScrollId, scroll_offset: LayoutVector2D) { if let Some(mut scroll_tree) = self.cached_scroll_tree_mut() { scroll_tree @@ -773,10 +778,10 @@ impl LayoutThread { .flush(guards, Some(root_element), Some(snapshot_map)); } - // #[cfg_attr( - // feature = "tracing", - // tracing::instrument(skip_all, fields(servo_profiling = true), level = "trace") - // )] + #[cfg_attr( + feature = "tracing", + tracing::instrument(skip_all, fields(servo_profiling = true), level = "trace") + )] fn restyle_and_build_trees( &self, reflow_request: &ReflowRequest, @@ -848,7 +853,8 @@ impl LayoutThread { *self.fragment_tree.borrow_mut() = Some(fragment_tree); // The FragmentTree has been updated, so any existing StackingContext tree that layout - // had is now out of date and should be rebuilt. + // had is now out of date and should be rebuilt. We could still uses some information + // from the previous StackingContext tree though. let old_stacking_context_tree = self.stacking_context_tree.borrow_mut().take(); if self.debug.dump_style_tree { diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 32c6607554c..7a115006f1a 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -77,7 +77,6 @@ use servo_arc::Arc as ServoArc; use servo_config::{opts, pref}; use servo_geometry::{DeviceIndependentIntRect, MaxRect, f32_rect_to_au_rect}; use servo_url::{ImmutableOrigin, MutableOrigin, ServoUrl}; -use style::dom::OpaqueNode; use style::error_reporting::{ContextualParseError, ParseErrorReporter}; use style::properties::PropertyId; use style::properties::style_structs::Font; @@ -2552,7 +2551,7 @@ impl Window { self.pipeline_id().into(), ); self.layout() - .update_scroll_offset(scroll_id, Vector2D::new(x, y)); + .update_scroll_offset(scroll_id, Vector2D::new(-x, -y)); // Step 6 // > Perform a scroll of box to position, element as the associated diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index aa2bfae45cd..861b6c15e2f 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -83,16 +83,13 @@ use percent_encoding::percent_decode; use profile_traits::mem::{ProcessReports, ReportsChan, perform_memory_report}; use profile_traits::time::ProfilerCategory; use profile_traits::time_profile; -use script_layout_interface::{ - LayoutConfig, LayoutFactory, ReflowGoal, ScriptThreadFactory, node_id_from_scroll_id, -}; +use script_layout_interface::{LayoutConfig, LayoutFactory, ReflowGoal, ScriptThreadFactory}; use script_traits::{ ConstellationInputEvent, DiscardBrowsingContext, DocumentActivity, InitialScriptState, NewLayoutInfo, Painter, ProgressiveWebMetricType, ScriptThreadMessage, UpdatePipelineIdReason, }; use servo_config::opts; use servo_url::{ImmutableOrigin, MutableOrigin, ServoUrl}; -use style::dom::OpaqueNode; use style::thread_state::{self, ThreadState}; use stylo_atoms::Atom; use timers::{TimerEventRequest, TimerScheduler}; @@ -1928,7 +1925,6 @@ impl ScriptThread { ScriptThreadEventCategory::SetScrollState, Some(pipeline_id), || { - // MYNOTES: FIXME window.layout_mut().set_scroll_offsets(&scroll_states); for scroll_state in scroll_states.into_iter() { @@ -1949,7 +1945,15 @@ impl ScriptThread { return; }; - window.layout_mut().update_scroll_offset(scroll_state.scroll_id, scroll_state.scroll_offset); + if scroll_state.scroll_id.is_root() { + window.update_viewport_for_scroll( + -scroll_state.scroll_offset.x, + -scroll_state.scroll_offset.y, + ); + } + window + .layout_mut() + .update_scroll_offset(scroll_state.scroll_id, scroll_state.scroll_offset); } #[cfg(feature = "webgpu")] diff --git a/components/shared/compositing/display_list.rs b/components/shared/compositing/display_list.rs index d684a058531..bdbe3fcf8ce 100644 --- a/components/shared/compositing/display_list.rs +++ b/components/shared/compositing/display_list.rs @@ -157,7 +157,7 @@ impl ScrollTreeNode { /// Set the offset for this node, returns false if this was a /// non-scrolling node for which you cannot set the offset. - // MYNOTES: should i make this &mut so it will modify the new_offset + // TODO(stevennovaryo): We should consider writing mode as well. pub fn set_offset(&mut self, new_offset: LayoutVector2D) -> bool { match self.info { SpatialTreeNodeInfo::Scroll(ref mut info) => { @@ -361,12 +361,11 @@ impl ScrollTree { ) -> bool { if let Some(node) = self.get_node_by_external_scroll_id_mut(external_scroll_id) { node.set_offset(offset); + true + } else { + false } - false } - - // Update this stacking context - pub fn persevere_old_stacking_context_tree_scroll() {} } /// A data structure which stores compositor-side information about diff --git a/components/shared/constellation/lib.rs b/components/shared/constellation/lib.rs index bc23a6f5ba1..2ab76a16125 100644 --- a/components/shared/constellation/lib.rs +++ b/components/shared/constellation/lib.rs @@ -90,7 +90,6 @@ pub enum EmbedderToConstellationMessage { SetWebViewThrottled(WebViewId, bool), /// The Servo renderer scrolled and is updating the scroll states of the nodes in the /// given pipeline via the constellation. - // MYNOTES: Do we still need this? SetScrollStates(PipelineId, Vec), /// The Servo renderer scrolled and is updating the scroll states of the nodes in the /// given pipeline via the constellation. diff --git a/components/shared/script_layout/lib.rs b/components/shared/script_layout/lib.rs index a3c6735cea9..c3a68395706 100644 --- a/components/shared/script_layout/lib.rs +++ b/components/shared/script_layout/lib.rs @@ -48,8 +48,8 @@ use style::properties::PropertyId; use style::properties::style_structs::Font; use style::selector_parser::{PseudoElement, RestyleDamage, Snapshot}; use style::stylesheets::Stylesheet; -use webrender_api::{ExternalScrollId, ImageKey}; use webrender_api::units::{DeviceIntSize, LayoutVector2D}; +use webrender_api::{ExternalScrollId, ImageKey}; pub trait GenericLayoutDataTrait: Any + MallocSizeOfTrait { fn as_any(&self) -> &dyn Any;