From 284cb8aae8ed0c01c12aa866e8e7b15b07fcb816 Mon Sep 17 00:00:00 2001 From: Pyfisch Date: Sat, 17 Jun 2017 15:33:34 +0200 Subject: [PATCH] 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. --- components/script/dom/document.rs | 1 + components/script/dom/element.rs | 6 +-- components/script/dom/window.rs | 40 +++++++++---------- tests/wpt/metadata/MANIFEST.json | 2 +- .../scroll-to-fragid/003.html.ini | 4 ++ .../cssom-view/elementScroll.html | 5 +++ 6 files changed, 33 insertions(+), 25 deletions(-) create mode 100644 tests/wpt/metadata/html/browsers/browsing-the-web/scroll-to-fragid/003.html.ini diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 5db39cf4feb..5e67d286363 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -703,6 +703,7 @@ impl Document { // Step 3 let global_scope = self.window.upcast::(); 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), diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 7e01bc66764..05fca3c97d2 100644 --- a/components/script/dom/element.rs +++ b/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 @@ -1794,7 +1794,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 @@ -1887,7 +1887,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 diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 1f6a7c26eea..427d9503d1a 100644 --- a/components/script/dom/window.rs +++ b/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::(); - self.perform_a_scroll(x.to_f32().unwrap_or(0.0f32), - y.to_f32().unwrap_or(0.0f32), + let x = x.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::(); 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 { - 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() { - Some(node) => node, - None => break, - } + if let Some(scroll_offset) = self.scroll_offsets + .borrow() + .get(&node.to_untrusted_node_address()) { + return *scroll_offset } - let vp_origin = self.current_viewport.get().origin; - Vector2D::new(vp_origin.x.to_f32_px(), vp_origin.y.to_f32_px()) + Vector2D::new(0.0, 0.0) } // 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 diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json index 02bcc380548..fcb42f6d710 100644 --- a/tests/wpt/metadata/MANIFEST.json +++ b/tests/wpt/metadata/MANIFEST.json @@ -553488,7 +553488,7 @@ "testharness" ], "cssom-view/elementScroll.html": [ - "5471dca08aae9d446c487d40853957e9290677f3", + "56d85d2973ad630dd28842df6479b1f571b7f340", "testharness" ], "cssom-view/elementsFromPoint.html": [ diff --git a/tests/wpt/metadata/html/browsers/browsing-the-web/scroll-to-fragid/003.html.ini b/tests/wpt/metadata/html/browsers/browsing-the-web/scroll-to-fragid/003.html.ini new file mode 100644 index 00000000000..ae630f04075 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/tests/wpt/web-platform-tests/cssom-view/elementScroll.html b/tests/wpt/web-platform-tests/cssom-view/elementScroll.html index 6227d665457..85bf4c5c95e 100644 --- a/tests/wpt/web-platform-tests/cssom-view/elementScroll.html +++ b/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. +