Fix the wrong non-element node handling in EventHanlder and MouseEventManager

third_party/WebKit/Source/core/input/GestureManager.cpp

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/input/GestureManager.h"
 
 #include "core/dom/Document.h"
 #include "core/dom/DocumentUserGestureToken.h"
 #include "core/editing/SelectionController.h"
 #include "core/events/GestureEvent.h"
@@ skipping 167 matching lines @@
     LocalFrame* main_frame = frame_->LocalFrameRoot();
     if (main_frame && main_frame->View())
       main_frame->View()->UpdateLifecycleToCompositingCleanPlusScrolling();
     adjusted_point = frame_view->RootFrameToContents(
         FlooredIntPoint(gesture_event.PositionInRootFrame()));
     current_hit_test = EventHandlingUtil::HitTestResultInFrame(
         frame_, adjusted_point, hit_type);
   }
 
   // Capture data for showUnhandledTapUIIfNeeded.
-  Node* tapped_node = current_hit_test.InnerNode();
   IntPoint tapped_position =
       FlooredIntPoint(gesture_event.PositionInRootFrame());
-  Node* tapped_non_text_node = tapped_node;
   UserGestureIndicator gesture_indicator(DocumentUserGestureToken::Create(
-      tapped_node ? &tapped_node->GetDocument() : nullptr));
+      current_hit_test.InnerNode()
+          ? &current_hit_test.InnerNode()->GetDocument()
+          : nullptr));
 
-  if (tapped_non_text_node && tapped_non_text_node->IsTextNode())
-    tapped_non_text_node = FlatTreeTraversal::Parent(*tapped_non_text_node);
-
-  mouse_event_manager_->SetClickNode(tapped_non_text_node);
+  Node* tapped_node = current_hit_test.InnerNode();
+  Element* tapped_element =
+      EventHandlingUtil::ParentElementIfNeeded(tapped_node);
+  mouse_event_manager_->SetClickElement(tapped_element);
 
   WebMouseEvent fake_mouse_down(
       WebInputEvent::kMouseDown, gesture_event,
       WebPointerProperties::Button::kLeft, gesture_event.TapCount(),
       static_cast<WebInputEvent::Modifiers>(
           modifiers | WebInputEvent::Modifiers::kLeftButtonDown |
           WebInputEvent::Modifiers::kIsCompatibilityEventForTouch),
       gesture_event.TimeStampSeconds());
 
   // TODO(mustaq): We suppress MEs plus all it's side effects. What would that
@@ skipping 46 matching lines @@
           modifiers | WebInputEvent::Modifiers::kIsCompatibilityEventForTouch),
       gesture_event.TimeStampSeconds());
   WebInputEventResult mouse_up_event_result =
       suppress_mouse_events_from_gestures_
           ? WebInputEventResult::kHandledSuppressed
           : mouse_event_manager_->SetMousePositionAndDispatchMouseEvent(
                 current_hit_test.InnerNode(), current_hit_test.CanvasRegionId(),
                 EventTypeNames::mouseup, fake_mouse_up);
 
   WebInputEventResult click_event_result = WebInputEventResult::kNotHandled;
-  if (tapped_non_text_node) {
+  if (tapped_element) {
     if (current_hit_test.InnerNode()) {
       // Updates distribution because a mouseup (or mousedown) event listener
       // can make the tree dirty at dispatchMouseEvent() invocation above.
       // Unless distribution is updated, commonAncestor would hit DCHECK.  Both
       // tappedNonTextNode and currentHitTest.innerNode()) don't need to be
       // updated because commonAncestor() will exit early if their documents are
       // different.
-      tapped_non_text_node->UpdateDistribution();
+      tapped_element->UpdateDistribution();
       Node* click_target_node = current_hit_test.InnerNode()->CommonAncestor(
-          *tapped_non_text_node, EventHandlingUtil::ParentForClickEvent);
+          *tapped_element, EventHandlingUtil::ParentForClickEvent);
       click_event_result =
           mouse_event_manager_->SetMousePositionAndDispatchMouseEvent(
               click_target_node, String(), EventTypeNames::click,
               fake_mouse_up);
     }
-    mouse_event_manager_->SetClickNode(nullptr);
+    mouse_event_manager_->SetClickElement(nullptr);
   }
 
   if (mouse_up_event_result == WebInputEventResult::kNotHandled)
     mouse_up_event_result = mouse_event_manager_->HandleMouseReleaseEvent(
         MouseEventWithHitTestResults(fake_mouse_up, current_hit_test));
   mouse_event_manager_->ClearDragHeuristicState();
 
   WebInputEventResult event_result = EventHandlingUtil::MergeEventResult(
       EventHandlingUtil::MergeEventResult(mouse_down_event_result,
                                           mouse_up_event_result),
       click_event_result);
-  if (event_result == WebInputEventResult::kNotHandled && tapped_node &&
+  if (event_result == WebInputEventResult::kNotHandled && tapped_element &&

[Donn Denman, 2017/04/12 17:12:19]

Nit: I wonder if it would be better to test the tapped_node here instead of the
tapped_element since that's what we're going to pass to the client.  I suppose
it's not possible for one to be null without the other right now, but seems
better and cleaner the other way.

[hayato, 2017/04/13 05:01:40]

Nice catch. tapped_node should be better.
Done.

       frame_->GetPage()) {
     bool dom_tree_changed = pre_dispatch_dom_tree_version !=
                             frame_->GetDocument()->DomTreeVersion();
     bool style_changed =
         pre_dispatch_style_version != frame_->GetDocument()->StyleVersion();
 
     IntPoint tapped_position_in_viewport =
         frame_->GetPage()->GetVisualViewport().RootFrameToViewport(
             tapped_position);
     frame_->GetChromeClient().ShowUnhandledTapUIIfNeeded(
@@ skipping 128 matching lines @@
   }
   return WebInputEventResult::kNotHandled;
 }
 
 WTF::Optional<WTF::TimeTicks> GestureManager::GetLastShowPressTimestamp()
     const {
   return last_show_press_timestamp_;
 }
 
 }  // namespace blink