Skip to content

Commit

Permalink
Make inverted viewport scroll order the default and remove flag.
Browse files Browse the repository at this point in the history 
This patch removes the invert-viewport-scroll-order flag, making this behavior
the default. This means scrolls while pinch-zoomed in scroll the visual viewport
first, bubbling remaining scrolls up to the layout viewport. Also fixed existing
unit tests that relied on the old behavior.

BUG=443724
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1283053002

Cr-Commit-Position: refs/heads/master@{#345636}
  • Loading branch information
@bokand
bokand authored and Commit bot committed Aug 26, 2015
1 parent c574e67 commit eac4df4
Show file tree
Hide file tree
Showing 15 changed files with 59 additions and 90 deletions.
23 changes: 4 additions & 19 deletions cc/layers/viewport.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,7 @@ Viewport::ScrollResult Viewport::ScrollBy(const gfx::Vector2dF& delta,

gfx::Vector2dF pending_content_delta = content_delta;

bool invert_scroll_order =
host_impl_->settings().invert_viewport_scroll_order;
LayerImpl* primary_layer =
invert_scroll_order ? InnerScrollLayer() : OuterScrollLayer();
LayerImpl* secondary_layer =
invert_scroll_order ? OuterScrollLayer() : InnerScrollLayer();

pending_content_delta -= host_impl_->ScrollLayer(primary_layer,
pending_content_delta -= host_impl_->ScrollLayer(InnerScrollLayer(),
pending_content_delta,
viewport_point,
is_direct_manipulation);
Expand All @@ -62,7 +55,7 @@ Viewport::ScrollResult Viewport::ScrollBy(const gfx::Vector2dF& delta,
if (gfx::ToRoundedVector2d(pending_content_delta).IsZero()) {
result.consumed_delta = delta;
} else {
pending_content_delta -= host_impl_->ScrollLayer(secondary_layer,
pending_content_delta -= host_impl_->ScrollLayer(OuterScrollLayer(),
pending_content_delta,
viewport_point,
is_direct_manipulation);
Expand Down Expand Up @@ -94,8 +87,7 @@ void Viewport::PinchUpdate(float magnify_delta, const gfx::Point& anchor) {
// length of the screen edge, offset all updates by the amount so that we
// effectively snap the pinch zoom to the edge of the screen. This makes it
// easy to zoom in on position: fixed elements.
if (host_impl_->settings().invert_viewport_scroll_order)
SnapPinchAnchorIfWithinMargin(anchor);
SnapPinchAnchorIfWithinMargin(anchor);

pinch_zoom_active_ = true;
}
Expand All @@ -122,14 +114,7 @@ void Viewport::PinchUpdate(float magnify_delta, const gfx::Point& anchor) {
// be accounted for from the intended move.
move -= InnerScrollLayer()->ClampScrollToMaxScrollOffset();

if (host_impl_->settings().invert_viewport_scroll_order) {
Pan(move);
} else {
gfx::Point viewport_point;
bool is_wheel_event = false;
bool affect_top_controls = false;
ScrollBy(move, viewport_point, is_wheel_event, affect_top_controls);
}
Pan(move);
}

void Viewport::PinchEnd() {
Expand Down
96 changes: 52 additions & 44 deletions cc/trees/layer_tree_host_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1138,7 +1138,6 @@ TEST_F(LayerTreeHostImplTest, ImplPinchZoom) {

TEST_F(LayerTreeHostImplTest, ViewportScrollOrder) {
LayerTreeSettings settings = DefaultSettings();
settings.invert_viewport_scroll_order = true;
CreateHostImpl(settings,
CreateOutputSurface());
host_impl_->active_tree()->PushPageScaleFromMainThread(1.f, 0.25f, 4.f);
Expand Down Expand Up @@ -1199,7 +1198,6 @@ TEST_F(LayerTreeHostImplTest, ViewportScrollOrder) {
// to the outer viewport.
TEST_F(LayerTreeHostImplTest, ScrollDuringPinchGesture) {
LayerTreeSettings settings = DefaultSettings();
settings.invert_viewport_scroll_order = true;
CreateHostImpl(settings,
CreateOutputSurface());
host_impl_->active_tree()->PushPageScaleFromMainThread(1.f, 1.f, 2.f);
Expand Down Expand Up @@ -1256,7 +1254,6 @@ TEST_F(LayerTreeHostImplTest, ScrollDuringPinchGesture) {
// should assume the user means to scroll into the edge of the screen.
TEST_F(LayerTreeHostImplTest, PinchZoomSnapsToScreenEdge) {
LayerTreeSettings settings = DefaultSettings();
settings.invert_viewport_scroll_order = true;
CreateHostImpl(settings,
CreateOutputSurface());
host_impl_->active_tree()->PushPageScaleFromMainThread(1.f, 1.f, 2.f);
Expand Down Expand Up @@ -1352,36 +1349,36 @@ TEST_F(LayerTreeHostImplTest, ImplPinchZoomWheelBubbleBetweenViewports) {
page_scale_factor, min_page_scale, max_page_scale);
host_impl_->SetPageScaleOnActiveTree(page_scale_factor);

// Scroll by a small amount, there should be no bubbling up to the inner
// Scroll by a small amount, there should be no bubbling to the outer
// viewport.
host_impl_->ScrollBegin(gfx::Point(0, 0), InputHandler::WHEEL);
host_impl_->ScrollBy(gfx::Point(0, 0), gfx::Vector2dF(10.f, 20.f));
host_impl_->ScrollEnd();

EXPECT_VECTOR_EQ(
gfx::Vector2dF(5, 10),
outer_scroll_layer->CurrentScrollOffset());
inner_scroll_layer->CurrentScrollOffset());
EXPECT_VECTOR_EQ(
gfx::Vector2dF(),
inner_scroll_layer->CurrentScrollOffset());
outer_scroll_layer->CurrentScrollOffset());

// Scroll by the outer viewport's max scroll extent, there the remainder
// should bubble up to the inner viewport.
// Scroll by the inner viewport's max scroll extent, the remainder
// should bubble up to the outer viewport.
host_impl_->ScrollBegin(gfx::Point(0, 0), InputHandler::WHEEL);
host_impl_->ScrollBy(gfx::Point(0, 0), gfx::Vector2dF(200.f, 200.f));
host_impl_->ScrollBy(gfx::Point(0, 0), gfx::Vector2dF(100.f, 100.f));
host_impl_->ScrollEnd();

EXPECT_VECTOR_EQ(
gfx::Vector2dF(100, 100),
outer_scroll_layer->CurrentScrollOffset());
gfx::Vector2dF(50, 50),
inner_scroll_layer->CurrentScrollOffset());
EXPECT_VECTOR_EQ(
gfx::Vector2dF(5, 10),
inner_scroll_layer->CurrentScrollOffset());
outer_scroll_layer->CurrentScrollOffset());

// Scroll by the inner viewport's max scroll extent, it should all go to the
// inner viewport.
// Scroll by the outer viewport's max scroll extent, it should all go to the
// outer viewport.
host_impl_->ScrollBegin(gfx::Point(0, 0), InputHandler::WHEEL);
host_impl_->ScrollBy(gfx::Point(0, 0), gfx::Vector2dF(90.f, 80.f));
host_impl_->ScrollBy(gfx::Point(0, 0), gfx::Vector2dF(190.f, 180.f));
host_impl_->ScrollEnd();

EXPECT_VECTOR_EQ(
Expand Down Expand Up @@ -1619,6 +1616,10 @@ TEST_F(LayerTreeHostImplTest, PinchGesture) {
host_impl_->PinchGestureBegin();
host_impl_->PinchGestureUpdate(2.f, gfx::Point(0, 0));
host_impl_->PinchGestureUpdate(1.f, gfx::Point(0, 0));

// Needed so layer transform includes page scale.
DrawFrame();

host_impl_->ScrollBy(gfx::Point(0, 0), gfx::Vector2d(10, 10));
host_impl_->PinchGestureUpdate(1.f, gfx::Point(10, 10));
host_impl_->PinchGestureEnd();
Expand All @@ -1628,7 +1629,7 @@ TEST_F(LayerTreeHostImplTest, PinchGesture) {
host_impl_->ProcessScrollDeltas();
EXPECT_EQ(scroll_info->page_scale_delta, 2.f);
EXPECT_TRUE(ScrollInfoContains(*scroll_info, scroll_layer->id(),
gfx::Vector2d(20, 20)));
gfx::Vector2d(10, 10)));
}
}

Expand Down Expand Up @@ -3139,12 +3140,12 @@ TEST_F(LayerTreeHostImplTopControlsTest,
// Now when we continue scrolling, make sure the outer viewport gets scrolled
// since it wasn't scrollable when the scroll began.
host_impl_->ScrollBy(gfx::Point(), gfx::Vector2dF(0.f, -20.f));
EXPECT_EQ(15.f, outer_scroll->CurrentScrollOffset().y());
EXPECT_EQ(25.f, inner_scroll->CurrentScrollOffset().y());
EXPECT_EQ(25.f, outer_scroll->CurrentScrollOffset().y());
EXPECT_EQ(15.f, inner_scroll->CurrentScrollOffset().y());

host_impl_->ScrollBy(gfx::Point(), gfx::Vector2dF(0.f, -30.f));
EXPECT_EQ(0.f, outer_scroll->CurrentScrollOffset().y());
EXPECT_EQ(25.f, inner_scroll->CurrentScrollOffset().y());
EXPECT_EQ(25.f, outer_scroll->CurrentScrollOffset().y());
EXPECT_EQ(0.f, inner_scroll->CurrentScrollOffset().y());

host_impl_->ScrollBy(gfx::Point(), gfx::Vector2dF(0.f, -50.f));
host_impl_->ScrollEnd();
Expand Down Expand Up @@ -3727,7 +3728,7 @@ TEST_F(LayerTreeHostImplTest, ScrollBlockedByContentLayer) {
}

TEST_F(LayerTreeHostImplTest, ScrollRootAndChangePageScaleOnMainThread) {
gfx::Size viewport_size(10, 10);
gfx::Size viewport_size(20, 20);
float page_scale = 2.f;

SetupScrollAndContentsLayers(viewport_size);
Expand All @@ -3736,12 +3737,14 @@ TEST_F(LayerTreeHostImplTest, ScrollRootAndChangePageScaleOnMainThread) {
host_impl_->active_tree()->InnerViewportScrollLayer()->parent()->SetBounds(
viewport_size);
host_impl_->active_tree()->OuterViewportScrollLayer()->SetBounds(
gfx::Size(20, 20));
gfx::Size(40, 40));
host_impl_->active_tree()->PushPageScaleFromMainThread(1.f, 1.f, 2.f);
DrawFrame();

LayerImpl* root_scroll =
host_impl_->active_tree()->OuterViewportScrollLayer();
LayerImpl* inner_scroll =
host_impl_->active_tree()->InnerViewportScrollLayer();
EXPECT_EQ(viewport_size, root_scroll->scroll_clip_layer()->bounds());

gfx::Vector2d scroll_delta(0, 10);
Expand All @@ -3756,7 +3759,7 @@ TEST_F(LayerTreeHostImplTest, ScrollRootAndChangePageScaleOnMainThread) {
host_impl_->active_tree()->PushPageScaleFromMainThread(page_scale, 1.f, 2.f);

scoped_ptr<ScrollAndScaleSet> scroll_info = host_impl_->ProcessScrollDeltas();
EXPECT_TRUE(ScrollInfoContains(*scroll_info.get(), root_scroll->id(),
EXPECT_TRUE(ScrollInfoContains(*scroll_info.get(), inner_scroll->id(),
expected_scroll_delta));

// The scroll range should also have been updated.
Expand All @@ -3768,7 +3771,7 @@ TEST_F(LayerTreeHostImplTest, ScrollRootAndChangePageScaleOnMainThread) {
}

TEST_F(LayerTreeHostImplTest, ScrollRootAndChangePageScaleOnImplThread) {
gfx::Size viewport_size(10, 10);
gfx::Size viewport_size(20, 20);
float page_scale = 2.f;

SetupScrollAndContentsLayers(viewport_size);
Expand All @@ -3777,12 +3780,14 @@ TEST_F(LayerTreeHostImplTest, ScrollRootAndChangePageScaleOnImplThread) {
host_impl_->active_tree()->InnerViewportScrollLayer()->parent()->SetBounds(
viewport_size);
host_impl_->active_tree()->OuterViewportScrollLayer()->SetBounds(
gfx::Size(20, 20));
gfx::Size(40, 40));
host_impl_->active_tree()->PushPageScaleFromMainThread(1.f, 1.f, 2.f);
DrawFrame();

LayerImpl* root_scroll =
host_impl_->active_tree()->OuterViewportScrollLayer();
LayerImpl* inner_scroll =
host_impl_->active_tree()->InnerViewportScrollLayer();
EXPECT_EQ(viewport_size, root_scroll->scroll_clip_layer()->bounds());

gfx::Vector2d scroll_delta(0, 10);
Expand All @@ -3803,7 +3808,7 @@ TEST_F(LayerTreeHostImplTest, ScrollRootAndChangePageScaleOnImplThread) {

// The scroll delta is not scaled because the main thread did not scale.
scoped_ptr<ScrollAndScaleSet> scroll_info = host_impl_->ProcessScrollDeltas();
EXPECT_TRUE(ScrollInfoContains(*scroll_info.get(), root_scroll->id(),
EXPECT_TRUE(ScrollInfoContains(*scroll_info.get(), inner_scroll->id(),
expected_scroll_delta));

// The scroll range should also have been updated.
Expand Down Expand Up @@ -3865,10 +3870,10 @@ TEST_F(LayerTreeHostImplTest, PageScaleDeltaAppliedToRootScrollLayerOnly) {
}

TEST_F(LayerTreeHostImplTest, ScrollChildAndChangePageScaleOnMainThread) {
gfx::Size surface_size(30, 30);
SetupScrollAndContentsLayers(surface_size);
SetupScrollAndContentsLayers(gfx::Size(30, 30));

LayerImpl* outer_scroll = host_impl_->OuterViewportScrollLayer();
LayerImpl* inner_scroll = host_impl_->InnerViewportScrollLayer();

// Make the outer scroll layer scrollable.
outer_scroll->SetBounds(gfx::Size(50, 50));
Expand All @@ -3890,7 +3895,7 @@ TEST_F(LayerTreeHostImplTest, ScrollChildAndChangePageScaleOnMainThread) {
DrawOneFrame();

scoped_ptr<ScrollAndScaleSet> scroll_info = host_impl_->ProcessScrollDeltas();
EXPECT_TRUE(ScrollInfoContains(*scroll_info.get(), outer_scroll->id(),
EXPECT_TRUE(ScrollInfoContains(*scroll_info.get(), inner_scroll->id(),
expected_scroll_delta));

// The scroll range should not have changed.
Expand Down Expand Up @@ -7588,9 +7593,9 @@ TEST_F(LayerTreeHostImplVirtualViewportTest, ScrollBothInnerAndOuterLayer) {
}

TEST_F(LayerTreeHostImplVirtualViewportTest, FlingScrollBubblesToInner) {
gfx::Size content_size = gfx::Size(100, 160);
gfx::Size outer_viewport = gfx::Size(50, 80);
gfx::Size inner_viewport = gfx::Size(25, 40);
gfx::Size content_size = gfx::Size(200, 320);
gfx::Size outer_viewport = gfx::Size(100, 160);
gfx::Size inner_viewport = gfx::Size(50, 80);

SetupVirtualViewportLayers(content_size, outer_viewport, inner_viewport);

Expand All @@ -7603,16 +7608,18 @@ TEST_F(LayerTreeHostImplVirtualViewportTest, FlingScrollBubblesToInner) {
EXPECT_VECTOR_EQ(inner_expected, inner_scroll->CurrentScrollOffset());
EXPECT_VECTOR_EQ(outer_expected, outer_scroll->CurrentScrollOffset());

// Make sure the fling goes to the outer viewport first
// Scrolling the viewport always sets the outer scroll layer as the
// currently scrolling layer.
EXPECT_EQ(InputHandler::SCROLL_STARTED,
host_impl_->ScrollBegin(gfx::Point(), InputHandler::GESTURE));
EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
EXPECT_EQ(InputHandler::SCROLL_STARTED, host_impl_->FlingScrollBegin());
EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());

gfx::Vector2d scroll_delta(inner_viewport.width(), inner_viewport.height());
gfx::Vector2d scroll_delta(inner_viewport.width() / 2.f,
inner_viewport.height() / 2.f);
host_impl_->ScrollBy(gfx::Point(), scroll_delta);
outer_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
inner_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
EXPECT_EQ(host_impl_->CurrentlyScrollingLayer(), outer_scroll);

host_impl_->ScrollEnd();
Expand All @@ -7621,19 +7628,19 @@ TEST_F(LayerTreeHostImplVirtualViewportTest, FlingScrollBubblesToInner) {
EXPECT_VECTOR_EQ(inner_expected, inner_scroll->CurrentScrollOffset());
EXPECT_VECTOR_EQ(outer_expected, outer_scroll->CurrentScrollOffset());

// Fling past the outer viewport boundry, make sure inner viewport scrolls.
// Fling past the inner viewport boundry, make sure outer viewport scrolls.
EXPECT_EQ(InputHandler::SCROLL_STARTED,
host_impl_->ScrollBegin(gfx::Point(), InputHandler::GESTURE));
EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
EXPECT_EQ(InputHandler::SCROLL_STARTED, host_impl_->FlingScrollBegin());
EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());

host_impl_->ScrollBy(gfx::Point(), scroll_delta);
outer_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
inner_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());

host_impl_->ScrollBy(gfx::Point(), scroll_delta);
inner_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
outer_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());

host_impl_->ScrollEnd();
Expand All @@ -7646,9 +7653,9 @@ TEST_F(LayerTreeHostImplVirtualViewportTest, FlingScrollBubblesToInner) {

TEST_F(LayerTreeHostImplVirtualViewportTest,
DiagonalScrollBubblesPerfectlyToInner) {
gfx::Size content_size = gfx::Size(100, 160);
gfx::Size outer_viewport = gfx::Size(50, 80);
gfx::Size inner_viewport = gfx::Size(25, 40);
gfx::Size content_size = gfx::Size(200, 320);
gfx::Size outer_viewport = gfx::Size(100, 160);
gfx::Size inner_viewport = gfx::Size(50, 80);

SetupVirtualViewportLayers(content_size, outer_viewport, inner_viewport);

Expand All @@ -7661,17 +7668,18 @@ TEST_F(LayerTreeHostImplVirtualViewportTest,
EXPECT_VECTOR_EQ(inner_expected, inner_scroll->CurrentScrollOffset());
EXPECT_VECTOR_EQ(outer_expected, outer_scroll->CurrentScrollOffset());

// Make sure the scroll goes to the outer viewport first.
// Make sure the scroll goes to the inner viewport first.
EXPECT_EQ(InputHandler::SCROLL_STARTED,
host_impl_->ScrollBegin(gfx::Point(), InputHandler::GESTURE));
EXPECT_EQ(InputHandler::SCROLL_STARTED, host_impl_->FlingScrollBegin());
EXPECT_TRUE(host_impl_->IsCurrentlyScrollingLayerAt(gfx::Point(),
InputHandler::GESTURE));

// Scroll near the edge of the outer viewport.
gfx::Vector2d scroll_delta(inner_viewport.width(), inner_viewport.height());
gfx::Vector2d scroll_delta(inner_viewport.width() / 2.f,
inner_viewport.height() / 2.f);
host_impl_->ScrollBy(gfx::Point(), scroll_delta);
outer_expected += scroll_delta;
inner_expected += scroll_delta;
EXPECT_TRUE(host_impl_->IsCurrentlyScrollingLayerAt(gfx::Point(),
InputHandler::GESTURE));

Expand Down
1 change: 0 additions & 1 deletion cc/trees/layer_tree_settings.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ LayerTreeSettings::LayerTreeSettings()
verify_property_trees(false),
gather_pixel_refs(false),
use_compositor_animation_timelines(false),
invert_viewport_scroll_order(false),
wait_for_beginframe_interval(true),
max_staging_buffers(32) {}

Expand Down
1 change: 0 additions & 1 deletion cc/trees/layer_tree_settings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ class CC_EXPORT LayerTreeSettings {
bool verify_property_trees;
bool gather_pixel_refs;
bool use_compositor_animation_timelines;
bool invert_viewport_scroll_order;
bool wait_for_beginframe_interval;
int max_staging_buffers;

Expand Down
6 changes: 0 additions & 6 deletions chrome/app/generated_resources.grd
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6467,12 +6467,6 @@ Keep your key file in a safe place. You will need it to create new versions of y
<message name="IDS_FLAGS_DISABLE_THREADED_SCROLLING_DESCRIPTION" desc="Description for the flag to disable threaded scrolling.">
Disabled threaded handling of scroll-related input events, forcing all such scroll events to be handled on the main thread. Note that this can dramatically hurt scrolling performance of most websites and is intended for testing purposes only.
</message>
<message name="IDS_FLAGS_INVERT_VIEWPORT_SCROLL_ORDER_NAME" desc="Title for the flag to enable the viewport scroll order experiment.">
Invert Viewport Scroll Order.
</message>
<message name="IDS_FLAGS_INVERT_VIEWPORT_SCROLL_ORDER_DESCRIPTION" desc="Description for the flag to enable the viewport scroll order experiment.">
Enables flipped viewport scroll order in some situations. Namely, during a pinch-zoom, the visual viewport should be scrolled first, then the layout viewport.
</message>
<message name="IDS_FLAGS_BLEEDING_RENDERER_NAME" desc="Name of the 'Stacked Tabs' lab.">
Bleeding Edge Renderer Paths - LIKELY TO CRASH YOUR BROWSER
</message>
Expand Down
5 changes: 0 additions & 5 deletions chrome/browser/about_flags.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1621,11 +1621,6 @@ const Experiment kExperiments[] = {
IDS_FLAGS_DISABLE_THREADED_SCROLLING_DESCRIPTION,
kOsWin | kOsLinux | kOsCrOS | kOsAndroid | kOsMac,
SINGLE_VALUE_TYPE(switches::kDisableThreadedScrolling)},
{"invert-viewport-scroll-order",
IDS_FLAGS_INVERT_VIEWPORT_SCROLL_ORDER_NAME,
IDS_FLAGS_INVERT_VIEWPORT_SCROLL_ORDER_DESCRIPTION,
kOsAll,
SINGLE_VALUE_TYPE(switches::kInvertViewportScrollOrder)},
{"bleeding-edge-renderer-mode",
IDS_FLAGS_BLEEDING_RENDERER_NAME,
IDS_FLAGS_BLEEDING_RENDERER_DESCRIPTION,
Expand Down
1 change: 0 additions & 1 deletion content/browser/renderer_host/render_process_host_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1321,7 +1321,6 @@ void RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
switches::kEnableUnsafeES3APIs,
switches::kEnableViewport,
switches::kEnableViewportMeta,
switches::kInvertViewportScrollOrder,
switches::kEnableVtune,
switches::kEnableWebBluetooth,
switches::kEnableWebGLDraftExtensions,
Expand Down
Loading

0 comments on commit eac4df4

Please sign in to comment.