1302297 - [Pointer Events] Correct the `button` values used by current mochitest wrappers.

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Ben Hsu [:HoPang]]

Current mochitest wrappers for Pointer Events web-platform testcases might send Widget Mouse Events with inappropriate `button` value, since `button:0` for a mouse means that the left button is pressed instead of no button is pressed. Should investigate what values are desired for each testcase.

Comment 1

[Ben Hsu [:HoPang]]

Attachment: Part 1: Exposing enums for button and buttons

Comment 2

[Ben Hsu [:HoPang]]

Attachment: Part 2: Checkout or compute button and buttons values

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Part 1: Enhance the new window test container for PE testcases

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 2: Introduce mouse and touch event helpers

Comment 5

[Ben Hsu [:HoPang]]

Attachment: Part 3: Enhance sendMouseEvent() and sendTouchEvent()

Comment 6

[Ben Hsu [:HoPang]]

Attachment: Part 4: Remove every button and buttons used in testcases

Comment 7

[Ben Hsu [:HoPang]]

Attachment: Part 5: Update related testcases

Comment 8

[Ben Hsu [:HoPang]]

There are five patches for this issue, 

Patch 1.
Since I suffered badly when add some test on the mochitest wrapper ("Result logged after SimpleTest.finish()");), I make the communication between the two windows aync. Doing this also makes the code there more clear.

Patch 2.
Introduce two object handling the event sending and exposing needed enums.

Patch 3.
The two function are ungraded, sendMouseEvent and sendTouchEvent now calculate the buttons itself and check the sequence as well(button/buttons mask check, and whether all button are release on exit). Worth mentioning, though buttons are now generated, the values have been verified matching the real devices.

Patch 4. [AUTO-GEN]
Some of the testcases use middle button, but after diving into those case, I find being tested with left button is more appropriate, so I removed all button/buttons here.

Patch 5.
Update the testcase. Most of them are matching mousedown/mouseup and touchstart/touchend. Only chorded-button is truly updated with the button/buttons values matching the real case.

Comment 9

[Ming-Chou Shih [:stone]]

Comment on attachment 8803640 [details] [diff] [review]
Part 5: Update related testcases

It would be easier to understand if you can merge part4 and part5.

Comment 10

[Ming-Chou Shih [:stone]]

Comment on attachment 8803638 [details] [diff] [review]
Part 3: Enhance sendMouseEvent() and sendTouchEvent()

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

::: dom/events/test/pointerevents/mochitest_support_external.js
@@ +126,5 @@
> +    // value of WidgetMouseEvent.button is still 0, which is the same value as
> +    // the one for mouse left button.
> +    if (mouseEventType === "mousemove") {
> +      eventObj.button = ME.BTN_LEFT;
> +    }

Should mousemove's button always be left button down even when some button (ex. middle button) pressed?

Comment 11

[Ben Hsu [:HoPang]]

Attachment: 1302297-Probe

Comment 12

[Ben Hsu [:HoPang]]

> Should mousemove's button always be left button down even when some button
> (ex. middle button) pressed?

Sorry for the late, since it took me some time to setup a windows machine. IMHO, the button value doesn't (and maybe shouldn't) represent anything. For example, when moving the mouse without any button pressed, the value is still 0. Moreover, when there are multiple buttons pressed, it's hard to assign an adequate value for this single property.

However, I make the value as resemble to the values used in Windows platform. To observe that, you can try to apply 1302297-Probe, and use the following command to observer the value in real world.

```
./mach run 2>&1 | grep --line-buffered "BW:" | uniq
```

In fact, the button value of mousemove is always 0 on windows and my Ubuntu, but it does carry a value on OSX.

Comment 13

[Ming-Chou Shih [:stone]]

Comment on attachment 8803638 [details] [diff] [review]
Part 3: Enhance sendMouseEvent() and sendTouchEvent()

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

::: dom/events/test/pointerevents/mochitest_support_external.js
@@ +102,5 @@
> +        return params.button;
> +      }
> +
> +      // Using the default value.
> +      return isButtonEvent ? ME.BTN_LEFT : ME.BTN_NONE;

According to [1] 4.3.4.4 ~ 4.3.4.8, the button value of mouseenter, mouseleave, mousemove, mouseout, and mouseover is 0. How about return 0 here?

[1] https://w3c.github.io/uievents/

@@ +126,5 @@
> +    // value of WidgetMouseEvent.button is still 0, which is the same value as
> +    // the one for mouse left button.
> +    if (mouseEventType === "mousemove") {
> +      eventObj.button = ME.BTN_LEFT;
> +    }

Looks like you don't need to override the button of mousemove since you already set it.

