Skip to content

GitLab

T
tangled
Commit d713bb2e authored by David Bokan's avatar David Bokan Committed by Commit Bot
Browse files
Options

Fix touch action filter for double-tap-drag zoom 

The touch action filter currently relies on checking the
GestureScrollBegin event for whether we need to filter or not. It
assumes a pinch-zoom will cause a GSB with a pointer_count == 2 and so
pinch-zoom is filtered in that way, since we always bracket GesturePinch
events between a GSB and a GestureScrollEnd.

However, this breaks down for double-tap-drag zoom (i.e. double tap but
hold and drag the second tap to zoom). In this case, the pointer_count
is 1 so we don't filter it.

In this CL, we remove the bracketing of pinch gestures in the
double-tap-drag zoom case and allow activating the filter at
GesturePinchBegin. We must now track in the TouchActionFilter whether
we've seen a GestureScrollBegin so that we know whether to apply and
remove the filtering at GesturePinchBegin/GesturePinchEnd.

Bug: 865090
Change-Id: I25e426f6f48dbf0ebcecfbddee5e2d8cbcc40504
Reviewed-on: https://chromium-review.googlesource.com/c/1279236
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: default avatarTimothy Dresser <tdresser@chromium.org>
Reviewed-by: default avatarAlex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600937}
parent 1f9439d0
Hide whitespace changes
Inline Side-by-side
Showing with 128 additions and 21 deletions
content/browser/renderer_host/input/touch_action_browsertest.cc
View file @ d713bb2e
......@@ -7,6 +7,7 @@
#include "base/auto_reset.h"
#include "base/bind.h"
#include "base/command_line.h"
#include "base/json/json_reader.h"
#include "base/macros.h"
#include "base/run_loop.h"
#include "base/strings/utf_string_conversions.h"
......@@ -18,7 +19,9 @@
#include "content/browser/renderer_host/render_widget_host_impl.h"
#include "content/browser/renderer_host/render_widget_host_view_base.h"
#include "content/browser/web_contents/web_contents_impl.h"
#include "content/common/input/actions_parser.h"
#include "content/common/input/synthetic_gesture_params.h"
#include "content/common/input/synthetic_pointer_action_list_params.h"
#include "content/common/input/synthetic_smooth_scroll_gesture_params.h"
#include "content/common/input_messages.h"
#include "content/public/browser/render_view_host.h"
......@@ -316,6 +319,45 @@ class TouchActionBrowserTest : public ContentBrowserTest {
expected_scroll_position_after_scroll);
}
// Generate touch events for a double tap and drag zoom gesture at
// coordinates (50, 50).
void DoDoubleTapDragZoom() {
DCHECK(URLLoaded());
const std::string pointer_actions_json = R"HTML(
[{
"source": "touch",
"actions": [
{ "name": "pointerDown", "x": 50, "y": 50 },
{ "name": "pointerUp" },
{ "name": "pause", "duration": 0.05 },
{ "name": "pointerDown", "x": 50, "y": 50 },
{ "name": "pointerMove", "x": 50, "y": 150 },
{ "name": "pointerUp" }
]
}]
)HTML";
base::JSONReader json_reader;
std::unique_ptr<base::Value> params =
json_reader.ReadToValue(pointer_actions_json);
ASSERT_TRUE(params.get()) << json_reader.GetErrorMessage();
ActionsParser actions_parser(params.get());
ASSERT_TRUE(actions_parser.ParsePointerActionSequence());
run_loop_ = std::make_unique<base::RunLoop>();
GetWidgetHost()->QueueSyntheticGesture(
SyntheticGesture::Create(actions_parser.gesture_params()),
base::BindOnce(&TouchActionBrowserTest::OnSyntheticGestureCompleted,
base::Unretained(this)));
// Runs until we get the OnSyntheticGestureCompleted callback
run_loop_->Run();
run_loop_.reset();
}
private:
void CheckScrollOffset(
bool wait_until_scrolled,
......@@ -532,4 +574,43 @@ IN_PROC_BROWSER_TEST_F(TouchActionBrowserTest,
gfx::Vector2d(45, 0), kShortJankTime);
}
namespace {
const std::string kDoubleTapZoomDataURL = R"HTML(
data:text/html,<!DOCTYPE html>
<meta name='viewport' content='width=device-width'/>
<style>
html, body {
margin: 0;
}
.spacer { height: 10000px; }
.touchaction { width: 75px; height: 75px; touch-action: none; }
</style>
<div class="touchaction"></div>
<div class=spacer></div>
<script>
document.title='ready';
</script>)HTML";
} // namespace
// Test that |touch-action: none| correctly blocks a double-tap and drag zoom
// gesture.
IN_PROC_BROWSER_TEST_F(TouchActionBrowserTest, BlockDoubleTapDragZoom) {
LoadURL(kDoubleTapZoomDataURL.c_str());
ASSERT_EQ(1, ExecuteScriptAndExtractDouble("window.visualViewport.scale"));
DoDoubleTapDragZoom();
// Since we don't expect anything to change, we don't know how long to wait
// before we're sure the zoom was blocked. Do a scroll so that we can wait
// until the offset changes. At that point, we know the zoom should have
// taken effect if it wasn't blocked by touch-action.
DoTouchScroll(gfx::Point(300, 300), gfx::Vector2d(0, 200), true, 10075,
gfx::Vector2d(0, 200), kNoJankTime);
EXPECT_EQ(1, ExecuteScriptAndExtractDouble("window.visualViewport.scale"));
}
} // namespace content
content/browser/renderer_host/input/touch_action_filter.cc
View file @ d713bb2e
......@@ -63,6 +63,7 @@ FilterGestureEventResult TouchActionFilter::FilterGestureEvent(
if (!scrolling_touch_action_.has_value())
SetTouchAction(cc::kTouchActionAuto);
}
gesture_scroll_in_progress_ = true;
gesture_sequence_.append("B");
if (!scrolling_touch_action_.has_value()) {
static auto* crash_key = base::debug::AllocateCrashKeyString(
......@@ -114,6 +115,7 @@ FilterGestureEventResult TouchActionFilter::FilterGestureEvent(
case WebInputEvent::kGestureScrollEnd:
gesture_sequence_.clear();
gesture_scroll_in_progress_ = false;
gesture_sequence_in_progress_ = false;
ReportGestureEventFiltered(suppress_manipulation_events_);
return FilterManipulationEventAndResetState()
......@@ -121,13 +123,32 @@ FilterGestureEventResult TouchActionFilter::FilterGestureEvent(
: FilterGestureEventResult::kFilterGestureEventAllowed;
case WebInputEvent::kGesturePinchBegin:
if (!gesture_scroll_in_progress_) {
suppress_manipulation_events_ =
ShouldSuppressManipulation(*gesture_event);
}
FALLTHROUGH;
case WebInputEvent::kGesturePinchUpdate:
case WebInputEvent::kGesturePinchEnd:
gesture_sequence_.append("P");
ReportGestureEventFiltered(suppress_manipulation_events_);
return suppress_manipulation_events_
? FilterGestureEventResult::kFilterGestureEventFiltered
: FilterGestureEventResult::kFilterGestureEventAllowed;
case WebInputEvent::kGesturePinchEnd:
ReportGestureEventFiltered(suppress_manipulation_events_);
// If we're in a double-tap and drag zoom, we won't be bracketed between
// a GSB/GSE pair so end the sequence now. Otherwise this will happen
// when we get the GSE.
if (!gesture_scroll_in_progress_) {
gesture_sequence_.clear();
gesture_sequence_in_progress_ = false;
return FilterManipulationEventAndResetState()
? FilterGestureEventResult::kFilterGestureEventFiltered
: FilterGestureEventResult::kFilterGestureEventAllowed;
}
return suppress_manipulation_events_
? FilterGestureEventResult::kFilterGestureEventFiltered
: FilterGestureEventResult::kFilterGestureEventAllowed;
// The double tap gesture is a tap ending event. If a double-tap gesture is
// filtered out, replace it with a tap event but preserve the tap-count to
......@@ -339,7 +360,12 @@ void TouchActionFilter::OnSetWhiteListedTouchAction(
bool TouchActionFilter::ShouldSuppressManipulation(
const blink::WebGestureEvent& gesture_event) {
DCHECK_EQ(gesture_event.GetType(), WebInputEvent::kGestureScrollBegin);
DCHECK(gesture_event.GetType() == WebInputEvent::kGestureScrollBegin ||
WebInputEvent::IsPinchGestureEventType(gesture_event.GetType()));
if (WebInputEvent::IsPinchGestureEventType(gesture_event.GetType())) {
return (scrolling_touch_action_.value() & cc::kTouchActionPinchZoom) == 0;
}
if (gesture_event.data.scroll_begin.pointer_count >= 2) {
// Any GestureScrollBegin with more than one fingers is like a pinch-zoom
......
content/browser/renderer_host/input/touch_action_filter.h
View file @ d713bb2e
......@@ -111,6 +111,9 @@ class CONTENT_EXPORT TouchActionFilter {
// before GSE.
bool gesture_sequence_in_progress_ = false;
// True if we're between a GSB and a GSE.
bool gesture_scroll_in_progress_ = false;
// Increment at receiving ACK for touch start and decrement at touch end.
int num_of_active_touches_ = 0;
......
content/common/BUILD.gn
View file @ d713bb2e
......@@ -131,6 +131,8 @@ source_set("common") {
"gin_java_bridge_messages.h",
"in_process_child_thread_params.cc",
"in_process_child_thread_params.h",
"input/actions_parser.cc",
"input/actions_parser.h",
"input/event_with_latency_info.h",
"input/gesture_event_stream_validator.cc",
"input/gesture_event_stream_validator.h",
......
content/renderer/gpu/actions_parser.cc → content/common/input/actions_parser.cc
View file @ d713bb2e
......@@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "content/renderer/gpu/actions_parser.h"
#include "content/common/input/actions_parser.h"
#include "base/format_macros.h"
#include "base/strings/stringprintf.h"
......
content/renderer/gpu/actions_parser.h → content/common/input/actions_parser.h
View file @ d713bb2e
......@@ -2,8 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CONTENT_RENDERER_GPU_ACTION_PARSER_H_
#define CONTENT_RENDERER_GPU_ACTION_PARSER_H_
#ifndef CONTENT_COMMON_INPUT_ACTIONS_PARSER_H_
#define CONTENT_COMMON_INPUT_ACTIONS_PARSER_H_
#include <cstddef>
#include <set>
......@@ -54,4 +54,4 @@ class CONTENT_EXPORT ActionsParser {
} // namespace content
#endif // CONTENT_RENDERER_GPU_ACTION_PARSER_H_
#endif // CONTENT_COMMON_INPUT_ACTIONS_PARSER_H_
content/renderer/gpu/actions_parser_unittest.cc → content/common/input/actions_parser_unittest.cc
View file @ d713bb2e
......@@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "content/renderer/gpu/actions_parser.h"
#include "content/common/input/actions_parser.h"
#include "base/json/json_reader.h"
#include "testing/gtest/include/gtest/gtest.h"
......
content/renderer/BUILD.gn
View file @ d713bb2e
......@@ -113,8 +113,6 @@ target(link_target_type, "renderer") {
"frame_blame_context.h",
"frame_owner_properties.cc",
"frame_owner_properties.h",
"gpu/actions_parser.cc",
"gpu/actions_parser.h",
"gpu/compositor_dependencies.h",
"gpu/frame_swap_message_queue.cc",
"gpu/frame_swap_message_queue.h",
......
content/renderer/gpu/gpu_benchmarking_extension.cc
View file @ d713bb2e
......@@ -21,6 +21,7 @@
#include "cc/layers/layer.h"
#include "cc/paint/skia_paint_canvas.h"
#include "cc/trees/layer_tree_host.h"
#include "content/common/input/actions_parser.h"
#include "content/common/input/synthetic_gesture_params.h"
#include "content/common/input/synthetic_pinch_gesture_params.h"
#include "content/common/input/synthetic_pointer_action_list_params.h"
......@@ -32,7 +33,6 @@
#include "content/public/renderer/chrome_object_extensions_utils.h"
#include "content/public/renderer/render_thread.h"
#include "content/public/renderer/v8_value_converter.h"
#include "content/renderer/gpu/actions_parser.h"
#include "content/renderer/gpu/layer_tree_view.h"
#include "content/renderer/render_thread_impl.h"
#include "content/renderer/render_view_impl.h"
......
content/test/BUILD.gn
View file @ d713bb2e
......@@ -1649,6 +1649,7 @@ test("content_unittests") {
"../common/content_switches_internal_unittest.cc",
"../common/cursors/webcursor_unittest.cc",
"../common/dom_storage/dom_storage_map_unittest.cc",
"../common/input/actions_parser_unittest.cc",
"../common/input/event_with_latency_info_unittest.cc",
"../common/input/gesture_event_stream_validator_unittest.cc",
"../common/input/synthetic_web_input_event_builders_unittest.cc",
......@@ -1688,7 +1689,6 @@ test("content_unittests") {
"../renderer/dom_storage/local_storage_cached_areas_unittest.cc",
"../renderer/dom_storage/mock_leveldb_wrapper.cc",
"../renderer/dom_storage/mock_leveldb_wrapper.h",
"../renderer/gpu/actions_parser_unittest.cc",
"../renderer/gpu/frame_swap_message_queue_unittest.cc",
"../renderer/gpu/layer_tree_view_unittest.cc",
"../renderer/gpu/queue_message_swap_promise_unittest.cc",
......
ui/events/gesture_detection/gesture_provider.cc
View file @ d713bb2e
......@@ -196,8 +196,10 @@ class GestureProvider::GestureListenerImpl : public ScaleGestureListener,
break;
case ET_GESTURE_PINCH_BEGIN:
DCHECK(!pinch_event_sent_);
if (!scroll_event_sent_)
if (!scroll_event_sent_ &&
!scale_gesture_detector_.InAnchoredScaleMode()) {
Send(GestureEventData(ET_GESTURE_SCROLL_BEGIN, gesture));
}
pinch_event_sent_ = true;
break;
case ET_GESTURE_PINCH_END:
......
ui/events/gesture_detection/gesture_provider_unittest.cc
View file @ d713bb2e
......@@ -773,7 +773,6 @@ TEST_F(GestureProviderTest, DoubleTapDragZoomBasic) {
MotionEvent::Action::MOVE, kFakeCoordX,
kFakeCoordY + 100);
EXPECT_TRUE(gesture_provider_->OnTouchEvent(event));
EXPECT_TRUE(HasReceivedGesture(ET_GESTURE_SCROLL_BEGIN));
ASSERT_EQ(ET_GESTURE_PINCH_BEGIN, GetNthMostRecentGestureEventType(1));
ASSERT_EQ(ET_GESTURE_PINCH_UPDATE, GetMostRecentGestureEventType());
EXPECT_LT(1.f, GetMostRecentGestureEvent().details.scale());
......@@ -803,7 +802,7 @@ TEST_F(GestureProviderTest, DoubleTapDragZoomBasic) {
kFakeCoordY - 200);
EXPECT_TRUE(gesture_provider_->OnTouchEvent(event));
EXPECT_TRUE(HasReceivedGesture(ET_GESTURE_PINCH_END));
EXPECT_EQ(ET_GESTURE_SCROLL_END, GetMostRecentGestureEventType());
EXPECT_EQ(ET_GESTURE_PINCH_END, GetMostRecentGestureEventType());
EXPECT_EQ(BoundsForSingleMockTouchAtLocation(kFakeCoordX, kFakeCoordY - 200),
GetMostRecentGestureEvent().details.bounding_box_f());
}
......@@ -1273,7 +1272,7 @@ TEST_F(GestureProviderTest, NoGestureLongPressDuringDoubleTap) {
MotionEvent::Action::UP, kFakeCoordX, kFakeCoordY + 1);
event.SetPrimaryPointerId(motion_event_id);
EXPECT_TRUE(gesture_provider_->OnTouchEvent(event));
EXPECT_EQ(ET_GESTURE_SCROLL_END, GetMostRecentGestureEventType());
EXPECT_EQ(ET_GESTURE_PINCH_END, GetMostRecentGestureEventType());
EXPECT_EQ(motion_event_id, GetMostRecentGestureEvent().motion_event_id);
EXPECT_EQ(1, GetMostRecentGestureEvent().details.touch_points());
EXPECT_FALSE(gesture_provider_->IsDoubleTapInProgress());
......@@ -1641,7 +1640,6 @@ TEST_F(GestureProviderTest, FixedPageScaleDuringDoubleTapDragZoom) {
MotionEvent::Action::MOVE, kFakeCoordX,
kFakeCoordY + 100);
EXPECT_TRUE(gesture_provider_->OnTouchEvent(event));
EXPECT_TRUE(HasReceivedGesture(ET_GESTURE_SCROLL_BEGIN));
EXPECT_EQ(ET_GESTURE_PINCH_BEGIN, GetNthMostRecentGestureEventType(1));
EXPECT_EQ(ET_GESTURE_PINCH_UPDATE, GetMostRecentGestureEventType());
EXPECT_LT(1.f, GetMostRecentGestureEvent().details.scale());
......@@ -1664,7 +1662,7 @@ TEST_F(GestureProviderTest, FixedPageScaleDuringDoubleTapDragZoom) {
kFakeCoordY + 200);
EXPECT_TRUE(gesture_provider_->OnTouchEvent(event));
EXPECT_TRUE(HasReceivedGesture(ET_GESTURE_PINCH_END));
EXPECT_EQ(ET_GESTURE_SCROLL_END, GetMostRecentGestureEventType());
EXPECT_EQ(ET_GESTURE_PINCH_END, GetMostRecentGestureEventType());
EXPECT_EQ(1, GetMostRecentGestureEvent().details.touch_points());
// The double-tap gesture has finished, but the page scale is fixed.
......@@ -2094,7 +2092,6 @@ TEST_F(GestureProviderTest, DoubleTapDragZoomCancelledOnSecondaryPointerDown) {
MotionEvent::Action::MOVE, kFakeCoordX,
kFakeCoordY - 30);
EXPECT_TRUE(gesture_provider_->OnTouchEvent(event));
EXPECT_TRUE(HasReceivedGesture(ET_GESTURE_SCROLL_BEGIN));
EXPECT_EQ(ET_GESTURE_PINCH_BEGIN, GetNthMostRecentGestureEventType(1));
EXPECT_EQ(ET_GESTURE_PINCH_UPDATE, GetMostRecentGestureEventType());
EXPECT_EQ(1, GetMostRecentGestureEvent().details.touch_points());
......@@ -2115,9 +2112,7 @@ TEST_F(GestureProviderTest, DoubleTapDragZoomCancelledOnSecondaryPointerDown) {
event = ObtainMotionEvent(down_time_2 + kOneSecond, MotionEvent::Action::UP);
EXPECT_TRUE(gesture_provider_->OnTouchEvent(event));
EXPECT_EQ(gesture_count + 1, GetReceivedGestureCount());
EXPECT_EQ(ET_GESTURE_SCROLL_END, GetMostRecentGestureEventType());
EXPECT_EQ(1, GetMostRecentGestureEvent().details.touch_points());
EXPECT_EQ(ET_GESTURE_PINCH_END, GetMostRecentGestureEventType());
}
// Verify that gesture begin and gesture end events are dispatched correctly.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment