From c63311af02e8f115788e9e47198438ee35d81ec4 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Tue, 23 Sep 2025 12:10:00 +0200 Subject: [PATCH] script: Remove absolute positioning workaround from `scrollIntoView` implementation (#39441) This isn't needed as the border box query already takes into account the containing block chain. Instead, consistently calculate the new scroll position for a scroller relative to its current scroll offset. In addition, fix a small bug where the border of a scroll container was considered part of scrollport. Testing: A new WPT test is added. Signed-off-by: Martin Robinson Co-authored-by: Oriol Brufau --- components/script/dom/element.rs | 187 +++++++----------- components/script/dom/node.rs | 5 + tests/wpt/meta/MANIFEST.json | 7 + ...ment-in-overflow-hidden-container.html.ini | 2 + ...oView-scrolling-box-with-large-border.html | 22 +++ 5 files changed, 104 insertions(+), 119 deletions(-) create mode 100644 tests/wpt/meta/focus/focus-large-element-in-overflow-hidden-container.html.ini create mode 100644 tests/wpt/tests/css/cssom-view/scrollIntoView-scrolling-box-with-large-border.html diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index c41e3dcfc60..b5f31e89bf6 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -36,7 +36,6 @@ use selectors::sink::Push; use servo_arc::Arc; use style::applicable_declarations::ApplicableDeclarationBlock; use style::attr::{AttrValue, LengthOrPercentageOrAuto}; -use style::computed_values::position::T as Position; use style::context::QuirksMode; use style::invalidation::element::restyle_hints::RestyleHint; use style::properties::longhands::{ @@ -966,108 +965,55 @@ impl Element { } } - /// + /// fn determine_scroll_into_view_position( &self, scrolling_box: &ScrollingBox, block: ScrollLogicalPosition, inline: ScrollLogicalPosition, ) -> LayoutVector2D { - let target_bounding_box = self.upcast::().border_box().unwrap_or_default(); - let device_pixel_ratio = self .upcast::() .owner_doc() .window() .device_pixel_ratio() .get(); - - // Target element bounds let to_pixel = |value: Au| value.to_nearest_pixel(device_pixel_ratio); - let element_top_left = target_bounding_box.origin.map(to_pixel).to_untyped(); - let element_bottom_right = target_bounding_box.max().map(to_pixel); - let element_size = target_bounding_box.size.to_vector().map(to_pixel); - let scrolling_box_size = scrolling_box.size(); - let current_scroll_position = scrolling_box.scroll_position().to_untyped(); + // > Step 1: Let target bounding border box be the box represented by the return + // > value of invoking Element’s getBoundingClientRect(), if target is an Element, + // > or Range’s getBoundingClientRect(), if target is a Range. + let target_border_box = self.upcast::().border_box().unwrap_or_default(); + let target_top_left = target_border_box.origin.map(to_pixel).to_untyped(); + let target_bottom_right = target_border_box.max().map(to_pixel); + + // The rest of the steps diverge from the specification here, but essentially try + // to follow it using our own geometry types. + // + // TODO: This makes the code below wrong for the purposes of writing modes. let (adjusted_element_top_left, adjusted_element_bottom_right) = match scrolling_box { - ScrollingBox::Viewport(_) => { - // For viewport scrolling, we need to add current scroll to get document-relative positions - ( - element_top_left + current_scroll_position, - element_bottom_right + current_scroll_position, - ) - }, + ScrollingBox::Viewport(_) => (target_top_left, target_bottom_right), ScrollingBox::Element(scrolling_element) => { - let scrolling_node = scrolling_element.upcast::(); - let scrolling_box = scrolling_node.border_box().unwrap_or_default(); - let scrolling_top_left = scrolling_box.origin.map(to_pixel); - - // Calculate element position in scroller's content coordinate system - // Element's viewport position relative to scroller, then add scroll offset to get content position - let viewport_relative_top_left = element_top_left - scrolling_top_left.to_vector(); - let viewport_relative_bottom_right = - element_bottom_right - scrolling_top_left.to_vector(); - - // For absolutely positioned elements, we need to account for the positioning context - // If the element is positioned relative to an ancestor that's within the scrolling container, - // we need to adjust coordinates accordingly - let (adjusted_relative_top_left, adjusted_relative_bottom_right) = { - // Check if this element has a positioned ancestor between it and the scrolling container - let mut current_node = self.upcast::().GetParentNode(); - let mut final_coords = - (viewport_relative_top_left, viewport_relative_bottom_right); - - while let Some(node) = current_node { - // Stop if we reach the scrolling container - if &*node == scrolling_node { - break; - } - - // Check if this node establishes a positioning context and has position relative/absolute - if let Some(element) = node.downcast::() { - if let Some(computed_style) = element.style() { - let position = computed_style.get_box().position; - - if matches!(position, Position::Relative | Position::Absolute) { - // If this element establishes a positioning context, - // Get its bounding box to calculate the offset - let positioning_box = node.border_box().unwrap_or_default(); - let positioning_top_left = positioning_box.origin.map(to_pixel); - - // Calculate the offset of the positioning context relative to the scrolling container - let offset_top_left = positioning_top_left - scrolling_top_left; - - // Adjust the coordinates by subtracting the positioning context offset - final_coords = ( - viewport_relative_top_left - offset_top_left, - viewport_relative_bottom_right - offset_top_left, - ); - break; - } - } - } - - current_node = node.GetParentNode(); - } - - final_coords - }; - - let content_element_top_left = adjusted_relative_top_left + current_scroll_position; - let content_element_bottom_right = - adjusted_relative_bottom_right + current_scroll_position; - - (content_element_top_left, content_element_bottom_right) + let scrolling_padding_rect_top_left = scrolling_element + .upcast::() + .padding_box() + .unwrap_or_default() + .origin + .map(to_pixel); + ( + target_top_left - scrolling_padding_rect_top_left.to_vector(), + target_bottom_right - scrolling_padding_rect_top_left.to_vector(), + ) }, }; + let scrolling_box_size = scrolling_box.size(); + let current_scroll_position = scrolling_box.scroll_position(); Vector2D::new( self.calculate_scroll_position_one_axis( inline, adjusted_element_top_left.x, adjusted_element_bottom_right.x, - element_size.x, scrolling_box_size.width, current_scroll_position.x, ), @@ -1075,61 +1021,64 @@ impl Element { block, adjusted_element_top_left.y, adjusted_element_bottom_right.y, - element_size.y, scrolling_box_size.height, current_scroll_position.y, ), ) } + /// Step 10 from : fn calculate_scroll_position_one_axis( &self, alignment: ScrollLogicalPosition, 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, - // Step 2 & 6: If inline is "end", then align element end edge with - // scrolling box end edge. - ScrollLogicalPosition::End => element_end - container_size, - // Step 3 & 7: If inline is "center", then align the center of target bounding - // border box with the center of scrolling box in scrolling box’s inline base direction. - ScrollLogicalPosition::Center => element_start + (element_size - container_size) / 2.0, - // Step 4 & 8: If inline is "nearest", - ScrollLogicalPosition::Nearest => { - let viewport_start = current_scroll_offset; - let viewport_end = current_scroll_offset + container_size; + let element_size = element_end - element_start; + current_scroll_offset + + match alignment { + // Step 1 & 5: If inline is "start", then align element start edge with scrolling box start edge. + ScrollLogicalPosition::Start => element_start, + // Step 2 & 6: If inline is "end", then align element end edge with + // scrolling box end edge. + ScrollLogicalPosition::End => element_end - container_size, + // Step 3 & 7: If inline is "center", then align the center of target bounding + // border box with the center of scrolling box in scrolling box’s inline base direction. + ScrollLogicalPosition::Center => { + element_start + (element_size - container_size) / 2.0 + }, + // Step 4 & 8: If inline is "nearest", + ScrollLogicalPosition::Nearest => { + let viewport_start = current_scroll_offset; + let viewport_end = current_scroll_offset + container_size; - // Step 4.2 & 8.2: If element start edge is outside scrolling box start edge and element - // size is less than scrolling box size or If element end edge is outside - // scrolling box end edge and element size is greater than scrolling box size: - // Align element start edge with scrolling box start edge. - if (element_start < viewport_start && element_size <= container_size) || - (element_end > viewport_end && element_size >= container_size) - { - element_start - } - // Step 4.3 & 8.3: If element end edge is outside scrolling box start edge and element - // size is greater than scrolling box size or If element start edge is outside - // scrolling box end edge and element size is less than scrolling box size: - // Align element end edge with scrolling box end edge. - else if (element_end > viewport_end && element_size < container_size) || - (element_start < viewport_start && element_size > container_size) - { - element_end - container_size - } - // Step 4.1 & 8.1: If element start edge and element end edge are both outside scrolling - // box start edge and scrolling box end edge or an invalid situation: Do nothing. - else { - current_scroll_offset - } - }, - } + // Step 4.2 & 8.2: If element start edge is outside scrolling box start edge and element + // size is less than scrolling box size or If element end edge is outside + // scrolling box end edge and element size is greater than scrolling box size: + // Align element start edge with scrolling box start edge. + if (element_start < viewport_start && element_size <= container_size) || + (element_end > viewport_end && element_size >= container_size) + { + element_start + } + // Step 4.3 & 8.3: If element end edge is outside scrolling box start edge and element + // size is greater than scrolling box size or If element start edge is outside + // scrolling box end edge and element size is less than scrolling box size: + // Align element end edge with scrolling box end edge. + else if (element_end > viewport_end && element_size < container_size) || + (element_start < viewport_start && element_size > container_size) + { + element_end - container_size + } + // Step 4.1 & 8.1: If element start edge and element end edge are both outside scrolling + // box start edge and scrolling box end edge or an invalid situation: Do nothing. + else { + current_scroll_offset + } + }, + } } } diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 85fc7a8ec19..a0f3e7e5dad 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -956,6 +956,11 @@ impl Node { .box_area_query(self, BoxAreaType::Border) } + pub(crate) fn padding_box(&self) -> Option> { + self.owner_window() + .box_area_query(self, BoxAreaType::Padding) + } + pub(crate) fn border_boxes(&self) -> Vec> { self.owner_window() .box_areas_query(self, BoxAreaType::Border) diff --git a/tests/wpt/meta/MANIFEST.json b/tests/wpt/meta/MANIFEST.json index 13de4a77261..4f24ac911ef 100644 --- a/tests/wpt/meta/MANIFEST.json +++ b/tests/wpt/meta/MANIFEST.json @@ -632462,6 +632462,13 @@ {} ] ], + "scrollIntoView-scrolling-box-with-large-border.html": [ + "424c6f56bd0b8fb01e268184533a6b081253be71", + [ + null, + {} + ] + ], "scrollIntoView-scrolling-container.html": [ "9f7c88aaeb39610d5adffd046f72bb3a10491fb4", [ diff --git a/tests/wpt/meta/focus/focus-large-element-in-overflow-hidden-container.html.ini b/tests/wpt/meta/focus/focus-large-element-in-overflow-hidden-container.html.ini new file mode 100644 index 00000000000..703dfe13847 --- /dev/null +++ b/tests/wpt/meta/focus/focus-large-element-in-overflow-hidden-container.html.ini @@ -0,0 +1,2 @@ +[focus-large-element-in-overflow-hidden-container.html] + expected: FAIL diff --git a/tests/wpt/tests/css/cssom-view/scrollIntoView-scrolling-box-with-large-border.html b/tests/wpt/tests/css/cssom-view/scrollIntoView-scrolling-box-with-large-border.html new file mode 100644 index 00000000000..424c6f56bd0 --- /dev/null +++ b/tests/wpt/tests/css/cssom-view/scrollIntoView-scrolling-box-with-large-border.html @@ -0,0 +1,22 @@ + + +CSSOM View - scrollIntoView should account for the border of scrollable elements. + + + + + + + +
+
+
+
+
+ +