WIP Logging for Contextual Search: Node->Element.

WIP Logging for Contextual Search: Node->Element.

Base patch description below.
============================================
Fix the wrong TextNode handling in EventHanlder and MouseEventManager

EventHandler and MouseEventManager are storing a clicked node (or a touched node) as Node*, instead of Element*,
That would be a bad practice. Actually, that would be the cause of several wrong behaviors in Blink, such as:

(A) 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.

(B) 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 only Document and Element.

For reference, (A) would happen in the following scenario:

1. User presses a mouse button
2. UA did a hittest. The result's innerNode() is a text node.
3. MouseEventManager#click_node_ is set to the parent element of the clicked
    text node
4. Dispatch mouse press events (or anything)
5. User releases a mouse button
6. UA did another hittest because UA would dispatch a click event at the commo
   n ancestor of:
   - A location of mouse pressed; MouseEventManager#click_node_ is used in this case.
   - and A location of mouse released; The 2nd hit test's result is used in this case.
7. Users removed a text node, which is the result of 2nd hit test.
8. Since there is no common ancestor of MouseEventManager#click_node_ and the
    2nd hit test's result, a click event is not dispatched.

To prevent these wrong behaviors, this CL does:

- Replaces the usage of |Node*| to |Element*| as much as possible so that it rejects the wrong code at type level.
- To prevent a false-negative event firing, like in the mentioned case, preserve the parent *element*, if necessary, of a clicked node
   before dispatching MouseRelease event so that a following click event should be fired correctly even when a text node has been removed in mouseup.

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

This CL mainly aims to fix bug 708394. I think there are other places, such as other events, which should be fixed, which can be done in another CL.

