script: More consistently use f32 and have scrolling methods follow the specification more closely (#39104)

This clarifies the units for scrolling:

 - `f32` is used for internal Servo scrolling APIs as that is the unit
   used in WebRender.
 - `f64` is used for the web-exposed scrolling APIs as that is what the
   WebIDL code generator gives us.

Conversions are done consistently at the boundaries of the two APIs.

In addition, web-exposed scrolling methods are refactored a bit to more
closely follow the specification text. In addition, specification text
is added to those methods so that it is clearer that we are following
it.

Testing: This should not change behavior and is thus covered by existing
tests.

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

components/script/dom/document.rs

@@ -1065,8 +1065,7 @@ impl Document {
             });
 
         if let Some((x, y)) = point {
-            self.window
+            self.window.scroll(x, y, ScrollBehavior::Instant)
-                .scroll(x as f64, y as f64, ScrollBehavior::Instant)
         }
     }
 

components/script/dom/element.rs

@@ -64,7 +64,7 @@ use style::{ArcSlice, CaseSensitivityExt, dom_apis, thread_state};
 use style_traits::ParsingMode as CssParsingMode;
 use stylo_atoms::Atom;
 use stylo_dom::ElementState;
-use webrender_api::units::LayoutPixel;
+use webrender_api::units::LayoutVector2D;
 use xml5ever::serialize::TraversalScope::{
     ChildrenOnly as XmlChildrenOnly, IncludeNode as XmlIncludeNode,
 };
