1299209 - [Pointer Events] PointerId should be covered in PE test automation.

Status: RESOLVED FIXED

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

Description

[Ben Hsu [:HoPang]]

PointerId now is a mandatory parameters for Blink's test automation APIs. I think it's a good idea to do so. Now most platform message sequences used in Gecko are consists of mouse messages, whose pointerId are always assumed to 0. where pointerId are set to 0 in Gecko under certain circumstances. In fact, it's a little risky, since we might skip something like IPC serializer without testing the implementation with other pointerId.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Ben, if I understand your plan correctly, this is something we aim at by the end of this quarter. So, I mark it as P2, i.e. in the next release/few months. Let me know if I misunderstand the plan. Thanks!

Comment 2

[Ben Hsu [:HoPang]]

Attachment: Part 1: Add PointerId to sendMouseEvent() in nsIDOMWindowUtils

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Part 2: Make `pointerId` a mandatory parameter of PE test automation API

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 3: Update the callers of sendMouseEvent()

Comment 5

[Ben Hsu [:HoPang]]

Attachment: Part 4: Update the callers of sendTouchEvent()

Comment 6

[Ben Hsu [:HoPang]]

Attachment: Simple Test

Comment 7

[Ben Hsu [:HoPang]]

In this issue, we want pointerId to be one of the mandatory parameter of PE test automation API. However, after digging into the code for a little bit, I found that it's very difficult to make this happen along the path sending out synthesized widget event. The sending path is listed as follows,

1. Testcase
2. mochitest_support_external.js // PE only
3. EventUtils.js                 // Shared among mochitest testcases
4. nsDOMWindowUtils              // Shared by many including Gecko internal usages, and it's an XPCOM object
5. nsContentUtils                
6. PresShell (Target!)

Since sendMouseEvent of nsDOMWindowUtils is shared by many consumers, it would take too much time update them all, and it might be not that necessary. In order to proceed to other issues, I decided to only make pointerId a mandatory for APIs in mochitest_support_external in this bug.

There are total 4 patches and 1 file attached to this bug.
Patch 1 adds pointerId as an optional parameter to `sendMouseEvent` in nsDOMWindowUtils, and leaving other consumer unaffected.
 
Patch 2 makes pointerId a mandatory parameter of the two PE test automation APIs, while it remains an optional parameter since EventUtils is shared with others.

Patch 3 and Patch 4 respectively update the callers of sendMouseEvent and sendTouchEvent.

Simple test is a patch apply to the helper provided by web-pletform-test to make sure everything works. However, I don't think it's appropriate to land this patch as well, since the patch uses some heuristics to the other patches (PointerId of mouse is always 0, pen's is 1, and touch's is 10).

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8792835 [details] [diff] [review]
Part 2: Make `pointerId` a mandatory parameter of PE test automation API

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

::: testing/mochitest/tests/SimpleTest/EventUtils.js
@@ +393,5 @@
>  {
>    var utils = _getDOMWindowUtils(aWindow);
>  
>    if (utils) {
> +    var pointerId = aEvent.pointerId || 0;

the aEvent.id is used by mozelement.js
http://searchfox.org/mozilla-central/source/services/sync/tps/extensions/mozmill/resource/driver/mozelement.js#519

It looks NG to me if we just rename id to pointerId here.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8792834 [details] [diff] [review]
Part 1: Add PointerId to sendMouseEvent() in nsIDOMWindowUtils

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

::: dom/base/nsContentUtils.cpp
@@ +8203,5 @@
>                             WidgetMouseEvent::eSynthesized :
>                             WidgetMouseEvent::eReal,
>                           contextMenuKey ? WidgetMouseEvent::eContextMenuKey :
>                                            WidgetMouseEvent::eNormal);
> +  event.pointerId = aIdentifier;

The assignment of pointerId is not part of WidgetMouseEvent's constructor.

We should make sure that the value of pointerId of each WidgetMouseEvent instance will be given either a default value of the specified value as what has been done to event.buttons, event.pressure, etc.

Comment 10

[Ben Hsu [:HoPang]]

Attachment: Part 2: Make `pointerId` a mandatory parameter of PE test automation API

Comment 11

[Ben Hsu [:HoPang]]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #9)
> Comment on attachment 8792834 [details] [diff] [review]
> Part 1: Add PointerId to sendMouseEvent() in nsIDOMWindowUtils
> 
> Review of attachment 8792834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsContentUtils.cpp
> @@ +8203,5 @@
> >                             WidgetMouseEvent::eSynthesized :
> >                             WidgetMouseEvent::eReal,
> >                           contextMenuKey ? WidgetMouseEvent::eContextMenuKey :
> >                                            WidgetMouseEvent::eNormal);
> > +  event.pointerId = aIdentifier;
> 
> The assignment of pointerId is not part of WidgetMouseEvent's constructor.
> 
> We should make sure that the value of pointerId of each WidgetMouseEvent
> instance will be given either a default value of the specified value as what
> has been done to event.buttons, event.pressure, etc.