Comment 14

[Ben Hsu [:HoPang]]

Thanks for your input.

> @@ +102,5 @@
> > +        return params.button;
> > +      }
> > +
> > +      // Using the default value.
> > +      return isButtonEvent ? ME.BTN_LEFT : ME.BTN_NONE;
> 
> According to [1] 4.3.4.4 ~ 4.3.4.8, the button value of mouseenter,
> mouseleave, mousemove, mouseout, and mouseover is 0. How about return 0 here?
> 
> [1] https://w3c.github.io/uievents/

Firstly, I should still care about mouseup and mousedown here. Then, I need BTN_NONE(-1) here to compute the right buttons mask, so I can't use 0 here. Last, I should reproduce the real information provided by the platform loyally here, as a result, I cannot use the value defined by the spec directly. IMHO, if there is difference between the spec and the platform information, the difference should be dealt inside our implementation.

> @@ +126,5 @@
> > +    // value of WidgetMouseEvent.button is still 0, which is the same value as
> > +    // the one for mouse left button.
> > +    if (mouseEventType === "mousemove") {
> > +      eventObj.button = ME.BTN_LEFT;
> > +    }
> 
> Looks like you don't need to override the button of mousemove since you
> already set it.

Since the button value might be BTN_NONE(-1), I should replace the value to make it conform the value given by the platform.

Comment 15

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2: Introduce mouse and touch event helpers

rebased

Comment 16

[Ben Hsu [:HoPang]]

Hi Olli,

In this bug, I revisit the all the testcases synthesizing mouseup/mousedown and touchstart/touchend. I mainly check the sanity of the used widget event sequences. In addition, since mochitest use the same test container to run all the testcases, I balance the number of mouseups/mousedowns and touchstart/touchend in each testcase to avoid the side-effect caused by accumulating button state within gecko. I also check the used button values and generate the desired buttons values to improve the simplicity of the testcases. Below is a more detailed description of each patches.

Patch 1:
Make the communication between the mochitest test main window and the web-platform-test window become asynchronized, since I suffered badly when exercising some checks in the mochitest test main window, which is also needed for following test automation improvement.

Patch 2:
Introduce two helper objects to do some checks and log needed information for synthesize mouse events and synthesized touch event, respectively.

Patch 3:
Enhance the sendMouseEvent by checking the `button` values passed in and generating the `buttons` value automatically, where the button value is always assigned 0. Beside, the checks for whether the numbers of  mouseups/mousedowns and touchstart/touchend match is also implemented in this patch.

Another interesting thing is that button value for mouse move differs on different platforms (Windows, Linux and OSX). Say a mousemove with middle button pressed, the button value on Windows and Linux is 0 (stands for left button), while the one on OSX is 1 (stands for middle button). In fact, stone pointed out the button value of a "mousemove" should be 0 in the nightly spec[0], maybe we can fire a bug for it if needed.

[0] https://w3c.github.io/uievents/#event-type-mousemove

Patch 4:
This patch is auto-gen'ed. After investigating the testcases, I found some of our testcases using the middle button (value 1) instead of using the left button (value 0), which can all be tested with left button, so I remove the all the optional parameters including the button value, and later manually  restore them in Patch 5.

Patch 5:
Manually Balance the number of mouseups/mousedowns and touchstart/touchend, and update the sequence used in test_pointerevent_pointermove-on-chorded-mouse-button.html, which is the testcase check the button/buttons for chorded button cases.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

If I need to review something, my review queue is open again :)

Comment 18

[Ben Hsu [:HoPang]]

Thank you so much!

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8803636 [details] [diff] [review]
Part 1: Enhance the new window test container for PE testcases

rs+

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8814850 [details] [diff] [review]
(V2) Part 2: Introduce mouse and touch event helpers

