Closed
Bug 1073563
Opened 10 years ago
Closed 10 years ago
The lostpointercapture event must be dispatched before any other pointer events
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: alessarik, Assigned: alessarik)
References
Details
Attachments
(2 files, 3 obsolete files)
13.76 KB,
patch
|
smaug
:
review+
cbook
:
checkin+
|
Details | Diff | Splinter Review |
5.51 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
Patch resolved 11.1 assertion in official tests
Attachment #8496025 -
Flags: review?(bugs)
Attachment #8496025 -
Flags: feedback?(mbrubeck)
Assignee | ||
Comment 2•10 years ago
|
||
Couple of tests to check async behavior of lostpointercapture event
Attachment #8496027 -
Flags: review?(bugs)
Attachment #8496027 -
Flags: feedback?(mbrubeck)
Updated•10 years ago
|
Component: Untriaged → Layout
Product: Firefox → Core
Comment 3•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8496027 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8496025 -
Flags: feedback?(mbrubeck) → feedback+
Updated•10 years ago
|
Attachment #8496027 -
Flags: feedback?(mbrubeck) → feedback+
Assignee | ||
Comment 4•10 years ago
|
||
+ Changes according with comments
Attachment #8496025 -
Attachment is obsolete: true
Attachment #8501223 -
Flags: review?(bugs)
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
+ Changes for correct testing
Attachment #8496027 -
Attachment is obsolete: true
Attachment #8501595 -
Flags: review?(bugs)
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
- Changes according with comments
Attachment #8501223 -
Attachment is obsolete: true
Attachment #8501605 -
Flags: review?(bugs)
Assignee | ||
Comment 9•10 years ago
|
||
Tests without patch: https://tbpl.mozilla.org/?tree=Try&rev=ec69471a0d9b Tests with patch: https://tbpl.mozilla.org/?tree=Try&rev=a9f49a1e6999
Updated•10 years ago
|
Attachment #8501605 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8501595 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 10•10 years ago
|
||
If nobody have objections, I will put checkin flag...
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/245fec9779cf
Assignee: nobody → alessarik
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/245fec9779cf
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee | ||
Comment 13•10 years ago
|
||
Wes, could You please check lostpc_before_others_test_ver2.diff was push into m-c or not?
Flags: needinfo?(kwierso)
Comment 14•10 years ago
|
||
You can see exactly what landed by looking at the link in comment 12. Is that the wrong version?
Comment 15•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
(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
Updated•10 years ago
|
Attachment #8501595 -
Flags: checkin?
Comment 17•10 years ago
|
||
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
Comment 18•10 years ago
|
||
last -> land
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/167e3e069f95
Keywords: checkin-needed
Updated•10 years ago
|
Attachment #8501595 -
Flags: checkin? → checkin+
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/167e3e069f95
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Flags: needinfo?(kwierso)
You need to log in
before you can comment on or make changes to this bug.
Description
•