Closed Bug 1073563 Opened 7 years ago Closed 7 years ago

The lostpointercapture event must be dispatched before any other pointer events

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: alessarik, Assigned: alessarik)

References

Details

Attachments

(2 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140825202822

Steps to reproduce:

pointerevent_lostpointercapture_is_first-manual.html


Actual results:

Test assertion 11.1


Expected results:

After pointer capture is released for a pointer, and prior to any subsequent events for the pointer, the lostpointercapture event must be dispatched to the element from which pointer capture was removed.
Blocks: 1042108
OS: Windows 8 → All
Hardware: x86_64 → All
Version: 32 Branch → Trunk
Attached patch lostpc_before_others_ver1.diff (obsolete) — Splinter Review
Patch resolved 11.1 assertion in official tests
Attachment #8496025 - Flags: review?(bugs)
Attachment #8496025 - Flags: feedback?(mbrubeck)
Couple of tests to check async behavior of lostpointercapture event
Attachment #8496027 - Flags: review?(bugs)
Attachment #8496027 - Flags: feedback?(mbrubeck)
Component: Untriaged → Layout
Product: Firefox → Core
Comment on attachment 8496025 [details] [diff] [review]
lostpc_before_others_ver1.diff

>-  static void CheckPointerCaptureState(uint32_t aPointerId);
>+  static bool CheckPointerCaptureState(uint32_t aPointerId);
The method needs some comment, especially what the return value means.

>-/* static */ void
>+/* static */ bool
> nsIPresShell::CheckPointerCaptureState(uint32_t aPointerId)
> {
>+  bool ret = false;
Perhaps rename ret to didDispatchEvent

>     if (aEvent->mClass == ePointerEventClass) {
>       if (WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent()) {
>         // Before any pointer events, we should check state of pointer capture,
>         // Thus got/lostpointercapture events emulate asynchronous behavior.
>-        CheckPointerCaptureState(pointerEvent->pointerId);
>+        while(true) {
>+          // Got/lostpointercapture handlers can change capturing state,
>+          // That's why we should re-check pointer capture state.
>+          if (!CheckPointerCaptureState(pointerEvent->pointerId))
>+            break;
>+        }
Why not just 
while (CheckPointerCaptureState(pointerEvent->pointerId));
Please test manually that slow-script warning of the browser kicks in at some point if we actually end up to this loop.
(IIRC it takes ~20s before slow-script warning is shown)
Attachment #8496025 - Flags: review?(bugs) → review+
Attachment #8496027 - Flags: review?(bugs) → review+
Attachment #8496025 - Flags: feedback?(mbrubeck) → feedback+
Attachment #8496027 - Flags: feedback?(mbrubeck) → feedback+
Attached patch lostpc_before_others_ver2.diff (obsolete) — Splinter Review
+ Changes according with comments
Attachment #8496025 - Attachment is obsolete: true
Attachment #8501223 - Flags: review?(bugs)
Comment on attachment 8501223 [details] [diff] [review]
lostpc_before_others_ver2.diff

>+        while(CheckPointerCaptureState(pointerEvent->pointerId))
>+          ;
while(CheckPointerCaptureState(pointerEvent->pointerId));

Did you check that if we enter that loop, slow-script warning kicks in at some point?
If not, please do.
Attachment #8501223 - Flags: review?(bugs) → review+
+ Changes for correct testing
Attachment #8496027 - Attachment is obsolete: true
Attachment #8501595 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #3)
> Please test manually that slow-script warning of the browser kicks in at
> some point if we actually end up to this loop.
> (IIRC it takes ~20s before slow-script warning is shown)
(In reply to Olli Pettay [:smaug] from comment #5)
> Did you check that if we enter that loop, slow-script warning kicks in at
> some point? If not, please do.
I tested manually in DEBUG build. Yes, sometimes I saw slow-script warning.
- Changes according with comments
Attachment #8501223 - Attachment is obsolete: true
Attachment #8501605 - Flags: review?(bugs)
Attachment #8501605 - Flags: review?(bugs) → review+
Attachment #8501595 - Flags: review?(bugs) → review+
If nobody have objections, I will put checkin flag...
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/245fec9779cf
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Wes, could You please check lostpc_before_others_test_ver2.diff was push into m-c or not?
Flags: needinfo?(kwierso)
You can see exactly what landed by looking at the link in comment 12. Is that the wrong version?
Also, you can avoid confusing about which patch is the right one by going to "details" for the patch, and checking the "obsolete" box which will hide it from view by default.
(In reply to Andrew McCreight [:mccr8] from comment #14)
> You can see exactly what landed by looking at the link in comment 12. Is
> that the wrong version?
Why lostpc_before_others_test_ver2.diff was not pushed to m-c?

> Also, you can avoid confusing about which patch is the right one by going to
> "details" for the patch, and checking the "obsolete" box which will hide it
> from view by default.
Why I should put "obsolete" flag into correct patch? The both patches should be landed to m-c.
Maybe, You missed thought, but patch named as lostpc_before_others_TEST_ver2.diff
Attachment #8501595 - Flags: checkin?
Ah, I see what you mean.  I'll put the checkin-needed keyword back.  Sheriffs, please last the patch with the checkin? flag.
Keywords: checkin-needed
last -> land
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Attachment #8501595 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/167e3e069f95
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.