Stop discarding events to disabled views

Currently, events that would be dispatched to a disabled view are
discarded instead. This seems to be a holdover from old event handling
code and not necessary anymore.

To allow pre-target handlers to still see these events, this CL changes
these methods to dispatch the events to the closest enabled ancestor
view.

Bug: 1043374, 1011082
Change-Id: I82bca238e98b9b820c0c7619a5880fbf3497d886
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2029087
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744102}

ui/views/view_targeter_delegate.cc

@@ -46,7 +46,7 @@ View* ViewTargeterDelegate::TargetForRect(View* root, const gfx::Rect& rect) {
   View::Views children = root->GetChildrenInZOrder();
   DCHECK_EQ(root->children().size(), children.size());
   for (auto* child : base::Reversed(children)) {
-    if (!child->CanProcessEventsWithinSubtree())
+    if (!child->CanProcessEventsWithinSubtree() || !child->GetEnabled())
       continue;
 
     // Ignore any children which are invisible or do not intersect |rect|.

ui/views/view_unittest.cc

@@ -5403,6 +5403,27 @@ TEST_F(ViewTest, TestVisibleChangedCallback) {
   EXPECT_FALSE(test_view.GetVisible());
 }
 
+TEST_F(ViewTest, TooltipShowsForDisabledView) {
+  auto widget = std::make_unique<Widget>();
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
+  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  widget->Init(std::move(params));
+  widget->SetBounds(gfx::Rect(0, 0, 100, 100));
+
+  View enabled_parent;
+  View* const disabled_child =
+      enabled_parent.AddChildView(std::make_unique<View>());
+  disabled_child->SetEnabled(false);
+
+  enabled_parent.SetBoundsRect(gfx::Rect(0, 0, 100, 100));
+  disabled_child->SetBoundsRect(gfx::Rect(0, 0, 100, 100));
+  widget->GetContentsView()->AddChildView(&enabled_parent);
+  widget->Show();
+
+  EXPECT_EQ(disabled_child,
+            enabled_parent.GetTooltipHandlerForPoint(gfx::Point(50, 50)));
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Observer tests.
 ////////////////////////////////////////////////////////////////////////////////

ui/views/widget/root_view.cc

@@ -141,6 +141,7 @@ class PostEventDispatchHandler : public ui::EventHandler {
       return;
 
     View* target = static_cast<View*>(event->target());
+
     gfx::Point location = event->location();
     if (touch_dnd_enabled_ && event->type() == ui::ET_GESTURE_LONG_PRESS &&
         (!target->drag_controller() ||
@@ -379,18 +380,14 @@ bool RootView::OnMousePressed(const ui::MouseEvent& event) {
   }
   DCHECK(!explicit_mouse_handler_);
 
-  bool hit_disabled_view = false;
-  // Walk up the tree until we find a view that wants the mouse event.
+  // Walk up the tree from the target until we find a view that wants
+  // the mouse event.
   for (mouse_pressed_handler_ = GetEventHandlerForPoint(event.location());
        mouse_pressed_handler_ && (mouse_pressed_handler_ != this);
        mouse_pressed_handler_ = mouse_pressed_handler_->parent()) {
     DVLOG(1) << "OnMousePressed testing "
              << mouse_pressed_handler_->GetClassName();
-    if (!mouse_pressed_handler_->GetEnabled()) {
-      // Disabled views should eat events instead of propagating them upwards.
-      hit_disabled_view = true;
-      break;
-    }
+    DCHECK(mouse_pressed_handler_->GetEnabled());
 
     // See if this view wants to handle the mouse press.
     ui::MouseEvent mouse_pressed_event(event, static_cast<View*>(this),
@@ -430,16 +427,13 @@ bool RootView::OnMousePressed(const ui::MouseEvent& event) {
 
   // Reset mouse_pressed_handler_ to indicate that no processing is occurring.
   mouse_pressed_handler_ = nullptr;
+  last_click_handler_ = nullptr;
 
   // In the event that a double-click is not handled after traversing the
   // entire hierarchy (even as a single-click when sent to a different view),
   // it must be marked as handled to avoid anything happening from default
   // processing if it the first click-part was handled by us.
-  if (last_click_handler_ && (event.flags() & ui::EF_IS_DOUBLE_CLICK))
-    hit_disabled_view = true;
-
-  last_click_handler_ = nullptr;
-  return hit_disabled_view;
+  return event.flags() & ui::EF_IS_DOUBLE_CLICK;
 }
 
 bool RootView::OnMouseDragged(const ui::MouseEvent& event) {
@@ -500,12 +494,14 @@ void RootView::OnMouseCaptureLost() {
 
 void RootView::OnMouseMoved(const ui::MouseEvent& event) {
   View* v = GetEventHandlerForPoint(event.location());
-  // Find the first enabled view, or the existing move handler, whichever comes
-  // first.  The check for the existing handler is because if a view becomes
-  // disabled while handling moves, it's wrong to suddenly send ET_MOUSE_EXITED
-  // and ET_MOUSE_ENTERED events, because the mouse hasn't actually exited yet.
-  while (v && !v->GetEnabled() && (v != mouse_move_handler_))
-    v = v->parent();
+  // Check for a disabled move handler. If the move handler became
+  // disabled while handling moves, it's wrong to suddenly send
+  // ET_MOUSE_EXITED and ET_MOUSE_ENTERED events, because the mouse
+  // hasn't actually exited yet.
+  if (mouse_move_handler_ && !mouse_move_handler_->GetEnabled() &&
+      v->Contains(mouse_move_handler_))
+    v = mouse_move_handler_;
+
   if (v && v != this) {
     if (v != mouse_move_handler_) {
       if (mouse_move_handler_ != nullptr &&
@@ -763,12 +759,6 @@ ui::EventDispatchDetails RootView::PreDispatchEvent(ui::EventTarget* target,
     //                   |gesture_handler_| to detect if the view has been
     //                   removed from the tree.
     gesture_handler_ = view;
-
-    // Disabled views are permitted to be targets of gesture events, but
-    // gesture events should never actually be dispatched to them. Prevent
-    // dispatch by marking the event as handled.
-    if (!view->GetEnabled())
-      event->SetHandled();
   }
 
   old_dispatch_target_ = event_dispatch_target_;

ui/views/widget/root_view_unittest.cc

@@ -12,6 +12,7 @@
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/dom/dom_code.h"
 #include "ui/views/context_menu_controller.h"
+#include "ui/views/test/test_views.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/view_targeter.h"
 #include "ui/views/widget/root_view.h"
@@ -803,5 +804,60 @@ TEST_F(RootViewTest, AnnounceTextTest) {
 
 #endif  // !defined(OS_MACOSX)
 
+TEST_F(RootViewTest, MouseEventDispatchedToClosestEnabledView) {
+  Widget widget;
+  Widget::InitParams init_params =
+      CreateParams(Widget::InitParams::TYPE_WINDOW_FRAMELESS);
+  init_params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  init_params.bounds = {100, 100, 100, 100};
+  widget.Init(std::move(init_params));
+  widget.Show();
+  internal::RootView* root_view =
+      static_cast<internal::RootView*>(widget.GetRootView());
+  root_view->SetContentsView(new View());
+
+  View* const contents_view = root_view->GetContentsView();
+  EventCountView* const v1 =
+      contents_view->AddChildView(std::make_unique<EventCountView>());
+  EventCountView* const v2 =
+      v1->AddChildView(std::make_unique<EventCountView>());
+  EventCountView* const v3 =
+      v2->AddChildView(std::make_unique<EventCountView>());
+
+  contents_view->SetBoundsRect(gfx::Rect(0, 0, 10, 10));
+  v1->SetBoundsRect(gfx::Rect(0, 0, 10, 10));
+  v2->SetBoundsRect(gfx::Rect(0, 0, 10, 10));
+  v3->SetBoundsRect(gfx::Rect(0, 0, 10, 10));
+
+  v1->set_handle_mode(EventCountView::CONSUME_EVENTS);
+  v2->set_handle_mode(EventCountView::CONSUME_EVENTS);
+  v3->set_handle_mode(EventCountView::CONSUME_EVENTS);
+
+  ui::MouseEvent pressed_event(ui::ET_MOUSE_PRESSED, gfx::Point(5, 5),
+                               gfx::Point(5, 5), ui::EventTimeForNow(), 0, 0);
+  ui::MouseEvent released_event(ui::ET_MOUSE_RELEASED, gfx::Point(5, 5),
+                                gfx::Point(5, 5), ui::EventTimeForNow(), 0, 0);
+  root_view->OnMousePressed(pressed_event);
+  root_view->OnMouseReleased(released_event);
+  EXPECT_EQ(0, v1->GetEventCount(ui::ET_MOUSE_PRESSED));
+  EXPECT_EQ(0, v2->GetEventCount(ui::ET_MOUSE_PRESSED));
+  EXPECT_EQ(1, v3->GetEventCount(ui::ET_MOUSE_PRESSED));
+
+  v3->SetEnabled(false);
+  root_view->OnMousePressed(pressed_event);
+  root_view->OnMouseReleased(released_event);
+  EXPECT_EQ(0, v1->GetEventCount(ui::ET_MOUSE_PRESSED));
+  EXPECT_EQ(1, v2->GetEventCount(ui::ET_MOUSE_PRESSED));
+  EXPECT_EQ(1, v3->GetEventCount(ui::ET_MOUSE_PRESSED));
+
+  v3->SetEnabled(true);
+  v2->SetEnabled(false);
+  root_view->OnMousePressed(pressed_event);
+  root_view->OnMouseReleased(released_event);
+  EXPECT_EQ(1, v1->GetEventCount(ui::ET_MOUSE_PRESSED));
+  EXPECT_EQ(1, v2->GetEventCount(ui::ET_MOUSE_PRESSED));
+  EXPECT_EQ(1, v3->GetEventCount(ui::ET_MOUSE_PRESSED));
+}
+
 }  // namespace test
 }  // namespace views

ui/views/widget/widget_unittest.cc

@@ -2992,125 +2992,6 @@ TEST_F(WidgetTest, GestureEventLocationWhileBubbling) {
   widget->Close();
 }
 
-// Verifies that disabled views are permitted to be set as the default gesture
-// handler in RootView. Also verifies that gesture events targeted to a disabled
-// view are not actually dispatched to the view, but are still marked as
-// handled.
-TEST_F(WidgetTest, DisabledGestureEventTarget) {
-  Widget* widget = CreateTopLevelNativeWidget();
-  widget->SetBounds(gfx::Rect(0, 0, 300, 300));
-
-  // Define a hierarchy of four views (coordinates are in
-  // their parent coordinate space).
-  // v1 (0, 0, 300, 300)
-  //   v2 (0, 0, 100, 100)
-  //     v3 (0, 0, 50, 50)
-  //       v4(0, 0, 10, 10)
-  EventCountView* v1 = new EventCountView();
-  v1->SetBounds(0, 0, 300, 300);
-  EventCountView* v2 = new EventCountView();
-  v2->SetBounds(0, 0, 100, 100);
-  EventCountView* v3 = new EventCountView();
-  v3->SetBounds(0, 0, 50, 50);
-  EventCountView* v4 = new EventCountView();
-  v4->SetBounds(0, 0, 10, 10);
-  internal::RootView* root_view =
-      static_cast<internal::RootView*>(widget->GetRootView());
-  root_view->AddChildView(v1);
-  v1->AddChildView(v2);
-  v2->AddChildView(v3);
-  v3->AddChildView(v4);
-
-  widget->Show();
-
-  // |v1|, |v2|, and |v3| all handle gesture events but |v3| is marked as
-  // disabled.
-  v1->set_handle_mode(EventCountView::CONSUME_EVENTS);
-  v2->set_handle_mode(EventCountView::CONSUME_EVENTS);
-  v3->set_handle_mode(EventCountView::CONSUME_EVENTS);
-  v3->SetEnabled(false);
-
-  // No gesture handler is set in the root view. In this case the tap event
-  // should be dispatched only to |v4|, the gesture handler should be set to
-  // |v3|, and the event should be marked as handled.
-  GestureEventForTest tap(ui::ET_GESTURE_TAP, 5, 5);
-  widget->OnGestureEvent(&tap);
-  EXPECT_EQ(0, v1->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(0, v2->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(0, v3->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(1, v4->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(v3, GetGestureHandler(root_view));
-  EXPECT_TRUE(tap.handled());
-  v1->ResetCounts();
-  v2->ResetCounts();
-  v3->ResetCounts();
-  v4->ResetCounts();
-
-  // A subsequent gesture event should be marked as handled but not dispatched.
-  tap = GestureEventForTest(ui::ET_GESTURE_TAP, 5, 5);
-  widget->OnGestureEvent(&tap);
-  EXPECT_EQ(0, v1->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(0, v2->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(0, v3->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(0, v4->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(v3, GetGestureHandler(root_view));
-  EXPECT_TRUE(tap.handled());
-  v1->ResetCounts();
-  v2->ResetCounts();
-  v3->ResetCounts();
-  v4->ResetCounts();
-
-  // A GESTURE_END should reset the default gesture handler to NULL. It should
-  // also not be dispatched to |v3| but still 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(0, v4->GetEventCount(ui::ET_GESTURE_END));
-  EXPECT_EQ(nullptr, GetGestureHandler(root_view));
-  EXPECT_TRUE(end.handled());
-  v1->ResetCounts();
-  v2->ResetCounts();
-  v3->ResetCounts();
-  v4->ResetCounts();
-
-  // Change the handle mode of |v3| to indicate that it would no longer like
-  // to handle events which are dispatched to it.
-  v3->set_handle_mode(EventCountView::PROPAGATE_EVENTS);
-
-  // No gesture handler is set in the root view. In this case the tap event
-  // should be dispatched only to |v4| and the event should be marked as
-  // handled. Furthermore, the gesture handler should be set to
-  // |v3|; even though |v3| does not explicitly handle events, it is a
-  // valid target for the tap event because it is disabled.
-  tap = GestureEventForTest(ui::ET_GESTURE_TAP, 5, 5);
-  widget->OnGestureEvent(&tap);
-  EXPECT_EQ(0, v1->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(0, v2->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(0, v3->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(1, v4->GetEventCount(ui::ET_GESTURE_TAP));
-  EXPECT_EQ(v3, GetGestureHandler(root_view));
-  EXPECT_TRUE(tap.handled());
-  v1->ResetCounts();
-  v2->ResetCounts();
-  v3->ResetCounts();
-  v4->ResetCounts();
-
-  // A GESTURE_END should reset the default gesture handler to NULL. It should
-  // also not be dispatched to |v3| but still marked as handled.
-  end = GestureEventForTest(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(0, v4->GetEventCount(ui::ET_GESTURE_END));
-  EXPECT_EQ(nullptr, GetGestureHandler(root_view));
-  EXPECT_TRUE(end.handled());
-
-  widget->Close();
-}
-
 // Test the result of Widget::GetAllChildWidgets().
 TEST_F(WidgetTest, GetAllChildWidgets) {
   // Create the following widget hierarchy: