script: Get scroll offsets from layout (#37509)

No longer store scroll offsets for elements in the DOM. Instead
consistently get and set these in layout's `ScrollTree`. This more
consistently requires layout to run when querying scroll offsets, which
ensures that they are up-to-date and properly bounded by scrollable
overflow area.

Testing: This causes several WPT tests to start passing, and one to
start
failing. In the case of
`/shadow-dom/scroll-to-the-fragment-in-shadow-tree.html`, I believe the
issue
is that we don't properly handle scrolling and shadow DOM elements.
Before, the
faulty scrolling was hiding this issue.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>

components/layout/layout_impl.rs

@@ -472,6 +472,14 @@ impl Layout for LayoutThread {
             .scroll_tree
             .set_all_scroll_offsets(scroll_states);
     }
+
+    fn scroll_offset(&self, id: ExternalScrollId) -> Option<LayoutVector2D> {
+        self.stacking_context_tree
+            .borrow_mut()
+            .as_mut()
+            .and_then(|tree| tree.compositor_info.scroll_tree.scroll_offset(id))
+            .map(|scroll_offset| -scroll_offset)
+    }
 }
 
 impl LayoutThread {
@@ -996,7 +1004,7 @@ impl LayoutThread {
             .scroll_tree
             .set_scroll_offset_for_node_with_external_scroll_id(
                 external_scroll_id,
-                offset,
+                -offset,
                 ScrollType::Script,
             )
         {

components/script/dom/element.rs

@@ -3080,7 +3080,7 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
         }
 
         // Step 9
-        let point = node.scroll_offset();
+        let point = win.scroll_offset_query(node, can_gc);
         point.y.abs() as f64
     }
 
@@ -3179,7 +3179,7 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
         }
 
         // Step 9
-        let point = node.scroll_offset();
+        let point = win.scroll_offset_query(node, can_gc);
         point.x.abs() as f64
     }
 

components/script/dom/mouseevent.rs

@@ -9,6 +9,7 @@ use dom_struct::dom_struct;
 use embedder_traits::CompositorHitTestResult;
 use euclid::default::Point2D;
 use js::rust::HandleObject;
+use script_bindings::codegen::GenericBindings::WindowBinding::WindowMethods;
 use servo_config::pref;
 
 use crate::dom::bindings::codegen::Bindings::EventBinding::Event_Binding::EventMethods;
@@ -373,9 +374,7 @@ impl MouseEventMethods<crate::DomTypeHolder> for MouseEvent {
         if self.upcast::<Event>().dispatching() {
             self.page_x.get()
         } else {
-            let global = self.global();
-            let window = global.as_window();
-            window.current_viewport().origin.x.to_px() + self.client_x.get()
+            self.global().as_window().ScrollX() + self.client_x.get()
         }
     }
 
@@ -384,9 +383,7 @@ impl MouseEventMethods<crate::DomTypeHolder> for MouseEvent {
         if self.upcast::<Event>().dispatching() {
             self.page_y.get()
         } else {
-            let global = self.global();
-            let window = global.as_window();
-            window.current_viewport().origin.y.to_px() + self.client_y.get()
+            self.global().as_window().ScrollY() + self.client_y.get()
         }
     }
 

components/script/dom/node.rs

@@ -18,7 +18,7 @@ use bitflags::bitflags;
 use devtools_traits::NodeInfo;
 use dom_struct::dom_struct;
 use embedder_traits::UntrustedNodeAddress;
-use euclid::default::{Rect, Size2D, Vector2D};
+use euclid::default::{Rect, Size2D};
 use html5ever::serialize::HtmlSerializer;
 use html5ever::{Namespace, Prefix, QualName, ns, serialize as html_serialize};
 use js::jsapi::JSObject;
@@ -980,12 +980,6 @@ impl Node {
         window.scrolling_area_query(Some(self), can_gc)
     }
 
-    pub(crate) fn scroll_offset(&self) -> Vector2D<f32> {
-        let document = self.owner_doc();
-        let window = document.window();
-        window.scroll_offset_query(self).to_untyped()
-    }
-
     /// <https://dom.spec.whatwg.org/#dom-childnode-before>
     pub(crate) fn before(&self, nodes: Vec<NodeOrString>, can_gc: CanGc) -> ErrorResult {
         // Step 1.

components/script/dom/window.rs

@@ -36,8 +36,8 @@ use embedder_traits::{
     GamepadUpdateType, PromptResponse, SimpleDialog, Theme, ViewportDetails, WebDriverJSError,
     WebDriverJSResult,
 };
-use euclid::default::{Point2D as UntypedPoint2D, Rect as UntypedRect};
-use euclid::{Point2D, Rect, Scale, Size2D, Vector2D};
+use euclid::default::{Point2D as UntypedPoint2D, Rect as UntypedRect, Size2D as UntypedSize2D};
+use euclid::{Point2D, Scale, Size2D, Vector2D};
 use fonts::FontContext;
 use ipc_channel::ipc::{self, IpcSender};
 use js::conversions::ToJSValConvertible;
@@ -77,7 +77,6 @@ use servo_arc::Arc as ServoArc;
 use servo_config::{opts, pref};
 use servo_geometry::{DeviceIndependentIntRect, 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;
@@ -311,15 +310,13 @@ pub(crate) struct Window {
     /// The current state of the window object
     current_state: Cell<WindowState>,
 
+    /// The current size of the viewport. This might change if the `WebView` or containing `<iframe>`
+    /// for this `Window` object change.
     #[no_trace]
-    current_viewport: Cell<UntypedRect<Au>>,
+    current_viewport_size: Cell<UntypedSize2D<Au>>,
 
     error_reporter: CSSErrorReporter,
 
-    /// A list of scroll offsets for each scrollable element.
-    #[no_trace]
-    scroll_offsets: DomRefCell<HashMap<OpaqueNode, Vector2D<f32, LayoutPixel>>>,
-
     /// All the MediaQueryLists we need to update
     media_query_lists: DOMTracker<MediaQueryList>,
 
@@ -540,20 +537,6 @@ impl Window {
         Some(&self.error_reporter)
     }
 
-    /// Sets a new list of scroll offsets.
-    ///
-    /// This is called when layout gives us new ones and WebRender is in use.
-    pub(crate) fn set_scroll_offsets(
-        &self,
-        offsets: HashMap<OpaqueNode, Vector2D<f32, LayoutPixel>>,
-    ) {
-        *self.scroll_offsets.borrow_mut() = offsets
-    }
-
-    pub(crate) fn current_viewport(&self) -> UntypedRect<Au> {
-        self.current_viewport.clone().get()
-    }
-
     pub(crate) fn webgl_chan(&self) -> Option<WebGLCommandSender> {
         self.webgl_chan
             .as_ref()
@@ -1547,9 +1530,13 @@ impl WindowMethods<crate::DomTypeHolder> for Window {
         self.viewport_details.get().size.width.to_i32().unwrap_or(0)
     }
 
-    // https://drafts.csswg.org/cssom-view/#dom-window-scrollx
+    /// <https://drafts.csswg.org/cssom-view/#dom-window-scrollx>
     fn ScrollX(&self) -> i32 {
-        self.current_viewport.get().origin.x.to_px()
+        self.scroll_offset_query_with_external_scroll_id(
+            self.pipeline_id().root_scroll_id(),
+            CanGc::note(),
+        )
+        .x as i32
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-window-pagexoffset
@@ -1557,9 +1544,13 @@ impl WindowMethods<crate::DomTypeHolder> for Window {
         self.ScrollX()
     }
 
-    // https://drafts.csswg.org/cssom-view/#dom-window-scrolly
+    /// <https://drafts.csswg.org/cssom-view/#dom-window-scrolly>
     fn ScrollY(&self) -> i32 {
-        self.current_viewport.get().origin.y.to_px()
+        self.scroll_offset_query_with_external_scroll_id(
+            self.pipeline_id().root_scroll_id(),
+            CanGc::note(),
+        )
+        .y as i32
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-window-pageyoffset
@@ -2073,14 +2064,13 @@ impl Window {
         }
 
         //TODO Step 11
-        //let document = self.Document();
-        // Step 12
-        let x = x.to_f32().unwrap_or(0.0f32);
-        let y = y.to_f32().unwrap_or(0.0f32);
-        self.update_viewport_for_scroll(x, y);
+
+        // Step 12: Perform a scroll of the viewport to position, document’s root element
+        // as the associated element, if there is one, or null otherwise, and the scroll
+        // behavior being the value of the behavior dictionary member of options.
         self.perform_a_scroll(
-            x,
-            y,
+            x.to_f32().unwrap_or(0.0f32),
+            y.to_f32().unwrap_or(0.0f32),
             self.pipeline_id().root_scroll_id(),
             behavior,
             None,
@@ -2102,17 +2092,11 @@ impl Window {
         // TODO(mrobinson, #18709): Add smooth scrolling support to WebRender so that we can
         // properly process ScrollBehavior here.
         self.reflow(
-            ReflowGoal::UpdateScrollNode(scroll_id, Vector2D::new(-x, -y)),
+            ReflowGoal::UpdateScrollNode(scroll_id, Vector2D::new(x, y)),
             can_gc,
         );
     }
 
-    pub(crate) fn update_viewport_for_scroll(&self, x: f32, y: f32) {
-        let size = self.current_viewport.get().size;
-        let new_viewport = Rect::new(Point2D::new(Au::from_f32_px(x), Au::from_f32_px(y)), size);
-        self.current_viewport.set(new_viewport)
-    }
-
     pub(crate) fn device_pixel_ratio(&self) -> Scale<f32, CSSPixel, DevicePixel> {
         self.viewport_details.get().hidpi_scale_factor
     }
@@ -2549,17 +2533,34 @@ impl Window {
         node: Option<&Node>,
         can_gc: CanGc,
     ) -> UntypedRect<i32> {
-        self.layout_reflow(QueryMsg::ScrollingAreaQuery, can_gc);
+        self.layout_reflow(QueryMsg::ScrollingAreaOrOffsetQuery, can_gc);
         self.layout
             .borrow()
             .query_scrolling_area(node.map(Node::to_trusted_node_address))
     }
 
-    pub(crate) fn scroll_offset_query(&self, node: &Node) -> Vector2D<f32, LayoutPixel> {
-        if let Some(scroll_offset) = self.scroll_offsets.borrow().get(&node.to_opaque()) {
-            return *scroll_offset;
+    pub(crate) fn scroll_offset_query(
+        &self,
+        node: &Node,
+        can_gc: CanGc,
+    ) -> Vector2D<f32, LayoutPixel> {
+        let external_scroll_id = ExternalScrollId(
+            combine_id_with_fragment_type(node.to_opaque().id(), FragmentType::FragmentBody),
+            self.pipeline_id().into(),
+        );
+        self.scroll_offset_query_with_external_scroll_id(external_scroll_id, can_gc)
     }
-        Vector2D::new(0.0, 0.0)
+
+    fn scroll_offset_query_with_external_scroll_id(
+        &self,
+        external_scroll_id: ExternalScrollId,
+        can_gc: CanGc,
+    ) -> Vector2D<f32, LayoutPixel> {
+        self.layout_reflow(QueryMsg::ScrollingAreaOrOffsetQuery, can_gc);
+        self.layout
+            .borrow()
+            .scroll_offset(external_scroll_id)
+            .unwrap_or_default()
     }
 
     // https://drafts.csswg.org/cssom-view/#element-scrolling-members
@@ -2571,12 +2572,6 @@ impl Window {
         behavior: ScrollBehavior,
         can_gc: CanGc,
     ) {
-        // The scroll offsets are immediatly updated since later calls
-        // to topScroll and others may access the properties before
-        // webrender has a chance to update the offsets.
-        self.scroll_offsets
-            .borrow_mut()
-            .insert(node.to_opaque(), Vector2D::new(x_ as f32, y_ as f32));
         let scroll_id = ExternalScrollId(
             combine_id_with_fragment_type(node.to_opaque().id(), FragmentType::FragmentBody),
             self.pipeline_id().into(),
@@ -2823,13 +2818,16 @@ impl Window {
         self.unhandled_resize_event.borrow_mut().take()
     }
 
-    pub(crate) fn set_viewport(&self, new_viewport: UntypedRect<f32>) {
-        let new_viewport = f32_rect_to_au_rect(new_viewport);
-        if new_viewport == self.current_viewport.get() {
+    pub(crate) fn set_viewport_size(&self, new_viewport_size: UntypedSize2D<f32>) {
+        let new_viewport_size = Size2D::new(
+            Au::from_f32_px(new_viewport_size.width),
+            Au::from_f32_px(new_viewport_size.height),
+        );
+        if new_viewport_size == self.current_viewport_size.get() {
             return;
         }
 
-        self.current_viewport.set(new_viewport);
+        self.current_viewport_size.set(new_viewport_size);
 
         // The document needs to be repainted, because the initial containing block
         // is now a different size.
@@ -3124,14 +3122,13 @@ impl Window {
             bluetooth_extra_permission_data: BluetoothExtraPermissionData::new(),
             unhandled_resize_event: Default::default(),
             viewport_details: Cell::new(viewport_details),
-            current_viewport: Cell::new(initial_viewport.to_untyped()),
+            current_viewport_size: Cell::new(initial_viewport.to_untyped().size),
             layout_blocker: Cell::new(LayoutBlocker::WaitingForParse),
             current_state: Cell::new(WindowState::Alive),
             devtools_marker_sender: Default::default(),
             devtools_markers: Default::default(),
             webdriver_script_chan: Default::default(),
             error_reporter,
-            scroll_offsets: Default::default(),
             media_query_lists: DOMTracker::new(),
             #[cfg(feature = "bluetooth")]
             test_runner: Default::default(),
@@ -3245,7 +3242,7 @@ fn debug_reflow_events(id: PipelineId, reflow_goal: &ReflowGoal) {
             QueryMsg::ContentBoxes => "\tContentBoxesQuery",
             QueryMsg::NodesFromPointQuery => "\tNodesFromPointQuery",
             QueryMsg::ClientRectQuery => "\tClientRectQuery",
-            QueryMsg::ScrollingAreaQuery => "\tNodeScrollGeometryQuery",
+            QueryMsg::ScrollingAreaOrOffsetQuery => "\tNodeScrollGeometryQuery",
             QueryMsg::ResolvedStyleQuery => "\tResolvedStyleQuery",
             QueryMsg::ResolvedFontStyleQuery => "\nResolvedFontStyleQuery",
             QueryMsg::OffsetParentQuery => "\tOffsetParentQuery",

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, TimerId, TimerScheduler};
@@ -2054,16 +2051,6 @@ impl ScriptThread {
                 window
                     .layout_mut()
                     .set_scroll_offsets_from_renderer(&scroll_states);
-
-                let mut scroll_offsets = HashMap::new();
-                for (scroll_id, scroll_offset) in scroll_states.into_iter() {
-                    if scroll_id.is_root() {
-                        window.update_viewport_for_scroll(-scroll_offset.x, -scroll_offset.y);
-                    } else if let Some(node_id) = node_id_from_scroll_id(scroll_id.0 as usize) {
-                        scroll_offsets.insert(OpaqueNode(node_id), -scroll_offset);
-                    }
-                }
-                window.set_scroll_offsets(scroll_offsets)
             },
         )
     }
@@ -2580,7 +2567,7 @@ impl ScriptThread {
     fn handle_viewport(&self, id: PipelineId, rect: Rect<f32>) {
         let document = self.documents.borrow().find_document(id);
         if let Some(document) = document {
-            document.window().set_viewport(rect);
+            document.window().set_viewport_size(rect.size);
             return;
         }
         let loads = self.incomplete_loads.borrow();

components/shared/compositing/display_list.rs

@@ -424,6 +424,15 @@ impl ScrollTree {
             _ => None,
         }))
     }
+
+    /// Get the scroll offset for the given [`ExternalScrollId`] or `None` if that node cannot
+    /// be found in the tree.
+    pub fn scroll_offset(&self, id: ExternalScrollId) -> Option<LayoutVector2D> {
+        self.nodes.iter().find_map(|node| match node.info {
+            SpatialTreeNodeInfo::Scroll(ref info) if info.external_id == id => Some(info.offset),
+            _ => None,
+        })
+    }
 }
 
 /// In order to pretty print the [ScrollTree] structure, we are converting

components/shared/script_layout/lib.rs

@@ -251,6 +251,10 @@ pub trait Layout {
         scroll_states: &HashMap<ExternalScrollId, LayoutVector2D>,
     );
 
+    /// Get the scroll offset of the given scroll node with id of [`ExternalScrollId`] or `None` if it does
+    /// not exist in the tree.
+    fn scroll_offset(&self, id: ExternalScrollId) -> Option<LayoutVector2D>;
+
     fn query_content_box(&self, node: TrustedNodeAddress) -> Option<Rect<Au>>;
     fn query_content_boxes(&self, node: TrustedNodeAddress) -> Vec<Rect<Au>>;
     fn query_client_rect(&self, node: TrustedNodeAddress) -> Rect<i32>;
@@ -309,7 +313,7 @@ pub enum QueryMsg {
     ContentBox,
     ContentBoxes,
     ClientRectQuery,
-    ScrollingAreaQuery,
+    ScrollingAreaOrOffsetQuery,
     OffsetParentQuery,
     TextIndexQuery,
     NodesFromPointQuery,
@@ -351,13 +355,13 @@ impl ReflowGoal {
                 QueryMsg::InnerWindowDimensionsQuery |
                 QueryMsg::NodesFromPointQuery |
                 QueryMsg::ResolvedStyleQuery |
+                QueryMsg::ScrollingAreaOrOffsetQuery |
                 QueryMsg::TextIndexQuery => true,
                 QueryMsg::ClientRectQuery |
                 QueryMsg::ContentBox |
                 QueryMsg::ContentBoxes |
                 QueryMsg::OffsetParentQuery |
                 QueryMsg::ResolvedFontStyleQuery |
-                QueryMsg::ScrollingAreaQuery |
                 QueryMsg::StyleQuery => false,
             },
         }
@@ -375,7 +379,7 @@ impl ReflowGoal {
                 QueryMsg::ContentBox |
                 QueryMsg::ContentBoxes |
                 QueryMsg::ClientRectQuery |
-                QueryMsg::ScrollingAreaQuery |
+                QueryMsg::ScrollingAreaOrOffsetQuery |
                 QueryMsg::ResolvedStyleQuery |
                 QueryMsg::ResolvedFontStyleQuery |
                 QueryMsg::OffsetParentQuery |

tests/wpt/meta/css/CSS2/positioning/relpos-percentage-top-in-scrollable.html.ini

@@ -1,3 +0,0 @@
-[relpos-percentage-top-in-scrollable.html]
-  [Top percentage resolved correctly for overflow contribution]
-    expected: FAIL

tests/wpt/meta/css/css-overflow/incremental-scroll-002.html.ini

@@ -1,2 +0,0 @@
-[incremental-scroll-002.html]
-  expected: TIMEOUT

tests/wpt/meta/css/cssom-view/elementScroll.html.ini

@@ -1,3 +0,0 @@
-[elementScroll.html]
-  [Element scroll maximum test]
-    expected: FAIL

tests/wpt/meta/css/cssom-view/scrollLeftTop.html.ini

@@ -1,7 +1,4 @@
 [scrollLeftTop.html]
-  [writing-mode:vertical-lr; direction:ltr]
-    expected: FAIL
-
   [writing-mode:vertical-rl; direction:rtl]
     expected: FAIL
 
@@ -11,9 +8,5 @@
   [writing-mode:vertical-rl; direction:ltr]
     expected: FAIL
 
-  [writing-mode:horizontal-tb; direction:ltr]
-    expected: FAIL
-
   [writing-mode:horizontal-tb; direction:rtl]
     expected: FAIL
-

tests/wpt/meta/css/cssom-view/scrolling-quirks-vs-nonquirks.html.ini

@@ -8,15 +8,6 @@
   [scrollWidth/scrollHeight on the HTML body element in quirks mode]
     expected: FAIL
 
-  [scroll() on the HTML body element in non-quirks mode]
-    expected: FAIL
-
-  [scrollBy() on the HTML body element in non-quirks mode]
-    expected: FAIL
-
-  [scrollLeft/scrollTop on the HTML body element in non-quirks mode]
-    expected: FAIL
-
   [scroll() on the root element in non-quirks mode]
     expected: FAIL
 

tests/wpt/meta/shadow-dom/scroll-to-the-fragment-in-shadow-tree.html.ini

@@ -0,0 +1,27 @@
+[scroll-to-the-fragment-in-shadow-tree.html]
+  [The user agent scroll to the fragment when there is an anchor element with a name attribute exactly equal to the decoded fragid]
+    expected: FAIL
+
+  [The user agent should not scroll to an element with an ID exactly equal to the decoded fragid in an open shadow tree]
+    expected: FAIL
+
+  [The user agent should not scroll to an element with an ID exactly equal to the decoded fragid in a closed shadow tree]
+    expected: FAIL
+
+  [The user agent should not scroll to an anchor element with a name attribute exactly equal to the decoded fragid in an open shadow tree]
+    expected: FAIL
+
+  [The user agent should not scroll to an anchor element with a name attribute exactly equal to the decoded fragid in a closed shadow tree]
+    expected: FAIL
+
+  [The user agent should scroll to an element with an ID exactly equal to the decoded fragid in the document tree even if there was another element with the same ID inside an open shadow tree earlier in tree order]
+    expected: FAIL
+
+  [The user agent should scroll to an element with an ID exactly equal to the decoded fragid in the document tree even if there was another element with the same ID inside a closed shadow tree earlier in tree order]
+    expected: FAIL
+
+  [The user agent should scroll to an anchor element with a name attribute exactly equal to the decoded fragid in the document tree even if there was another element with the same ID inside an open shadow tree earlier in tree order]
+    expected: FAIL
+
+  [The user agent should scroll to an anchor element with a name attribute exactly equal to the decoded fragid in the document tree even if there was another element with the same ID inside a closed shadow tree earlier in tree order]
+    expected: FAIL