@@ -285,13 +285,12 @@ enum ScrollingBox {
 }
 
 impl ScrollingBox {
-    fn scroll_position(&self) -> Vector2D<f64, LayoutPixel> {
+    fn scroll_position(&self) -> LayoutVector2D {
         match self {
             ScrollingBox::Element(element) => element
                 .owner_window()
-                .scroll_offset_query(element.upcast::<Node>())
+                .scroll_offset_query(element.upcast::<Node>()),
-                .cast(),
+            ScrollingBox::Viewport(document) => document.window().scroll_offset(),
-            ScrollingBox::Viewport(document) => document.window().scroll_offset().cast(),
         }
     }
 }
@@ -975,7 +974,7 @@ impl Element {
         scrolling_node: &Node,
         block: ScrollLogicalPosition,
         inline: ScrollLogicalPosition,
-    ) -> Vector2D<f64, LayoutPixel> {
+    ) -> LayoutVector2D {
         let target_bounding_box = self.upcast::<Node>().border_box().unwrap_or_default();
 
         let device_pixel_ratio = self
@@ -989,19 +988,19 @@ impl Element {
         let element_left = target_bounding_box
             .origin
             .x
-            .to_nearest_pixel(device_pixel_ratio) as f64;
+            .to_nearest_pixel(device_pixel_ratio);
         let element_top = target_bounding_box
             .origin
             .y
-            .to_nearest_pixel(device_pixel_ratio) as f64;
+            .to_nearest_pixel(device_pixel_ratio);
         let element_width = target_bounding_box
             .size
             .width
-            .to_nearest_pixel(device_pixel_ratio) as f64;
+            .to_nearest_pixel(device_pixel_ratio);
         let element_height = target_bounding_box
             .size
             .height
-            .to_nearest_pixel(device_pixel_ratio) as f64;
+            .to_nearest_pixel(device_pixel_ratio);
         let element_right = element_left + element_width;
         let element_bottom = element_top + element_height;
 
@@ -1010,10 +1009,10 @@ impl Element {
             // Viewport bounds and current scroll position
             let owner_doc = self.upcast::<Node>().owner_doc();
             let window = owner_doc.window();
-            let viewport_width = window.InnerWidth() as f64;
+            let viewport_width = window.InnerWidth() as f32;
-            let viewport_height = window.InnerHeight() as f64;
+            let viewport_height = window.InnerHeight() as f32;
-            let current_scroll_x = window.ScrollX() as f64;
+            let current_scroll_x = window.ScrollX() as f32;
-            let current_scroll_y = window.ScrollY() as f64;
+            let current_scroll_y = window.ScrollY() as f32;
 
             // For viewport scrolling, we need to add current scroll to get document-relative positions
             let document_element_left = element_left + current_scroll_x;
@@ -1043,19 +1042,23 @@ impl Element {
             // Handle element-specific scrolling
             // Scrolling box bounds and current scroll position
             let scrolling_box = scrolling_node.border_box().unwrap_or_default();
-            let scrolling_left = scrolling_box.origin.x.to_nearest_pixel(device_pixel_ratio) as f64;
+            let scrolling_left = scrolling_box.origin.x.to_nearest_pixel(device_pixel_ratio);
-            let scrolling_top = scrolling_box.origin.y.to_nearest_pixel(device_pixel_ratio) as f64;
+            let scrolling_top = scrolling_box.origin.y.to_nearest_pixel(device_pixel_ratio);
             let scrolling_width = scrolling_box
                 .size
                 .width
-                .to_nearest_pixel(device_pixel_ratio) as f64;
+                .to_nearest_pixel(device_pixel_ratio);
             let scrolling_height = scrolling_box
                 .size
                 .height
-                .to_nearest_pixel(device_pixel_ratio) as f64;
+                .to_nearest_pixel(device_pixel_ratio);
 
-            let current_scroll_x = scrolling_node.downcast::<Element>().unwrap().ScrollLeft();
+            // TODO: This function should accept `Element` and not `Node`.
-            let current_scroll_y = scrolling_node.downcast::<Element>().unwrap().ScrollTop();
+            let scrolling_element = scrolling_node
+                .downcast::<Element>()
+                .expect("Should be provided an Element.");
+            let current_scroll_x = scrolling_element.ScrollLeft() as f32;
+            let current_scroll_y = scrolling_element.ScrollTop() as f32;
 
             // Calculate element position in scroller's content coordinate system
             // Element's viewport position relative to scroller, then add scroll offset to get content position
@@ -1100,13 +1103,11 @@ impl Element {
                                 let positioning_left = positioning_box
                                     .origin
                                     .x
-                                    .to_nearest_pixel(device_pixel_ratio)
+                                    .to_nearest_pixel(device_pixel_ratio);
-                                    as f64;
                                 let positioning_top = positioning_box
                                     .origin
                                     .y
-                                    .to_nearest_pixel(device_pixel_ratio)
+                                    .to_nearest_pixel(device_pixel_ratio);
-                                    as f64;
 
                                 // Calculate the offset of the positioning context relative to the scrolling container
                                 let offset_left = positioning_left - scrolling_left;
@@ -1161,12 +1162,12 @@ impl Element {
     fn calculate_scroll_position_one_axis(
         &self,
         alignment: ScrollLogicalPosition,
-        element_start: f64,
+        element_start: f32,
-        element_end: f64,
+        element_end: f32,
-        element_size: f64,
+        element_size: f32,
-        container_size: f64,
+        container_size: f32,
-        current_scroll_offset: f64,
+        current_scroll_offset: f32,
-    ) -> f64 {
+    ) -> f32 {
         match alignment {
             // Step 1 & 5: If inline is "start", then align element start edge with scrolling box start edge.
             ScrollLogicalPosition::Start => element_start,
@@ -2876,12 +2877,14 @@ impl Element {
         }
     }
 
-    // https://drafts.csswg.org/cssom-view/#dom-element-scroll
+    /// <https://drafts.csswg.org/cssom-view/#dom-element-scroll>
-    // TODO(stevennovaryo): Need to update the scroll API to follow the spec since it is quite outdated.
+    ///
-    pub(crate) fn scroll(&self, x_: f64, y_: f64, behavior: ScrollBehavior) {
+    /// TODO(stevennovaryo): Need to update the scroll API to follow the spec since it is
+    /// quite outdated.
+    pub(crate) fn scroll(&self, x: f64, y: f64, behavior: ScrollBehavior) {
         // Step 1.2 or 2.3
-        let x = if x_.is_finite() { x_ } else { 0.0f64 };
+        let x = if x.is_finite() { x } else { 0.0 } as f32;
-        let y = if y_.is_finite() { y_ } else { 0.0f64 };
+        let y = if y.is_finite() { y } else { 0.0 } as f32;
 
         let node = self.upcast::<Node>();
 
@@ -3549,7 +3552,7 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
         let behavior = ScrollBehavior::Auto;
 
         // Step 1, 2
-        let y = if y_.is_finite() { y_ } else { 0.0f64 };
+        let y = if y_.is_finite() { y_ } else { 0.0 } as f32;
 
         let node = self.upcast::<Node>();
 
@@ -3570,7 +3573,7 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
         // Step 7
         if *self.root_element() == *self {
             if doc.quirks_mode() != QuirksMode::Quirks {
-                win.scroll(win.ScrollX() as f64, y, behavior);
+                win.scroll(win.ScrollX() as f32, y, behavior);
             }
 
             return;
@@ -3581,7 +3584,7 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
             doc.quirks_mode() == QuirksMode::Quirks &&
             !self.is_potentially_scrollable_body()
         {
-            win.scroll(win.ScrollX() as f64, y, behavior);
+            win.scroll(win.ScrollX() as f32, y, behavior);
             return;
         }
 
@@ -3591,7 +3594,7 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
         }
 
         // Step 11
-        win.scroll_an_element(self, self.ScrollLeft(), y, behavior);
+        win.scroll_an_element(self, self.ScrollLeft() as f32, y, behavior);
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-element-scrollleft
@@ -3641,11 +3644,11 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-element-scrollleft
-    fn SetScrollLeft(&self, x_: f64) {
+    fn SetScrollLeft(&self, x: f64) {
         let behavior = ScrollBehavior::Auto;
 
         // Step 1, 2
-        let x = if x_.is_finite() { x_ } else { 0.0f64 };
+        let x = if x.is_finite() { x } else { 0.0 } as f32;
 
         let node = self.upcast::<Node>();
 
@@ -3669,7 +3672,7 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
                 return;
             }
 
-            win.scroll(x, win.ScrollY() as f64, behavior);
+            win.scroll(x, win.ScrollY() as f32, behavior);
             return;
         }
 
@@ -3678,7 +3681,7 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
             doc.quirks_mode() == QuirksMode::Quirks &&
             !self.is_potentially_scrollable_body()
         {
-            win.scroll(x, win.ScrollY() as f64, behavior);
+            win.scroll(x, win.ScrollY() as f32, behavior);
             return;
         }
 
@@ -3688,7 +3691,7 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
         }
 
         // Step 11
-        win.scroll_an_element(self, x, self.ScrollTop(), behavior);
+        win.scroll_an_element(self, x, self.ScrollTop() as f32, behavior);
     }
 
     /// <https://drafts.csswg.org/cssom-view/#dom-element-scrollintoview>

components/script/dom/window.rs

@@ -71,6 +71,7 @@ use num_traits::ToPrimitive;
 use profile_traits::generic_channel as ProfiledGenericChannel;
 use profile_traits::mem::ProfilerChan as MemProfilerChan;
 use profile_traits::time::ProfilerChan as TimeProfilerChan;
+use script_bindings::codegen::GenericBindings::WindowBinding::ScrollToOptions;
 use script_bindings::conversions::SafeToJSValConvertible;
 use script_bindings::interfaces::WindowHelpers;
 use script_bindings::root::Root;
@@ -109,8 +110,7 @@ use crate::dom::bindings::codegen::Bindings::ReportingObserverBinding::Report;
 use crate::dom::bindings::codegen::Bindings::RequestBinding::RequestInit;
 use crate::dom::bindings::codegen::Bindings::VoidFunctionBinding::VoidFunction;
 use crate::dom::bindings::codegen::Bindings::WindowBinding::{
-    self, FrameRequestCallback, ScrollBehavior, ScrollToOptions, WindowMethods,
+    self, FrameRequestCallback, ScrollBehavior, WindowMethods, WindowPostMessageOptions,
-    WindowPostMessageOptions,
 };
 use crate::dom::bindings::codegen::UnionTypes::{
     RequestOrUSVString, TrustedScriptOrString, TrustedScriptOrStringOrFunction,
@@ -1603,48 +1603,85 @@ impl WindowMethods<crate::DomTypeHolder> for Window {
         self.ScrollY()
     }
 
-    // https://drafts.csswg.org/cssom-view/#dom-window-scroll
+    /// <https://drafts.csswg.org/cssom-view/#dom-window-scroll>
     fn Scroll(&self, options: &ScrollToOptions) {
-        // Step 1
+        // Step 1: If invoked with one argument, follow these substeps:
-        let left = options.left.unwrap_or(0.0f64);
+        // Step 1.1: Let options be the argument.
-        let top = options.top.unwrap_or(0.0f64);
+        // Step 1.2: Let x be the value of the left dictionary member of options, if
-        self.scroll(left, top, options.parent.behavior);
+        // present, or the viewport’s current scroll position on the x axis otherwise.
+        let x = options.left.unwrap_or(0.0) as f32;
+
+        // Step 1.3: Let y be the value of the top dictionary member of options, if
+        // present, or the viewport’s current scroll position on the y axis otherwise.
+        let y = options.top.unwrap_or(0.0) as f32;
+
+        // The rest of the specification continues from `Self::scroll`.
+        self.scroll(x, y, options.parent.behavior);
     }
 
-    // https://drafts.csswg.org/cssom-view/#dom-window-scroll
+    /// <https://drafts.csswg.org/cssom-view/#dom-window-scroll>
     fn Scroll_(&self, x: f64, y: f64) {
-        self.scroll(x, y, ScrollBehavior::Auto);
+        // Step 2: If invoked with two arguments, follow these substeps:
+        // Step 2.1 Let options be null converted to a ScrollToOptions dictionary. [WEBIDL]
+        // Step 2.2: Let x and y be the arguments, respectively.
+        self.scroll(x as f32, y as f32, ScrollBehavior::Auto);
     }
 
-    // https://drafts.csswg.org/cssom-view/#dom-window-scrollto
+    /// <https://drafts.csswg.org/cssom-view/#dom-window-scrollto>
+    ///
+    /// > When the scrollTo() method is invoked, the user agent must act as if the
+    /// > scroll() method was invoked with the same arguments.
     fn ScrollTo(&self, options: &ScrollToOptions) {
         self.Scroll(options);
     }
 
-    // https://drafts.csswg.org/cssom-view/#dom-window-scrollto
+    /// <https://drafts.csswg.org/cssom-view/#dom-window-scrollto>:
+    ///
+    /// > When the scrollTo() method is invoked, the user agent must act as if the
+    /// > scroll() method was invoked with the same arguments.
     fn ScrollTo_(&self, x: f64, y: f64) {
-        self.scroll(x, y, ScrollBehavior::Auto);
+        self.Scroll_(x, y)
     }
 
-    // https://drafts.csswg.org/cssom-view/#dom-window-scrollby
+    /// <https://drafts.csswg.org/cssom-view/#dom-window-scrollby>
     fn ScrollBy(&self, options: &ScrollToOptions) {
-        // Step 1
+        // When the scrollBy() method is invoked, the user agent must run these steps:
-        let x = options.left.unwrap_or(0.0f64);
+        // Step 1: If invoked with two arguments, follow these substeps:
-        let y = options.top.unwrap_or(0.0f64);
+        //   This doesn't apply here.
-        self.ScrollBy_(x, y);
+
-        self.scroll(x, y, options.parent.behavior);
+        // Step 2: Normalize non-finite values for the left and top dictionary members of options.
+        let mut options = options.clone();
+        let x = options.left.unwrap_or(0.0);
+        let x = if x.is_finite() { x } else { 0.0 };
+        let y = options.top.unwrap_or(0.0);
+        let y = if y.is_finite() { y } else { 0.0 };
+
+        // Step 3: Add the value of scrollX to the left dictionary member.
+        options.left.replace(x + self.ScrollX() as f64);
+
+        // Step 4. Add the value of scrollY to the top dictionary member.
+        options.top.replace(y + self.ScrollY() as f64);
+
+        // Step 5: Act as if the scroll() method was invoked with options as the only argument.
+        self.Scroll(&options)
     }
 
-    // https://drafts.csswg.org/cssom-view/#dom-window-scrollby
+    /// <https://drafts.csswg.org/cssom-view/#dom-window-scrollby>
     fn ScrollBy_(&self, x: f64, y: f64) {
-        let scroll_offset = self.scroll_offset();
+        // When the scrollBy() method is invoked, the user agent must run these steps:
-        // Step 3
+        // Step 1: If invoked with two arguments, follow these substeps:
-        let left = x + scroll_offset.x as f64;
+        // Step 1.1: Let options be null converted to a ScrollToOptions dictionary.
-        // Step 4
+        let mut options = ScrollToOptions::empty();
-        let top = y + scroll_offset.y as f64;
 
-        // Step 5
+        // Step 1.2: Let x and y be the arguments, respectively.
-        self.scroll(left, top, ScrollBehavior::Auto);
+        // Step 1.3: Let the left dictionary member of options have the value x.
+        options.left.replace(x);
+
+        // Step 1.5:  Let the top dictionary member of options have the value y.
+        options.top.replace(y);
+
+        // Now follow the specification for the one argument option.
+        self.ScrollBy(&options);
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-window-resizeto
@@ -2062,46 +2099,56 @@ impl Window {
     }
 
     /// <https://drafts.csswg.org/cssom-view/#dom-window-scroll>
-    pub(crate) fn scroll(&self, x_: f64, y_: f64, behavior: ScrollBehavior) {
+    pub(crate) fn scroll(&self, x: f32, y: f32, behavior: ScrollBehavior) {
-        // Step 3
+        // Step 3: Normalize non-finite values for x and y.
-        let xfinite = if x_.is_finite() { x_ } else { 0.0f64 };
+        let xfinite = if x.is_finite() { x } else { 0.0 };
-        let yfinite = if y_.is_finite() { y_ } else { 0.0f64 };
+        let yfinite = if y.is_finite() { y } else { 0.0 };
 
-        // TODO Step 4 - determine if a window has a viewport
+        // Step 4: If there is no viewport, abort these steps.
+        // Currently every frame has a viewport in Servo.
 
-        // Step 5 & 6
+        // Step 5. Let `viewport width` be the width of the viewport excluding the width
-        // TODO: Remove scrollbar dimensions.
+        // of the scroll bar, if any.
+        // Step 6. `Let viewport height` be the height of the viewport excluding the
+        // height of the scroll bar, if any.
+        //
+        // TODO: Servo does not yet support scrollbars.
         let viewport = self.viewport_details.get().size;
 
-        // Step 7 & 8
+        // Step 7:
-        // TODO: Consider `block-end` and `inline-end` overflow direction.
+        // If the viewport has rightward overflow direction
-        let scrolling_area = self.scrolling_area_query(None);
+        //    Let x be max(0, min(x, viewport scrolling area width - viewport width)).
-        let x = xfinite
+        // If the viewport has leftward overflow direction
-            .min(scrolling_area.width() as f64 - viewport.width as f64)
+        //    Let x be min(0, max(x, viewport width - viewport scrolling area width)).
-            .max(0.0f64);
+        // TODO: Implement this.
-        let y = yfinite
-            .min(scrolling_area.height() as f64 - viewport.height as f64)
-            .max(0.0f64);
 
-        // Step 10
+        // Step 8:
-        // TODO handling ongoing smooth scrolling
+        // If the viewport has downward overflow direction
+        //    Let y be max(0, min(y, viewport scrolling area height - viewport height)).
+        // If the viewport has upward overflow direction
+        //    Let y be min(0, max(y, viewport height - viewport scrolling area height)).
+        // TODO: Implement this.
+
+        // Step 9: Let position be the scroll position the viewport would have by aligning
+        // the x-coordinate x of the viewport scrolling area with the left of the viewport
+        // and aligning the y-coordinate y of the viewport scrolling area with the top of
+        // the viewport.
+        let scrolling_area = self.scrolling_area_query(None);
+        let x = xfinite.clamp(0.0, scrolling_area.width() as f32 - viewport.width);
+        let y = yfinite.clamp(0.0, scrolling_area.height() as f32 - viewport.height);
+
+        // Step 10: If position is the same as the viewport’s current scroll position, and
+        // the viewport does not have an ongoing smooth scroll, abort these steps.
         let scroll_offset = self.scroll_offset();
-        if x == scroll_offset.x as f64 && y == scroll_offset.y as f64 {
+        if x == scroll_offset.x && y == scroll_offset.y {
             return;
         }
 
-        // TODO Step 11
+        // Step 11: Let document be the viewport’s associated Document.
-
         // 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(
+        self.perform_a_scroll(x, y, self.pipeline_id().root_scroll_id(), behavior, None);
-            x.to_f32().unwrap_or(0.0f32),
-            y.to_f32().unwrap_or(0.0f32),
-            self.pipeline_id().root_scroll_id(),
-            behavior,
-            None,
-        );
     }
 
     /// <https://drafts.csswg.org/cssom-view/#perform-a-scroll>
@@ -2483,8 +2530,8 @@ impl Window {
     pub(crate) fn scroll_an_element(
         &self,
         element: &Element,
-        x_: f64,
+        x: f32,
-        y_: f64,
+        y: f32,
         behavior: ScrollBehavior,
     ) {
         let scroll_id = ExternalScrollId(
@@ -2498,13 +2545,7 @@ impl Window {
         // Step 6.
         // > Perform a scroll of box to position, element as the associated element and behavior as
         // > the scroll behavior.
-        self.perform_a_scroll(
+        self.perform_a_scroll(x, y, scroll_id, behavior, Some(element));
-            x_.to_f32().unwrap_or(0.0f32),
-            y_.to_f32().unwrap_or(0.0f32),
-            scroll_id,
-            behavior,
-            Some(element),
-        );
     }
 
     pub(crate) fn resolved_style_query(

components/script_bindings/codegen/Bindings.conf

@@ -860,6 +860,14 @@ Dictionaries = {
     'derives': ['Clone', 'MallocSizeOf'],
 },
 
+'ScrollOptions': {
+    'derives': ['Clone'],
+},
+
+'ScrollToOptions': {
+    'derives': ['Clone'],
+},
+
 'StereoPannerOptions': {
     'derives': ['Clone', 'Copy'],
 },