Closed Bug 1098139 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Events, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: evgeny.agafonchikov, Assigned: alessarik)

References

Details

(Keywords: testcase)

Attachments

(1 file, 4 obsolete files)

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."
Blocks: 1042108
Component: Untriaged → DOM: Events
Keywords: testcase
Product: Firefox → Core
+ PATCH for this issue and TEST for this in the same file.
Attachment #8522343 - Flags: review?(bugs)
Attachment #8522343 - Flags: feedback?(mbrubeck)
+ Small changes for naming
Attachment #8522343 - Attachment is obsolete: true
Attachment #8522343 - Flags: review?(bugs)
Attachment #8522343 - Flags: feedback?(mbrubeck)
Attachment #8522362 - Flags: review?(bugs)
Attachment #8522362 - Flags: feedback?(mbrubeck)
Comment on attachment 8522362 [details] [diff] [review]
gotlostpc_events_isprimary_ver1.diff

Why we set mIsPrimary always to true in DispatchGotOrLostPointerCaptureEvent?
Attachment #8522362 - Flags: review?(bugs) → review-
Attachment #8522362 - Flags: feedback?(mbrubeck)
+ Changes: Save isPrimary attribute in PointerInfo
+ Changes: Detect state of isPrimary attribute for got/lostpointercapture events
Attachment #8522362 - Attachment is obsolete: true
Attachment #8523079 - Flags: review?(bugs)
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.
Attachment #8523079 - Flags: review?(bugs) → review-
- Changes: Remove mIsPrimary from PointerInfo
+ Changes: Add mIsPrimary to PointerCaptureInfo
+ Changes: Add global primaryPointers counter
+ Changes: Add CurrentPointerEventCounter
Attachment #8523079 - Attachment is obsolete: true
Attachment #8523868 - Flags: review?(bugs)
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.
Attachment #8523868 - Flags: review?(bugs) → review-
(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
+ 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.
Attachment #8523868 - Attachment is obsolete: true
Attachment #8524406 - Flags: review?(bugs)
(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?
(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.)
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.
(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 on attachment 8524406 [details] [diff] [review]
gotlostpc_events_isprimary_ver4.diff

I guess this works.
Attachment #8524406 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #18)
> I guess this works.
Tests verified that this is works.
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)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/59f3a9bd26f0
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.