Skip to content

Commit

Permalink
Fix several bugs related to scrolling
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
pyfisch committed Jun 17, 2017
1 parent effd6f2 commit 6ab0a71
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 22 deletions.
6 changes: 3 additions & 3 deletions components/script/dom/element.rs
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
30 changes: 11 additions & 19 deletions components/script/dom/window.rs
Expand Up @@ -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};
Expand Down Expand Up @@ -1158,9 +1157,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();
Expand Down Expand Up @@ -1450,33 +1446,29 @@ 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() {
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;
}

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
Expand Down
5 changes: 5 additions & 0 deletions tests/wpt/web-platform-tests/cssom-view/elementScroll.html
Expand Up @@ -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");
Expand All @@ -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 () {
Expand Down

0 comments on commit 6ab0a71

Please sign in to comment.