Closed Bug 1292070 Opened 3 years ago Closed 3 years ago

[Pointer Event] Incorrect values of button and buttons of pointer events when multiple buttons pressed

Categories

(Core :: DOM: Events, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: stone, Assigned: stone)

References

Details

Attachments

(4 files, 11 obsolete files)

1.91 KB, patch
stone
: review+
stone
: feedback+
Details | Diff | Splinter Review
17.26 KB, patch
stone
: review+
stone
: feedback+
Details | Diff | Splinter Review
3.92 KB, patch
stone
: review+
stone
: feedback+
Details | Diff | Splinter Review
3.86 KB, patch
stone
: review+
stone
: feedback+
Details | Diff | Splinter Review
Blocks: 1292062
Priority: -- → P3
Summary: [Pointer Event] Values of button and buttons in a chorded pointerdown event should follow pointer events spec → [Pointer Event] Incorrect values of button and buttons of pointer events when multiple buttons pressed
Assignee: nobody → sshih
For example, when user clicks left and middle buttons, Edge and Chrome fire pointer events as followings
pointermove (button=-1, buttons=0)
pointerdown (button=0, buttons=1)    //pressed left button
pointermove (button=-1, buttons=1)
pointermove (button=1, buttons=5)    //pressed middle button
pointermove (button=-1, buttons=5)
pointermove (button=1, buttons=1)    //release middle button
pointermove (button=-1, buttons=1)
pointerup (button=0, buttons=0)      //release left button
pointermove (button=-1, buttons=0)
Attachment #8781434 - Flags: feedback?(btseng)
Attachment #8781434 - Attachment is obsolete: true
Attachment #8782364 - Flags: feedback?(btseng)
Attachment #8782365 - Attachment is obsolete: true
Attachment #8782369 - Flags: feedback?(btseng)
Attachment #8782366 - Flags: feedback?(btseng)
Attachment #8782367 - Flags: feedback?(btseng)
(In reply to Ming-Chou Shih [:stone] from comment #0)
> Reference
> https://github.com/w3c/pointerevents/issues/4

Update reference in PE level 2 sepc:
https://w3c.github.io/pointerevents/#the-pointermove-event
"
Additionally, when a pointer changes button state, pressure, tangential pressure, tilt, twist, or contact geometry (e.g. width and height) and the circumstances produce no other pointer events defined in this specification then a user agent MUST fire a pointer event named pointermove.
"
Comment on attachment 8782364 [details] [diff] [review]
Part1: Fire pointermove when no button state changes (V2)

Review of attachment 8782364 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks!
Attachment #8782364 - Flags: feedback?(btseng) → feedback+
Comment on attachment 8782369 [details] [diff] [review]
Part2: Add API to synthesize mouse event with buttons. (V2)

Review of attachment 8782369 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to revisit the patch again after the following issue addressed, thanks!

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +453,5 @@
>                                [optional] in boolean aIsDOMEventSynthesized,
>                                [optional] in boolean aIsWidgetEventSynthesized);
>  
> +  /** The same as sendMouseEvent but adds additional argument aButtons to send
> +   *  mouse events with multiple buttons pressed

Let add "buttons" as a new optional parameter in sendMouseEvent instead.
In addition, please also define button mask constants in this idl for the caller to refer to.
Attachment #8782369 - Flags: feedback?(btseng)
Comment on attachment 8782367 [details] [diff] [review]
Part4: Enable test_pointerevent_pointermove-on-chorded-mouse-button.html. (V2)

Review of attachment 8782367 [details] [diff] [review]:
-----------------------------------------------------------------

f=me after the following issues are addressed.

::: dom/events/test/pointerevents/test_pointerevent_pointermove-on-chorded-mouse-button.html
@@ +16,5 @@
>        function startTest() {
>          var iframe = document.getElementById("testFrame");
>          iframe.src = "pointerevent_pointermove-on-chorded-mouse-button.html";
>        }
> +      function fireMouseEvent(int_win, eventType, params) {

we can move this to the test helper script instead.

@@ +37,5 @@
> +        fireMouseEvent(int_win, "mousedown", {button:1,  buttons:5});
> +        fireMouseEvent(int_win, "mousemove", {button:1,  buttons:5});
> +        fireMouseEvent(int_win, "mouseup",   {button:1,  buttons:1});
> +        fireMouseEvent(int_win, "mousemove", {button:1,  buttons:1});
> +        fireMouseEvent(int_win, "mouseup",   {button:0,  buttons:0});

We could have named constants of these bit masks in the helper script to improve the readability of these mouse events.
Attachment #8782367 - Flags: feedback?(btseng) → feedback+
Comment on attachment 8782366 [details] [diff] [review]
Part3: Update test cases to synthesize mouse events with correct attributes. (V2)

Review of attachment 8782366 [details] [diff] [review]:
-----------------------------------------------------------------

f=me after the nits are addressed, thanks!

::: dom/events/test/pointerevents/test_pointerevent_lostpointercapture_is_first-manual.html
@@ +18,5 @@
>          iframe.src = "pointerevent_lostpointercapture_is_first-manual.html";
>        }
>        function executeTest(int_win) {
> +        sendMouseEvent(int_win, "btnCapture", "mousedown", {button:0});
> +        sendMouseEvent(int_win, "btnCapture", "mouseup",   {button:0});

nit: we don't need the alignment of the parameters amount different function calls:
, {button:0});

::: dom/events/test/pointerevents/test_pointerevent_pointertype_mouse-manual.html
@@ +18,5 @@
>          iframe.src = "pointerevent_pointertype_mouse-manual.html";
>        }
>        function executeTest(int_win) {
> +        sendMouseEvent(int_win, "target0", "mousedown", {button:0});
> +        sendMouseEvent(int_win, "target0", "mouseup",   {button:0});

ditto.

::: dom/events/test/pointerevents/test_pointerevent_pointerup_isprimary_same_as_pointerdown-manual.html
@@ +18,5 @@
>          iframe.src = "pointerevent_pointerup_isprimary_same_as_pointerdown-manual.html";
>        }
>        function executeTest(int_win) {
> +        sendMouseEvent(int_win, "target0", "mousedown", {button:0});
> +        sendMouseEvent(int_win, "target0", "mouseup",   {button:0});

ditto.
Attachment #8782366 - Flags: feedback?(btseng) → feedback+
rebased
Attachment #8782364 - Attachment is obsolete: true
Attachment #8784228 - Flags: feedback+
Refined the API to synthesize mouse events
Attachment #8782369 - Attachment is obsolete: true
Attachment #8784229 - Flags: feedback?(btseng)
Rebased
Attachment #8782366 - Attachment is obsolete: true
Attachment #8784230 - Flags: feedback+
Refined arguments and used the modified API to synthesize mouse events
Attachment #8782367 - Attachment is obsolete: true
Attachment #8784231 - Flags: feedback+
Comment on attachment 8784229 [details] [diff] [review]
Add API to synthesize mouse event with buttons. (V3)

Review of attachment 8784229 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to review again after the followings are addressed, thanks!

::: dom/base/nsContentUtils.cpp
@@ +8148,5 @@
>                           contextMenuKey ? WidgetMouseEvent::eContextMenuKey :
>                                            WidgetMouseEvent::eNormal);
>    event.mModifiers = GetWidgetModifiers(aModifiers);
>    event.button = aButton;
> +  event.buttons = aButtons != nsIDOMWindowUtils::MOUSE_BUTTONS_UNINIT ?

s/MOUSE_BUTTONS_UNINIT/MOUSE_BUTTONS_NOT_SPECIFIED

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1950,5 @@
> +  const long MOUSE_BUTTONS_NO_BUTTON = 0x00;
> +  const long MOUSE_BUTTONS_LEFT_BUTTON = 0x01;
> +  const long MOUSE_BUTTONS_RIGHT_BUTTON = 0x02;
> +  const long MOUSE_BUTTONS_MIDDLE_BUTTON = 0x04;
> +  // typicall, "back" button being left side of 5-button

s/typicall/Typically

@@ +1953,5 @@
> +  const long MOUSE_BUTTONS_MIDDLE_BUTTON = 0x04;
> +  // typicall, "back" button being left side of 5-button
> +  // mice, see "buttons" attribute document of DOM3 Events.
> +  const long MOUSE_BUTTONS_4TH_BUTTON = 0x08;
> +  // typicall, "forward" button being right side of 5-button

ditto

@@ +1956,5 @@
> +  const long MOUSE_BUTTONS_4TH_BUTTON = 0x08;
> +  // typicall, "forward" button being right side of 5-button
> +  // mice, see "buttons" attribute document of DOM3 Events.
> +  const long MOUSE_BUTTONS_5TH_BUTTON = 0x10;
> +  // uninitialized, let nsContentUtils::SendMouseEvent calculate it from button

// Buttons are not specified, will be calculated from |aButton|.

@@ +1957,5 @@
> +  // typicall, "forward" button being right side of 5-button
> +  // mice, see "buttons" attribute document of DOM3 Events.
> +  const long MOUSE_BUTTONS_5TH_BUTTON = 0x10;
> +  // uninitialized, let nsContentUtils::SendMouseEvent calculate it from button
> +  const long MOUSE_BUTTONS_UNINIT = 0xFF;

we should set this one as -1 instead, otherwise this will restrict the number of flags to 7.
In addition, I'd prefer to MOUSE_BUTTONS_NOT_SPECIFIED.

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +507,5 @@
>    NS_ENSURE_TRUE(aPresShell, true);
>  
>    bool defaultPrevented = false;
>    nsContentUtils::SendMouseEvent(aPresShell, aType, aPoint.x, aPoint.y,
> +      aButton, /* aButtons = */ nsIDOMWindowUtils::MOUSE_BUTTONS_UNINIT,

