Fixed & refactored mouse event firing at gesture context menu

Fixed & refactored mouse event firing at gesture context menu

The context menu code for gesture long-press used to fire mousedown &
contextmenu events with wrong button/buttons attributes (w.r.t. both
the spec & other browsers). This CL fixes those attribute values.

Moreover, the existing code achieved the desired outcome of a
long-press (setting focus before context menu) through calling the raw
PlatformMouseEvent entrypoint. This CL cherry-picks the needed steps,
thus provides a cleaner alternative to double-pointer-event firing
(minimally fixed through part of crrev.com/2141993003). We don't expect
any change in behavior for this part of the CL.

BUG=579564, 629876
Committed: https://crrev.com/f8139269d2dab360ec45b10c9f3fbcee67f504e7
Cr-Commit-Position: refs/heads/master@{#414773}

[mustaq, 2016-08-15 17:39:07]

Patchset #1 (id:1) has been deleted

[mustaq, 2016-08-17 15:27:38]

Description was changed from

==========
Minimize mouse bookkeeping at sendContextMenuEventForGesture

This CL replaces the existing code (a call to handleMousePressEvent())
with a minimal firing of DOM events.

BUG=629876
==========

to

==========
Minimize mouse bookkeeping at sendContextMenuEventForGesture

This CL replaces the existing code (a call to
handleMousePressEvent()) with a direct firing of DOM events.
This provides a cleaner alternative to the part of
crrev.com/2141993003 that minimally fixed double pointer
event firing on gesture long-press.

BUG=629876
==========

[mustaq, 2016-08-17 15:27:55]

mustaq@chromium.org changed reviewers:
+ dtapuska@chromium.org

[mustaq, 2016-08-17 15:27:55]

ptal

[mustaq, 2016-08-18 20:29:12]

Description was changed from

==========
Minimize mouse bookkeeping at sendContextMenuEventForGesture

This CL replaces the existing code (a call to
handleMousePressEvent()) with a direct firing of DOM events.
This provides a cleaner alternative to the part of
crrev.com/2141993003 that minimally fixed double pointer
event firing on gesture long-press.

BUG=629876
==========

to

==========
Fixed & refactored mouse event firing at gesture context menu

The context menu code for gesture long-press used to fire a redundant
mousedown event, and also mousedown & contextmenu events with wrong
button/buttons attributes (w.r.t. both the spec & other browsers). This
CL fixes both event firing & attributes.

Moreover, the existing code achieved the desired outcome of a
long-press (setting focus before context menu) through calling the raw
PlatformMouseEvent entrypoint. This CL cherry-picks the needed steps,
thus provides a cleaner alternative to double-pointer-event firing
(minimally fixed through part of crrev.com/2141993003).

BUG=579564,629876
==========

[mustaq, 2016-08-19 16:58:11]

mustaq@chromium.org changed reviewers:
+ varunjain@chromium.org

[mustaq, 2016-08-19 16:58:11]

dtapuska@: ptal, this is ready for review now.

varunjain@: I tested my patch on CrOS, both long-press and two-finger context
menus show proper "contexts" on a text area: I placed the cursor on a typo (T1),
then used the gestures on another typo (T2); the suggested spelling always point
to the tapped-on typo (T2). So I think this test is a bit overfitting:
  right-click-gestures-set-cursor-at-correct-position.html.
Please let me know if you have any concerns.

[dtapuska, 2016-08-19 19:07:12]

lgtm

https://codereview.chromium.org/2249663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/GestureManager.h (right):

https://codereview.chromium.org/2249663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.h:17: class HitTestResult;
Its unclear why this change is necessary. Is it in the wrong file?

[mustaq, 2016-08-19 19:38:36]

https://codereview.chromium.org/2249663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/GestureManager.h (right):

https://codereview.chromium.org/2249663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.h:17: class HitTestResult;
On 2016/08/19 19:07:12, dtapuska wrote:
> Its unclear why this change is necessary. Is it in the wrong file?

Removed, thanks.

[mustaq, 2016-08-22 20:27:13]

mustaq@chromium.org changed reviewers:
+ mohsen@chromium.org

[mustaq, 2016-08-22 20:27:14]

mohsen@: Any clue why the test is timing out at Wait()?

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

[mustaq, 2016-08-25 20:56:41]

Patchset #5 (id:100001) has been deleted

[mustaq, 2016-08-26 14:54:36]

The CQ bit was checked by mustaq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 14:54:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mustaq, 2016-08-26 14:55:19]

