1223366 - mouseup event.buttons is incorrect on Linux (X11/gtk)

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Tzvetelin Vassilev]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

"Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0"

document.addEventListener("mouseup", function(e) {console.log(e.buttons);});


Actual results:

with left button result is 1


Expected results:

with left button result to be 0

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

You use e.buttons for which 1 is the correct value for the left mouse button, see https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons

For e.button 0 is the expected value: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button

If you filed this bug because the returned value is different from Windows (I get 0 for all button on Windows 8.1), can you update the bug title? Else we can also close this bug and open a new one.

Comment 2

[Tzvetelin Vassilev]

Well, 0 is result in Firefox for Windows (i have tested on 8.1 and 10) and MacOS. 0 is result in Chrome (Windows, Mac, Linux), IE, Edge. Safari result is undefined, but is better than 1. Only in Linux (tested on Ubuntu only) is 1. I don't know another way to check is any mouse button is down after this mouseup event. The result should be a sum of still pressed buttons.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

See here:
http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsWindow.cpp?rev=49a5f1fe2128&mark=2684-2688#2677

State of modifiers and mouse buttons is represented with bit flags. However, X modifies the flags *after* sending press event and *before* sending release event. So, therefore, we don't have a way to set correct value on Linux... You can see same result with .getModifierState("Shift") etc.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, I forgot bug 768287. So, .getModifierState() should work fine.

We could fix this bug with same approach. See here:
http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsGtkKeyUtils.cpp?rev=4a2797061277&mark=895-904#859

Comment 5

[Matt Brubeck (:mbrubeck)]

> We could fix this bug with same approach. See here:
> http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsGtkKeyUtils.cpp?rev=4a2797061277&mark=895-904#859

I don't see how to fix this bug with the same approach. Is there any X11 or XInput2 event that notifies of button state changes, like XkbStateNotifyEvent notifies of modifier state changes?

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This patch uses GetCurrentModifierState to get the current modifier state on mouseup.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8686707 [details] [diff] [review]
patch

Did you confirm this patch fixes this bug actually? In the modifier state case, the modifier state hasn't been modified yet at this timing.

If not, we should investigate if XkbStateNotify is sent for GDK_BUTTON[1-3]_MASK too since they are also modifier state.

Comment 8

[Matt Brubeck (:mbrubeck)]

Yes, this fixes the bug for me.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8686707 [details] [diff] [review]
patch

Okay, then, we should take this.

