Auto merge of #17385 - pyfisch:better-scroll, r=mrobinson

Fix several bugs related to scrolling

* scrollLeft/scrollTop returned values of parent or even document root
   Only the scroll of the node itself is returned. Otherwise 0.0.
* Scrolling via script had set viewport.
   This resulted in other nodes appearing scrolled.
   Now scroll_offsets are updated with correct node id.

These bugs caused other odd behavior like both body and
document.documentElement being scrolled or the view for scrolled
elements jumping.

Also try scrolling this [example page](https://pyfisch.org/stuff/scrolltest.html) in servo with and without this change.

<!-- 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 #17342 (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes (partially)

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17385)
<!-- Reviewable:end -->

components/script/dom/document.rs

@@ -703,6 +703,7 @@ impl Document {
             // Step 3
             let global_scope = self.window.upcast::<GlobalScope>();
             let webrender_pipeline_id = global_scope.pipeline_id().to_webrender();
+            self.window.update_viewport_for_scroll(x, y);
             self.window.perform_a_scroll(x,
                                          y,
                                          ClipId::root_scroll_node(webrender_pipeline_id),

components/script/dom/element.rs

@@ -1356,7 +1356,7 @@ impl Element {
         // Step 10 (TODO)
 
         // Step 11
-        win.scroll_node(node.to_trusted_node_address(), x, y, behavior);
+        win.scroll_node(node, x, y, behavior);
     }
 
     // https://w3c.github.io/DOM-Parsing/#parsing
@@ -1798,7 +1798,7 @@ impl ElementMethods for Element {
         // Step 10 (TODO)
 
         // Step 11
-        win.scroll_node(node.to_trusted_node_address(), self.ScrollLeft(), y, behavior);
+        win.scroll_node(node, self.ScrollLeft(), y, behavior);
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-element-scrolltop
@@ -1891,7 +1891,7 @@ impl ElementMethods for Element {
         // Step 10 (TODO)
 
         // Step 11
-        win.scroll_node(node.to_trusted_node_address(), x, self.ScrollTop(), behavior);
+        win.scroll_node(node, x, self.ScrollTop(), behavior);
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-element-scrollwidth

components/script/dom/window.rs

@@ -13,7 +13,6 @@ use dom::bindings::codegen::Bindings::EventHandlerBinding::EventHandlerNonNull;
 use dom::bindings::codegen::Bindings::EventHandlerBinding::OnBeforeUnloadEventHandlerNonNull;
 use dom::bindings::codegen::Bindings::EventHandlerBinding::OnErrorEventHandlerNonNull;
 use dom::bindings::codegen::Bindings::FunctionBinding::Function;
-use dom::bindings::codegen::Bindings::NodeBinding::NodeMethods;
 use dom::bindings::codegen::Bindings::PermissionStatusBinding::PermissionState;
 use dom::bindings::codegen::Bindings::RequestBinding::RequestInit;
 use dom::bindings::codegen::Bindings::WindowBinding::{self, FrameRequestCallback, WindowMethods};
@@ -1126,8 +1125,11 @@ impl Window {
         //let document = self.Document();
         // Step 12
         let global_scope = self.upcast::<GlobalScope>();
-        self.perform_a_scroll(x.to_f32().unwrap_or(0.0f32),
+        let x = x.to_f32().unwrap_or(0.0f32);
-                              y.to_f32().unwrap_or(0.0f32),
+        let y = y.to_f32().unwrap_or(0.0f32);
+        self.update_viewport_for_scroll(x, y);
+        self.perform_a_scroll(x,
+                              y,
                               global_scope.pipeline_id().root_scroll_node(),
                               behavior,
                               None);
@@ -1158,9 +1160,6 @@ impl Window {
             scroll_offset: Vector2D::new(-x, -y),
         })).unwrap();
 
-        // TODO (farodin91): Raise an event to stop the current_viewport
-        self.update_viewport_for_scroll(x, y);
-
         let global_scope = self.upcast::<GlobalScope>();
         let message = ConstellationMsg::ScrollFragmentPoint(scroll_root_id, point, smooth);
         global_scope.constellation_chan().send(message).unwrap();
@@ -1450,33 +1449,32 @@ impl Window {
     }
 
     pub fn scroll_offset_query(&self, node: &Node) -> Vector2D<f32> {
-        let mut node = Root::from_ref(node);
-        loop {
         if let Some(scroll_offset) = self.scroll_offsets
                                          .borrow()
                                          .get(&node.to_untrusted_node_address()) {
             return *scroll_offset
         }
-            node = match node.GetParentNode() {
+        Vector2D::new(0.0, 0.0)
-                Some(node) => node,
-                None => break,
-            }
-        }
-        let vp_origin = self.current_viewport.get().origin;
-        Vector2D::new(vp_origin.x.to_f32_px(), vp_origin.y.to_f32_px())
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-element-scroll
     pub fn scroll_node(&self,
-                       node: TrustedNodeAddress,
+                       node: &Node,
                        x_: f64,
                        y_: f64,
                        behavior: ScrollBehavior) {
         if !self.reflow(ReflowGoal::ForScriptQuery,
-                        ReflowQueryType::NodeScrollRootIdQuery(node),
+                        ReflowQueryType::NodeScrollRootIdQuery(node.to_trusted_node_address()),
                         ReflowReason::Query) {
             return;
         }
+
+        // The scroll offsets are immediatly updated since later calls
+        // to topScroll and others may access the properties before
+        // webrender has a chance to update the offsets.
+        self.scroll_offsets.borrow_mut().insert(node.to_untrusted_node_address(),
+                                                Vector2D::new(x_ as f32, y_ as f32));
+
         let NodeScrollRootIdResponse(scroll_root_id) = self.layout_rpc.node_scroll_root_id();
 
         // Step 12

tests/wpt/metadata/MANIFEST.json

@@ -553578,7 +553578,7 @@
    "testharness"
   ],
   "cssom-view/elementScroll.html": [
-   "5471dca08aae9d446c487d40853957e9290677f3",
+   "56d85d2973ad630dd28842df6479b1f571b7f340",
    "testharness"
   ],
   "cssom-view/elementsFromPoint.html": [

tests/wpt/metadata/html/browsers/browsing-the-web/scroll-to-fragid/003.html.ini

@@ -0,0 +1,4 @@
+[003.html]
+  type: testharness
+  [Fragment Navigation: Updating scroll position]
+    expected: FAIL

tests/wpt/web-platform-tests/cssom-view/elementScroll.html

@@ -33,12 +33,14 @@
 
     Curabitur elit lacus, bibendum non tempus a, bibendum sit amet ante. Mauris eget nibh quis leo rhoncus consequat. Integer iaculis sed sapien eu pellentesque. In aliquet elementum lorem, ut consequat elit ultrices id. Phasellus vestibulum ex ex, ac sagittis tortor convallis et. Curabitur placerat id lectus at aliquam. Morbi sed nisl sem. Nam sit amet arcu maximus, volutpat nisl ac, dignissim neque. Etiam nec efficitur libero. Quisque tristique pulvinar est, eget dictum ex vehicula non. Nam dignissim non felis a iaculis. Nullam vel dolor vitae libero aliquet congue. Donec mi eros, semper non lectus at, commodo ullamcorper ligula. Donec commodo, sem vel lacinia porttitor, elit orci maximus felis, eget eleifend est velit id lorem.
     </div>
+    <div id="unrelated"></div>
 </section>
 
 <script>
     setup({explicit_done:true});
     window.onload = function () {
         var section = document.getElementById("section");
+        var unrelated = document.getElementById("unrelated");
 
         test(function () {
             assert_equals(section.scrollTop, 0, "initial scrollTop should be 0");
@@ -49,6 +51,9 @@
 
             assert_equals(section.scrollTop, 30, "changed scrollTop should be 40");
             assert_equals(section.scrollLeft, 40, "changed scrollLeft should be 40");
+
+            assert_equals(unrelated.scrollTop, 0, "unrelated element should not scroll");
+            assert_equals(unrelated.scrollLeft, 0, "unrelated element should not scroll");
         },  "Element scrollTop/Left getter/setter test");
 
         test(function () {