From 29fc878e1583b6b9f25eb89f0e9530a50b6279fd Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Thu, 12 Jun 2025 19:32:50 +0200 Subject: [PATCH] compositor: Do not allow script to scroll beyond node boundaries (#37412) The compositor was accepting scroll offsets from the ScriptThread without checking their boundaries. In some cases this could cause a temporary discrepancy with the rendered scroll offset. This change makes it so that all offset updates for scroll ayers in the compositor do not scroll past the scroll boundaries of the node. Testing: Two new tests pass with this change: - `/css/css-position/sticky/position-sticky-left-003.html` - `/css/css-position/sticky/position-sticky-top-003.html` Signed-off-by: Martin Robinson --- Cargo.lock | 1 + components/compositing/compositor.rs | 14 +- components/compositing/webview_renderer.rs | 9 +- components/shared/compositing/Cargo.toml | 1 + components/shared/compositing/display_list.rs | 190 ++++++++++-------- .../shared/compositing/tests/compositor.rs | 30 ++- .../sticky/position-sticky-left-003.html.ini | 2 - .../sticky/position-sticky-top-003.html.ini | 2 - 8 files changed, 142 insertions(+), 107 deletions(-) delete mode 100644 tests/wpt/meta/css/css-position/sticky/position-sticky-left-003.html.ini delete mode 100644 tests/wpt/meta/css/css-position/sticky/position-sticky-top-003.html.ini diff --git a/Cargo.lock b/Cargo.lock index c60b0e9b9fd..d46cdff3b07 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1123,6 +1123,7 @@ version = "0.0.1" dependencies = [ "base", "bincode", + "bitflags 2.9.1", "crossbeam-channel", "dpi", "embedder_traits", diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index b598ed939f9..9247187afc1 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -15,7 +15,9 @@ use base::cross_process_instant::CrossProcessInstant; use base::id::{PipelineId, WebViewId}; use base::{Epoch, WebRenderEpochToU16}; use bitflags::bitflags; -use compositing_traits::display_list::{CompositorDisplayListInfo, HitTestInfo, ScrollTree}; +use compositing_traits::display_list::{ + CompositorDisplayListInfo, HitTestInfo, ScrollTree, ScrollType, +}; use compositing_traits::rendering_context::RenderingContext; use compositing_traits::{ CompositionPipeline, CompositorMsg, ImageUpdate, SendableFrameTree, WebViewTrait, @@ -738,22 +740,22 @@ impl IOCompositor { }; let offset = LayoutVector2D::new(point.x, point.y); - if !pipeline_details + let Some(offset) = pipeline_details .scroll_tree .set_scroll_offsets_for_node_with_external_scroll_id( external_scroll_id, -offset, + ScrollType::Script, ) - { - warn!("Could not scroll not with id: {external_scroll_id:?}"); + else { return; - } + }; let mut txn = Transaction::new(); 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 1c099f72714..c1dd04ca011 100644 --- a/components/compositing/webview_renderer.rs +++ b/components/compositing/webview_renderer.rs @@ -8,6 +8,7 @@ use std::collections::{HashMap, VecDeque}; use std::rc::Rc; use base::id::{PipelineId, WebViewId}; +use compositing_traits::display_list::ScrollType; use compositing_traits::viewport_description::{ DEFAULT_ZOOM, MAX_ZOOM, MIN_ZOOM, ViewportDescription, }; @@ -931,9 +932,11 @@ impl WebViewRenderer { { let pipeline_details = self.pipelines.get_mut(pipeline_id)?; if previous_pipeline_id.replace(pipeline_id) != Some(pipeline_id) { - let scroll_result = pipeline_details - .scroll_tree - .scroll_node_or_ancestor(scroll_tree_node, scroll_location); + let scroll_result = pipeline_details.scroll_tree.scroll_node_or_ancestor( + scroll_tree_node, + scroll_location, + ScrollType::InputEvents, + ); if let Some((external_scroll_id, offset)) = scroll_result { return Some(ScrollResult { pipeline_id: *pipeline_id, diff --git a/components/shared/compositing/Cargo.toml b/components/shared/compositing/Cargo.toml index 2e39ef5397b..8578c7dabd2 100644 --- a/components/shared/compositing/Cargo.toml +++ b/components/shared/compositing/Cargo.toml @@ -17,6 +17,7 @@ no-wgl = ["surfman/sm-angle-default"] [dependencies] base = { workspace = true } bincode = { workspace = true } +bitflags = { workspace = true } crossbeam-channel = { workspace = true } dpi = { version = "0.1" } embedder_traits = { workspace = true } diff --git a/components/shared/compositing/display_list.rs b/components/shared/compositing/display_list.rs index 4fb0d5e94db..f716bf0a7a8 100644 --- a/components/shared/compositing/display_list.rs +++ b/components/shared/compositing/display_list.rs @@ -5,6 +5,7 @@ //! Defines data structures which are consumed by the Compositor. use base::id::ScrollTreeNodeId; +use bitflags::bitflags; use embedder_traits::Cursor; use euclid::SideOffsets2D; use malloc_size_of_derive::MallocSizeOf; @@ -18,25 +19,29 @@ use webrender_api::{ StickyOffsetBounds, TransformStyle, }; -/// The scroll sensitivity of a scroll node in a particular axis ie whether it can be scrolled due to -/// input events and script events or only script events. +/// A scroll type, describing whether what kind of action originated this scroll request. +/// This is a bitflag as it is also used to track what kinds of [`ScrollType`]s scroll +/// nodes are sensitive to. #[derive(Clone, Copy, Debug, Deserialize, MallocSizeOf, PartialEq, Serialize)] -pub enum ScrollSensitivity { - /// This node can be scrolled by input and script events. - ScriptAndInputEvents, - /// This node can only be scrolled by script events. - Script, - /// This node cannot be scrolled. - None, +pub struct ScrollType(u8); + +bitflags! { + impl ScrollType: u8 { + /// This node can be scrolled by input events or an input event originated this + /// scroll. + const InputEvents = 1 << 0; + /// This node can be scrolled by script events or script originated this scroll. + const Script = 1 << 1; + } } /// Convert [Overflow] to [ScrollSensitivity]. -impl From for ScrollSensitivity { +impl From for ScrollType { fn from(overflow: Overflow) -> Self { match overflow { - Overflow::Hidden => ScrollSensitivity::Script, - Overflow::Scroll | Overflow::Auto => ScrollSensitivity::ScriptAndInputEvents, - Overflow::Visible | Overflow::Clip => ScrollSensitivity::None, + Overflow::Hidden => ScrollType::Script, + Overflow::Scroll | Overflow::Auto => ScrollType::Script | ScrollType::InputEvents, + Overflow::Visible | Overflow::Clip => ScrollType::empty(), } } } @@ -44,8 +49,8 @@ impl From for ScrollSensitivity { /// The [ScrollSensitivity] of particular node in the vertical and horizontal axes. #[derive(Clone, Copy, Debug, Deserialize, MallocSizeOf, PartialEq, Serialize)] pub struct AxesScrollSensitivity { - pub x: ScrollSensitivity, - pub y: ScrollSensitivity, + pub x: ScrollType, + pub y: ScrollType, } /// Information that Servo keeps alongside WebRender display items @@ -106,6 +111,74 @@ pub struct ScrollableNodeInfo { pub offset: LayoutVector2D, } +impl ScrollableNodeInfo { + fn scroll_to_offset( + &mut self, + new_offset: LayoutVector2D, + context: ScrollType, + ) -> Option { + if !self.scroll_sensitivity.x.contains(context) && + !self.scroll_sensitivity.y.contains(context) + { + return None; + } + + let scrollable_size = self.scrollable_size(); + let original_layer_scroll_offset = self.offset; + + if scrollable_size.width > 0. && self.scroll_sensitivity.x.contains(context) { + self.offset.x = new_offset.x.min(0.0).max(-scrollable_size.width); + } + + if scrollable_size.height > 0. && self.scroll_sensitivity.y.contains(context) { + self.offset.y = new_offset.y.min(0.0).max(-scrollable_size.height); + } + + if self.offset != original_layer_scroll_offset { + Some(self.offset) + } else { + None + } + } + + fn scroll_to_webrender_location( + &mut self, + scroll_location: ScrollLocation, + context: ScrollType, + ) -> Option { + if !self.scroll_sensitivity.x.contains(context) && + !self.scroll_sensitivity.y.contains(context) + { + return None; + } + + let delta = match scroll_location { + ScrollLocation::Delta(delta) => delta, + ScrollLocation::Start => { + if self.offset.y.round() >= 0.0 { + // Nothing to do on this layer. + return None; + } + + self.offset.y = 0.0; + return Some(self.offset); + }, + ScrollLocation::End => { + let end_pos = -self.scrollable_size().height; + if self.offset.y.round() <= end_pos { + // Nothing to do on this layer. + return None; + } + + self.offset.y = end_pos; + return Some(self.offset); + }, + }; + + self.scroll_to_offset(self.offset + delta, context) + } +} + impl ScrollableNodeInfo { fn scrollable_size(&self) -> LayoutSize { self.content_rect.size() - self.clip_rect.size() @@ -178,65 +251,14 @@ impl ScrollTreeNode { pub fn scroll( &mut self, scroll_location: ScrollLocation, + context: ScrollType, ) -> Option<(ExternalScrollId, LayoutVector2D)> { - let info = match self.info { - SpatialTreeNodeInfo::Scroll(ref mut info) => info, - _ => return None, - }; - - if info.scroll_sensitivity.x != ScrollSensitivity::ScriptAndInputEvents && - info.scroll_sensitivity.y != ScrollSensitivity::ScriptAndInputEvents - { + let SpatialTreeNodeInfo::Scroll(ref mut info) = self.info else { return None; - } - - let delta = match scroll_location { - ScrollLocation::Delta(delta) => delta, - ScrollLocation::Start => { - if info.offset.y.round() >= 0.0 { - // Nothing to do on this layer. - return None; - } - - info.offset.y = 0.0; - return Some((info.external_id, info.offset)); - }, - ScrollLocation::End => { - let end_pos = -info.scrollable_size().height; - if info.offset.y.round() <= end_pos { - // Nothing to do on this layer. - return None; - } - - info.offset.y = end_pos; - return Some((info.external_id, info.offset)); - }, }; - let scrollable_size = info.scrollable_size(); - let original_layer_scroll_offset = info.offset; - - if scrollable_size.width > 0. && - info.scroll_sensitivity.x == ScrollSensitivity::ScriptAndInputEvents - { - info.offset.x = (info.offset.x + delta.x) - .min(0.0) - .max(-scrollable_size.width); - } - - if scrollable_size.height > 0. && - info.scroll_sensitivity.y == ScrollSensitivity::ScriptAndInputEvents - { - info.offset.y = (info.offset.y + delta.y) - .min(0.0) - .max(-scrollable_size.height); - } - - if info.offset != original_layer_scroll_offset { - Some((info.external_id, info.offset)) - } else { - None - } + info.scroll_to_webrender_location(scroll_location, context) + .map(|location| (info.external_id, location)) } } @@ -300,17 +322,18 @@ impl ScrollTree { &mut self, scroll_node_id: &ScrollTreeNodeId, scroll_location: ScrollLocation, + context: ScrollType, ) -> Option<(ExternalScrollId, LayoutVector2D)> { let parent = { let node = &mut self.get_node_mut(scroll_node_id); - let result = node.scroll(scroll_location); + let result = node.scroll(scroll_location, context); if result.is_some() { return result; } node.parent }; - parent.and_then(|parent| self.scroll_node_or_ancestor(&parent, scroll_location)) + parent.and_then(|parent| self.scroll_node_or_ancestor(&parent, scroll_location, context)) } /// Given an [`ExternalScrollId`] and an offset, update the scroll offset of the scroll node @@ -319,19 +342,16 @@ impl ScrollTree { &mut self, external_scroll_id: ExternalScrollId, offset: LayoutVector2D, - ) -> bool { - for node in self.nodes.iter_mut() { - match node.info { - SpatialTreeNodeInfo::Scroll(ref mut scroll_info) - if scroll_info.external_id == external_scroll_id => - { - scroll_info.offset = offset; - return true; - }, - _ => {}, - } - } - false + context: ScrollType, + ) -> Option { + self.nodes.iter_mut().find_map(|node| match node.info { + SpatialTreeNodeInfo::Scroll(ref mut scroll_info) + if scroll_info.external_id == external_scroll_id => + { + scroll_info.scroll_to_offset(offset, context) + }, + _ => None, + }) } } diff --git a/components/shared/compositing/tests/compositor.rs b/components/shared/compositing/tests/compositor.rs index e04f1770964..a061d467086 100644 --- a/components/shared/compositing/tests/compositor.rs +++ b/components/shared/compositing/tests/compositor.rs @@ -4,7 +4,7 @@ use base::id::ScrollTreeNodeId; use compositing_traits::display_list::{ - AxesScrollSensitivity, ScrollSensitivity, ScrollTree, ScrollableNodeInfo, SpatialTreeNodeInfo, + AxesScrollSensitivity, ScrollTree, ScrollType, ScrollableNodeInfo, SpatialTreeNodeInfo, StickyNodeInfo, }; use euclid::{SideOffsets2D, Size2D}; @@ -29,8 +29,8 @@ fn add_mock_scroll_node(tree: &mut ScrollTree) -> ScrollTreeNodeId { content_rect: Size2D::new(200.0, 200.0).into(), clip_rect: Size2D::new(100.0, 100.0).into(), scroll_sensitivity: AxesScrollSensitivity { - x: ScrollSensitivity::ScriptAndInputEvents, - y: ScrollSensitivity::ScriptAndInputEvents, + x: ScrollType::Script | ScrollType::InputEvents, + y: ScrollType::Script | ScrollType::InputEvents, }, offset: LayoutVector2D::zero(), }), @@ -47,6 +47,7 @@ fn test_scroll_tree_simple_scroll() { .scroll_node_or_ancestor( &id, ScrollLocation::Delta(LayoutVector2D::new(-20.0, -40.0)), + ScrollType::Script, ) .unwrap(); let expected_offset = LayoutVector2D::new(-20.0, -40.0); @@ -55,7 +56,11 @@ fn test_scroll_tree_simple_scroll() { assert_eq!(scroll_tree.get_node(&id).offset(), Some(expected_offset)); let (scrolled_id, offset) = scroll_tree - .scroll_node_or_ancestor(&id, ScrollLocation::Delta(LayoutVector2D::new(20.0, 40.0))) + .scroll_node_or_ancestor( + &id, + ScrollLocation::Delta(LayoutVector2D::new(20.0, 40.0)), + ScrollType::Script, + ) .unwrap(); let expected_offset = LayoutVector2D::new(0.0, 0.0); assert_eq!(scrolled_id, ExternalScrollId(0, pipeline_id)); @@ -63,8 +68,11 @@ fn test_scroll_tree_simple_scroll() { assert_eq!(scroll_tree.get_node(&id).offset(), Some(expected_offset)); // Scroll offsets must be negative. - let result = scroll_tree - .scroll_node_or_ancestor(&id, ScrollLocation::Delta(LayoutVector2D::new(20.0, 40.0))); + let result = scroll_tree.scroll_node_or_ancestor( + &id, + ScrollLocation::Delta(LayoutVector2D::new(20.0, 40.0)), + ScrollType::Script, + ); assert!(result.is_none()); assert_eq!( scroll_tree.get_node(&id).offset(), @@ -92,6 +100,7 @@ fn test_scroll_tree_simple_scroll_chaining() { .scroll_node_or_ancestor( &unscrollable_child_id, ScrollLocation::Delta(LayoutVector2D::new(-20.0, -40.0)), + ScrollType::Script, ) .unwrap(); let expected_offset = LayoutVector2D::new(-20.0, -40.0); @@ -106,6 +115,7 @@ fn test_scroll_tree_simple_scroll_chaining() { .scroll_node_or_ancestor( &unscrollable_child_id, ScrollLocation::Delta(LayoutVector2D::new(-10.0, -15.0)), + ScrollType::Script, ) .unwrap(); let expected_offset = LayoutVector2D::new(-30.0, -55.0); @@ -127,7 +137,7 @@ fn test_scroll_tree_chain_when_at_extent() { let child_id = add_mock_scroll_node(&mut scroll_tree); let (scrolled_id, offset) = scroll_tree - .scroll_node_or_ancestor(&child_id, ScrollLocation::End) + .scroll_node_or_ancestor(&child_id, ScrollLocation::End, ScrollType::Script) .unwrap(); let expected_offset = LayoutVector2D::new(0.0, -100.0); @@ -144,6 +154,7 @@ fn test_scroll_tree_chain_when_at_extent() { .scroll_node_or_ancestor( &child_id, ScrollLocation::Delta(LayoutVector2D::new(0.0, -10.0)), + ScrollType::Script, ) .unwrap(); let expected_offset = LayoutVector2D::new(0.0, -10.0); @@ -168,8 +179,8 @@ fn test_scroll_tree_chain_through_overflow_hidden() { if let SpatialTreeNodeInfo::Scroll(ref mut scroll_node_info) = node.info { scroll_node_info.scroll_sensitivity = AxesScrollSensitivity { - x: ScrollSensitivity::Script, - y: ScrollSensitivity::Script, + x: ScrollType::Script, + y: ScrollType::Script, }; } @@ -177,6 +188,7 @@ fn test_scroll_tree_chain_through_overflow_hidden() { .scroll_node_or_ancestor( &overflow_hidden_id, ScrollLocation::Delta(LayoutVector2D::new(-20.0, -40.0)), + ScrollType::InputEvents, ) .unwrap(); let expected_offset = LayoutVector2D::new(-20.0, -40.0); diff --git a/tests/wpt/meta/css/css-position/sticky/position-sticky-left-003.html.ini b/tests/wpt/meta/css/css-position/sticky/position-sticky-left-003.html.ini deleted file mode 100644 index 605d9563e86..00000000000 --- a/tests/wpt/meta/css/css-position/sticky/position-sticky-left-003.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[position-sticky-left-003.html] - expected: FAIL diff --git a/tests/wpt/meta/css/css-position/sticky/position-sticky-top-003.html.ini b/tests/wpt/meta/css/css-position/sticky/position-sticky-top-003.html.ini deleted file mode 100644 index c8832f78b51..00000000000 --- a/tests/wpt/meta/css/css-position/sticky/position-sticky-top-003.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[position-sticky-top-003.html] - expected: FAIL