Description was changed from

==========
Fixed & refactored mouse event firing at gesture context menu

The context menu code for gesture long-press used to fire a redundant
mousedown event, and also mousedown & contextmenu events with wrong
button/buttons attributes (w.r.t. both the spec & other browsers). This
CL fixes both event firing & attributes.

Moreover, the existing code achieved the desired outcome of a
long-press (setting focus before context menu) through calling the raw
PlatformMouseEvent entrypoint. This CL cherry-picks the needed steps,
thus provides a cleaner alternative to double-pointer-event firing
(minimally fixed through part of crrev.com/2141993003).

BUG=579564,629876
==========

to

==========
Fixed & refactored mouse event firing at gesture context menu

The context menu code for gesture long-press used to fire mousedown &
contextmenu events with wrong button/buttons attributes (w.r.t. both
the spec & other browsers). This CL fixes both event firing &
attributes.

Moreover, the existing code achieved the desired outcome of a
long-press (setting focus before context menu) through calling the raw
PlatformMouseEvent entrypoint. This CL cherry-picks the needed steps,
thus provides a cleaner alternative to double-pointer-event firing
(minimally fixed through part of crrev.com/2141993003).

BUG=579564,629876
==========

[mustaq, 2016-08-26 14:56:21]

mustaq@chromium.org changed reviewers:
+ bokan@chromium.org

[mustaq, 2016-08-26 14:56:22]

bokan@chromium.org: Please review changes in Source/core.

[mustaq, 2016-08-26 15:30:54]

On 2016/08/22 20:27:14, mustaq wrote:
> mohsen@: Any clue why the test is timing out at Wait()?
> 
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

Re the timing out of the test: skipping the mousedown causes long-tap on an
empty out-of-focus textbox to only bring the focus in (w/o a cursor). The test
  TouchSelectionControllerClientAuraTest.EmptyTextfieldInsertionOnLongPress
is correctly failing because of that. It's not clear why sending the mousedown
should be necessary here. I tried to cherrypick the selection-controller call
w/o any luck.

I brought back the mousedown firing.

[commit-bot: I haz the power, 2016-08-26 16:51:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 16:51:31]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-08-26 16:55:15]

Just some clarification questions.

Also, could you elaborate in the description of exactly what changes are
happening. i.e. What the current behavior is and what the new behavior is.

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1521: if
(mouseEvent.getSyntheticEventType() == PlatformMouseEvent::FromTouch)
Help me understand, why was this removed?

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:311: HitTestRequest
request(HitTestRequest::Active);
We're doing another hit test here right? Can we not reuse the result from the
GestureEvent?

[mustaq, 2016-08-26 17:18:15]

Description was changed from

==========
Fixed & refactored mouse event firing at gesture context menu

The context menu code for gesture long-press used to fire mousedown &
contextmenu events with wrong button/buttons attributes (w.r.t. both
the spec & other browsers). This CL fixes both event firing &
attributes.

Moreover, the existing code achieved the desired outcome of a
long-press (setting focus before context menu) through calling the raw
PlatformMouseEvent entrypoint. This CL cherry-picks the needed steps,
thus provides a cleaner alternative to double-pointer-event firing
(minimally fixed through part of crrev.com/2141993003).

BUG=579564,629876
==========

to

==========
Fixed & refactored mouse event firing at gesture context menu

The context menu code for gesture long-press used to fire mousedown &
contextmenu events with wrong button/buttons attributes (w.r.t. both
the spec & other browsers). This CL fixes those attribute values.

Moreover, the existing code achieved the desired outcome of a
long-press (setting focus before context menu) through calling the raw
PlatformMouseEvent entrypoint. This CL cherry-picks the needed steps,
thus provides a cleaner alternative to double-pointer-event firing
(minimally fixed through part of crrev.com/2141993003). We don't expect
any change in behavior for this part of the CL.

BUG=579564,629876
==========

[mustaq, 2016-08-26 17:19:03]

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1521: if
(mouseEvent.getSyntheticEventType() == PlatformMouseEvent::FromTouch)
On 2016/08/26 16:55:14, bokan wrote:
> Help me understand, why was this removed?

