Auto merge of #29680 - mrobinson:fix-root-scrolling-area, r=<try>

Fix scrolls run on the `window` object from script

The calculation of the viewport scrolling area was incorrect, because it was based on the body element instead of the viewport. This change makes the argument to the layout query optional and when it is not specified, the return value reflects the viewport's scrolling area.

In addition, scrolling the window object early on in the page load could sometimes lead to a situation where WebRender would scroll, but not request a frame with the scrolled content. Always request a frame when updating scroll positions from script.

This patch was written in collaboration with Rakhi Sharma <atbrakhi@igalia.com>.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21321.
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

components/compositing/compositor.rs

@@ -623,8 +623,12 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
                 scroll_id,
                 clamping,
             )) => {
+                self.waiting_on_pending_frame = true;
+
                 let mut txn = webrender_api::Transaction::new();
                 txn.scroll_node_with_id(point, scroll_id, clamping);
+                txn.generate_frame();
+
                 self.webrender_api
                     .send_transaction(self.webrender_document, txn);
             },

components/layout/query.rs

@@ -76,7 +76,7 @@ pub struct LayoutThreadData {
     pub scroll_id_response: Option<ExternalScrollId>,
 
     /// A queued response for the scroll {top, left, width, height} of a node in pixels.
-    pub scroll_area_response: Rect<i32>,
+    pub scrolling_area_response: Rect<i32>,
 
     /// A queued response for the resolved style property of an element.
     pub resolved_style_response: String,
@@ -158,9 +158,9 @@ impl LayoutRPC for LayoutRPCImpl {
         }
     }
 
-    fn node_scroll_area(&self) -> NodeGeometryResponse {
+    fn scrolling_area(&self) -> NodeGeometryResponse {
         NodeGeometryResponse {
-            client_rect: self.0.lock().unwrap().scroll_area_response,
+            client_rect: self.0.lock().unwrap().scrolling_area_response,
         }
     }
 
@@ -732,10 +732,21 @@ pub fn process_node_scroll_id_request<'dom>(
 }
 
 /// https://drafts.csswg.org/cssom-view/#scrolling-area