Hi Bevis, 

Thanks for your insight, I think it's time to makes things right, so I filed a bug making pointerId a mandatory parameter also when creating Widget Mouse Event and its variants. Please refer to Bug 1305346 for more information.

Comment 12

[Ben Hsu [:HoPang]]

Attachment: Part 1: Add PointerId to sendMouseEvent() in nsIDOMWindowUtils

Comment 13

[Ben Hsu [:HoPang]]

Attachment: Part 2: Make `pointerId` a mandatory parameter of PE test automation API

Comment 14

[Ben Hsu [:HoPang]]

Attachment: Part 1: Add PointerId to sendMouseEvent() in nsIDOMWindowUtils

Comment 15

[Ben Hsu [:HoPang]]

Hi Bevis, and sorry for bothering you again.

I've updated the patches based on your comments :)

In addition, I've also exposed the default pointer Id as nsIDOMMouseEvent::MOZ_POINTERID_DEFAULT, which should be used when pointerId not specified. In most of current case, this assumed triggered by a mouse. On the other hand, since I am starting to validate the widget event sequences, I'd like to land this bug before 1305346, and thus I've passed pointerId when creating WigetMouseEvent and its descendants.

Do you mind reviewing those patched again?

Comment 16

[Ben Hsu [:HoPang]]

Attachment: Part 1: Add PointerId to sendMouseEvent() in nsIDOMWindowUtils

Re-uploaded ;P

Comment 17

[Ben Hsu [:HoPang]]

Since we are planning to send the synthesized widget events to Chrome process(1312201), it becomes impossible to change the desired pointerId of an input device during testing. Thus, we have to fall back using preset pointerId for testing, and as a result, it becomes not appropriate to add pointerId in to the APIs.

Comment 18

[Ben Hsu [:HoPang]]

Attachment: Part 1: Define an enum for default pointerId

Comment 19

[Ben Hsu [:HoPang]]

Attachment: Part 2: Support setting pointerId in nsDOMWindowwUtils and nsContentUtils

Comment 20

[Ben Hsu [:HoPang]]

Attachment: Part 3: Use the default pointerId if needed in EventUtils.js

Comment 21

[Ben Hsu [:HoPang]]

Attachment: Part 4: Test event.pointerId in PE Framework

Comment 22

[Ben Hsu [:HoPang]]

There are four new patches in total.

Patch 1.
Defining an enum for the default pointerId of different input devices, using those value and passing the pointerId when new widget mouse events are created.

Patch 2.
Support pointerId in nsDOMWindowUtils and nsContentUtils.

Patch 3.
Update EventUtils.js

Patch 4. 
Test the pointerId inside the framework. The desired pointerIds are initiated in mochitest_support_internal, and send the mochitest_support_external. When sending synthesized events, mochitest_support_external uses those pointerIds, and later be checked on every pointerevent in the mochitest_support_internal.

Comment 23

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 1: Define an enum for default pointerId

rebased

Comment 24

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2: Support setting pointerId in nsDOMWindowwUtils and nsContentUtils

rebased

Comment 25

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 3: Use the default pointerId if needed in EventUtils.js

rebased