BUG=708394, 710425
patch from issue 2807123002 at patchset 80001 (http://crrev.com/2807123002#ps80001)

[Donn Denman, 2017-04-12 02:12:47]

donnd@chromium.org changed reviewers:
+ hayato@chromium.org

[Donn Denman, 2017-04-12 02:12:47]

Hayato, I did some debugging of what's going wrong with your cl by patching it
into my client and adding debug output.  Here's what I see.

04-11 18:49:58.333  2846  2862 I chromium: [INFO:GestureManager.cpp(188)] ctxs
Capture data for ShowUnhandledTapUIIfNeeded
04-11 18:49:58.352  2846  2862 I chromium: [INFO:GestureManager.cpp(303)] ctxs
ShowUnhandledTapUIIfNeeded
04-11 18:49:58.352  2846  2862 I chromium: [INFO:ChromeClientImpl.cpp(1093)]
ctxs ChromeClientImpl::ShowUnhandledTapUIIfNeeded
04-11 18:49:58.352  2846  2862 I chromium: [INFO:ChromeClientImpl.cpp(1095)]
ctxs ChromeClientImpl::ShowUnhandledTapUIIfNeeded calling client
04-11 18:49:58.352  2846  2862 I chromium: [INFO:render_widget.cc(2143)] ctxs
RenderWidget::ShowUnhandledTapUIIfNeeded
04-11 18:49:58.352  2846  2862 I chromium: [INFO:render_widget.cc(2148)] ctxs
RenderWidget::ShowUnhandledTapUIIfNeeded should_trigger: 0
04-11 18:49:58.352  2846  2862 I chromium: [INFO:render_widget.cc(2149)] ctxs
RenderWidget::ShowUnhandledTapUIIfNeeded page_changed: 0
04-11 18:49:58.352  2846  2862 I chromium: [INFO:render_widget.cc(2150)] ctxs
RenderWidget::ShowUnhandledTapUIIfNeeded IsTextNode: 0
04-11 18:49:58.352  2846  2862 I chromium: [INFO:render_widget.cc(2151)] ctxs
RenderWidget::ShowUnhandledTapUIIfNeeded IsContentEditable: 0
04-11 18:49:58.353  2846  2862 I chromium: [INFO:render_widget.cc(2152)] ctxs
RenderWidget::ShowUnhandledTapUIIfNeeded IsInsideFocusableElementOrARIAWidget: 0


I think you may need to modify ChromeClientImpl because it seems to think it's
still getting a Node* in ShowUnhandledTapUIIfNeeded, but now you're passing in
an Element*?

https://codereview.chromium.org/2809333002/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2809333002/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:2150: VLOG(0) << "ctxs
RenderWidget::ShowUnhandledTapUIIfNeeded IsTextNode: " <<
tapped_node.IsTextNode();
This is logging 0, needs to be 1.

[Donn Denman, 2017-04-12 02:13:52]

Description was changed from

==========
WIP Logging for Contextual Search: Node->Element.


Fix the wrong TextNode handling in EventHanlder and MouseEventManager

EventHandler and MouseEventManager are storing a clicked node (or a touched
node) as Node*, instead of Element*,
That would be a bad practice. Actually, that would be the cause of several wrong
behaviors in Blink, such as:

(A) 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.

(B) 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 only Document and Element.

For reference, (A) would happen in the following scenario:

1. User presses a mouse button
2. UA did a hittest. The result's innerNode() is a text node.
3. MouseEventManager#click_node_ is set to the parent element of the clicked
    text node
4. Dispatch mouse press events (or anything)
5. User releases a mouse button
6. UA did another hittest because UA would dispatch a click event at the commo
   n ancestor of:
   - A location of mouse pressed; MouseEventManager#click_node_ is used in this
case.
   - and A location of mouse released; The 2nd hit test's result is used in this
case.
7. Users removed a text node, which is the result of 2nd hit test.
8. Since there is no common ancestor of MouseEventManager#click_node_ and the
    2nd hit test's result, a click event is not dispatched.

To prevent these wrong behaviors, this CL does:

- Replaces the usage of |Node*| to |Element*| as much as possible so that it
rejects the wrong code at type level.
- To prevent a false-negative event firing, like in the mentioned case, preserve
the parent *element*, if necessary, of a clicked node
   before dispatching MouseRelease event so that a following click event should
be fired correctly even when a text node has been removed in mouseup.

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

This CL mainly aims to fix bug 708394. I think there are other places, such as
other events, which should be fixed, which can be done in another CL.

BUG=708394,710425

patch from issue 2807123002 at patchset 80001
(http://crrev.com/2807123002#ps80001)
==========

to

==========
WIP Logging for Contextual Search: Node->Element.

Base patch description below.
============================================
Fix the wrong TextNode handling in EventHanlder and MouseEventManager

EventHandler and MouseEventManager are storing a clicked node (or a touched
node) as Node*, instead of Element*,
That would be a bad practice. Actually, that would be the cause of several wrong
behaviors in Blink, such as:

(A) 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.

(B) 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 only Document and Element.

For reference, (A) would happen in the following scenario:

1. User presses a mouse button
2. UA did a hittest. The result's innerNode() is a text node.
3. MouseEventManager#click_node_ is set to the parent element of the clicked
    text node
4. Dispatch mouse press events (or anything)
5. User releases a mouse button
6. UA did another hittest because UA would dispatch a click event at the commo
   n ancestor of:
   - A location of mouse pressed; MouseEventManager#click_node_ is used in this
case.
   - and A location of mouse released; The 2nd hit test's result is used in this
case.
7. Users removed a text node, which is the result of 2nd hit test.
8. Since there is no common ancestor of MouseEventManager#click_node_ and the
    2nd hit test's result, a click event is not dispatched.

To prevent these wrong behaviors, this CL does:

- Replaces the usage of |Node*| to |Element*| as much as possible so that it
rejects the wrong code at type level.
- To prevent a false-negative event firing, like in the mentioned case, preserve
the parent *element*, if necessary, of a clicked node
   before dispatching MouseRelease event so that a following click event should
be fired correctly even when a text node has been removed in mouseup.

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

This CL mainly aims to fix bug 708394. I think there are other places, such as
other events, which should be fixed, which can be done in another CL.

BUG=708394,710425

patch from issue 2807123002 at patchset 80001
(http://crrev.com/2807123002#ps80001)
==========

[hayato, 2017-04-12 03:27:49]

https://codereview.chromium.org/2809333002/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2809333002/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:2150: VLOG(0) << "ctxs
RenderWidget::ShowUnhandledTapUIIfNeeded IsTextNode: " <<
tapped_node.IsTextNode();
On 2017/04/12 at 02:12:47, Donn Denman wrote:
> This is logging 0, needs to be 1.

Ah, thanks! Let me try to update RenderWidget.

[hayato, 2017-04-12 04:04:07]

On 2017/04/12 at 03:27:49, hayato wrote:
>
https://codereview.chromium.org/2809333002/diff/1/content/renderer/render_wid...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/2809333002/diff/1/content/renderer/render_wid...
> content/renderer/render_widget.cc:2150: VLOG(0) << "ctxs
RenderWidget::ShowUnhandledTapUIIfNeeded IsTextNode: " <<
tapped_node.IsTextNode();
> On 2017/04/12 at 02:12:47, Donn Denman wrote:
> > This is logging 0, needs to be 1.
> 
> Ah, thanks! Let me try to update RenderWidget.

I meant to say ChromeClientImpl, instead of RenderWidget. :)

[hayato, 2017-04-12 07:04:24]

https://codereview.chromium.org/2809333002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2809333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/GestureManager.cpp:307:
tapped_position_in_viewport, tapped_element,
I finally understand what ShowUnhandledTapUIIfNeeded is meant for.
We should pass the original tapped_node here, before adjusting it to parent
element.

This is a kind of *by-pass* so that it can get a text node, which is never
accessible via Web's Touch Event feature.

[hayato, 2017-04-12 07:14:51]

I have updated the CL, https://codereview.chromium.org/2807123002, and it looks
all green.
Thank you for your advice!