script: Use HTMLElement.scrollParent to implement Element.scrollIntoView (#39144)

To find scrolling ancestors, we need to walk up the flat tree and only
consider the elements that are in the chain of containing block
ancestors of an element. `scrollParent` now does this so we can use it
to properly implement `scrollIntoView`.

Testing: There are WPT tests for this change.

---------

Signed-off-by: Martin Robinson <mrobinson@igalia.com>
Co-authored-by: Oriol Brufau <obrufau@igalia.com>

components/layout/layout_impl.rs

@@ -29,8 +29,8 @@ use layout_api::wrapper_traits::LayoutNode;
 use layout_api::{
     BoxAreaType, IFrameSizes, Layout, LayoutConfig, LayoutDamage, LayoutFactory,
     OffsetParentResponse, PropertyRegistration, QueryMsg, ReflowGoal, ReflowPhasesRun,
-    ReflowRequest, ReflowRequestRestyle, ReflowResult, RegisterPropertyError, ScrollParentResponse,
-    TrustedNodeAddress,
+    ReflowRequest, ReflowRequestRestyle, ReflowResult, RegisterPropertyError,
+    ScrollContainerQueryType, ScrollContainerResponse, TrustedNodeAddress,
 };
 use log::{debug, error, warn};
 use malloc_size_of::{MallocConditionalSizeOf, MallocSizeOf, MallocSizeOfOps};
@@ -92,8 +92,8 @@ use crate::display_list::{DisplayListBuilder, HitTest, StackingContextTree};
 use crate::query::{
     get_the_text_steps, process_box_area_request, process_box_areas_request,
     process_client_rect_request, process_node_scroll_area_request, process_offset_parent_query,
-    process_resolved_font_style_query, process_resolved_style_request, process_scroll_parent_query,
-    process_text_index_request,
+    process_resolved_font_style_query, process_resolved_style_request,
+    process_scroll_container_query, process_text_index_request,
 };
 use crate::traversal::{RecalcStyle, compute_damage_and_repair_style};
 use crate::{BoxTree, FragmentTree};
@@ -326,9 +326,13 @@ impl Layout for LayoutThread {
     }
 
     #[servo_tracing::instrument(skip_all)]
-    fn query_scroll_parent(&self, node: TrustedNodeAddress) -> Option<ScrollParentResponse> {
+    fn query_scroll_container(
+        &self,
+        node: TrustedNodeAddress,
+        query_type: ScrollContainerQueryType,
+    ) -> Option<ScrollContainerResponse> {
         let node = unsafe { ServoLayoutNode::new(&node) };
-        process_scroll_parent_query(node)
+        process_scroll_container_query(node, query_type)
     }
 
     #[servo_tracing::instrument(skip_all)]

components/layout/query.rs

@@ -12,7 +12,8 @@ use euclid::{SideOffsets2D, Size2D};
 use itertools::Itertools;
 use layout_api::wrapper_traits::{LayoutNode, ThreadSafeLayoutElement, ThreadSafeLayoutNode};
 use layout_api::{
-    BoxAreaType, LayoutElementType, LayoutNodeType, OffsetParentResponse, ScrollParentResponse,
+    BoxAreaType, LayoutElementType, LayoutNodeType, OffsetParentResponse, ScrollContainerQueryType,
+    ScrollContainerResponse,
 };
 use script::layout_dom::{ServoLayoutNode, ServoThreadSafeLayoutNode};
 use servo_arc::Arc as ServoArc;
@@ -647,12 +648,14 @@ pub fn process_offset_parent_query(node: ServoLayoutNode<'_>) -> Option<OffsetPa
     })
 }
 
-/// This is an implementation of
+/// An implementation of `scrollParent` that can also be used to for `scrollIntoView`:
 /// <https://drafts.csswg.org/cssom-view/#dom-htmlelement-scrollparent>.
+///
 #[inline]