Comment 26

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 4: Test event.pointerId in PE Framework

rebased

Comment 27

[Ben Hsu [:HoPang]]

Hi Olli,

This bug is for testing the PointerId in current testcases. There are 4 patches in total, and their description is written below.

Patch 1:
Create an enum for default pointerId of various input source, and pass pointerId when new WidgetMouseEvent and its subclasses are created.

Patch 2:
Support pointerId in nsContentUtils and nsDOMWindowUtils.

Patch 3:
Enhance EventUtils.js.

Patch 4:
Since web platform tests don't check pointerId, we have to use some heuristic to test them. and thus pointerIds are send to mochitest_support_external.js. Thus, we set the pointerId for testing in mochitest_support_internal.js, pass it to mochitest_support_external.js for later testing.

Comment 28

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

I'm a bit lost with this bug. Why do we need to pass different kinds of (predefined) pointerIDs when sending mouse events?

Comment 29

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

I guess the idea is that the generated pointer events will then have different pointer IDs

Comment 30

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

Comment on attachment 8819744 [details] [diff] [review]
(V2) Part 1: Define an enum for default pointerId

>@@ -1553,17 +1553,19 @@ TabChild::RecvMouseEvent(const nsString& aType,
>                          const float&    aX,
>                          const float&    aY,
>                          const int32_t&  aButton,
>                          const int32_t&  aClickCount,
>                          const int32_t&  aModifiers,
>                          const bool&     aIgnoreRootScrollFrame)
> {
>   APZCCallbackHelper::DispatchMouseEvent(GetPresShell(), aType, CSSPoint(aX, aY),
>-      aButton, aClickCount, aModifiers, aIgnoreRootScrollFrame, nsIDOMMouseEvent::MOZ_SOURCE_UNKNOWN);
>+      aButton, aClickCount, aModifiers, aIgnoreRootScrollFrame,
>+      nsIDOMMouseEvent::MOZ_SOURCE_UNKNOWN,
>+      WidgetPointerHelper::eDefaultMousePointerId);
>   return IPC_OK();
> }
Looks like SendMouseEvent could take some more params to pass source and id. Or, could we even remove it?
File a followup bug, please.


