Skip to content

Commit

Permalink
Use visual scroll offset for snapping.
Browse files Browse the repository at this point in the history
Currently, snap points only consider the scroll offset of the
LayoutViewport(outer viewport scroll layer in cc), ignoring the scroll
offset of the VisualViewport(inner viewport scroll layer in cc). This
would confuse the user as they enlarge the page.

This patch makes sure that the coordinate for snapping is based on
visual viewport, and the snapping behavior scrolls visual viewport when
necessary.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I29ad271b04ac3fffec733f764e9168bc08150507

Bug: 839971
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I29ad271b04ac3fffec733f764e9168bc08150507
Reviewed-on: https://chromium-review.googlesource.com/1033351
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Sandra Sun <sunyunjia@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559729}
  • Loading branch information
sunyunjia authored and Commit Bot committed May 17, 2018
1 parent 5ed5bb2 commit 5b21b4e
Show file tree
Hide file tree
Showing 12 changed files with 180 additions and 23 deletions.
6 changes: 4 additions & 2 deletions cc/input/input_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ struct CC_EXPORT InputHandlerScrollResult {
// property scroll-boundary-behavior.
OverscrollBehavior overscroll_behavior;
// The current offset of the currently scrolling node. It is in DIP or
// physical pixels depending on the use-zoom-for-dsf flag.
gfx::Vector2dF current_offset;
// physical pixels depending on the use-zoom-for-dsf flag. If the currently
// scrolling node is the viewport, this would be the sum of the scroll offsets
// of the inner and outer node, representing the visual scroll offset.
gfx::Vector2dF current_visual_offset;
};

