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

@@ -263,7 +263,7 @@ TEST_F(ViewTargeterTest, ViewTargeterForGestureEvents) {
       static_cast<internal::RootView*>(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()

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_ &&

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();