Require a mouse button release event before counting a press event as a new click.

Require a mouse button release event before counting a press event as a new click.

BUG=389162
TEST=EventTest.DoubleClickRequiresRelease

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284479

[flackr, 2014-07-17 19:11:56]

This is a fairly simple way to avoid the problem of handling multiple of the
same press event. I was thinking either this or changing last_click to track the
released event as well, but that would slightly change the behavior (requiring
release to be near the press) whereas should have no multi-click detection
changes beyond double handling presses.

I was also considering trying to determine if the event was the same but relying
on something like the timestamp seemed fragile.

WDYT?

[sadrul, 2014-07-17 20:01:34]

LGTM This should work correctly for X11.

+sky@ to see if this would work correctly on Windows too.

[sky, 2014-07-17 21:37:55]

https://codereview.chromium.org/398393003/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/398393003/diff/1/ui/events/event.cc#newcode321
ui/events/event.cc:321: last_click_released_ = true;
If you get a capture change I'm not sure if you are guaranteed a release. I also
think you need to check the changed flags. For example, if you first click the
right button then click/release/click the left button that likely shouldn't be a
double click.

[flackr, 2014-07-18 16:20:49]

https://codereview.chromium.org/398393003/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/398393003/diff/1/ui/events/event.cc#newcode321
ui/events/event.cc:321: last_click_released_ = true;
On 2014/07/17 21:37:55, sky wrote:
> If you get a capture change I'm not sure if you are guaranteed a release.

If the event is handled within chrome this is called from the MouseEvent
constructor, we should see it I think. Or am I missing something? Is there a
circumstance I should be testing for this?

> I also
> think you need to check the changed flags. For example, if you first click the
> right button then click/release/click the left button that likely shouldn't be
a
> double click.

The left click/release/click is a double click isn't it? I have added a check to
only set last_click_released_ if the changed button flag matches and added a
test with the event sequence you described, please let me know if I
misunderstood, thanks!

[sky, 2014-07-18 19:20:59]

https://codereview.chromium.org/398393003/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/398393003/diff/1/ui/events/event.cc#newcode321
ui/events/event.cc:321: last_click_released_ = true;
On 2014/07/18 16:20:49, flackr wrote:
> On 2014/07/17 21:37:55, sky wrote:
> > If you get a capture change I'm not sure if you are guaranteed a release.
> 
> If the event is handled within chrome this is called from the MouseEvent
> constructor, we should see it I think. Or am I missing something? Is there a
> circumstance I should be testing for this?

What I'm saying is that I'm not sure if we necessarily get a release on a
capture change. If that happens won't these variables be wrong?

> > I also
> > think you need to check the changed flags. For example, if you first click
the
> > right button then click/release/click the left button that likely shouldn't
be
> a
> > double click.
> 
> The left click/release/click is a double click isn't it? I have added a check
to
> only set last_click_released_ if the changed button flag matches and added a
> test with the event sequence you described, please let me know if I
> misunderstood, thanks!

I believe that is what I'm after.

[flackr, 2014-07-18 19:45:23]

https://codereview.chromium.org/398393003/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/398393003/diff/1/ui/events/event.cc#newcode321
ui/events/event.cc:321: last_click_released_ = true;
On 2014/07/18 19:20:59, sky wrote:
> On 2014/07/18 16:20:49, flackr wrote:
> > On 2014/07/17 21:37:55, sky wrote:
> > > If you get a capture change I'm not sure if you are guaranteed a release.
> > 
> > If the event is handled within chrome this is called from the MouseEvent
> > constructor, we should see it I think. Or am I missing something? Is there a
> > circumstance I should be testing for this?
> 
> What I'm saying is that I'm not sure if we necessarily get a release on a
> capture change. If that happens won't these variables be wrong?

Ah I see what you're saying. If I double click, and lose the release for the
second click, the next click on the application would assume it was still part
of that double click.

For the case I'm trying to handle the native timestamps of event and
last_click_event_ should be the same, how about if we set last_click_released_ =
true on a later timestamp as well?

Alternately, since these clicks will be far apart I could call
IsRepeatedClickEvent and reset click_count if false.

> 
> > > I also
> > > think you need to check the changed flags. For example, if you first click
> the
> > > right button then click/release/click the left button that likely
shouldn't
> be
> > a
> > > double click.
> > 
> > The left click/release/click is a double click isn't it? I have added a
check
> to
> > only set last_click_released_ if the changed button flag matches and added a
> > test with the event sequence you described, please let me know if I
> > misunderstood, thanks!
> 
> I believe that is what I'm after.

[flackr, 2014-07-18 20:26:37]

I've updated this to only deal with pressed events having the same timestamp,
and renamed the variable since released_ didn't make sense anymore. This should
correctly handle the capture case since the next click will have a different
timestamp. Let me know what you think.

[sky, 2014-07-18 22:42:51]

https://codereview.chromium.org/398393003/diff/40001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/398393003/diff/40001/ui/events/event.cc#newco...
ui/events/event.cc:342: last_click_complete_ = false;
Do we need both last_click_complete_ and last_click_event_? When complete could
you delete and NULL out last_click_event_ instead of maintaining both?

[flackr, 2014-07-19 12:55:11]

https://codereview.chromium.org/398393003/diff/40001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/398393003/diff/40001/ui/events/event.cc#newco...
ui/events/event.cc:342: last_click_complete_ = false;
On 2014/07/18 22:42:51, sky wrote:
> Do we need both last_click_complete_ and last_click_event_? When complete
could
> you delete and NULL out last_click_event_ instead of maintaining both?

It's marked complete when another pressed event should be considered as a new
unique press, at which point we can use last_click_event_ to determine if they
were close together enough to increase the click count. If we NULL'd out
last_click_event_ we would lose the click count and wouldn't be able to double
click.

I was also considering whether just checking that the timestamps of the pressed
events were different but I'm not sure if we might have low precision timestamps
on some OS's such that the two presses in a double click would have the same
timestamp.

[sky, 2014-07-21 15:46:35]

Ok, LGTM

[flackr, 2014-07-21 15:49:01]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-07-21 15:49:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/flackr@chromium.org/398393003/40001

[commit-bot: I haz the power, 2014-07-21 19:38:15]

Change committed as 284479