class CC_EXPORT InputHandlerClient {
Expand Down
1 change: 0 additions & 1 deletion cc/layers/viewport.cc
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ gfx::Vector2dF Viewport::ScrollAnimated(const gfx::Vector2dF& delta,
will_animate =
host_impl_->ScrollAnimationCreate(outer_node, outer_delta, delayed_by);
}

if (will_animate) {
// Consume entire scroll delta as long as we are starting an animation.
return delta;
Expand Down
3 changes: 2 additions & 1 deletion cc/layers/viewport.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ class CC_EXPORT Viewport {
gfx::Vector2dF ScrollAnimated(const gfx::Vector2dF& delta,
base::TimeDelta delayed_by);

gfx::ScrollOffset TotalScrollOffset() const;

void PinchUpdate(float magnify_delta, const gfx::Point& anchor);
void PinchEnd(const gfx::Point& anchor, bool snap_to_min);

Expand All @@ -84,7 +86,6 @@ class CC_EXPORT Viewport {
gfx::Vector2dF ScrollBrowserControls(const gfx::Vector2dF& delta);

gfx::ScrollOffset MaxTotalScrollOffset() const;
gfx::ScrollOffset TotalScrollOffset() const;

LayerImpl* InnerScrollLayer() const;
LayerImpl* OuterScrollLayer() const;
Expand Down
52 changes: 40 additions & 12 deletions cc/trees/layer_tree_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4036,10 +4036,10 @@ InputHandlerScrollResult LayerTreeHostImpl::ScrollBy(
UpdateRootLayerStateForSynchronousInputHandler();
}

scroll_result.current_offset = ScrollOffsetToVector2dF(
scroll_tree.current_scroll_offset(scroll_node->element_id));
scroll_result.current_visual_offset =
ScrollOffsetToVector2dF(GetVisualScrollOffset(*scroll_node));
float scale_factor = active_tree()->current_page_scale_factor();
scroll_result.current_offset.Scale(scale_factor);
scroll_result.current_visual_offset.Scale(scale_factor);

// Run animations which need to respond to updated scroll offset.
mutator_host_->TickScrollAnimations(
Expand Down Expand Up @@ -4077,9 +4077,7 @@ bool LayerTreeHostImpl::SnapAtScrollEnd() {
return false;

const SnapContainerData& data = scroll_node->snap_container_data.value();
ScrollTree& scroll_tree = active_tree()->property_trees()->scroll_tree;
gfx::ScrollOffset current_position =
scroll_tree.current_scroll_offset(scroll_node->element_id);
gfx::ScrollOffset current_position = GetVisualScrollOffset(*scroll_node);

gfx::ScrollOffset snap_position;
if (!data.FindSnapPosition(current_position, did_scroll_x_for_scroll_gesture_,
Expand All @@ -4088,12 +4086,43 @@ bool LayerTreeHostImpl::SnapAtScrollEnd() {
return false;
}

ScrollAnimationCreate(
scroll_node, ScrollOffsetToVector2dF(snap_position - current_position),
base::TimeDelta());
gfx::Vector2dF delta =
ScrollOffsetToVector2dF(snap_position - current_position);
bool scrolls_main_viewport_scroll_layer =
scroll_node == ViewportMainScrollNode();
if (scrolls_main_viewport_scroll_layer) {
// Flash the overlay scrollbar even if the scroll dalta is 0.
if (settings_.scrollbar_flash_after_any_scroll_update) {
FlashAllScrollbars(false);
} else {
ScrollbarAnimationController* animation_controller =
ScrollbarAnimationControllerForElementId(scroll_node->element_id);
if (animation_controller)
animation_controller->WillUpdateScroll();
}
gfx::Vector2dF scaled_delta(delta);
scaled_delta.Scale(active_tree()->current_page_scale_factor());
viewport()->ScrollAnimated(scaled_delta, base::TimeDelta());
} else {
ScrollAnimationCreate(scroll_node, delta, base::TimeDelta());
}
return true;
}

gfx::ScrollOffset LayerTreeHostImpl::GetVisualScrollOffset(
const ScrollNode& scroll_node) const {
const ScrollTree& scroll_tree = active_tree()->property_trees()->scroll_tree;

bool scrolls_main_viewport_scroll_layer =
viewport()->MainScrollLayer() &&
viewport()->MainScrollLayer()->scroll_tree_index() == scroll_node.id;

if (scrolls_main_viewport_scroll_layer)
return viewport()->TotalScrollOffset();
else
return scroll_tree.current_scroll_offset(scroll_node.element_id);
}

bool LayerTreeHostImpl::GetSnapFlingInfo(
const gfx::Vector2dF& natural_displacement_in_viewport,
gfx::Vector2dF* initial_offset,
Expand All @@ -4107,9 +4136,8 @@ bool LayerTreeHostImpl::GetSnapFlingInfo(
gfx::Vector2dF natural_displacement_in_content =
gfx::ScaleVector2d(natural_displacement_in_viewport, 1.f / scale_factor);

const ScrollTree& scroll_tree = active_tree()->property_trees()->scroll_tree;
*initial_offset = ScrollOffsetToVector2dF(
scroll_tree.current_scroll_offset(scroll_node->element_id));
*initial_offset =
ScrollOffsetToVector2dF(GetVisualScrollOffset(*scroll_node));

bool did_scroll_x = did_scroll_x_for_scroll_gesture_ ||
natural_displacement_in_content.x() != 0;
Expand Down
4 changes: 4 additions & 0 deletions cc/trees/layer_tree_host_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,10 @@ class CC_EXPORT LayerTreeHostImpl

virtual bool IsUIResourceOpaque(UIResourceId uid) const;

// This method gets the scroll offset for a regular scroller, or the combined
// visual and layout offsets of the viewport.
gfx::ScrollOffset GetVisualScrollOffset(const ScrollNode& scroll_node) const;

bool GetSnapFlingInfo(const gfx::Vector2dF& natural_displacement_in_viewport,
gfx::Vector2dF* initial_offset,
gfx::Vector2dF* target_offset) const override;
Expand Down
2 changes: 1 addition & 1 deletion cc/trees/layer_tree_host_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1652,7 +1652,7 @@ TEST_F(LayerTreeHostImplTest, GetSnapFlingInfoWhenZoomed) {
InputHandlerScrollResult result =
host_impl_->ScrollBy(UpdateState(scroll_position, delta).get());
EXPECT_VECTOR_EQ(gfx::Vector2dF(20, 20), overflow->CurrentScrollOffset());
EXPECT_VECTOR_EQ(gfx::Vector2dF(4, 4), result.current_offset);
EXPECT_VECTOR_EQ(gfx::Vector2dF(4, 4), result.current_visual_offset);

gfx::Vector2dF initial_offset, target_offset;
EXPECT_TRUE(host_impl_->GetSnapFlingInfo(gfx::Vector2dF(10, 10),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
<!DOCTYPE html>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<style>
body {
overflow: scroll;
scroll-snap-type: both proximity;
height: 300px;
width: 300px;
margin: 0px;
padding: 0px;
}
#container {
margin: 0px;
padding: 0px;
width: 600px;
height: 2000px;
}
#area {
position: relative;
left: 100px;
top: 700px;
width: 200px;
height: 200px;
scroll-snap-align: start;
}

</style>

<div id="container">
<div id="area"></div>
</div>

<script>
const TOUCH_SOURCE_TYPE = 1;
const WHEEL_SOURCE_TYPE = 2;
const SOURCE_TYPE = navigator.platform.indexOf('Mac') == 0
? WHEEL_SOURCE_TYPE
: TOUCH_SOURCE_TYPE;

function smoothScroll(pixels_to_scroll, start_x, start_y, gesture_source_type, direction, speed_in_pixels_s) {
return new Promise((resolve, reject) => {
chrome.gpuBenchmarking.smoothScrollBy(pixels_to_scroll, resolve, start_x, start_y, gesture_source_type, direction, speed_in_pixels_s);
});
}

function waitForAnimationEnd() {
const MAX_FRAME = 500;
var last_changed_frame = 0;
var layout_viewport_y = window.scrollY;
var visual_viewport_y = window.visualViewport.offsetTop;
return new Promise((resolve, reject) => {
function tick(frames) {
// We requestAnimationFrame either for 500 frames or until 15 frames with
// no change have been observed.
if (frames >= MAX_FRAME || frames - last_changed_frame > 15) {
resolve();
} else {
if (window.scrollY != layout_viewport_y ||
window.visualViewport.offsetTop != visual_viewport_y) {
last_changed_frame = frames;
layout_viewport_y = window.scrollY;
visual_viewport_y = window.visualViewport.offsetTop;
}
requestAnimationFrame(tick.bind(this, frames + 1));
}
}
tick(0);
});
}

function set_and_scroll_and_snap(
initial_position,
scroll_delta,
scroll_direction,
layout_viewport_y,
visual_viewport_y) {
internals.setPageScaleFactor(2);
window.scrollTo(0, initial_position);
internals.setVisualViewportOffset(0, 0);
assert_equals(window.visualViewport.scale, 2);
assert_equals(window.scrollY, initial_position);
assert_equals(window.visualViewport.offsetTop, 0);

return smoothScroll(2 * scroll_delta, 200, 200, SOURCE_TYPE, scroll_direction, 4000)
.then(waitForAnimationEnd)
.then(() => {
assert_approx_equals(window.scrollY, layout_viewport_y, 1);
assert_approx_equals(window.visualViewport.offsetTop, visual_viewport_y, 1);
})
}

promise_test(t => {
// The starting scroll position is 1000.
// We scroll upwards 400 to land on 600.
// As the visual_viewport's position is 0 and has room of 300 to scroll
// downwards, the layout_viewport can stay at 600 and only the visual_viewport
// needs to scroll to 100.
return set_and_scroll_and_snap(1000, 400, 'up', 600, 100);
}, "Snap scrolls visual viewport when it's scrollable.");

promise_test(t => {
// The starting scroll position is 1000.
// We scroll upwards 200 to land on 800.
// As the visual_viewport's position is 0 and has no room to scroll upwards,
// the layout_viewport is scrolled to 700 to snap to #area.
return set_and_scroll_and_snap(1000, 200, 'up', 700, 0);
}, "Snap scrolls layout viewport when visual viewport has reached its scroll extent.");

promise_test(t => {
// The starting scroll position is 300.
// We scroll downwards 250 to bring visual_viewport to 250, while still keeping
// layout_viewport at 300. So the total offset is 550.
// As the visual_viewport's position is 250 and only has room of 50 to scroll
// downwards, the layout_viewport needs to scroll another 100 downwards to
// make the total offset as 700.
return set_and_scroll_and_snap(300, 250, 'down', 400, 300);
}, "Snap scrolls both layout and visual viewports when visual viewport is" +
"scrollable but does not has enough room to reach the snap position.");

</script>
4 changes: 4 additions & 0 deletions third_party/blink/renderer/core/frame/root_frame_viewport.cc
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,10 @@ IntSize RootFrameViewport::ScrollOffsetInt() const {
return FlooredIntSize(GetScrollOffset());
}

IntPoint RootFrameViewport::ScrollOrigin() const {
return LayoutViewport().ScrollOrigin() + VisualViewport().ScrollOrigin();
}

ScrollOffset RootFrameViewport::GetScrollOffset() const {
return LayoutViewport().GetScrollOffset() +
VisualViewport().GetScrollOffset();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class CORE_EXPORT RootFrameViewport final
IntRect ScrollCornerRect() const override;
void UpdateScrollOffset(const ScrollOffset&, ScrollType) override;
IntSize ScrollOffsetInt() const override;
IntPoint ScrollOrigin() const override;
ScrollOffset GetScrollOffset() const override;
IntSize MinimumScrollOffsetInt() const override;
IntSize MaximumScrollOffsetInt() const override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ void SnapCoordinator::UpdateAllSnapContainerData() {

static ScrollableArea* ScrollableAreaForSnapping(const LayoutBox& layout_box) {
return layout_box.IsLayoutView()
? layout_box.GetFrameView()->LayoutViewportScrollableArea()
? layout_box.GetFrameView()->GetScrollableArea()
: layout_box.GetScrollableArea();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ class PLATFORM_EXPORT ScrollableArea : public GarbageCollectedMixin {
}

// See Source/core/layout/README.md for an explanation of scroll origin.
const IntPoint& ScrollOrigin() const { return scroll_origin_; }
virtual IntPoint ScrollOrigin() const { return scroll_origin_; }
bool ScrollOriginChanged() const { return scroll_origin_changed_; }

// This is used to determine whether the incoming fractional scroll offset
Expand Down
5 changes: 1 addition & 4 deletions ui/events/blink/input_handler_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1223,12 +1223,9 @@ bool InputHandlerProxy::GetSnapFlingInfo(
gfx::Vector2dF InputHandlerProxy::ScrollByForSnapFling(
const gfx::Vector2dF& delta) {
cc::ScrollState scroll_state = CreateScrollStateForInertialUpdate(delta);
// TODO(sunyunjia): We should consider moving the scroll to main, handling
// overscroll, and handling elastic scroll after ScrollBy().
// https://crbug.com/819855
cc::InputHandlerScrollResult scroll_result =
input_handler_->ScrollBy(&scroll_state);
return scroll_result.current_offset;
return scroll_result.current_visual_offset;
}

void InputHandlerProxy::ScrollEndForSnapFling() {
Expand Down

0 comments on commit 5b21b4e

Please sign in to comment.