> +    }
> +    else {

nit: } else {

Asking additional review to karlt.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, I have another question.

Does it work properly when you press both left mouse button and right mouse button, and release only left or right mouse button? The keeping pressing button's flag should be set to MouseEvent.buttons.

Comment 11

[Matt Brubeck (:mbrubeck)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #10)
> Does it work properly when you press both left mouse button and right mouse
> button, and release only left or right mouse button? The keeping pressing
> button's flag should be set to MouseEvent.buttons.

Yes, it works. For example here is the event sequence I get when I press left button, press right button, release right button, release left button:

without patch:

> mousedown, button: 0, buttons: 1
> mousedown, button: 2, buttons: 3
> mouseup,   button: 2, buttons: 3
> mouseup,   button: 0, buttons: 1

with patch:

> mousedown, button: 0, buttons: 1
> mousedown, button: 2, buttons: 3
> mouseup,   button: 2, buttons: 1
> mouseup,   button: 0, buttons: 0

(It also works correctly if I press and release buttons on multiple input devices.)

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks!

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8686707 [details] [diff] [review]
patch

>+        // We can't assume that aGdkEvent->button is no longer pressed,
>+        // because there may be a different pointer device with the same
>+        // button still pressed.  Instead, query the display for the new state.
>+        modifierState = KeymapWrapper::GetCurrentModifierState();

As indicated in the doc for this function "if some key events are not still
dispatched by GDK, the state may mismatch with GdkEventKey::state."
We want the deterministic state immediately after the button release, not the
racy state when the event is processed.

(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> > We could fix this bug with same approach. See here:
> > http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsGtkKeyUtils.cpp?rev=4a2797061277&mark=895-904#859
> 
> I don't see how to fix this bug with the same approach. Is there any X11 or
> XInput2 event that notifies of button state changes, like
> XkbStateNotifyEvent notifies of modifier state changes?

"The Xkb extension reports XkbStateNotify events to clients wanting
notification whenever the Xkb state changes. The changes reported include
changes to any aspect of the keyboard state: when a modifier is set or unset,
when the current group changes, or when a pointer button is pressed or
released."

XkbPointerButtonMask looks like what is required.

XkbStateNotifyEvent::ptr_button "gives the state of the core pointer buttons
as a mask composed of an inclusive OR of zero or more of the core pointer
button masks."

http://www.x.org/releases/X11R7.6/doc/libX11/specs/XKB/xkblib.html

Comment 14

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

I discovered that this simpler approach works.

The original code commented that this won't work because a GDK_BUTTON_RELEASE event might be generated while the same button is still pressed on another device, but this is not actually the case in my testing.  If I press the same button on multiple pointer devices, then release it on each device, I receive only a single RELEASE event, after the button has been released on all devices.

Comment 15

[Matt Brubeck (:mbrubeck)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #13)

> As indicated in the doc for this function "if some key events are not still
> dispatched by GDK, the state may mismatch with GdkEventKey::state."
> We want the deterministic state immediately after the button release, not the
> racy state when the event is processed.

But right before that it says that “GetCurrentModifierState() returns current modifier key state. The ‘current’ means actual state of hardware keyboard when this is called.”

Its implementation calls `gdk_display_get_pointer` which in turn calls `XQueryPointer.`  If I understand the code correctly, the reason for the possible mismatch is that `GdkEventKey::state` can contain out-of-date info, not that `GetCurrentModifierState` can.  And my tests bear this out.

Peeking at the next event seems more racy to me, since I don't see any guarantee that there's only one XKbStateNotify event in the queue, or that it's at the front of the queue.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8688040 [details] [diff] [review]
patch v2

(In reply to Matt Brubeck (:mbrubeck) from comment #14)
> The original code commented that this won't work because a
> GDK_BUTTON_RELEASE event might be generated while the same button is still
> pressed on another device, but this is not actually the case in my testing. 
> If I press the same button on multiple pointer devices, then release it on
> each device, I receive only a single RELEASE event, after the button has
> been released on all devices.

I get the same results here.  Thanks!

>+                modifierState &= ~GDK_BUTTON1_MASK;

Can you cast to guint before the one's complement operation (~), please, so as
to remove any assumption of enum underlying type or two's complement
representation of negative signed integers?

A simpler option is to have a single switch statement, independent of
aGdkEvent->type, to set a guint buttonMask variable.  Then buttonMask can be
combined with modifierState according to aGdkEvent->type and no explicit casts
are required.

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to Matt Brubeck (:mbrubeck) from comment #15)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #13)
> 
> > As indicated in the doc for this function "if some key events are not still
> > dispatched by GDK, the state may mismatch with GdkEventKey::state."
> > We want the deterministic state immediately after the button release, not the
> > racy state when the event is processed.
> 
> But right before that it says that “GetCurrentModifierState() returns
> current modifier key state. The ‘current’ means actual state of hardware
> keyboard when this is called.”
> 
> Its implementation calls `gdk_display_get_pointer` which in turn calls
> `XQueryPointer.`  If I understand the code correctly, the reason for the
> possible mismatch is that `GdkEventKey::state` can contain out-of-date info,
> not that `GetCurrentModifierState` can.  And my tests bear this out.

Yes, the current state is not what we want.  Events need to indicate the
"out-of-date" state at the time the user action happened.

> Peeking at the next event seems more racy to me, since I don't see any
> guarantee that there's only one XKbStateNotify event in the queue, or that
> it's at the front of the queue.

It would be racy if more than one XKbStateNotify event were considered.  We
only want the state change caused by the event being processed, which is the
first XKbStateNotify event, if any.  If the state change is caused by another
user action, then there will be a different event in the queue before the
XKbStateNotify event.  However, I don't know whether the front-of-queue
assumption will still be valid when XI2 events are introduced.  Your solution
is better for button events regardless, thanks.

Comment 18

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

Updated based on karlt's suggestion from comment 16.  Thanks, this looks much nicer.  Re-requesting review.

(In reply to Karl Tomlinson (ni?:karlt) from comment #17)
> Yes, the current state is not what we want.  Events need to indicate the
> "out-of-date" state at the time the user action happened.

Aha! Now I understand.  Thanks for the explanation.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 8688087 [details] [diff] [review]
patch v3

Great, thanks.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #19)
> Comment on attachment 8688087 [details] [diff] [review]
> patch v3
> 
> Great, thanks.

This patch won't work with multiple mice. Is it okay? IIRC, this was an issue of setting modifier key state. Although, I don't think we need to support correctly instead of not supporting usual cases.

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #20)
> This patch won't work with multiple mice. Is it okay?

I tested with multiple mice and got the behaviour described in comment 14, which means this patch gives the expected results.  I suspect this approach works fine for mouse buttons, but key-associated modifiers need the other approach (which we already have).

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #21)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #20)
> > This patch won't work with multiple mice. Is it okay?
> 
> I tested with multiple mice and got the behaviour described in comment 14,
> which means this patch gives the expected results.  I suspect this approach
> works fine for mouse buttons, but key-associated modifiers need the other
> approach (which we already have).

Oh, I see. Thanks. I missed to catch the comment.

Comment 23

[Matt Brubeck (:mbrubeck)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d06605a330

Comment 24

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f103f5a44ffcbef4fc27617a6a6431b96699631
Bug 1223366 - Update event.buttons on GDK_BUTTON_RELEASE [r=karlt]

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f103f5a44ff