Closed Bug 1302297 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bhsu, Assigned: bhsu)

References

Details

Attachments

(6 files, 6 obsolete files)

4.91 KB, patch
smaug
: review+
stone
: feedback+
Details | Diff | Splinter Review
25.28 KB, patch
smaug
: review+
stone
: feedback+
Details | Diff | Splinter Review
1.03 KB, text/plain
Details
4.51 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
4.35 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
11.98 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
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.
Blocks: 1299024
Summary: [Pointer Events] Correcting the `button` value used by current mochitest wrappers. → [Pointer Events] Correct the `button` values used by current mochitest wrappers.
Assignee: nobody → bhsu
Attachment #8803586 - Attachment is obsolete: true
Attached patch Part 5: Update related testcases (obsolete) — Splinter Review
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.
Attachment #8803636 - Flags: feedback?(sshih)
Attachment #8803637 - Flags: feedback?(sshih)
Attachment #8803638 - Flags: feedback?(sshih)
Attachment #8803639 - Flags: feedback?(sshih)
Attachment #8803640 - Flags: feedback?(sshih)
Attachment #8803636 - Flags: feedback?(sshih) → feedback+
Attachment #8803639 - Flags: feedback?(sshih) → feedback+
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.
Attachment #8803640 - Flags: feedback?(sshih) → feedback+
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?
Attachment #8803638 - Flags: feedback?(sshih)
Attachment #8803637 - Flags: feedback?(sshih) → feedback+
Attached file 1302297-Probe
> 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.
Attachment #8803638 - Flags: feedback?(sshih)
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.
Attachment #8803638 - Flags: feedback?(sshih) → feedback+
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.
rebased
Attachment #8803637 - Attachment is obsolete: true
Attachment #8814850 - Flags: feedback+
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.
If I need to review something, my review queue is open again :)
Attachment #8803636 - Flags: review?(bugs)
Attachment #8803638 - Flags: review?(bugs)
Attachment #8803639 - Flags: review?(bugs)
Attachment #8803640 - Flags: review?(bugs)
Attachment #8814850 - Flags: review?(bugs)
Thank you so much!
Comment on attachment 8803636 [details] [diff] [review]
Part 1: Enhance the new window test container for PE testcases

rs+
Attachment #8803636 - Flags: review?(bugs) → review+
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+
Attachment #8814850 - Flags: review?(bugs) → review+
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.
Attachment #8803638 - Flags: review?(bugs) → review+
Comment on attachment 8803639 [details] [diff] [review]
Part 4: Remove every button and buttons used in testcases

rs+
Attachment #8803639 - Flags: review?(bugs) → review+
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?
Attachment #8803640 - Flags: review?(bugs) → review+
Comments addressed.
Attachment #8814850 - Attachment is obsolete: true
Attachment #8817602 - Flags: review+
Comment addressed.
Attachment #8803638 - Attachment is obsolete: true
Attachment #8817603 - Flags: review+
Attachment #8803640 - Attachment is obsolete: true
Attachment #8817604 - Flags: review+
(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 ;)
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.