Closed Bug 1299209 Opened 8 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bhsu, Assigned: bhsu)

References

Details

Attachments

(4 files, 20 obsolete files)

8.47 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
14.59 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
3.43 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
8.77 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
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.
Blocks: 1299024
Component: Event Handling → DOM: Events
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!
Priority: -- → P2
Assignee: nobody → bhsu
Summary: [Pointer Events] Make `pointerId` a mandatory parameter for test automation APIs → [Pointer Events] Make `pointerId` a mandatory parameter in PE test automation APIs
Summary: [Pointer Events] Make `pointerId` a mandatory parameter in PE test automation APIs → [Pointer Events] Make `pointerId` a mandatory parameter of PE test automation APIs
Attached file Simple Test (obsolete) —
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).
Attachment #8792834 - Flags: feedback?(btseng)
Attachment #8792835 - Flags: feedback?(btseng)
Attachment #8792836 - Flags: feedback?(btseng)
Attachment #8792837 - Flags: feedback?(btseng)
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.
Attachment #8792835 - Flags: feedback?(btseng)
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.
Attachment #8792834 - Flags: feedback?(btseng)
Attachment #8792836 - Flags: feedback?(btseng) → feedback+
Attachment #8792837 - Flags: feedback?(btseng) → feedback+
(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.
Depends on: 1305346
Attachment #8792834 - Attachment is obsolete: true
Attachment #8799306 - Attachment is obsolete: true
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?
Attachment #8799307 - Flags: feedback?(btseng)
Attachment #8799314 - Flags: feedback?(btseng)
Re-uploaded ;P
Attachment #8799314 - Attachment is obsolete: true
Attachment #8799314 - Flags: feedback?(btseng)
Attachment #8799393 - Flags: feedback?(ftseng)
Attachment #8799393 - Flags: feedback?(ftseng) → feedback?(btseng)
Attachment #8799393 - Flags: feedback?(btseng) → feedback+
Attachment #8799307 - Flags: feedback?(btseng) → feedback+
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.
Summary: [Pointer Events] Make `pointerId` a mandatory parameter of PE test automation APIs → [Pointer Events] PointerId should be covered in PE test automation.
Attachment #8792836 - Attachment is obsolete: true
Attachment #8792837 - Attachment is obsolete: true
Attachment #8792838 - Attachment is obsolete: true
Attachment #8799307 - Attachment is obsolete: true
Attachment #8799393 - Attachment is obsolete: true
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.
Attachment #8803756 - Flags: feedback?(sshih)
Attachment #8803757 - Flags: feedback?(sshih)
Attachment #8803758 - Flags: feedback?(sshih)
Attachment #8803759 - Flags: feedback?(sshih)
Attachment #8803757 - Flags: feedback?(sshih) → feedback+
Attachment #8803756 - Flags: feedback?(sshih) → feedback+
Attachment #8803758 - Flags: feedback?(sshih) → feedback+
Attachment #8803759 - Flags: feedback?(sshih) → feedback+
rebased
rebased
Attachment #8803757 - Attachment is obsolete: true
Attachment #8803756 - Attachment is obsolete: true
rebased
Attachment #8803758 - Attachment is obsolete: true
rebased
Attachment #8803759 - Attachment is obsolete: true
Attachment #8819744 - Flags: feedback+
Attachment #8819745 - Flags: feedback+
Attachment #8819746 - Flags: feedback+
Attachment #8819747 - Flags: feedback+
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.
Attachment #8819744 - Flags: review?(bugs)
Attachment #8819745 - Flags: review?(bugs)
Attachment #8819746 - Flags: review?(bugs)
Attachment #8819747 - Flags: review?(bugs)
I'm a bit lost with this bug. Why do we need to pass different kinds of (predefined) pointerIDs when sending mouse events?
I guess the idea is that the generated pointer events will then have different pointer IDs
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.
Attachment #8819744 - Flags: review?(bugs) → review+
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.
Attachment #8819745 - Flags: review?(bugs) → review+
Attachment #8819746 - Flags: review?(bugs) → review+
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.
Attachment #8819747 - Flags: review?(bugs) → review+
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.
Attachment #8819745 - Attachment is obsolete: true
Attachment #8820571 - Flags: review+
Attachment #8819747 - Attachment is obsolete: true
Attachment #8820572 - Flags: review+
Blocks: 1325900
No longer depends on: 1305346
No longer blocks: 1325900
Attachment #8820572 - Attachment is obsolete: true
Attachment #8822027 - Flags: review+
Pushed by ihsiao@mozilla.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: