Commit 85e1871d authored by hayato's avatar hayato Committed by Commit bot

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

{EventHandler,MouseEventManager} is storing a {clicked,tapped} node as Node*,
instead of Element*, that would be a bad practice. Actually, that has been the
cause of several wrong behaviors in Blink, such as:

1. Blink does NOT fire a click event when a clicked text node has been removed
   in mouseup.  This is wrong because a click event should be fired on an
   element node.  See [1] for details. This bug was just tentatively fixed at
   another CL [2].

2. Blink DOES fire a touch event on a non-element node.  This is wrong because
   TouchEvent specification says all kinds of touch events' event target types
  are limited to Document and Element [3].

To prevent these wrong behaviors, this CL does:
- Have EventHandlingUtil::ParentElementIfNeeded and use it everywhere so that
  we surely adjust a node to the appropriate element, at which an event should be
  fired.
- Replaces the usage of Node* to Element* as much as possible so that it rejects
  the wrong code at type level check.

- [1]: topmost event target;
  https://www.w3.org/TR/uievents/#topmost-event-target
- [2] https://codereview.chromium.org/2812613004
- [3]: Trusted proximal event target types are: Document and Element;
  https://w3c.github.io/touch-events/#list-of-touchevent-types

BUG=708394,710425

Review-Url: https://codereview.chromium.org/2807123002
Cr-Commit-Position: refs/heads/master@{#464344}
parent 3b8cd561
......@@ -13,7 +13,6 @@ Received mousemove in child frame
Adding scrollbars to iframe
Received mousedown in child frame
Received mouseup in child frame
Received click in child frame
Test case: Add scrollbars during mousedown
Sending GestureTap
......
......@@ -651,10 +651,8 @@ WebInputEventResult EventHandler::HandleMousePressEvent(
}
mouse_event_manager_->SetClickCount(mouse_event.click_count);
mouse_event_manager_->SetClickNode(
mev.InnerNode()->IsTextNode()
? FlatTreeTraversal::Parent(*mev.InnerNode())
: mev.InnerNode());
mouse_event_manager_->SetClickElement(
EventHandlingUtil::ParentElementIfNeeded(mev.InnerNode()));
if (!mouse_event.FromTouch())
frame_->Selection().SetCaretBlinkingSuspended(true);
......@@ -959,9 +957,8 @@ WebInputEventResult EventHandler::HandleMouseReleaseEvent(
HitTestRequest request(hit_type);
MouseEventWithHitTestResults mev =
EventHandlingUtil::PerformMouseEventHitTest(frame_, request, mouse_event);
Node* release_node = (mev.InnerNode() && mev.InnerNode()->IsTextNode())
? FlatTreeTraversal::Parent(*mev.InnerNode())
: mev.InnerNode();
Element* mouse_release_target =
EventHandlingUtil::ParentElementIfNeeded(mev.InnerNode());
LocalFrame* subframe =
capturing_mouse_events_node_.Get()
? SubframeForTargetNode(capturing_mouse_events_node_.Get())
......@@ -994,7 +991,9 @@ WebInputEventResult EventHandler::HandleMouseReleaseEvent(
mev.Event(), Vector<WebMouseEvent>());
WebInputEventResult click_event_result =
mouse_event_manager_->DispatchMouseClickIfNeeded(mev, release_node);
mouse_release_target ? mouse_event_manager_->DispatchMouseClickIfNeeded(
mev, *mouse_release_target)
: WebInputEventResult::kNotHandled;
scroll_manager_->ClearResizeScrollableArea(false);
......
......@@ -92,6 +92,14 @@ ScrollableArea* AssociatedScrollableArea(const PaintLayer* layer) {
return nullptr;
}
Element* ParentElementIfNeeded(Node* node) {
if (!node)
return nullptr;
if (node->IsElementNode())
return ToElement(node);
return FlatTreeTraversal::ParentElement(*node);
}
ContainerNode* ParentForClickEvent(const Node& node) {
// IE doesn't dispatch click events for mousedown/mouseup events across form
// controls.
......
......@@ -31,6 +31,7 @@ WebInputEventResult ToWebInputEventResult(DispatchEventResult);
PaintLayer* LayerForNode(Node*);
ScrollableArea* AssociatedScrollableArea(const PaintLayer*);
Element* ParentElementIfNeeded(Node*);
ContainerNode* ParentForClickEvent(const Node&);
LayoutPoint ContentPointFromRootFrame(LocalFrame*,
......
......@@ -185,17 +185,15 @@ WebInputEventResult GestureManager::HandleGestureTap(
}
// 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;
Node* tapped_node = current_hit_test.InnerNode();
Element* tapped_element =
EventHandlingUtil::ParentElementIfNeeded(tapped_node);
UserGestureIndicator gesture_indicator(DocumentUserGestureToken::Create(
tapped_node ? &tapped_node->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);
mouse_event_manager_->SetClickElement(tapped_element);
WebMouseEvent fake_mouse_down(
WebInputEvent::kMouseDown, gesture_event,
......@@ -262,7 +260,7 @@ WebInputEventResult GestureManager::HandleGestureTap(
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.
......@@ -270,15 +268,15 @@ WebInputEventResult GestureManager::HandleGestureTap(
// 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)
......
......@@ -94,7 +94,7 @@ void MouseEventManager::Clear() {
last_known_mouse_global_position_ = IntPoint();
mouse_pressed_ = false;
click_count_ = 0;
click_node_ = nullptr;
click_element_ = nullptr;
mouse_down_pos_ = IntPoint();
mouse_down_timestamp_ = TimeTicks();
mouse_down_ = WebMouseEvent();
......@@ -111,7 +111,7 @@ DEFINE_TRACE(MouseEventManager) {
visitor->Trace(scroll_manager_);
visitor->Trace(node_under_mouse_);
visitor->Trace(mouse_press_node_);
visitor->Trace(click_node_);
visitor->Trace(click_element_);
SynchronousMutationObserver::Trace(visitor);
}
......@@ -233,7 +233,7 @@ WebInputEventResult MouseEventManager::SetMousePositionAndDispatchMouseEvent(
WebInputEventResult MouseEventManager::DispatchMouseClickIfNeeded(
const MouseEventWithHitTestResults& mev,
Node* release_node) {
Element& mouse_release_target) {
// We only prevent click event when the click may cause contextmenu to popup.
// However, we always send auxclick.
bool context_menu_event =
......@@ -248,39 +248,39 @@ WebInputEventResult MouseEventManager::DispatchMouseClickIfNeeded(
context_menu_event = true;
#endif
WebInputEventResult click_event_result = WebInputEventResult::kNotHandled;
const bool should_dispatch_click_event =
click_count_ > 0 && !context_menu_event && release_node && click_node_ &&
release_node->CanParticipateInFlatTree() &&
click_node_->CanParticipateInFlatTree() &&
click_count_ > 0 && !context_menu_event && click_element_ &&
mouse_release_target.CanParticipateInFlatTree() &&
click_element_->CanParticipateInFlatTree() &&
!(frame_->GetEventHandler()
.GetSelectionController()
.HasExtendedSelection() &&
IsLinkSelection(mev));
if (should_dispatch_click_event) {
Node* click_target_node = nullptr;
if (click_node_ == release_node) {
click_target_node = click_node_;
} else if (click_node_->GetDocument() == release_node->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();
release_node->UpdateDistribution();
click_target_node = release_node->CommonAncestor(
*click_node_, EventHandlingUtil::ParentForClickEvent);
}
if (click_target_node) {
click_event_result = DispatchMouseEvent(
click_target_node,
!RuntimeEnabledFeatures::auxclickEnabled() ||
(mev.Event().button == WebPointerProperties::Button::kLeft)
? EventTypeNames::click
: EventTypeNames::auxclick,
mev.Event(), mev.CanvasRegionId(), nullptr);
}
if (!should_dispatch_click_event)
return WebInputEventResult::kNotHandled;
Node* click_target_node = nullptr;
if (click_element_ == mouse_release_target) {
click_target_node = click_element_;
} else if (click_element_->GetDocument() ==
mouse_release_target.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_element_->UpdateDistribution();
mouse_release_target.UpdateDistribution();
click_target_node = mouse_release_target.CommonAncestor(
*click_element_, EventHandlingUtil::ParentForClickEvent);
}
return click_event_result;
if (!click_target_node)
return WebInputEventResult::kNotHandled;
return DispatchMouseEvent(
click_target_node,
!RuntimeEnabledFeatures::auxclickEnabled() ||
(mev.Event().button == WebPointerProperties::Button::kLeft)
? EventTypeNames::click
: EventTypeNames::auxclick,
mev.Event(), mev.CanvasRegionId(), nullptr);
}
void MouseEventManager::FakeMouseMoveEventTimerFired(TimerBase* timer) {
......@@ -381,19 +381,19 @@ void MouseEventManager::SetNodeUnderMouse(
}
void MouseEventManager::NodeChildrenWillBeRemoved(ContainerNode& container) {
if (container == click_node_)
if (container == click_element_)
return;
if (!container.IsShadowIncludingInclusiveAncestorOf(click_node_.Get()))
if (!container.IsShadowIncludingInclusiveAncestorOf(click_element_.Get()))
return;
click_node_ = nullptr;
click_element_ = nullptr;
}
void MouseEventManager::NodeWillBeRemoved(Node& node_to_be_removed) {
if (node_to_be_removed.IsShadowIncludingInclusiveAncestorOf(
click_node_.Get())) {
click_element_.Get())) {
// We don't dispatch click events if the mousedown node is removed
// before a mouseup event. It is compatible with IE and Firefox.
click_node_ = nullptr;
click_element_ = nullptr;
}
}
......@@ -1005,7 +1005,7 @@ bool MouseEventManager::HandleSvgPanIfNeeded(bool is_release_event) {
void MouseEventManager::InvalidateClick() {
click_count_ = 0;
click_node_ = nullptr;
click_element_ = nullptr;
}
bool MouseEventManager::MousePressed() {
......@@ -1032,9 +1032,9 @@ void MouseEventManager::SetMousePressNode(Node* node) {
mouse_press_node_ = node;
}
void MouseEventManager::SetClickNode(Node* node) {
SetContext(node ? node->ownerDocument() : nullptr);
click_node_ = node;
void MouseEventManager::SetClickElement(Element* element) {
SetContext(element ? element->ownerDocument() : nullptr);
click_element_ = element;
}
void MouseEventManager::SetClickCount(int click_count) {
......
......@@ -58,7 +58,7 @@ class CORE_EXPORT MouseEventManager final
WebInputEventResult DispatchMouseClickIfNeeded(
const MouseEventWithHitTestResults&,
Node* release_node);
Element& mouse_release_target);
WebInputEventResult DispatchDragSrcEvent(const AtomicString& event_type,
const WebMouseEvent&);
......@@ -139,7 +139,7 @@ class CORE_EXPORT MouseEventManager final
Node* MousePressNode();
void SetMousePressNode(Node*);
void SetClickNode(Node*);
void SetClickElement(Element*);
void SetClickCount(int);
bool MouseDownMayStartDrag();
......@@ -225,7 +225,7 @@ class CORE_EXPORT MouseEventManager final
Member<Node> mouse_press_node_;
int click_count_;
Member<Node> click_node_;
Member<Element> click_element_;
IntPoint mouse_down_pos_; // In our view's coords.
TimeTicks mouse_down_timestamp_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment