From 906a3939392344cc829470aa99819b9d559196f5 Mon Sep 17 00:00:00 2001 From: tdanderson Date: Tue, 2 Sep 2014 13:01:27 -0700 Subject: [PATCH] Only target ui::ET_GESTURE_END to the default gesture handler If no gesture handler has been established prior to RootView receiving a ui::ET_GESTURE_END event, do not perform any targeting based on the event's location (and therefore do not dispatch this event to any view); GESTURE_END should only ever be targeted and dispatched to the gesture handler set by a previous gesture. BUG=409918 TEST=ViewTargeterTest.ViewTargeterForGestureEvents, WidgetTest.GestureBeginAndEndEvents, WidgetTest.DisabledGestureEventTarget Review URL: https://codereview.chromium.org/533793002 Cr-Commit-Position: refs/heads/master@{#292983} --- ui/views/view_targeter_unittest.cc | 38 +++++++++++++++++++++------ ui/views/widget/root_view_targeter.cc | 11 ++++++++ ui/views/widget/widget_unittest.cc | 19 +++++++------- 3 files changed, 51 insertions(+), 17 deletions(-) diff --git a/ui/views/view_targeter_unittest.cc b/ui/views/view_targeter_unittest.cc index 6564a8800a79f8..98110382457de6 100644 --- a/ui/views/view_targeter_unittest.cc +++ b/ui/views/view_targeter_unittest.cc @@ -263,7 +263,7 @@ TEST_F(ViewTargeterTest, ViewTargeterForGestureEvents) { static_cast(widget.GetRootView()); ui::EventTargeter* targeter = root_view->targeter(); - // Define a GESTURE_TAP and a GESTURE_SCROLL_BEGIN. + // Define some gesture events for testing. gfx::Rect bounding_box(gfx::Point(46, 46), gfx::Size(8, 8)); gfx::Point center_point(bounding_box.CenterPoint()); ui::GestureEventDetails details(ui::ET_GESTURE_TAP, 0.0f, 0.0f); @@ -272,12 +272,16 @@ TEST_F(ViewTargeterTest, ViewTargeterForGestureEvents) { details = ui::GestureEventDetails(ui::ET_GESTURE_SCROLL_BEGIN, 0.0f, 0.0f); details.set_bounding_box(bounding_box); GestureEventForTest scroll_begin(details, center_point.x(), center_point.y()); + details = ui::GestureEventDetails(ui::ET_GESTURE_END, 0.0f, 0.0f); + details.set_bounding_box(bounding_box); + GestureEventForTest end(details, center_point.x(), center_point.y()); // Assume that the view currently handling gestures has been set as - // |grandchild| by a previous gesture event. Thus subsequent gesture events - // should be initially targeted to |grandchild|, and re-targeting should - // be prohibited for all gesture event types except for GESTURE_SCROLL_BEGIN - // (which should be re-targeted to the parent of |grandchild|). + // |grandchild| by a previous gesture event. Thus subsequent TAP and + // SCROLL_BEGIN events should be initially targeted to |grandchild|, and + // re-targeting should be prohibited for TAP but permitted for + // GESTURE_SCROLL_BEGIN (which should be re-targeted to the parent of + // |grandchild|). SetAllowGestureEventRetargeting(root_view, false); SetGestureHandler(root_view, grandchild); EXPECT_EQ(grandchild, targeter->FindTargetForEvent(root_view, &tap)); @@ -285,15 +289,25 @@ TEST_F(ViewTargeterTest, ViewTargeterForGestureEvents) { EXPECT_EQ(grandchild, targeter->FindTargetForEvent(root_view, &scroll_begin)); EXPECT_EQ(child, targeter->FindNextBestTarget(grandchild, &scroll_begin)); + // GESTURE_END events should be targeted to the existing gesture handler, + // but re-targeting should be prohibited. + EXPECT_EQ(grandchild, targeter->FindTargetForEvent(root_view, &end)); + EXPECT_EQ(NULL, targeter->FindNextBestTarget(grandchild, &tap)); + // Assume that the view currently handling gestures is still set as // |grandchild|, but this was not done by a previous gesture. Thus we are // in the process of finding the View to which subsequent gestures will be - // dispatched, so all gesture events should be re-targeted up the ancestor - // chain. + // dispatched, so TAP and SCROLL_BEGIN events should be re-targeted up + // the ancestor chain. SetAllowGestureEventRetargeting(root_view, true); EXPECT_EQ(child, targeter->FindNextBestTarget(grandchild, &tap)); EXPECT_EQ(child, targeter->FindNextBestTarget(grandchild, &scroll_begin)); + // GESTURE_END events are not permitted to be re-targeted up the ancestor + // chain; they are only ever targeted in the case where the gesture handler + // was established by a previous gesture. + EXPECT_EQ(NULL, targeter->FindNextBestTarget(grandchild, &end)); + // Assume that the default gesture handler was set by the previous gesture, // but that this handler is currently NULL. No gesture events should be // re-targeted in this case (regardless of the view that is passed in to @@ -303,12 +317,20 @@ TEST_F(ViewTargeterTest, ViewTargeterForGestureEvents) { EXPECT_EQ(NULL, targeter->FindNextBestTarget(child, &tap)); EXPECT_EQ(NULL, targeter->FindNextBestTarget(NULL, &tap)); EXPECT_EQ(NULL, targeter->FindNextBestTarget(content, &scroll_begin)); + EXPECT_EQ(NULL, targeter->FindNextBestTarget(content, &end)); // If no default gesture handler is currently set, targeting should be - // performed using the location of the gesture event. + // performed using the location of the gesture event for a TAP and a + // SCROLL_BEGIN. SetAllowGestureEventRetargeting(root_view, true); EXPECT_EQ(grandchild, targeter->FindTargetForEvent(root_view, &tap)); EXPECT_EQ(grandchild, targeter->FindTargetForEvent(root_view, &scroll_begin)); + + // If no default gesture handler is currently set, GESTURE_END events + // should never be targeted or re-targeted to any View. + EXPECT_EQ(NULL, targeter->FindTargetForEvent(root_view, &end)); + EXPECT_EQ(NULL, targeter->FindNextBestTarget(NULL, &end)); + EXPECT_EQ(NULL, targeter->FindNextBestTarget(child, &end)); } // Tests that the functions ViewTargeterDelegate::DoesIntersectRect() diff --git a/ui/views/widget/root_view_targeter.cc b/ui/views/widget/root_view_targeter.cc index 3c4b9e21c40587..78e007d988a72e 100644 --- a/ui/views/widget/root_view_targeter.cc +++ b/ui/views/widget/root_view_targeter.cc @@ -31,6 +31,11 @@ View* RootViewTargeter::FindTargetForGestureEvent( return root_view_->gesture_handler_; } + // If no default gesture handler has already been set, do not perform any + // targeting for a ET_GESTURE_END event. + if (gesture.type() == ui::ET_GESTURE_END) + return NULL; + // If rect-based targeting is enabled, use the gesture's bounding box to // determine the target. Otherwise use the center point of the gesture's // bounding box to determine the target. @@ -49,6 +54,12 @@ View* RootViewTargeter::FindTargetForGestureEvent( ui::EventTarget* RootViewTargeter::FindNextBestTargetForGestureEvent( ui::EventTarget* previous_target, const ui::GestureEvent& gesture) { + // ET_GESTURE_END events should only ever be targeted to the default + // gesture handler set by a previous gesture, if one exists. Thus we do not + // permit any re-targeting of ET_GESTURE_END events. + if (gesture.type() == ui::ET_GESTURE_END) + return NULL; + // GESTURE_SCROLL_BEGIN events are always permitted to be re-targeted, even // if |allow_gesture_event_retargeting_| is false. if (!root_view_->allow_gesture_event_retargeting_ && diff --git a/ui/views/widget/widget_unittest.cc b/ui/views/widget/widget_unittest.cc index 96bf01207e478d..6cc7d1aaeb0aea 100644 --- a/ui/views/widget/widget_unittest.cc +++ b/ui/views/widget/widget_unittest.cc @@ -1973,7 +1973,7 @@ class GestureEventForTest : public ui::GestureEvent { // after the dispatch of a ui::ET_GESTURE_END event corresponding to // the release of the final touch point on the screen and that // ui::ET_GESTURE_END events corresponding to the removal of any other touch -// point are never dispatched to a view. Also verifies that +// point are never dispatched to a view. Also verifies that // ui::ET_GESTURE_BEGIN is never dispatched to a view and does not change the // value of |gesture_handler_|. TEST_F(WidgetTest, GestureBeginAndEndEvents) { @@ -2025,11 +2025,12 @@ TEST_F(WidgetTest, GestureBeginAndEndEvents) { // If no gesture handler is set, dispatching only a ui::ET_GESTURE_END // event corresponding to the final touch point should not set the gesture - // handler, but it should be marked as handled because it was dispatched to - // the view targeted by the event's location. + // handler. Furthermore, it should not be marked as handled because it was + // not dispatched (GESTURE_END events are only dispatched in cases where + // a gesture handler is already set). end = GestureEventForTest(ui::ET_GESTURE_END, 15, 15); widget->OnGestureEvent(&end); - EXPECT_TRUE(end.handled()); + EXPECT_FALSE(end.handled()); EXPECT_EQ(NULL, GetGestureHandler(root_view)); // If the gesture handler has been set by a previous gesture, then it should @@ -2387,17 +2388,17 @@ TEST_F(WidgetTest, DisabledGestureEventTarget) { v3->SetEnabled(false); // No gesture handler is set in the root view, so it should remain unset - // after a GESTURE_END. The event is first dispatched to |v4| (which does - // not want to handle the event) and is then targeted at |v3|. Since |v3| - // is disabled, the event is not dispatched but is still marked as handled. + // after a GESTURE_END. GESTURE_END events are not dispatched unless + // a gesture handler is already set in the root view, so none of the + // views should see this event and it should not be marked as handled. GestureEventForTest end(ui::ET_GESTURE_END, 5, 5); widget->OnGestureEvent(&end); EXPECT_EQ(0, v1->GetEventCount(ui::ET_GESTURE_END)); EXPECT_EQ(0, v2->GetEventCount(ui::ET_GESTURE_END)); EXPECT_EQ(0, v3->GetEventCount(ui::ET_GESTURE_END)); - EXPECT_EQ(1, v4->GetEventCount(ui::ET_GESTURE_END)); + EXPECT_EQ(0, v4->GetEventCount(ui::ET_GESTURE_END)); EXPECT_EQ(NULL, GetGestureHandler(root_view)); - EXPECT_TRUE(end.handled()); + EXPECT_FALSE(end.handled()); v1->ResetCounts(); v2->ResetCounts(); v3->ResetCounts();