-pub(crate) fn process_scroll_parent_query(
+pub(crate) fn process_scroll_container_query(
     node: ServoLayoutNode<'_>,
-) -> Option<ScrollParentResponse> {
+    query_type: ScrollContainerQueryType,
+) -> Option<ScrollContainerResponse> {
     let layout_data = node.to_threadsafe().inner_layout_data()?;
 
     // 1. If any of the following holds true, return null and terminate this algorithm:
@@ -665,14 +668,22 @@ pub(crate) fn process_scroll_parent_query(
 
     // - The element is the root element.
     // - The element is the body element.
-    // - The element’s computed value of the position property is fixed and no ancestor
-    //   establishes a fixed position containing block.
-    if flags.intersects(
+    //
+    // Note: We only do this for `scrollParent`, which needs to be null. But `scrollIntoView` on the
+    // `<body>` or root element should still bring it into view by scrolling the viewport.
+    if query_type == ScrollContainerQueryType::ForScrollParent &&
+        flags.intersects(
             FragmentFlags::IS_ROOT_ELEMENT | FragmentFlags::IS_BODY_ELEMENT_OF_HTML_ELEMENT_ROOT,
-    ) {
+        )
+    {
         return None;
     }
 
+    // - The element’s computed value of the position property is fixed and no ancestor
+    //   establishes a fixed position containing block.
+    //
+    // This is handled below in step 2.
+
     // 2. Let ancestor be the containing block of the element in the flat tree and repeat these substeps:
     // - If ancestor is the initial containing block, return the scrollingElement for the
     //   element’s document if it is not closed-shadow-hidden from the element, otherwise
@@ -721,7 +732,7 @@ pub(crate) fn process_scroll_parent_query(
         }
 
         if ancestor_style.establishes_scroll_container(ancestor_flags) {
-            return Some(ScrollParentResponse::Element(
+            return Some(ScrollContainerResponse::Element(
                 ancestor.as_node().opaque().into(),
             ));
         }
@@ -731,7 +742,7 @@ pub(crate) fn process_scroll_parent_query(
 
     match current_position_value {
         Position::Fixed => None,
-        _ => Some(ScrollParentResponse::DocumentScrollingElement),
+        _ => Some(ScrollContainerResponse::Viewport),
     }
 }
 

components/script/dom/element.rs

@@ -24,7 +24,7 @@ use html5ever::{LocalName, Namespace, Prefix, QualName, local_name, namespace_pr
 use js::jsapi::Heap;
 use js::jsval::JSVal;
 use js::rust::HandleObject;
-use layout_api::LayoutDamage;
+use layout_api::{LayoutDamage, ScrollContainerQueryType, ScrollContainerResponse};
 use net_traits::ReferrerPolicy;
 use net_traits::request::CorsSettings;
 use selectors::Element as SelectorsElement;
@@ -161,7 +161,7 @@ use crate::dom::mutationobserver::{Mutation, MutationObserver};
 use crate::dom::namednodemap::NamedNodeMap;
 use crate::dom::node::{
     BindContext, ChildrenMutation, CloneChildrenFlag, LayoutNodeHelpers, Node, NodeDamage,
-    NodeFlags, NodeTraits, ShadowIncluding, UnbindContext,
+    NodeFlags, NodeTraits, ShadowIncluding, UnbindContext, from_untrusted_node_address,
 };
 use crate::dom::nodelist::NodeList;
 use crate::dom::promise::Promise;
@@ -290,6 +290,20 @@ impl ScrollingBox {
             ScrollingBox::Viewport(document) => document.window().scroll_offset(),
         }
     }
+
+    fn parent(&self) -> Option<ScrollingBox> {
+        match self {
+            ScrollingBox::Element(element) => element.scrolling_box(),
+            ScrollingBox::Viewport(_) => None,
+        }
+    }
+
+    fn node(&self) -> &Node {
+        match self {
+            ScrollingBox::Element(element) => element.upcast(),
+            ScrollingBox::Viewport(document) => document.upcast(),
+        }
+    }
 }
 
 //
@@ -856,6 +870,21 @@ impl Element {
             .retain(|reg_obs| *reg_obs.observer != *observer)
     }
 
+    #[allow(unsafe_code)]
+    fn scrolling_box(&self) -> Option<ScrollingBox> {
+        self.owner_window()
+            .scroll_container_query(self.upcast(), ScrollContainerQueryType::ForScrollIntoView)
+            .and_then(|response| match response {
+                ScrollContainerResponse::Viewport => {
+                    Some(ScrollingBox::Viewport(self.owner_document()))
+                },
+                ScrollContainerResponse::Element(parent_node_address) => {
+                    let node = unsafe { from_untrusted_node_address(parent_node_address) };
+                    Some(ScrollingBox::Element(DomRoot::downcast(node)?))
+                },
+            })
+    }
+
     /// <https://drafts.csswg.org/cssom-view/#scroll-a-target-into-view>
     fn scroll_into_view_with_options(
         &self,
@@ -864,30 +893,11 @@ impl Element {
         inline: ScrollLogicalPosition,
         container: Option<&Element>,
     ) {
-        // Shadow-inclusive ancestors of this node, skipping the node itself.
-        //
-        // TODO: This should be the ancestors of the node in the flat tree.
-        let ancestors = self
-            .upcast::<Node>()
-            .inclusive_ancestors(ShadowIncluding::Yes)
-            .skip(1);
-
         // Step 1: For each ancestor element or viewport that establishes a scrolling box `scrolling
         // box`, in order of innermost to outermost scrolling box, run these substeps:
-        for node in ancestors {
-            if !node.establishes_scrolling_box() {
-                continue;
-            }
-
-            // TODO: This should skip elements which are not containing blocks of the element we
-            // are scrolling.
-            let scrolling_box = if let Some(document) = node.downcast::<Document>() {
-                ScrollingBox::Viewport(DomRoot::from_ref(document))
-            } else if let Some(element) = node.downcast::<Element>() {
-                ScrollingBox::Element(DomRoot::from_ref(element))
-            } else {
-                continue;
-            };
+        let mut parent_scrolling_box = self.scrolling_box();
+        while let Some(scrolling_box) = parent_scrolling_box {
+            parent_scrolling_box = scrolling_box.parent();
 
             // Step 1.1: If the Document associated with `target` is not same origin with the
             // Document associated with the element or viewport associated with `scrolling box`,
@@ -906,14 +916,15 @@ impl Element {
             //
             // TODO: Handle smooth scrolling.
             if position != scrolling_box.scroll_position() {
-                match scrolling_box {
+                match &scrolling_box {
                     //  ↪ If `scrolling box` is associated with an element
                     ScrollingBox::Element(element) => {
                         // Perform a scroll of the element’s scrolling box to `position`,
                         // with the `element` as the associated element and `behavior` as the
                         // scroll behavior.
-                        let window = element.owner_window();
-                        window.scroll_an_element(&element, position.x, position.y, behavior);
+                        element
+                            .owner_window()
+                            .scroll_an_element(element, position.x, position.y, behavior);
                     },
                     //  ↪ If `scrolling box` is associated with a viewport
                     ScrollingBox::Viewport(document) => {
@@ -932,7 +943,9 @@ impl Element {
             // inclusive ancestor of `container`, abort the rest of these steps.
             if container.is_some_and(|container| {
                 let container_node = container.upcast::<Node>();
-                node.is_shadow_including_inclusive_ancestor_of(container_node)
+                scrolling_box
+                    .node()
+                    .is_shadow_including_inclusive_ancestor_of(container_node)
             }) {
                 break;
             }

components/script/dom/html/htmlelement.rs

@@ -9,7 +9,8 @@ use std::rc::Rc;
 use dom_struct::dom_struct;
 use html5ever::{LocalName, Prefix, local_name, ns};
 use js::rust::HandleObject;
-use layout_api::QueryMsg;
+use layout_api::{QueryMsg, ScrollContainerQueryType, ScrollContainerResponse};
+use script_bindings::codegen::GenericBindings::DocumentBinding::DocumentMethods;
 use style::attr::AttrValue;
 use stylo_dom::ElementState;
 
@@ -48,7 +49,9 @@ use crate::dom::html::htmlhtmlelement::HTMLHtmlElement;
 use crate::dom::html::htmlinputelement::{HTMLInputElement, InputType};
 use crate::dom::html::htmllabelelement::HTMLLabelElement;
 use crate::dom::html::htmltextareaelement::HTMLTextAreaElement;
-use crate::dom::node::{BindContext, Node, NodeTraits, ShadowIncluding, UnbindContext};
+use crate::dom::node::{
+    BindContext, Node, NodeTraits, ShadowIncluding, UnbindContext, from_untrusted_node_address,
+};
 use crate::dom::shadowroot::ShadowRoot;
 use crate::dom::text::Text;
 use crate::dom::virtualmethods::VirtualMethods;
@@ -445,8 +448,17 @@ impl HTMLElementMethods<crate::DomTypeHolder> for HTMLElement {
     }
 
     /// <https://drafts.csswg.org/cssom-view/#dom-htmlelement-scrollparent>
+    #[allow(unsafe_code)]
     fn GetScrollParent(&self) -> Option<DomRoot<Element>> {
-        self.owner_window().scroll_parent_query(self.upcast())
+        self.owner_window()
+            .scroll_container_query(self.upcast(), ScrollContainerQueryType::ForScrollParent)
+            .and_then(|response| match response {
+                ScrollContainerResponse::Viewport => self.owner_document().GetScrollingElement(),
+                ScrollContainerResponse::Element(parent_node_address) => {
+                    let node = unsafe { from_untrusted_node_address(parent_node_address) };
+                    DomRoot::downcast(node)
+                },
+            })
     }
 
     /// <https://drafts.csswg.org/cssom-view/#dom-htmlelement-offsetparent>

components/script/dom/window.rs

@@ -56,8 +56,8 @@ use js::rust::{
 use layout_api::{
     BoxAreaType, ElementsFromPointFlags, ElementsFromPointResult, FragmentType, Layout,
     PendingImage, PendingImageState, PendingRasterizationImage, QueryMsg, ReflowGoal,
-    ReflowPhasesRun, ReflowRequest, ReflowRequestRestyle, RestyleReason, ScrollParentResponse,
-    TrustedNodeAddress, combine_id_with_fragment_type,
+    ReflowPhasesRun, ReflowRequest, ReflowRequestRestyle, RestyleReason, ScrollContainerQueryType,
+    ScrollContainerResponse, TrustedNodeAddress, combine_id_with_fragment_type,
 };
 use malloc_size_of::MallocSizeOf;
 use media::WindowGLContext;
@@ -2599,21 +2599,15 @@ impl Window {
         (element, response.rect)
     }
 
-    #[allow(unsafe_code)]
-    pub(crate) fn scroll_parent_query(&self, node: &Node) -> Option<DomRoot<Element>> {
+    pub(crate) fn scroll_container_query(
+        &self,
+        node: &Node,
+        query_type: ScrollContainerQueryType,
+    ) -> Option<ScrollContainerResponse> {
         self.layout_reflow(QueryMsg::ScrollParentQuery);
         self.layout
             .borrow()
-            .query_scroll_parent(node.to_trusted_node_address())
-            .and_then(|response| match response {
-                ScrollParentResponse::DocumentScrollingElement => {
-                    self.Document().GetScrollingElement()
-                },
-                ScrollParentResponse::Element(parent_node_address) => {
-                    let node = unsafe { from_untrusted_node_address(parent_node_address) };
-                    DomRoot::downcast(node)
-                },
-            })
+            .query_scroll_container(node.to_trusted_node_address(), query_type)
     }
 
     pub(crate) fn text_index_query(

components/shared/layout/lib.rs

@@ -295,7 +295,11 @@ pub trait Layout {
     fn query_client_rect(&self, node: TrustedNodeAddress) -> Rect<i32>;
     fn query_element_inner_outer_text(&self, node: TrustedNodeAddress) -> String;
     fn query_offset_parent(&self, node: TrustedNodeAddress) -> OffsetParentResponse;
-    fn query_scroll_parent(&self, node: TrustedNodeAddress) -> Option<ScrollParentResponse>;
+    fn query_scroll_container(
+        &self,
+        node: TrustedNodeAddress,
+        query_type: ScrollContainerQueryType,
+    ) -> Option<ScrollContainerResponse>;
     fn query_resolved_style(
         &self,
         node: TrustedNodeAddress,
@@ -352,9 +356,15 @@ pub struct OffsetParentResponse {
     pub rect: Rect<Au>,
 }
 
+#[derive(PartialEq)]
+pub enum ScrollContainerQueryType {
+    ForScrollParent,
+    ForScrollIntoView,
+}
+
 #[derive(Clone)]
-pub enum ScrollParentResponse {
-    DocumentScrollingElement,
+pub enum ScrollContainerResponse {
+    Viewport,
     Element(UntrustedNodeAddress),
 }
 

tests/wpt/meta/MANIFEST.json

@@ -334450,6 +334450,19 @@
        {}
       ]
      ],
+     "scrollIntoView-should-treat-slot-as-scroll-container.html": [
+      "a5211cc1095b82101a22300bcf4d1d233ccdf41a",
+      [
+       null,
+       [
+        [
+         "/css/reference/ref-filled-green-100px-square.xht",
+         "=="
+        ]
+       ],
+       {}
+      ]
+     ],
      "scrollTo-zoom.html": [
       "49ec49325034f9d2e10cf9b5f52add24bbcc57f6",
       [

tests/wpt/meta/css/cssom-view/scrollIntoView-fixed.html.ini

@@ -1,12 +1,6 @@
 [scrollIntoView-fixed.html]
-  [[Box A\] scrollIntoView from unscrollable position:fixed]
-    expected: FAIL
-
   [[Box B\] scrollIntoView from unscrollable position:fixed in iframe]
     expected: FAIL
 
-  [[Box C\] scrollIntoView from scrollable position:fixed]
-    expected: FAIL
-
   [[Box D\] scrollIntoView from scrollable position:fixed in iframe]
     expected: FAIL

tests/wpt/tests/css/cssom-view/scrollIntoView-should-treat-slot-as-scroll-container.html

@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+
+<link rel="author" title="Oriol Brufau" href="mailto:obrufau@igalia.com">
+<link rel="help" href="https://drafts.csswg.org/css-overflow-3/#overflow-control">
+<meta name="assert" content="scrollIntoView called on the descendant of shadow host with a slot should treat the slot as a pontential scroll container.">
+<link rel="match" href="../reference/ref-filled-green-100px-square.xht">
+
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div id="host">
+  <div style="height: 200px"></div>
+  <div id="target" style="height: 100px; width: 100px; background: green"></div>
+</div>
+<script>
+    var shadow = host.attachShadow({mode: "open"});
+    var slot = document.createElement("slot");
+    slot.style="display: block; overflow: hidden; width: 100px; height: 100px; background: red";
+    shadow.appendChild(slot);
+    target.scrollIntoView();
+</script>