>+// Mouse Event Helper Object
>+var ME = (function() {
Could you possibly call this MouseEventHelper or some such. ME is rather short and vague

>+  var utils = SpecialPowers.Ci.nsIDOMWindowUtils;
>+
>+  return {
>+    // State
>+    // TODO: Sperate this to support mouse and pen simultaneously.
Separate

>+    BTNS_STATE: utils.MOUSE_BUTTONS_NO_BUTTON,
Would be easier to read if the state was called BUTTONS_STATE

>+
>+    // Button
>+    BTN_NONE:   -1, // Used by test framework only. (replaced before sending)
>+    BTN_LEFT:   utils.MOUSE_BUTTON_LEFT_BUTTON,
>+    BTN_MIDDLE: utils.MOUSE_BUTTON_MIDDLE_BUTTON,
>+    BTN_RIGHT:  utils.MOUSE_BUTTON_RIGHT_BUTTON,
and here BUTTON_NONE, BUTTON_LEFT etc

>+
>+    // Buttons
>+    BTNS_NONE:   utils.MOUSE_BUTTONS_NO_BUTTON,
>+    BTNS_LEFT:   utils.MOUSE_BUTTONS_LEFT_BUTTON,
>+    BTNS_MIDDLE: utils.MOUSE_BUTTONS_MIDDLE_BUTTON,
>+    BTNS_RIGHT:  utils.MOUSE_BUTTONS_RIGHT_BUTTON,
>+    BTNS_4TH:    utils.MOUSE_BUTTONS_4TH_BUTTON,
>+    BTNS_5TH:    utils.MOUSE_BUTTONS_5TH_BUTTON,
here BUTTONS_NONE, etc.
And rename in the code using this stuff.

>+    computeButtonsMaskFromButton: function(aButton) {
>+      // Since the range of button values is 0 ~ 2 (see nsIDOMWindowUtils.idl),
>+      // we can use an array to find out the desired mask.
>+      var mask = [
>+        this.BTNS_NONE,   // -1 (ME.BTN_NONE)
>+        this.BTNS_LEFT,   // 0
>+        this.BTNS_MIDDLE, // 1
>+        this.BTNS_RIGHT   // 2
>+      ][aButton + 1];
>+
>+      ok(mask !== undefined, "Unrecognised button value caught!");
Unrecognized


>+// Touch Event Helper Object
>+var TE = {
This could be TouchEventHelper

>   /**
>     * Returns the GPU process pid, or -1 if there is no GPU process.
>     */
>   readonly attribute int32_t gpuProcessPid;
> 
>+  // Match WidgetMouseEventBase::buttonsFlag.
Not buttonsFlag, but buttonType


Those fixed, r+

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8803638 [details] [diff] [review]
Part 3: Enhance sendMouseEvent() and sendTouchEvent()

>+    // Check or generate a |button| value.
>+    eventObj.button = (function() {
Why you need this function. Why not just some ifs and elses here?

>+      var isButtonEvent = mouseEventType === "mouseup" ||
>+                          mouseEventType === "mousedown";
>+
>+      // |button| is passed, use and check it.
>+      if (params && "button" in params) {
>+        var hasButtonValue = params.button !== ME.BTN_NONE;
>+        ok(!isButtonEvent || hasButtonValue,
>+           "Inappropriate |button| value caught.");
>+        return params.button;
>+      }
>+
>+      // Using the default value.
>+      return isButtonEvent ? ME.BTN_LEFT : ME.BTN_NONE;
>+    }) ();
>+
>+    // Generate a |buttons| value and update buttons state
>+    eventObj.buttons = (function() {
same here.
I think using functions make this code harder to read than needed.
Plain old if-else is simpler.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8803639 [details] [diff] [review]
Part 4: Remove every button and buttons used in testcases

rs+

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8803640 [details] [diff] [review]
Part 5: Update related testcases

are these tests from wpt? If so, do we need to update also the actual wpt tests so that fixes get to W3C's github repo?

Comment 24

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 2: Introduce mouse and touch event helpers

Comments addressed.

Comment 25

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 3: Enhance sendMouseEvent() and sendTouchEvent()

Comment addressed.

Comment 26

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 5: Update related testcases

Comment 27

[Ben Hsu [:HoPang]]

(In reply to Olli Pettay [:smaug] from comment #23)
> Comment on attachment 8803640 [details] [diff] [review]
> Part 5: Update related testcases
> 
> are these tests from wpt? If so, do we need to update also the actual wpt
> tests so that fixes get to W3C's github repo?

Since only the mochitest wrappers are updated, the wpt tests remain untouched ;)

Comment 28

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=056752a86e317c3733059b8ef3d5b17707b95fb4

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7538c4d8bd02
Part 1: Enhance the new window test container for PE testcases. f=stone, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5574e0e702d3
Part 2: Introduce mouse and touch event helpers. f=stone, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7615dd40ba
Part 3: Enhance sendMouseEvent() and sendTouchEvent(). f=stone, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/13fd65d8a143
Part 4: Remove every button and buttons used in testcases. f=stone, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/55a0e013901a
Part 5: Update related testcases. f=stone, r=smaug

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7538c4d8bd02
https://hg.mozilla.org/mozilla-central/rev/5574e0e702d3
https://hg.mozilla.org/mozilla-central/rev/9f7615dd40ba
https://hg.mozilla.org/mozilla-central/rev/13fd65d8a143
https://hg.mozilla.org/mozilla-central/rev/55a0e013901a