/* XXX */ is to improve the readability for true|false arguements.
The MOUSE_BUTTONS_XXX is already readable.

::: testing/mochitest/tests/SimpleTest/EventUtils.js
@@ +365,5 @@
>        ("isSynthesized" in aEvent) ? aEvent.isSynthesized : true;
>      var isWidgetEventSynthesized =
>        ("isWidgetEventSynthesized" in aEvent) ? aEvent.isWidgetEventSynthesized : false;
> +    var buttons = ("buttons" in aEvent) ? aEvent.buttons :
> +                                          utils.MOUSE_BUTTONS_UNINIT;

MOUSE_BUTTONS_NOT_SPECIFIED
Attachment #8784229 - Flags: feedback?(btseng)
Attachment #8784229 - Attachment is obsolete: true
Attachment #8784277 - Flags: feedback?(btseng)
Comment on attachment 8784277 [details] [diff] [review]
Part2: Add API to synthesize mouse event with buttons. (V4)

Review of attachment 8784277 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +509,5 @@
>    bool defaultPrevented = false;
>    nsContentUtils::SendMouseEvent(aPresShell, aType, aPoint.x, aPoint.y,
> +      aButton, nsIDOMWindowUtils::MOUSE_BUTTONS_NOT_SPECIFIED, aClickCount,
> +      aModifiers, aIgnoreRootScrollFrame, 0, aInputSourceArg, false,
> +      &defaultPrevented, false, /* aIsWidgetEventSynthesized = */ false);

nit: /* aIsWidgetEventSynthesized */ without "="
Attachment #8784277 - Flags: feedback?(btseng) → feedback+
Attachment #8784228 - Flags: review?(bugs)
Attachment #8784230 - Flags: review?(bugs)
Attachment #8784231 - Flags: review?(bugs)
Attachment #8784277 - Flags: review?(bugs)
Attachment #8784228 - Flags: review?(bugs) → review+
Comment on attachment 8784277 [details] [diff] [review]
Part2: Add API to synthesize mouse event with buttons. (V4)

>-  event.buttons = GetButtonsFlagForButton(aButton);
>+  event.buttons = aButtons != nsIDOMWindowUtils::MOUSE_BUTTONS_NOT_SPECIFIED ?
>+                  aButtons :
>+                  msg == eMouseUp ? 0 : GetButtonsFlagForButton(aButton);
Why 0 when msg == eMouseUp?
That looks wrong. buttons can be non-zero when doing mouseup if there are still other buttons down.

So, msg == eMouseUp ? 0 :
removed, r+.
Attachment #8784277 - Flags: review?(bugs) → review+
though, I guess one should then pass buttons explicitly. So, nevermind my comment, the patch is fine.
Attachment #8784230 - Flags: review?(bugs) → review+
Attachment #8784231 - Flags: review?(bugs) → review+
Rebased and updated the patch summary.
Attachment #8784228 - Attachment is obsolete: true
Attachment #8786170 - Flags: review+
Attachment #8786170 - Flags: feedback+
Rebased and updated the patch summary.
Attachment #8784277 - Attachment is obsolete: true
Attachment #8786171 - Flags: review+
Attachment #8786171 - Flags: feedback+
Rebased and updated the patch summary.
Attachment #8784230 - Attachment is obsolete: true
Attachment #8786172 - Flags: review+
Attachment #8786172 - Flags: feedback+
Rebased and updated the patch summary.
Attachment #8784231 - Attachment is obsolete: true
Attachment #8786174 - Flags: review+
Attachment #8786174 - Flags: feedback+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1689533031c3
Part 1: Fire pointermove when no button state changes. f=bevistseng, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd4a10213eb
Part 2: Add API to synthesize mouse event with buttons. f=bevistseng, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd3d0b6b7f71
Part 3: Update test cases to synthesize mouse events with correct attributes. f=bevistseng, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d33661c03e1e
Part 4: Enable test_pointerevent_pointermove-on-chorded-mouse-button.html. f=bevistseng, r=smaug
Keywords: checkin-needed
Blocks: 1296494
Duplicate of this bug: 1299215
You need to log in before you can comment on or make changes to this bug.