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 <mrobinson@igalia.com>

Cargo.lock

@@ -1123,6 +1123,7 @@ version = "0.0.1"
 dependencies = [
  "base",
  "bincode",
+ "bitflags 2.9.1",
  "crossbeam-channel",
  "dpi",
  "embedder_traits",

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,
                     )
-                {
+                else {
-                    warn!("Could not scroll not with id: {external_scroll_id:?}");
                     return;
-                }
+                };
 
                 let mut txn = Transaction::new();
                 txn.set_scroll_offsets(
                     external_scroll_id,
                     vec![SampledScrollOffset {
-                        offset,
+                        offset: -offset,
                         generation: 0,
                     }],
                 );

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
+                let scroll_result = pipeline_details.scroll_tree.scroll_node_or_ancestor(
-                    .scroll_tree
+                    scroll_tree_node,
-                    .scroll_node_or_ancestor(scroll_tree_node, scroll_location);
+                    scroll_location,
+                    ScrollType::InputEvents,
+                );
                 if let Some((external_scroll_id, offset)) = scroll_result {
                     return Some(ScrollResult {
                         pipeline_id: *pipeline_id,

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 }

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
+/// A scroll type, describing whether what kind of action originated this scroll request.
-/// input events and script events or only script events.
+/// 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 {
+pub struct ScrollType(u8);
-    /// This node can be scrolled by input and script events.
+
-    ScriptAndInputEvents,
+bitflags! {
-    /// This node can only be scrolled by script events.
+    impl ScrollType: u8 {
-    Script,
+        /// This node can be scrolled by input events or an input event originated this
-    /// This node cannot be scrolled.
+        /// scroll.
-    None,
+        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<Overflow> for ScrollSensitivity {
+impl From<Overflow> for ScrollType {
     fn from(overflow: Overflow) -> Self {
         match overflow {
-            Overflow::Hidden => ScrollSensitivity::Script,
+            Overflow::Hidden => ScrollType::Script,
-            Overflow::Scroll | Overflow::Auto => ScrollSensitivity::ScriptAndInputEvents,
+            Overflow::Scroll | Overflow::Auto => ScrollType::Script | ScrollType::InputEvents,
-            Overflow::Visible | Overflow::Clip => ScrollSensitivity::None,
+            Overflow::Visible | Overflow::Clip => ScrollType::empty(),
         }
     }
 }
@@ -44,8 +49,8 @@ impl From<Overflow> 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 x: ScrollType,
-    pub y: ScrollSensitivity,
+    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<LayoutVector2D> {
+        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<LayoutVector2D> {
+        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 {
+        let SpatialTreeNodeInfo::Scroll(ref mut info) = self.info else {
-            SpatialTreeNodeInfo::Scroll(ref mut info) => info,
-            _ => return None,
-        };
-
-        if info.scroll_sensitivity.x != ScrollSensitivity::ScriptAndInputEvents &&
-            info.scroll_sensitivity.y != ScrollSensitivity::ScriptAndInputEvents
-        {
             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();
+        info.scroll_to_webrender_location(scroll_location, context)
-        let original_layer_scroll_offset = info.offset;
+            .map(|location| (info.external_id, location))
-
-        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
-        }
     }
 }
 
@@ -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 {
+        context: ScrollType,
-        for node in self.nodes.iter_mut() {
+    ) -> Option<LayoutVector2D> {
-            match node.info {
+        self.nodes.iter_mut().find_map(|node| match node.info {
-                SpatialTreeNodeInfo::Scroll(ref mut scroll_info)
+            SpatialTreeNodeInfo::Scroll(ref mut scroll_info)
-                    if scroll_info.external_id == external_scroll_id =>
+                if scroll_info.external_id == external_scroll_id =>
-                {
+            {
-                    scroll_info.offset = offset;
+                scroll_info.scroll_to_offset(offset, context)
-                    return true;
+            },
-                },
+            _ => None,
-                _ => {},
+        })
-            }
-        }
-        false
     }
 }
 

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,
+                x: ScrollType::Script | ScrollType::InputEvents,
-                y: ScrollSensitivity::ScriptAndInputEvents,
+                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
+    let result = scroll_tree.scroll_node_or_ancestor(
-        .scroll_node_or_ancestor(&id, ScrollLocation::Delta(LayoutVector2D::new(20.0, 40.0)));
+        &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,
+            x: ScrollType::Script,
-            y: ScrollSensitivity::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);

tests/wpt/meta/css/css-position/sticky/position-sticky-left-003.html.ini

@@ -1,2 +0,0 @@
-[position-sticky-left-003.html]
-  expected: FAIL

tests/wpt/meta/css/css-position/sticky/position-sticky-top-003.html.ini

@@ -1,2 +0,0 @@
-[position-sticky-top-003.html]
-  expected: FAIL