1098139 - isPrimary pointer event attribute of (got|lost)pointercapture is wrong for mouse

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Evgeny]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141106120505

Steps to reproduce:

1. Open http://www.w3c-test.org/pointerevents/pointerevent_releasepointercapture_events_to_original_target-manual.html test
2. Perform actions according to the test description using mouse


Actual results:

Two Failures are shown in the test results as isPrimary == false for mouse


Expected results:

isPrimary == true for mouse => PASS for all of the tests

Spec item:
https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#the-primary-pointer

"When firing a pointer event, a pointer is considered primary if:
    The pointer represents a mouse device."

Comment 1

[Maksim Lebedev]

Attachment: gotlostpc_events_isprimary_ver1.diff

+ PATCH for this issue and TEST for this in the same file.

Comment 2

[Maksim Lebedev]

Test without patch: https://tbpl.mozilla.org/?tree=Try&rev=982a78cdfe92
Test with patch: https://tbpl.mozilla.org/?tree=Try&rev=44182de1f0c5

Comment 3

[Maksim Lebedev]

Attachment: gotlostpc_events_isprimary_ver1.diff

+ Small changes for naming

Comment 4

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=e7e43f887002

Comment 5

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

Comment on attachment 8522362 [details] [diff] [review]
gotlostpc_events_isprimary_ver1.diff

Why we set mIsPrimary always to true in DispatchGotOrLostPointerCaptureEvent?

Comment 6

[Maksim Lebedev]

Attachment: gotlostpc_events_isprimary_ver2.diff

+ Changes: Save isPrimary attribute in PointerInfo
+ Changes: Detect state of isPrimary attribute for got/lostpointercapture events

Comment 7

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

Comment on attachment 8523079 [details] [diff] [review]
gotlostpc_events_isprimary_ver2.diff

I don't understand how this works with touch based pointer events.
GetPointerPrimaryState relies on gActivePointersIds to have pointer to the pointerinfo object, but such thing is possibly removed in
UpdateActivePointerState.
So I don't see what guarantees that DispatchGotOrLostPointerCaptureEvent actually get the valid value using GetPointerPrimaryState.

Comment 8

[Maksim Lebedev]

Attachment: gotlostpc_events_isprimary_ver3.diff

- Changes: Remove mIsPrimary from PointerInfo
+ Changes: Add mIsPrimary to PointerCaptureInfo
+ Changes: Add global primaryPointers counter
+ Changes: Add CurrentPointerEventCounter

Comment 9

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=dfc180e6ad37

Comment 10

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

Comment on attachment 8523868 [details] [diff] [review]
gotlostpc_events_isprimary_ver3.diff

>+  // GetPointerType returns pointer type like mouse, pen or touch for pointer event with pointerId
>+  static uint16_t GetPointerType(uint32_t aPointerId);
>+
>+  // Keeps information about count of pointer events (with isPrimary = true), passed through HandleEvent
>+  static nsBaseHashtable<nsUint32HashKey, int32_t, int32_t>* gPrimaryPointersCounter;
Use nsDataHashtable
https://developer.mozilla.org/en-US/docs/Detailed_XPCOM_hashtable_guide#Which_Hashtable_Should_I_Use.3F

>+class CurrentPointerEventCounter
Quite odd name. The class doesn't really count any pointer events.
Perhaps PrimaryPointerEventCapturer 

>+{
>+public:
>+  CurrentPointerEventCounter() :
>+    mPointerId(-1)
>+  {
>+  }
>+  ~CurrentPointerEventCounter()
>+  {
>+    if (mPointerId != -1) {
>+      int32_t count = 0;
>+      if (nsIPresShell::gPrimaryPointersCounter->Get(mPointerId, &count)) {
>+        --count;
>+      }
>+      if (count) {
>+        nsIPresShell::gPrimaryPointersCounter->Put(mPointerId, count);
>+      } else {
>+        nsIPresShell::gPrimaryPointersCounter->Remove(mPointerId);
>+      }
>+    }
>+  }
>+  void CalculatePointerEvent(WidgetPointerEvent* aEvent)
>+  {
>+    if (aEvent->isPrimary) {
>+      mPointerId = aEvent->pointerId;
>+      int32_t count = 0;
>+      nsIPresShell::gPrimaryPointersCounter->Get(mPointerId, &count);
>+      nsIPresShell::gPrimaryPointersCounter->Put(mPointerId, ++count);
>+    }
>+  }
I wouldn't do any count magic. Just store information if the hashtable had anything for the pointer already, and if so, do nothing.
If CalculatePointerEvent adds something to the hashtable, the destructor should remove it.

Comment 11

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #10)
> I wouldn't do any count magic. Just store information if the hashtable had
> anything for the pointer already, and if so, do nothing.
> If CalculatePointerEvent adds something to the hashtable, the destructor should remove it.
In this case we can get wrong results when HandleEvent receives multiple events with same pointerId

Comment 12

[Maksim Lebedev]

Attachment: gotlostpc_events_isprimary_ver4.diff

+ Changes: Re-add primaryState into PointerInfo
+ Changes: Add correct work with isPrimary attribute in test suite
- Changes: Remove gPrimaryPointersCounter as unwanted
- Changes: Remove CurrentPointerEventCounter as unused

The main idea of patch_4 is collaborate technologies from patch_2 and patch_3.

Comment 13

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=f101061079a2

Comment 14

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

(In reply to Maksim Lebedev from comment #11)
> In this case we can get wrong results when HandleEvent receives multiple
> events with same pointerId
How so?

Comment 15

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #14)
> (In reply to Maksim Lebedev from comment #11)
> > In this case we can get wrong results when HandleEvent receives multiple
> > events with same pointerId
> How so?
For example: pointerDown (on onPointerDown call setPointerCapture) after it PointerCaptureEvent pass through HandleEvent. In this case we have 2 events with the same pointerId. If we will remove data from table at destructor we receives that after second event we get empty table (but pointerDown is still exist in HandleEvent.)

Comment 16

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

But you would remove the data only when the outer "CalculatePointerEvent" goes out of scope, since
the inner one wouldn't add anything, it wouldn't also remove anything.

Comment 17

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #16)
> But you would remove the data only when the outer "CalculatePointerEvent"
> goes out of scope, since the inner one wouldn't add anything, it wouldn't also remove anything.
I got your point. Maybe it is right.
But I found another idea for correct detecting primaryState without adding extra storage.

Comment 18

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

Comment on attachment 8524406 [details] [diff] [review]
gotlostpc_events_isprimary_ver4.diff

I guess this works.

Comment 19

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #18)
> I guess this works.
Tests verified that this is works.

Comment 20

[Evgeny]

Desktop Win 8 x64 version works correctly now (Can't start Win 8 x64 in Modern UI mode and Win XP version is missing in this build: https://tbpl.mozilla.org/?tree=Try&rev=5d503aba6803, according to bug 1100984)

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59f3a9bd26f0

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59f3a9bd26f0