Suppress mouseup/down when button is NoButton

Suppress mouseup/down when button is NoButton

Chrome sends the mousedown/up in some cases
when the 4th/5th button is pressed/released
without actually sending that button value as
we don't have yet the plumbing needed for that.

BUG=574959
Committed: https://crrev.com/620c6710dc14652ce936417c04a355c529cd89c1
Cr-Commit-Position: refs/heads/master@{#383812}

[Navid Zolghadr, 2016-03-23 17:56:18]

Description was changed from

==========
Supress mouseup/down when button not 0

Chrome sends the mousedown/up in some cases
when the 4th/5th button is pressed/released
without actually sending that button value as
we don't have yet the plumbing needed for that.

BUG=574959
==========

to

==========
Suppress mouseup/down when button is NoButton

Chrome sends the mousedown/up in some cases
when the 4th/5th button is pressed/released
without actually sending that button value as
we don't have yet the plumbing needed for that.

BUG=574959
==========

[Navid Zolghadr, 2016-03-23 17:57:39]

nzolghadr@chromium.org changed reviewers:
+ dtapuska@chromium.org, mustaq@chromium.org, rbyers@google.com

[Navid Zolghadr, 2016-03-23 17:57:39]

ptal

[Navid Zolghadr, 2016-03-23 17:58:11]

nzolghadr@chromium.org changed reviewers:
+ rbyers@chromium.org
- rbyers@google.com

[dtapuska, 2016-03-23 18:37:31]

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

https://codereview.chromium.org/1824733006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:1693: if (mouseEventType
!= EventTypeNames::mousemove && mouseEvent.button() == NoButton) {
braces aren't needed here

[mustaq, 2016-03-23 19:13:26]

https://codereview.chromium.org/1824733006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1824733006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1382: clickEventResult =
WebInputEventResult::HandledSystem;
I think we should make those buttons completely ineffective, not even changing
focus etc above. I suggest adding the "bailout code" earlier, near the entry
point to EventHandler, and keep the "deeper" code intact. I mean at both
handleMousePressEvent(PlatformMouseEvent&) and
handleMouseReleaseEvent(PlatformMouseEvent&).

[Navid Zolghadr, 2016-03-24 14:46:04]

ptal.

Dave, also does HandledSystem make sense in this case or should I return
NotHandled?

[dtapuska, 2016-03-24 15:01:07]

On 2016/03/24 14:46:04, Navid Zolghadr wrote:
> ptal.
> 
> Dave, also does HandledSystem make sense in this case or should I return
> NotHandled?

Personally I think HandledSuppressed is the correct one; because you aren't
dispatching it to the JS are you?

[Navid Zolghadr, 2016-03-24 15:11:39]

On 2016/03/24 15:01:07, dtapuska wrote:
> On 2016/03/24 14:46:04, Navid Zolghadr wrote:
> > ptal.
> > 
> > Dave, also does HandledSystem make sense in this case or should I return
> > NotHandled?
> 
> Personally I think HandledSuppressed is the correct one; because you aren't
> dispatching it to the JS are you?

Awesome.
I'm not. I return in the early steps to prevent generating and firing the event
to JS altogether.

[mustaq, 2016-03-24 15:43:44]

On 2016/03/24 15:11:39, Navid Zolghadr wrote:
> On 2016/03/24 15:01:07, dtapuska wrote:
> > On 2016/03/24 14:46:04, Navid Zolghadr wrote:
> > > ptal.
> > > 
> > > Dave, also does HandledSystem make sense in this case or should I return
> > > NotHandled?
> > 
> > Personally I think HandledSuppressed is the correct one; because you aren't
> > dispatching it to the JS are you?
> 
> Awesome.
> I'm not. I return in the early steps to prevent generating and firing the
event
> to JS altogether.

Dave, doesn't NotHandled seem the best choice here? Do you see any effect
upstream?

[dtapuska, 2016-03-24 15:52:25]

On 2016/03/24 15:43:44, mustaq wrote:
> On 2016/03/24 15:11:39, Navid Zolghadr wrote:
> > On 2016/03/24 15:01:07, dtapuska wrote:
> > > On 2016/03/24 14:46:04, Navid Zolghadr wrote:
> > > > ptal.
> > > > 
> > > > Dave, also does HandledSystem make sense in this case or should I return
> > > > NotHandled?
> > > 
> > > Personally I think HandledSuppressed is the correct one; because you
aren't
> > > dispatching it to the JS are you?
> > 
> > Awesome.
> > I'm not. I return in the early steps to prevent generating and firing the
> event
> > to JS altogether.
> 
> Dave, doesn't NotHandled seem the best choice here? Do you see any effect
> upstream?

I think Suppressed is the safest choice here since we are never giving them off
to javascript and it doesn't have the opportunity to cancel them.

If we encounter an issue in chrome wanting to know if JS handled these buttons
or
not then we will need to fix setting the buttons correctly and calling the JS
first.

[Navid Zolghadr, 2016-03-24 15:59:48]

ptal.

I changed it to HandledSuppressed.

[dtapuska, 2016-03-24 16:02:07]

On 2016/03/24 15:59:48, Navid Zolghadr wrote:
> ptal.
> 
> I changed it to HandledSuppressed.

lgtm

[mustaq, 2016-03-24 16:05:38]

lgtm

[Rick Byers, 2016-03-29 16:09:43]

Sorry for the delay - I'd written something last week but apparently I didn't
actually submit it.

This seems like good defense to me (although the right long-term thing here is
to fix the underlying bug with extra mouse buttons).

But what about a test?  I think EventSender should let you construct such an
event stream (since it's purpose is to unit test blink input handling by mocking
out the chromium side).  But if that turns out to be non-trivial, then it's
probably not worth the effort and I'm OK landing without a test.  But if it's
trivial as I suspect, then it's probably worth doing since it could be an easy
regression.

[Rick Byers, 2016-03-29 16:09:45]

Sorry for the delay - I'd written something last week but apparently I didn't
actually submit it.

This seems like good defense to me (although the right long-term thing here is
to fix the underlying bug with extra mouse buttons).

But what about a test?  I think EventSender should let you construct such an
event stream (since it's purpose is to unit test blink input handling by mocking
out the chromium side).  But if that turns out to be non-trivial, then it's
probably not worth the effort and I'm OK landing without a test.  But if it's
trivial as I suspect, then it's probably worth doing since it could be an easy
regression.

[Navid Zolghadr, 2016-03-29 16:21:18]

On 2016/03/29 16:09:45, Rick Byers wrote:
> Sorry for the delay - I'd written something last week but apparently I didn't
> actually submit it.
> 
> This seems like good defense to me (although the right long-term thing here is
> to fix the underlying bug with extra mouse buttons).
> 
> But what about a test?  I think EventSender should let you construct such an
> event stream (since it's purpose is to unit test blink input handling by
mocking
> out the chromium side).  But if that turns out to be non-trivial, then it's
> probably not worth the effort and I'm OK landing without a test.  But if it's
> trivial as I suspect, then it's probably worth doing since it could be an easy
> regression.

Right now event sender apis do not let me send mousedown/up with button=-1. Do
you want me to add a new api or modify the existing api to let the javascript
send mousedowns/ups with no button pressed?

[mustaq, 2016-03-29 16:25:20]

On 2016/03/29 16:21:18, Navid Zolghadr wrote:
> On 2016/03/29 16:09:45, Rick Byers wrote:
> > Sorry for the delay - I'd written something last week but apparently I
didn't
> > actually submit it.
> > 
> > This seems like good defense to me (although the right long-term thing here
is
> > to fix the underlying bug with extra mouse buttons).
> > 
> > But what about a test?  I think EventSender should let you construct such an
> > event stream (since it's purpose is to unit test blink input handling by
> mocking
> > out the chromium side).  But if that turns out to be non-trivial, then it's
> > probably not worth the effort and I'm OK landing without a test.  But if
it's
> > trivial as I suspect, then it's probably worth doing since it could be an
easy
> > regression.
> 
> Right now event sender apis do not let me send mousedown/up with button=-1. Do
> you want me to add a new api or modify the existing api to let the javascript
> send mousedowns/ups with no button pressed?

I also considered about testing through event_sender but I backed off because it
would require at least a semi-complete plumbing with WebMouseEvent with 4th/5th
buttons.

I would suggest filing a bug for the complete plumbing (if not already done).

[Navid Zolghadr, 2016-03-29 16:26:59]

On 2016/03/29 16:25:20, mustaq wrote:
> On 2016/03/29 16:21:18, Navid Zolghadr wrote:
> > On 2016/03/29 16:09:45, Rick Byers wrote:
> > > Sorry for the delay - I'd written something last week but apparently I
> didn't
> > > actually submit it.
> > > 
> > > This seems like good defense to me (although the right long-term thing
here
> is
> > > to fix the underlying bug with extra mouse buttons).
> > > 
> > > But what about a test?  I think EventSender should let you construct such
an
> > > event stream (since it's purpose is to unit test blink input handling by
> > mocking
> > > out the chromium side).  But if that turns out to be non-trivial, then
it's
> > > probably not worth the effort and I'm OK landing without a test.  But if
> it's
> > > trivial as I suspect, then it's probably worth doing since it could be an
> easy
> > > regression.
> > 
> > Right now event sender apis do not let me send mousedown/up with button=-1.
Do
> > you want me to add a new api or modify the existing api to let the
javascript
> > send mousedowns/ups with no button pressed?
> 
> I also considered about testing through event_sender but I backed off because
it
> would require at least a semi-complete plumbing with WebMouseEvent with
4th/5th
> buttons.
> 
> I would suggest filing a bug for the complete plumbing (if not already done).

There is this bug which talks about this full plumbing.

crbug.com/567771

[Rick Byers, 2016-03-29 19:20:35]

On 2016/03/29 16:26:59, Navid Zolghadr wrote:
> On 2016/03/29 16:25:20, mustaq wrote:
> > On 2016/03/29 16:21:18, Navid Zolghadr wrote:
> > > On 2016/03/29 16:09:45, Rick Byers wrote:
> > > > Sorry for the delay - I'd written something last week but apparently I
> > didn't
> > > > actually submit it.
> > > > 
> > > > This seems like good defense to me (although the right long-term thing
> here
> > is
> > > > to fix the underlying bug with extra mouse buttons).
> > > > 
> > > > But what about a test?  I think EventSender should let you construct
such
> an
> > > > event stream (since it's purpose is to unit test blink input handling by
> > > mocking
> > > > out the chromium side).  But if that turns out to be non-trivial, then
> it's
> > > > probably not worth the effort and I'm OK landing without a test.  But if
> > it's
> > > > trivial as I suspect, then it's probably worth doing since it could be
an
> > easy
> > > > regression.
> > > 
> > > Right now event sender apis do not let me send mousedown/up with
button=-1.
> Do
> > > you want me to add a new api or modify the existing api to let the
> javascript
> > > send mousedowns/ups with no button pressed?
> > 
> > I also considered about testing through event_sender but I backed off
because
> it
> > would require at least a semi-complete plumbing with WebMouseEvent with
> 4th/5th
> > buttons.

I don't think that's true - we'd just need the ability to send mousedown with
button=-1 as Navid said.  But still, EventSender hasn't been very consistent
about permitting "invalid" event sequences (if we KNOW chromium can't generate
the sequence, it's often not worth the hassle - especially since clusterfuzz
will trigger every crazy invalid combination it can get away with :-)

So I'm ok leaving the testing of 4th/5th button scenarios to crbug.com/567771
(by which time hopefully we'll be able to write more of a complete end-to-end
interop test instead of doing unit testing via eventsender).

LGTM
 
> > 
> > I would suggest filing a bug for the complete plumbing (if not already
done).
> 
> There is this bug which talks about this full plumbing.
> 
> crbug.com/567771

[Navid Zolghadr, 2016-03-29 19:38:58]

The CQ bit was checked by nzolghadr@chromium.org

[commit-bot: I haz the power, 2016-03-29 19:39:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1824733006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1824733006/60001

[commit-bot: I haz the power, 2016-03-29 20:48:40]

Description was changed from

==========
Suppress mouseup/down when button is NoButton

Chrome sends the mousedown/up in some cases
when the 4th/5th button is pressed/released
without actually sending that button value as
we don't have yet the plumbing needed for that.

BUG=574959
==========

to

==========
Suppress mouseup/down when button is NoButton

Chrome sends the mousedown/up in some cases
when the 4th/5th button is pressed/released
without actually sending that button value as
we don't have yet the plumbing needed for that.

BUG=574959
==========

[commit-bot: I haz the power, 2016-03-29 20:48:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-29 20:50:19]

Description was changed from

==========
Suppress mouseup/down when button is NoButton

Chrome sends the mousedown/up in some cases
when the 4th/5th button is pressed/released
without actually sending that button value as
we don't have yet the plumbing needed for that.

BUG=574959
==========

to

==========
Suppress mouseup/down when button is NoButton

Chrome sends the mousedown/up in some cases
when the 4th/5th button is pressed/released
without actually sending that button value as
we don't have yet the plumbing needed for that.

BUG=574959

Committed: https://crrev.com/620c6710dc14652ce936417c04a355c529cd89c1
Cr-Commit-Position: refs/heads/master@{#383812}
==========

[commit-bot: I haz the power, 2016-03-29 20:50:20]

Patchset 4 (id:??) landed as
https://crrev.com/620c6710dc14652ce936417c04a355c529cd89c1
Cr-Commit-Position: refs/heads/master@{#383812}