-pub fn process_node_scroll_area_request(
-    requested_node: OpaqueNode,
+pub fn process_scrolling_area_request(
+    requested_node: Option<OpaqueNode>,
     layout_root: &mut dyn Flow,
 ) -> Rect<i32> {
+    let requested_node = match requested_node {
+        Some(node) => node,
+        None => {
+            let rect = layout_root.base().overflow.scroll;
+            return Rect::new(
+                Point2D::new(rect.origin.x.to_nearest_px(), rect.origin.y.to_nearest_px()),
+                Size2D::new(rect.width().ceil_to_px(), rect.height().ceil_to_px()),
+            );
+        },
+    };
+
     let mut iterator = UnioningFragmentScrollAreaIterator::new(requested_node);
     sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root, &mut iterator);
     match iterator.overflow_direction {

components/layout_2020/flow/root.rs

@@ -38,7 +38,7 @@ use style::animation::AnimationSetKey;
 use style::dom::OpaqueNode;
 use style::properties::ComputedValues;
 use style::selector_parser::PseudoElement;
-use style::values::computed::Length;
+use style::values::computed::{CSSPixelLength, Length};
 use style_traits::CSSPixel;
 
 #[derive(Serialize)]
@@ -539,37 +539,30 @@ impl FragmentTree {
         .unwrap_or_else(Rect::zero)
     }
 
-    pub fn get_scroll_area_for_node(&self, requested_node: OpaqueNode) -> Rect<i32> {
-        let mut scroll_area: PhysicalRect<Length> = PhysicalRect::zero();
+    pub fn get_scrolling_area_for_viewport(&self) -> PhysicalRect<CSSPixelLength> {
+        let mut scroll_area = self.initial_containing_block;
+        for fragment in self.root_fragments.iter() {
+            scroll_area = fragment
+                .borrow()
+                .scrolling_area(&self.initial_containing_block)
+                .union(&scroll_area);
+        }
+        scroll_area
+    }
+
+    pub fn get_scrolling_area_for_node(
+        &self,
+        requested_node: OpaqueNode,
+    ) -> PhysicalRect<CSSPixelLength> {
         let tag_to_find = Tag::Node(requested_node);
-        self.find(|fragment, _, containing_block| {
-            if fragment.tag() != Some(tag_to_find) {
-                return None::<()>;
+        let scroll_area = self.find(|fragment, _, containing_block| {
+            if fragment.tag() == Some(tag_to_find) {
+                Some(fragment.scrolling_area(&containing_block))
+            } else {
+                None
             }
-
-            scroll_area = match fragment {
-                Fragment::Box(fragment) => fragment
-                    .scrollable_overflow(&containing_block)
-                    .translate(containing_block.origin.to_vector()),
-                Fragment::Text(_) |
-                Fragment::AbsoluteOrFixedPositioned(_) |
-                Fragment::Image(_) |
-                Fragment::IFrame(_) |
-                Fragment::Anonymous(_) => return None,
-            };
-            None::<()>
         });
-
-        Rect::new(
-            Point2D::new(
-                scroll_area.origin.x.px() as i32,
-                scroll_area.origin.y.px() as i32,
-            ),
-            Size2D::new(
-                scroll_area.size.width.px() as i32,
-                scroll_area.size.height.px() as i32,
-            ),
-        )
+        scroll_area.unwrap_or_else(PhysicalRect::<CSSPixelLength>::zero)
     }
 }
 

components/layout_2020/fragments.rs

@@ -242,6 +242,15 @@ impl Fragment {
         }
     }
 
+    pub fn scrolling_area(&self, containing_block: &PhysicalRect<Length>) -> PhysicalRect<Length> {
+        match self {
+            Fragment::Box(fragment) => fragment
+                .scrollable_overflow(&containing_block)
+                .translate(containing_block.origin.to_vector()),
+            _ => self.scrollable_overflow(containing_block),
+        }
+    }
+
     pub(crate) fn find<T>(
         &self,
         containing_block: &PhysicalRect<Length>,
@@ -406,14 +415,16 @@ impl BoxFragment {
                 \nborder rect={:?}\
                 \nscrollable_overflow={:?}\
                 \noverflow={:?} / {:?}\
-                \nstyle={:p}",
+                \nstyle={:p}
+                \nstyle={:?}",
             self.content_rect,
             self.padding_rect(),
             self.border_rect(),
             self.scrollable_overflow(&PhysicalRect::zero()),
             self.style.get_box().overflow_x,
             self.style.get_box().overflow_y,
             self.style,
+            self.tag,
         ));
 
         for child in &self.children {

components/layout_2020/query.rs

@@ -57,7 +57,7 @@ pub struct LayoutThreadData {
     pub scroll_id_response: Option<ExternalScrollId>,
 
     /// A queued response for the scroll {top, left, width, height} of a node in pixels.
-    pub scroll_area_response: Rect<i32>,
+    pub scrolling_area_response: Rect<i32>,
 
     /// A queued response for the resolved style property of an element.
     pub resolved_style_response: String,
@@ -116,9 +116,9 @@ impl LayoutRPC for LayoutRPCImpl {
         }
     }
 
-    fn node_scroll_area(&self) -> NodeGeometryResponse {
+    fn scrolling_area(&self) -> NodeGeometryResponse {
         NodeGeometryResponse {
-            client_rect: self.0.lock().unwrap().scroll_area_response,
+            client_rect: self.0.lock().unwrap().scrolling_area_response,
         }
     }
 
@@ -202,14 +202,19 @@ pub fn process_node_scroll_id_request<'dom>(
 
 /// https://drafts.csswg.org/cssom-view/#scrolling-area
 pub fn process_node_scroll_area_request(
-    requested_node: OpaqueNode,
+    requested_node: Option<OpaqueNode>,
     fragment_tree: Option<Arc<FragmentTree>>,
 ) -> Rect<i32> {
-    if let Some(fragment_tree) = fragment_tree {
-        fragment_tree.get_scroll_area_for_node(requested_node)
-    } else {
-        Rect::zero()
-    }
+    let rect = match (fragment_tree, requested_node) {
+        (Some(tree), Some(node)) => tree.get_scrolling_area_for_node(node),
+        (Some(tree), None) => tree.get_scrolling_area_for_viewport(),
+        _ => return Rect::zero(),
+    };
+
+    Rect::new(
+        Point2D::new(rect.origin.x.px() as i32, rect.origin.y.px() as i32),
+        Size2D::new(rect.size.width.px() as i32, rect.size.height.px() as i32),
+    )
 }
 
 /// Return the resolved value of property for a given (pseudo)element.

components/layout_thread/lib.rs

@@ -51,10 +51,9 @@ use layout::layout_debug;
 use layout::parallel;
 use layout::query::{
     process_client_rect_query, process_content_box_request, process_content_boxes_request,
-    process_element_inner_text_query, process_node_scroll_area_request,
-    process_node_scroll_id_request, process_offset_parent_query,
-    process_resolved_font_style_request, process_resolved_style_request, LayoutRPCImpl,
-    LayoutThreadData,
+    process_element_inner_text_query, process_node_scroll_id_request, process_offset_parent_query,
+    process_resolved_font_style_request, process_resolved_style_request,
+    process_scrolling_area_request, LayoutRPCImpl, LayoutThreadData,
 };
 use layout::sequential;
 use layout::traversal::{
@@ -553,7 +552,7 @@ impl LayoutThread {
                 content_boxes_response: Vec::new(),
                 client_rect_response: Rect::zero(),
                 scroll_id_response: None,
-                scroll_area_response: Rect::zero(),
+                scrolling_area_response: Rect::zero(),
                 resolved_style_response: String::new(),
                 resolved_font_style_response: None,
                 offset_parent_response: OffsetParentResponse::empty(),
@@ -1208,8 +1207,8 @@ impl LayoutThread {
                         &QueryMsg::ClientRectQuery(_) => {
                             rw_data.client_rect_response = Rect::zero();
                         },
-                        &QueryMsg::NodeScrollGeometryQuery(_) => {
-                            rw_data.scroll_area_response = Rect::zero();
+                        &QueryMsg::ScrollingAreaQuery(_) => {
+                            rw_data.scrolling_area_response = Rect::zero();
                         },
                         &QueryMsg::NodeScrollIdQuery(_) => {
                             rw_data.scroll_id_response = None;
@@ -1542,9 +1541,9 @@ impl LayoutThread {
                 &QueryMsg::ClientRectQuery(node) => {
                     rw_data.client_rect_response = process_client_rect_query(node, root_flow);
                 },
-                &QueryMsg::NodeScrollGeometryQuery(node) => {
-                    rw_data.scroll_area_response =
-                        process_node_scroll_area_request(node, root_flow);
+                &QueryMsg::ScrollingAreaQuery(node) => {
+                    rw_data.scrolling_area_response =
+                        process_scrolling_area_request(node, root_flow);
                 },
                 &QueryMsg::NodeScrollIdQuery(node) => {
                     let node = unsafe { ServoLayoutNode::new(&node) };

components/layout_thread_2020/lib.rs

@@ -526,7 +526,7 @@ impl LayoutThread {
                 content_boxes_response: Vec::new(),
                 client_rect_response: Rect::zero(),
                 scroll_id_response: None,
-                scroll_area_response: Rect::zero(),
+                scrolling_area_response: Rect::zero(),
                 resolved_style_response: String::new(),
                 resolved_font_style_response: None,
                 offset_parent_response: OffsetParentResponse::empty(),
@@ -889,8 +889,8 @@ impl LayoutThread {
                         &QueryMsg::ClientRectQuery(_) => {
                             rw_data.client_rect_response = Rect::zero();
                         },
-                        &QueryMsg::NodeScrollGeometryQuery(_) => {
-                            rw_data.scroll_area_response = Rect::zero();
+                        &QueryMsg::ScrollingAreaQuery(_) => {
+                            rw_data.scrolling_area_response = Rect::zero();
                         },
                         &QueryMsg::NodeScrollIdQuery(_) => {
                             rw_data.scroll_id_response = None;
@@ -1183,8 +1183,8 @@ impl LayoutThread {
                     rw_data.client_rect_response =
                         process_node_geometry_request(node, self.fragment_tree.borrow().clone());
                 },
-                &QueryMsg::NodeScrollGeometryQuery(node) => {
-                    rw_data.scroll_area_response =
+                &QueryMsg::ScrollingAreaQuery(node) => {
+                    rw_data.scrolling_area_response =
                         process_node_scroll_area_request(node, self.fragment_tree.borrow().clone());
                 },
                 &QueryMsg::NodeScrollIdQuery(node) => {

components/script/dom/node.rs

@@ -810,7 +810,7 @@ impl Node {
             .downcast::<HTMLBodyElement>()
             .map_or(false, |e| e.is_the_html_body_element());
 
-        let scroll_area = window.scroll_area_query(self);
+        let scrolling_area = window.scrolling_area_query(Some(self));
 
         match (
             document != window.Document(),
@@ -824,12 +824,12 @@ impl Node {
             (false, false, _, true) | (false, true, QuirksMode::Quirks, _) => Rect::new(
                 Point2D::new(window.ScrollX(), window.ScrollY()),
                 Size2D::new(
-                    cmp::max(window.InnerWidth(), scroll_area.size.width),
-                    cmp::max(window.InnerHeight(), scroll_area.size.height),
+                    cmp::max(window.InnerWidth(), scrolling_area.size.width),
+                    cmp::max(window.InnerHeight(), scrolling_area.size.height),
                 ),
             ),
             // Step 9
-            _ => scroll_area,
+            _ => scrolling_area,
         }
     }
 

components/script/dom/window.rs

@@ -1705,21 +1705,13 @@ impl Window {
 
         // Step 7 & 8
         // TODO: Consider `block-end` and `inline-end` overflow direction.
-        let body = self.Document().GetBody();
-        let (x, y) = match body {
-            Some(e) => {
-                let scroll_area = e.upcast::<Node>().scroll_area();
-                (
-                    xfinite
-                        .min(scroll_area.width() as f64 - viewport.width as f64)
-                        .max(0.0f64),
-                    yfinite
-                        .min(scroll_area.height() as f64 - viewport.height as f64)
-                        .max(0.0f64),
-                )
-            },
-            None => (xfinite.max(0.0f64), yfinite.max(0.0f64)),
-        };
+        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 10
         //TODO handling ongoing smooth scrolling
@@ -2092,11 +2084,14 @@ impl Window {
         self.layout_rpc.node_geometry().client_rect
     }
 
-    pub fn scroll_area_query(&self, node: &Node) -> UntypedRect<i32> {
-        if !self.layout_reflow(QueryMsg::NodeScrollGeometryQuery(node.to_opaque())) {
+    /// Find the scroll area of the given node, if it is not None. If the node
+    /// is None, find the scroll area of the viewport.
+    pub fn scrolling_area_query(&self, node: Option<&Node>) -> UntypedRect<i32> {
+        let opaque = node.map(|node| node.to_opaque());
+        if !self.layout_reflow(QueryMsg::ScrollingAreaQuery(opaque)) {
             return Rect::zero();
         }
-        self.layout_rpc.node_scroll_area().client_rect
+        self.layout_rpc.scrolling_area().client_rect
     }
 
     pub fn scroll_offset_query(&self, node: &Node) -> Vector2D<f32, LayoutPixel> {
@@ -2722,7 +2717,7 @@ fn debug_reflow_events(id: PipelineId, reflow_goal: &ReflowGoal, reason: &Reflow
             &QueryMsg::ContentBoxesQuery(_n) => "\tContentBoxesQuery",
             &QueryMsg::NodesFromPointQuery(..) => "\tNodesFromPointQuery",
             &QueryMsg::ClientRectQuery(_n) => "\tClientRectQuery",
-            &QueryMsg::NodeScrollGeometryQuery(_n) => "\tNodeScrollGeometryQuery",
+            &QueryMsg::ScrollingAreaQuery(_n) => "\tNodeScrollGeometryQuery",
             &QueryMsg::NodeScrollIdQuery(_n) => "\tNodeScrollIdQuery",
             &QueryMsg::ResolvedStyleQuery(_, _, _) => "\tResolvedStyleQuery",
             &QueryMsg::ResolvedFontStyleQuery(..) => "\nResolvedFontStyleQuery",

components/script_layout_interface/message.rs

@@ -99,7 +99,7 @@ pub enum QueryMsg {
     ContentBoxQuery(OpaqueNode),
     ContentBoxesQuery(OpaqueNode),
     ClientRectQuery(OpaqueNode),
-    NodeScrollGeometryQuery(OpaqueNode),
+    ScrollingAreaQuery(Option<OpaqueNode>),
     OffsetParentQuery(OpaqueNode),
     TextIndexQuery(OpaqueNode, Point2D<f32>),
     NodesFromPointQuery(Point2D<f32>, NodesFromPointQueryType),
@@ -141,7 +141,7 @@ impl ReflowGoal {
                 QueryMsg::ContentBoxQuery(_) |
                 QueryMsg::ContentBoxesQuery(_) |
                 QueryMsg::ClientRectQuery(_) |
-                QueryMsg::NodeScrollGeometryQuery(_) |
+                QueryMsg::ScrollingAreaQuery(_) |
                 QueryMsg::NodeScrollIdQuery(_) |
                 QueryMsg::ResolvedStyleQuery(..) |
                 QueryMsg::ResolvedFontStyleQuery(..) |
@@ -163,7 +163,7 @@ impl ReflowGoal {
                 QueryMsg::ContentBoxQuery(_) |
                 QueryMsg::ContentBoxesQuery(_) |
                 QueryMsg::ClientRectQuery(_) |
-                QueryMsg::NodeScrollGeometryQuery(_) |
+                QueryMsg::ScrollingAreaQuery(_) |
                 QueryMsg::NodeScrollIdQuery(_) |
                 QueryMsg::ResolvedStyleQuery(..) |
                 QueryMsg::ResolvedFontStyleQuery(..) |