From 0ae9ee28d58bf621d519c2e23d0f7de49d14dcc7 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Wed, 3 Sep 2025 10:29:56 -0700 Subject: [PATCH] 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 --- components/script/dom/document.rs | 3 +- components/script/dom/element.rs | 91 +++++----- components/script/dom/window.rs | 167 +++++++++++------- .../script_bindings/codegen/Bindings.conf | 8 + 4 files changed, 160 insertions(+), 109 deletions(-) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 901e2a74254..a22b177186b 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -1065,8 +1065,7 @@ impl Document { }); if let Some((x, y)) = point { - self.window - .scroll(x as f64, y as f64, ScrollBehavior::Instant) + self.window.scroll(x, y, ScrollBehavior::Instant) } } diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 139b3228193..461989a8ab2 100644 --- a/components/script/dom/element.rs +++ b/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 { + fn scroll_position(&self) -> LayoutVector2D { match self { ScrollingBox::Element(element) => element .owner_window() - .scroll_offset_query(element.upcast::()) - .cast(), - ScrollingBox::Viewport(document) => document.window().scroll_offset().cast(), + .scroll_offset_query(element.upcast::()), + ScrollingBox::Viewport(document) => document.window().scroll_offset(), } } } @@ -975,7 +974,7 @@ impl Element { scrolling_node: &Node, block: ScrollLogicalPosition, inline: ScrollLogicalPosition, - ) -> Vector2D { + ) -> LayoutVector2D { let target_bounding_box = self.upcast::().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::().owner_doc(); let window = owner_doc.window(); - let viewport_width = window.InnerWidth() as f64; - let viewport_height = window.InnerHeight() as f64; - let current_scroll_x = window.ScrollX() as f64; - let current_scroll_y = window.ScrollY() as f64; + let viewport_width = window.InnerWidth() as f32; + let viewport_height = window.InnerHeight() as f32; + let current_scroll_x = window.ScrollX() as f32; + 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_top = scrolling_box.origin.y.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); 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::().unwrap().ScrollLeft(); - let current_scroll_y = scrolling_node.downcast::().unwrap().ScrollTop(); + // TODO: This function should accept `Element` and not `Node`. + let scrolling_element = scrolling_node + .downcast::() + .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) - as f64; + .to_nearest_pixel(device_pixel_ratio); let positioning_top = positioning_box .origin .y - .to_nearest_pixel(device_pixel_ratio) - as f64; + .to_nearest_pixel(device_pixel_ratio); // 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_end: f64, - element_size: f64, - container_size: f64, - current_scroll_offset: f64, - ) -> f64 { + element_start: f32, + element_end: f32, + element_size: f32, + container_size: f32, + current_scroll_offset: f32, + ) -> 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 - // 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 y = if y_.is_finite() { y_ } else { 0.0f64 }; + let x = if x.is_finite() { x } else { 0.0 } as f32; + let y = if y.is_finite() { y } else { 0.0 } as f32; let node = self.upcast::(); @@ -3549,7 +3552,7 @@ impl ElementMethods 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::(); @@ -3570,7 +3573,7 @@ impl ElementMethods 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 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 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 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::(); @@ -3669,7 +3672,7 @@ impl ElementMethods 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 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 for Element { } // Step 11 - win.scroll_an_element(self, x, self.ScrollTop(), behavior); + win.scroll_an_element(self, x, self.ScrollTop() as f32, behavior); } /// diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index c0247a3999f..84d3f3eb15b 100644 --- a/components/script/dom/window.rs +++ b/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, - WindowPostMessageOptions, + self, FrameRequestCallback, ScrollBehavior, WindowMethods, WindowPostMessageOptions, }; use crate::dom::bindings::codegen::UnionTypes::{ RequestOrUSVString, TrustedScriptOrString, TrustedScriptOrStringOrFunction, @@ -1603,48 +1603,85 @@ impl WindowMethods for Window { self.ScrollY() } - // https://drafts.csswg.org/cssom-view/#dom-window-scroll + /// fn Scroll(&self, options: &ScrollToOptions) { - // Step 1 - let left = options.left.unwrap_or(0.0f64); - let top = options.top.unwrap_or(0.0f64); - self.scroll(left, top, options.parent.behavior); + // Step 1: If invoked with one argument, follow these substeps: + // Step 1.1: Let options be the argument. + // Step 1.2: Let x be the value of the left dictionary member of options, if + // 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 + /// 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 + /// + /// + /// > 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 + /// : + /// + /// > 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 + /// fn ScrollBy(&self, options: &ScrollToOptions) { - // Step 1 - let x = options.left.unwrap_or(0.0f64); - let y = options.top.unwrap_or(0.0f64); - self.ScrollBy_(x, y); - self.scroll(x, y, options.parent.behavior); + // When the scrollBy() method is invoked, the user agent must run these steps: + // Step 1: If invoked with two arguments, follow these substeps: + // This doesn't apply here. + + // 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 + /// fn ScrollBy_(&self, x: f64, y: f64) { - let scroll_offset = self.scroll_offset(); - // Step 3 - let left = x + scroll_offset.x as f64; - // Step 4 - let top = y + scroll_offset.y as f64; + // When the scrollBy() method is invoked, the user agent must run these steps: + // Step 1: If invoked with two arguments, follow these substeps: + // Step 1.1: Let options be null converted to a ScrollToOptions dictionary. + let mut options = ScrollToOptions::empty(); - // Step 5 - self.scroll(left, top, ScrollBehavior::Auto); + // Step 1.2: Let x and y be the arguments, respectively. + // 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 { } /// - pub(crate) fn scroll(&self, x_: f64, y_: f64, behavior: ScrollBehavior) { - // Step 3 - let xfinite = if x_.is_finite() { x_ } else { 0.0f64 }; - let yfinite = if y_.is_finite() { y_ } else { 0.0f64 }; + pub(crate) fn scroll(&self, x: f32, y: f32, behavior: ScrollBehavior) { + // Step 3: Normalize non-finite values for x and y. + let xfinite = if x.is_finite() { x } else { 0.0 }; + 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 - // TODO: Remove scrollbar dimensions. + // Step 5. Let `viewport width` be the width of the viewport excluding the width + // 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 - // TODO: Consider `block-end` and `inline-end` overflow direction. - let scrolling_area = self.scrolling_area_query(None); - let x = xfinite - .min(scrolling_area.width() as f64 - viewport.width as f64) - .max(0.0f64); - let y = yfinite - .min(scrolling_area.height() as f64 - viewport.height as f64) - .max(0.0f64); + // Step 7: + // If the viewport has rightward overflow direction + // Let x be max(0, min(x, viewport scrolling area width - viewport width)). + // If the viewport has leftward overflow direction + // Let x be min(0, max(x, viewport width - viewport scrolling area width)). + // TODO: Implement this. - // Step 10 - // TODO handling ongoing smooth scrolling + // Step 8: + // 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( - x.to_f32().unwrap_or(0.0f32), - y.to_f32().unwrap_or(0.0f32), - self.pipeline_id().root_scroll_id(), - behavior, - None, - ); + self.perform_a_scroll(x, y, self.pipeline_id().root_scroll_id(), behavior, None); } /// @@ -2483,8 +2530,8 @@ impl Window { pub(crate) fn scroll_an_element( &self, element: &Element, - x_: f64, - y_: f64, + x: f32, + 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( - x_.to_f32().unwrap_or(0.0f32), - y_.to_f32().unwrap_or(0.0f32), - scroll_id, - behavior, - Some(element), - ); + self.perform_a_scroll(x, y, scroll_id, behavior, Some(element)); } pub(crate) fn resolved_style_query( diff --git a/components/script_bindings/codegen/Bindings.conf b/components/script_bindings/codegen/Bindings.conf index 22b006265a7..04e5dd5c3b7 100644 --- a/components/script_bindings/codegen/Bindings.conf +++ b/components/script_bindings/codegen/Bindings.conf @@ -860,6 +860,14 @@ Dictionaries = { 'derives': ['Clone', 'MallocSizeOf'], }, +'ScrollOptions': { + 'derives': ['Clone'], +}, + +'ScrollToOptions': { + 'derives': ['Clone'], +}, + 'StereoPannerOptions': { 'derives': ['Clone', 'Copy'], },