This was added as a quick fix to prevent PE-firing after gesture events, see [A]
in crrev.com/2141993003 description. The main problem was that gesture code went
through the whole "raw" mouse handling code
(EventHandler::handleMousePressEvent(PlatformMouseEvent), which includes, among
other things, firing PointerEvents. The fix here replaces the call to
handleMousePressEvent(PlatformMouseEvent) with the tasks we really need, and PEs
are not one of them. We didn't include this fix in that CL because we wanted to
push a fix in quickly w/o risking breakages.

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:311: HitTestRequest
request(HitTestRequest::Active);
On 2016/08/26 16:55:14, bokan wrote:
> We're doing another hit test here right? Can we not reuse the result from the
> GestureEvent?

This is because the mousemove dispatched above can change the DOM.

[bokan, 2016-08-26 17:31:54]

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1521: if
(mouseEvent.getSyntheticEventType() == PlatformMouseEvent::FromTouch)
On 2016/08/26 17:19:03, mustaq wrote:
> On 2016/08/26 16:55:14, bokan wrote:
> > Help me understand, why was this removed?
> 
> This was added as a quick fix to prevent PE-firing after gesture events, see
[A]
> in crrev.com/2141993003 description. The main problem was that gesture code
went
> through the whole "raw" mouse handling code
> (EventHandler::handleMousePressEvent(PlatformMouseEvent), which includes,
among
> other things, firing PointerEvents. The fix here replaces the call to
> handleMousePressEvent(PlatformMouseEvent) with the tasks we really need, and
PEs
> are not one of them. We didn't include this fix in that CL because we wanted
to
> push a fix in quickly w/o risking breakages.

So is this now ok because gesture events don't come down this path?

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:311: HitTestRequest
request(HitTestRequest::Active);
On 2016/08/26 17:19:03, mustaq wrote:
> On 2016/08/26 16:55:14, bokan wrote:
> > We're doing another hit test here right? Can we not reuse the result from
the
> > GestureEvent?
> 
> This is because the mousemove dispatched above can change the DOM.

Ah, got it.

[mustaq, 2016-08-26 19:02:06]

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1521: if
(mouseEvent.getSyntheticEventType() == PlatformMouseEvent::FromTouch)
On 2016/08/26 17:31:53, bokan wrote:
> On 2016/08/26 17:19:03, mustaq wrote:
> > On 2016/08/26 16:55:14, bokan wrote:
> > > Help me understand, why was this removed?
> > 
> > This was added as a quick fix to prevent PE-firing after gesture events, see
> [A]
> > in crrev.com/2141993003 description. The main problem was that gesture code
> went
> > through the whole "raw" mouse handling code
> > (EventHandler::handleMousePressEvent(PlatformMouseEvent), which includes,
> among
> > other things, firing PointerEvents. The fix here replaces the call to
> > handleMousePressEvent(PlatformMouseEvent) with the tasks we really need, and
> PEs
> > are not one of them. We didn't include this fix in that CL because we wanted
> to
> > push a fix in quickly w/o risking breakages.
> 
> So is this now ok because gesture events don't come down this path?

Right: from the overall event handling perspective, this code has been taken by
the TE that has triggered the GE, so the GE handler shouldn't go though this
again.

[bokan, 2016-08-26 19:03:14]

On 2016/08/26 19:02:06, mustaq wrote:
>
https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (left):
> 
>
https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/input/EventHandler.cpp:1521: if
> (mouseEvent.getSyntheticEventType() == PlatformMouseEvent::FromTouch)
> On 2016/08/26 17:31:53, bokan wrote:
> > On 2016/08/26 17:19:03, mustaq wrote:
> > > On 2016/08/26 16:55:14, bokan wrote:
> > > > Help me understand, why was this removed?
> > > 
> > > This was added as a quick fix to prevent PE-firing after gesture events,
see
> > [A]
> > > in crrev.com/2141993003 description. The main problem was that gesture
code
> > went
> > > through the whole "raw" mouse handling code
> > > (EventHandler::handleMousePressEvent(PlatformMouseEvent), which includes,
> > among
> > > other things, firing PointerEvents. The fix here replaces the call to
> > > handleMousePressEvent(PlatformMouseEvent) with the tasks we really need,
and
> > PEs
> > > are not one of them. We didn't include this fix in that CL because we
wanted
> > to
> > > push a fix in quickly w/o risking breakages.
> > 
> > So is this now ok because gesture events don't come down this path?
> 
> Right: from the overall event handling perspective, this code has been taken
by
> the TE that has triggered the GE, so the GE handler shouldn't go though this
> again.

Awesome, thanks for the answers :)

LGTM

[mustaq, 2016-08-26 19:04:53]

The CQ bit was checked by mustaq@chromium.org

[mustaq, 2016-08-26 19:04:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2249663002/#ps140001
(title: "Realigned test outcomes.")

[commit-bot: I haz the power, 2016-08-26 19:05:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 19:10:27]

Description was changed from

==========
Fixed & refactored mouse event firing at gesture context menu

The context menu code for gesture long-press used to fire mousedown &
contextmenu events with wrong button/buttons attributes (w.r.t. both
the spec & other browsers). This CL fixes those attribute values.

Moreover, the existing code achieved the desired outcome of a
long-press (setting focus before context menu) through calling the raw
PlatformMouseEvent entrypoint. This CL cherry-picks the needed steps,
thus provides a cleaner alternative to double-pointer-event firing
(minimally fixed through part of crrev.com/2141993003). We don't expect
any change in behavior for this part of the CL.

BUG=579564,629876
==========

to

==========
Fixed & refactored mouse event firing at gesture context menu

The context menu code for gesture long-press used to fire mousedown &
contextmenu events with wrong button/buttons attributes (w.r.t. both
the spec & other browsers). This CL fixes those attribute values.

Moreover, the existing code achieved the desired outcome of a
long-press (setting focus before context menu) through calling the raw
PlatformMouseEvent entrypoint. This CL cherry-picks the needed steps,
thus provides a cleaner alternative to double-pointer-event firing
(minimally fixed through part of crrev.com/2141993003). We don't expect
any change in behavior for this part of the CL.

BUG=579564,629876
==========

[commit-bot: I haz the power, 2016-08-26 19:10:28]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-08-26 19:16:45]

Description was changed from

==========
Fixed & refactored mouse event firing at gesture context menu

The context menu code for gesture long-press used to fire mousedown &
contextmenu events with wrong button/buttons attributes (w.r.t. both
the spec & other browsers). This CL fixes those attribute values.

Moreover, the existing code achieved the desired outcome of a
long-press (setting focus before context menu) through calling the raw
PlatformMouseEvent entrypoint. This CL cherry-picks the needed steps,
thus provides a cleaner alternative to double-pointer-event firing
(minimally fixed through part of crrev.com/2141993003). We don't expect
any change in behavior for this part of the CL.

BUG=579564,629876
==========

to

==========
Fixed & refactored mouse event firing at gesture context menu

The context menu code for gesture long-press used to fire mousedown &
contextmenu events with wrong button/buttons attributes (w.r.t. both
the spec & other browsers). This CL fixes those attribute values.

Moreover, the existing code achieved the desired outcome of a
long-press (setting focus before context menu) through calling the raw
PlatformMouseEvent entrypoint. This CL cherry-picks the needed steps,
thus provides a cleaner alternative to double-pointer-event firing
(minimally fixed through part of crrev.com/2141993003). We don't expect
any change in behavior for this part of the CL.

BUG=579564,629876

Committed: https://crrev.com/f8139269d2dab360ec45b10c9f3fbcee67f504e7
Cr-Commit-Position: refs/heads/master@{#414773}
==========

[commit-bot: I haz the power, 2016-08-26 19:16:46]

Patchset 6 (id:??) landed as
https://crrev.com/f8139269d2dab360ec45b10c9f3fbcee67f504e7
Cr-Commit-Position: refs/heads/master@{#414773}

[Navid Zolghadr, 2016-08-29 15:19:50]

nzolghadr@chromium.org changed reviewers:
+ nzolghadr@chromium.org

[Navid Zolghadr, 2016-08-29 15:19:51]

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2249663002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:322:
m_frame->eventHandler().handleMousePressEvent(mev);
It seems we have changed the flow quite a bit here. For example previously that
we were calling handleMousePressEvent(PlatformMouseEvent) at first we were doing
some bookkeeping of m_mousePressNode or selection related stuff here
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Eve...

But now we are not doing any of that. Wouldn't that make any problem? Here is
one scenario which might have changed after this CL.

m_mousePressNode is used as the scrolling node when it is set and no other
element has the focus in scrollManager from the call here. So if we have no
scrollable element on a page except a small div somewhere. Now if you tap the
div. Then try to scroll with mouse wheel or keyboard. Before this change I
believe that div would scroll and after this change nothing would scroll I
believe.