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

third_party/WebKit/Source/core/input/MouseEventManager.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/MouseEventManager.h"
 
 #include "core/clipboard/DataObject.h"
 #include "core/clipboard/DataTransfer.h"
 #include "core/dom/Element.h"
 #include "core/dom/ElementTraversal.h"
@@ skipping 240 matching lines @@
   const bool should_dispatch_click_event =
       click_count_ > 0 && !context_menu_event && mev.InnerNode() &&
       click_node_ && mev.InnerNode()->CanParticipateInFlatTree() &&
       click_node_->CanParticipateInFlatTree() &&
       !(frame_->GetEventHandler()
             .GetSelectionController()
             .HasExtendedSelection() &&
         IsLinkSelection(mev));
   if (should_dispatch_click_event) {
     Node* click_target_node = nullptr;
-    // Updates distribution because a 'mouseup' event listener can make the
-    // tree dirty at dispatchMouseEvent() invocation above.
-    // Unless distribution is updated, commonAncestor would hit ASSERT.
-    if (click_node_ == mev.InnerNode()) {
+    // A click event should be fired even when a text node has been removed in
+    // mouseup event.
+    if ((mev.InnerNode()->IsTextNode() && click_node_->isConnected() &&

[tkent, 2017/04/11 04:16:14]

I think this code has the following unexpected behavior.

1. Open fast/events/remove-text-node-in-mouseup.html
2. Mousedown on a Text child of the first <p> element
3. Move mouse cursor onto the second <div> element
4. Mouseup 

In this case, click_node_ is <p>, mev.InnerNode() is the removed Text node, and
this code will dispatch 'click' on <p>.

[tkent, 2017/04/11 04:24:15]

Maybe a logic would be:

Node* mouseup_node = mev.InnerNode()->isTextNode() ?
mev.InnerNode()->parentNode() : mev.InnerNode();
if (click_node_ == mouseup_node) {
  ...

+         !mev.InnerNode()->isConnected()) ||

[kochi, 2017/04/11 04:25:00]

(I am not sure and just checking) - Isn't click_node_->isConnected()
already guaranteed somewhere above?  (I vaguely assumed that
click_node_->isConnected() is implied in |should_dispatch_click_event|,
but it seems not.)
If click_node_ is not connected, still click event should be dispatched
to it?

For readability, I prefer the following order:
 "mev.InnerNode()->IsTextNode() && !mev.InnerNode()->isConnected() &&
  click_node_->isConnected()"
but it's up to you.

+        click_node_ == mev.InnerNode()) {

[kochi, 2017/04/11 04:25:00]

I'd guess "click_node_ == mev.InnerNode()" is the more common case,
so in that case the condition may be better checked first?

       click_target_node = click_node_;
-      click_target_node->UpdateDistribution();
     } else if (click_node_->GetDocument() == mev.InnerNode()->GetDocument()) {
+      // Updates distribution because a 'mouseup' event listener can make the
+      // tree dirty at dispatchMouseEvent() invocation above.
+      // Unless distribution is updated, commonAncestor would hit ASSERT.
       click_node_->UpdateDistribution();
       mev.InnerNode()->UpdateDistribution();
       click_target_node = mev.InnerNode()->CommonAncestor(
           *click_node_, EventHandlingUtil::ParentForClickEvent);
     }
     if (click_target_node) {
       click_event_result = DispatchMouseEvent(
           click_target_node,
           !RuntimeEnabledFeatures::auxclickEnabled() ||
                   (mev.Event().button == WebPointerProperties::Button::kLeft)
@@ skipping 761 matching lines @@
 
 void MouseEventManager::SetClickCount(int click_count) {
   click_count_ = click_count;
 }
 
 bool MouseEventManager::MouseDownMayStartDrag() {
   return mouse_down_may_start_drag_;
 }
 
 }  // namespace blink