Closed Bug 1400143 Opened 3 years ago Closed 3 years ago

[Pointer Event] Update pointerevent's mLastRefPoint to get correct movementX/movementY values

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: stone, Assigned: stone)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → sshih
Priority: -- → P2
Blocks: pointerevent
Attachment #8914039 - Flags: review?(bugs)
Attachment #8914040 - Flags: review?(bugs)
Attachment #8914041 - Flags: review?(bugs)
ok, so the bug isn't about adding a test case but changing the implementation to do something...
Could you please explain what this bug is about?

(I'll try to understand it from the patches, but bugs really should have descriptive titles and some explanation if the title itself doesn't explain it all well enough.)
Flags: needinfo?(sshih)
Comment on attachment 8914039 [details] [diff] [review]
Separate the pointer lock implementations from GenerateMouseEnterExit to new functions.

>+/* static */ void
>+EventStateManager::UpdateLastRefPointOfMouseEvent(WidgetMouseEvent* aMouseEvent)
This isn't very descriptive name since also other stuff is done.

>+{
>+  if (aMouseEvent->mMessage != eMouseMove) {
>+    return;
>+  }
>+  // Mouse movement is reported on the MouseEvent.movement{X,Y} fields.
>+  // Movement is calculated in UIEvent::GetMovementPoint() as:
>+  //   previous_mousemove_mRefPoint - current_mousemove_mRefPoint.
>+  if (sIsPointerLocked && aMouseEvent->mWidget) {
>+    // The pointer is locked. If the pointer is not located at the center of
>+    // the window, dispatch a synthetic mousemove to return the pointer there.
>+    // Doing this between "real" pointer moves gives the impression that the
>+    // (locked) pointer can continue moving and won't stop at the screen
>+    // boundary. We cancel the synthetic event so that we don't end up
>+    // dispatching the centering move event to content.
>+    LayoutDeviceIntPoint center =
>+      GetWindowClientRectCenter(aMouseEvent->mWidget);
>+    aMouseEvent->mLastRefPoint = center;
>+    if (aMouseEvent->mRefPoint != center) {
>+      // Mouse move doesn't finish at the center of the window. Dispatch a
>+      // synthetic native mouse event to move the pointer back to the center
>+      // of the window, to faciliate more movement. But first, record that
>+      // we've dispatched a synthetic mouse movement, so we can cancel it
>+      // in the other branch here.
>+      sSynthCenteringPoint = center;
like this.

>+      aMouseEvent->mWidget->SynthesizeNativeMouseMove(
>+        center + aMouseEvent->mWidget->WidgetToScreenOffset(), nullptr);
>+    } else if (aMouseEvent->mRefPoint == sSynthCenteringPoint) {
>+      // This is the "synthetic native" event we dispatched to re-center the
>+      // pointer. Cancel it so we don't expose the centering move to content.
>+      aMouseEvent->StopPropagation();
this

>+      // Clear sSynthCenteringPoint so we don't cancel other events
>+      // targeted at the center.
>+      sSynthCenteringPoint = kInvalidRefPoint;
and this


r+ if you can come up with some better name for the method.
Attachment #8914039 - Flags: review?(bugs) → review+
Comment on attachment 8914040 [details] [diff] [review]
Part2: Update sLastRefPoint of ESM when handling mousedown and mouseup and reduce the indent level in UpdateLastRefPointOfMouseEvent.

I don't know why UpdateLastPointerPosition is called. Please explain.
And looks like the change to UpdateLastRefPointOfMouseEvent has nothing to do with those new UpdateLastPointerPosition calls.
Attachment #8914040 - Flags: review?(bugs) → review-
Comment on attachment 8914041 [details] [diff] [review]
Part3: Update sLastRefPoint by pointer events and add test case pointerevent_movementxy-manual.html.

>+++ b/dom/events/UIEvent.cpp
>@@ -119,17 +119,17 @@ UIEvent::GetMovementPoint()
>     return nsIntPoint(0, 0);
>   }
> 
>   if (mPrivateDataDuplicated || mEventIsInternal) {
>     return mMovementPoint;
>   }
> 
>   if (!mEvent || !mEvent->AsGUIEvent()->mWidget ||
>-      (mEvent->mMessage != eMouseMove)) {
>+      (mEvent->mMessage != eMouseMove && mEvent->mMessage != ePointerMove)) {

So per https://w3c.github.io/pointerlock/#dom-mouseevent-movementx this is wrong.
I reopened https://github.com/w3c/pointerevents/issues/131
Comment on attachment 8914041 [details] [diff] [review]
Part3: Update sLastRefPoint by pointer events and add test case pointerevent_movementxy-manual.html.

But I think the spec will allow this behavior.
Attachment #8914041 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #5)
> Could you please explain what this bug is about?
> 
> (I'll try to understand it from the patches, but bugs really should have
> descriptive titles and some explanation if the title itself doesn't explain
> it all well enough.)

Agree. I should separate this into different bugs.
Flags: needinfo?(sshih)
Summary: [Pointer Event] Add test case pointerevent_movementxy-manual.html → [Pointer Event] Update pointerevent's mLastRefPoint to get correct movementX/movementY values
Depends on: 1404921
Depends on: 1404896
Attachment #8914039 - Attachment is obsolete: true
Attachment #8914040 - Attachment is obsolete: true
Attachment #8914041 - Attachment is obsolete: true
Comment on attachment 8914318 [details] [diff] [review]
Update pointerevent's mLastRefPoint to get correct movementX/movementY values.

carry r+ from Attachment #8914041 [details] [diff]
Attachment #8914318 - Flags: review+
(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8914040 [details] [diff] [review]
> Part2: Update sLastRefPoint of ESM when handling mousedown and mouseup and
> reduce the indent level in UpdateLastRefPointOfMouseEvent.
> 
> I don't know why UpdateLastPointerPosition is called. Please explain.
> And looks like the change to UpdateLastRefPointOfMouseEvent has nothing to
> do with those new UpdateLastPointerPosition calls.

I misunderstand that we need to update last ref point by mousedown and mouseup. Create bug 1404921 for setting movementX/Y to zero for all mouse events except mousemove.
Pushed by sshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c608c6fd9a9a
[Pointer Event] Update pointerevent's mLastRefPoint to get correct movementX/movementY values. r=smaug.
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0eb4f3117c3
Backed out changeset c608c6fd9a9a for failing mochitest dom/events/test/pointerevents/pointerlock/test_pointerevent_pointerlock_supercedes_capture-manual.html on Windows 7 without e10s. r=backout
Backed out for failing mochitest dom/events/test/pointerevents/pointerlock/test_pointerevent_pointerlock_supercedes_capture-manual.html on Windows 7 without e10s:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0eb4f3117c3ea6a3fd1ee65757686321d8e4baf

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c608c6fd9a9ad37116636980095bdcb6af8e74d6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135563648&repo=mozilla-inbound

00:58:26     INFO -  3168 INFO TEST-PASS | dom/events/test/pointerevents/pointerlock/test_pointerevent_pointerlock_supercedes_capture-manual.html | Unrecognized button value caught!
00:58:26     INFO -  3169 INFO TEST-PASS | dom/events/test/pointerevents/pointerlock/test_pointerevent_pointerlock_supercedes_capture-manual.html | document must has been pointerlocked
00:58:26     INFO -  3170 INFO TEST-PASS | dom/events/test/pointerevents/pointerlock/test_pointerevent_pointerlock_supercedes_capture-manual.html | Unrecognized button value caught!
00:58:26     INFO -  3171 INFO TEST-PASS | dom/events/test/pointerevents/pointerlock/test_pointerevent_pointerlock_supercedes_capture-manual.html | Unrecognized button value caught!
00:58:27     INFO -  GECKO(472) | --DOCSHELL 1DABB400 == 9 [pid = 472] [id = {69d5b3f5-aad9-4302-83a5-018420318b46}]
00:58:27     INFO -  GECKO(472) | --DOCSHELL 12D34000 == 8 [pid = 472] [id = {ae71aeb6-037f-4e9b-8ca0-b3ab35fe709d}]
00:58:28     INFO -  GECKO(472) | --DOMWINDOW == 32 (1DABB800) [pid = 472] [serial = 27] [outer = 00000000] [url = http://mochi.test:8888/tests/dom/events/test/pointerevents/pointerlock/resources/pointerevent_movementxy-iframe.html]
00:58:28     INFO -  GECKO(472) | --DOMWINDOW == 31 (0F834000) [pid = 472] [serial = 1] [outer = 00000000] [url = chrome://gfxsanity/content/sanityparent.html]
00:58:28     INFO -  GECKO(472) | --DOMWINDOW == 30 (12D34400) [pid = 472] [serial = 9] [outer = 00000000] [url = chrome://gfxsanity/content/sanitytest.html]
00:58:28     INFO -  GECKO(472) | --DOMWINDOW == 29 (1DEEE800) [pid = 472] [serial = 21] [outer = 00000000] [url = http://mochi.test:8888/tests/dom/events/test/pointerevents/pointerlock/pointerevent_movementxy-manual.html]
00:58:32     INFO -  GECKO(472) | --DOCSHELL 1DEEE400 == 7 [pid = 472] [id = {7de3b071-adeb-4561-8a77-787973178142}]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 28 (1767F000) [pid = 472] [serial = 28] [outer = 00000000] [url = http://mochi.test:8888/tests/dom/events/test/pointerevents/pointerlock/resources/pointerevent_movementxy-iframe.html]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 27 (12D33C00) [pid = 472] [serial = 23] [outer = 00000000] [url = about:blank]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 26 (1DEFA400) [pid = 472] [serial = 22] [outer = 00000000] [url = about:blank]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 25 (0E366800) [pid = 472] [serial = 32] [outer = 00000000] [url = about:blank]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 24 (16341400) [pid = 472] [serial = 13] [outer = 00000000] [url = about:blank]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 23 (164B1000) [pid = 472] [serial = 15] [outer = 00000000] [url = about:blank]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 22 (1C262400) [pid = 472] [serial = 18] [outer = 00000000] [url = about:blank]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 21 (1889F800) [pid = 472] [serial = 19] [outer = 00000000] [url = about:blank]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 20 (1D279400) [pid = 472] [serial = 20] [outer = 00000000] [url = http://mochi.test:8888/tests/dom/events/test/pointerevents/pointerlock/test_pointerevent_movementxy-manual.html]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 19 (1DB95C00) [pid = 472] [serial = 29] [outer = 00000000] [url = http://mochi.test:8888/tests/SimpleTest/iframe-between-tests.html]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 18 (140D0400) [pid = 472] [serial = 10] [outer = 00000000] [url = about:blank]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 17 (142F1800) [pid = 472] [serial = 11] [outer = 00000000] [url = chrome://gfxsanity/content/sanitytest.html]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 16 (0DFB3000) [pid = 472] [serial = 4] [outer = 00000000] [url = about:blank]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 15 (1768B000) [pid = 472] [serial = 25] [outer = 00000000] [url = about:blank]
00:58:36     INFO -  GECKO(472) | --DOMWINDOW == 14 (0F834800) [pid = 472] [serial = 2] [outer = 00000000] [url = about:blank]
00:58:46     INFO -  GECKO(472) | [472, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file z:/build/build/src/dom/fetch/FetchDriver.cpp, line 412
00:58:46     INFO -  GECKO(472) | [472, Main Thread] WARNING: 'NS_FAILED(rr->RetargetDeliveryTo(sts))', file z:/build/build/src/dom/fetch/FetchDriver.cpp, line 661
01:00:21     INFO -  GECKO(472) | [472, Lazy Idle] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file z:/build/build/src/widget/windows/WinUtils.cpp, line 1490
01:02:21     INFO -  GECKO(472) | [472, Lazy Idle] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file z:/build/build/src/widget/windows/WinUtils.cpp, line 1490
01:03:52     INFO -  TEST-INFO | started process screenshot
01:03:52     INFO -  TEST-INFO | screenshot: exit 0
01:03:52    ERROR -  3172 INFO TEST-UNEXPECTED-FAIL | dom/events/test/pointerevents/pointerlock/test_pointerevent_pointerlock_supercedes_capture-manual.html | Test timed out.
01:03:52     INFO -      reportError@SimpleTest/TestRunner.js:121:7
01:03:52     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
01:03:52     INFO -      TestRunner.runTests@SimpleTest/TestRunner.js:380:5
01:03:52     INFO -      RunSet.runtests@SimpleTest/setup.js:194:3
01:03:52     INFO -      RunSet.runall@SimpleTest/setup.js:173:5
01:03:52     INFO -      hookupTests@SimpleTest/setup.js:266:5
01:03:52     INFO -  EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
01:03:52     INFO -      hookup@SimpleTest/setup.js:246:5
01:03:52     INFO -  EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=c%3A%5Cusers%5Cgenericworker%5Cappdata%5Clocal%5Ctemp&cleanupCrashes=true:11:1
Flags: needinfo?(sshih)
Pushed by sshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0300f67ce145
[Pointer Event] Update pointerevent's mLastRefPoint to get correct movementX/movementY values. r=smaug.
https://hg.mozilla.org/mozilla-central/rev/0300f67ce145
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Duplicate of this bug: 1402657
You need to log in before you can comment on or make changes to this bug.