>@@ -217,17 +217,18 @@ APZEventState::FireContextmenuEvents(const nsCOMPtr<nsIPresShell>& aPresShell,
> {
>   // Converting the modifiers to DOM format for the DispatchMouseEvent call
>   // is the most useless thing ever because nsDOMWindowUtils::SendMouseEvent
>   // just converts them back to widget format, but that API has many callers,
>   // including in JS code, so it's not trivial to change.
>   bool eventHandled =
>       APZCCallbackHelper::DispatchMouseEvent(aPresShell, NS_LITERAL_STRING("contextmenu"),
>                          aPoint, 2, 1, WidgetModifiersToDOMModifiers(aModifiers), true,
>-                         nsIDOMMouseEvent::MOZ_SOURCE_TOUCH);
>+                         nsIDOMMouseEvent::MOZ_SOURCE_TOUCH,
>+                         WidgetPointerHelper::eDefaultTouchPointerId);
This is a bit messy to pass some predefined (touch) id, even if we're dealing with mouse event here, which just has source from touch.
Since we don't want to leak these predefined ids to the web, perhaps better to pass eDefaultMousePointerId here to keep the old behavior and all.



> class WidgetPointerHelper
> {
> public:
>+  // This enum is for internal usage, such as event generating and testing,
>+  // where the pointerId of real WidgetEvents should be given when being
>+  // created in the widget level.
>+  enum defaultPointerId : uint32_t {
>+    eDefaultMousePointerId = 0,
>+    eDefaultPenPointerId   = 1,
>+    eDefaultTouchPointerId = 2  // Extend if multiple touch ids are needed.
>+  };
So should the comment say here that never use these enum value except for testing.

Comment 31

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

Comment on attachment 8819745 [details] [diff] [review]
(V2) Part 2: Support setting pointerId in nsDOMWindowwUtils and nsContentUtils

> 
>+  // Match WidgetPointerHelper::defaultPointerId.
>+  const long DEFAULT_MOUSE_POINTER_ID = 0;
>+  const long DEFAULT_PEN_POINTER_ID   = 1;
>+  const long DEFAULT_TOUCH_POINTER_ID = 2;
Why do we actually need the stuff in WidgetPointerHelper?

If this all is for testing only, shouldn't these consts in nsIDOMWindowUtils be enough? 
Please explain.

Comment 32

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

Comment on attachment 8819747 [details] [diff] [review]
(V2) Part 4: Test event.pointerId in PE Framework

>+// Since web platform tests don't check pointerId, we have to use some heuristic
>+// to test them. and thus pointerIds are send to mochitest_support_external.js
>+// before we start sending synthesized widget events. Here, we avoid using
>+// default values used in Gecko to insure everything works as expected.
>+const POINTER_MOUSE_ID = 9;
>+const POINTER_PEN_ID   = 8;
>+const POINTER_TOUCH_ID = 7; // Extend for multiple touch points if needed.
Why these values? Wouldn't it make sense for mouse to have smallest number, then pen, and touch could use then largest value, and all the values from that
in case we want to test multiple touches.

Comment 33

[Ben Hsu [:HoPang]]

Hi Olli,

(In reply to Olli Pettay [:smaug] from comment #29)
> I guess the idea is that the generated pointer events will then have
> different pointer IDs

Yes, you're totally correct.

> Looks like SendMouseEvent could take some more params to pass source and id.
> Or, could we even remove it?
> File a followup bug, please.

Yes, I also think passing both source and id is the best design, since we shouldn't count on the relation between them, though a mouse seems always get a 0 as its id. 

> This is a bit messy to pass some predefined (touch) id, even if we're
> dealing with mouse event here, which just has source from touch.
> Since we don't want to leak these predefined ids to the web, perhaps better
> to pass eDefaultMousePointerId here to keep the old behavior and all.

This is one of the most suffering part implementing this. If we do not want to leak anything to the Web, is it acceptable to setting those pointerId to 0 with comments addressing this fact at this moment?

> So should the comment say here that never use these enum value except for
> testing.

I'll try to remove the enum :)

> If this all is for testing only, shouldn't these consts in nsIDOMWindowUtils
> be enough? 
> Please explain.

It sounds very reasonabe!

> Why these values? Wouldn't it make sense for mouse to have smallest number,
> then pen, and touch could use then largest value, and all the values from
> that in case we want to test multiple touches.

I have no preference on those values, and I'll revert the order in the next patch :)

Again, thanks for your time reviewing these patches.

Comment 34

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 1: Passing PointerId when a new WidgetMouseEvent is created

Comment 35

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 2: Support setting pointerId in nsDOMWindowwUtils and nsContentUtils

Comment 36

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 4: Test event.pointerId in PE Framework

Comment 37

[Ben Hsu [:HoPang]]

Attachment: (V4) Part 2: Support setting pointerId in nsDOMWindowwUtils and nsContentUtils

rebased

Comment 38

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 3: Use the default pointerId if needed in EventUtils.js

Comment 39

[Ben Hsu [:HoPang]]

Attachment: (V4) Part 4: Test event.pointerId in PE Framework

Comment 40

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c4ced1f71ac325c0d17a8c0b5e6b66ecc6b43a3

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3af21a78f212
Part 1: Passing PointerId when a new WidgetMouseEvent is created. h=sshih, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e485454664f6
Part 2: Support setting pointerId in nsDOMWindowwUtils and nsContentUtils. f=sshih, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7173f691480f
Part 3: Use the default pointerId if needed in EventUtils.js. f=sshih, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d094a3cab23f
Part 4: Test event.pointerId in PE Framework. f=sshih, r=smaug

Comment 42

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

https://hg.mozilla.org/mozilla-central/rev/3af21a78f212
https://hg.mozilla.org/mozilla-central/rev/e485454664f6
https://hg.mozilla.org/mozilla-central/rev/7173f691480f
https://hg.mozilla.org/mozilla-central/rev/d094a3cab23f