Last Comment Bug 756936 - Incorrect MouseEvent.mozMovement{X,Y} values when pointer locked on secondary monitor
: Incorrect MouseEvent.mozMovement{X,Y} values when pointer locked on secondary...
Status: RESOLVED FIXED
[games:p2]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Chris Pearce (:cpearce)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: gecko-games 633602
  Show dependency treegraph
 
Reported: 2012-05-20 20:18 PDT by Chris Pearce (:cpearce)
Modified: 2012-07-16 11:14 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
unaffected
affected
fixed
fixed
unaffected


Attachments
Patch 1 v1: Coords relative to widget, not screen. (6.80 KB, patch)
2012-06-14 21:12 PDT, Chris Pearce (:cpearce)
bugs: review-
Details | Diff | Splinter Review
Patch 2 v1: Add comments and a little cleanup. (7.75 KB, patch)
2012-06-14 21:22 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 1 v2: Coords relative to widget, not screen. (9.02 KB, patch)
2012-06-18 17:45 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 1 v3: Coords relative to widget, not screen. (7.95 KB, patch)
2012-06-18 17:50 PDT, Chris Pearce (:cpearce)
bugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch 2 v2: Add comments and a little cleanup. (7.87 KB, patch)
2012-06-18 18:04 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 2 v3: Add comments and a little cleanup. (8.65 KB, patch)
2012-06-19 17:00 PDT, Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2012-05-20 20:18:37 PDT
On my Windows7 x64 machine, when you lock the pointer with the browser window fullscreen on my secondary monitor, the mozMovement{X,Y} values in the generated MouseEvent events are stuck at a constant value.

It doesn't seem to matter where I position my secondary monitor WRT my primary (i.e. above, below, left, right, etc). It's just pointer lock requests in the secondary monitor seem to have fairly constant mozMovement{X,Y} values.

I'm not sure if this bug exists on non-windows platforms.

Testing on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1

STR:
1. Configure your system to have a secondary monitor.
2. Open http://pearce.org.nz/fullscreen in Firefox Nightly.
3. With the firefox window on your primary display, press the 'p' key, or click "Fullscreen with pointer lock" button.
4. Move the mouse. Observe the "Pointer: delta=(*, *) screen=(*, *) client=(*, *)" text being updated as MouseEvents are dispatched.
5. Press ESC to exit fullscreen, move the Firefox window to your secondary monitor, press 'p', or click "Fullscreen with pointer lock" button.
4. Observe that the "Pointer: ..." text doesn't update the same as when pointer locked on the primary monitor. The "delta" values (the mozMovement{X,Y} values) are fairly constant.
Comment 1 Chris Pearce (:cpearce) 2012-05-20 20:21:38 PDT
On my linux box when I lock the pointer on my secondary display (following the STR in comment0) the mouse jumps to appear as visible on my primary, but remains hidden on my secondary display, and I still get incoherent mozMovement{X,Y} values.
Comment 2 Chris Pearce (:cpearce) 2012-05-20 20:24:03 PDT
Also note that the mozMovement{X,Y} values are coherent when in fullscreen move on the secondary monitor, it's only once the pointer is locked that they start reporting bogus values.
Comment 3 Chris Pearce (:cpearce) 2012-05-21 22:32:31 PDT
So when I change nsIntPoint nsEventStateManager::GetMouseCoords(nsIntRect aBounds) to just return (innerWidth/2, innerHeight/2), I get what looks like correct behaviour, and the mochitests still pass... Note aBounds has negative values in some fields in the secondary monitor case, whereas in the primary monitor case they're all >=0.

Humph: Why do you need to pass the bounds into this function?

And why is innerHeight used in the calculation but innerWidth isn't?

It looks like SynthesizeNativeMouseEvent takes coords relative to the window/widget's top-left corner, rather than in system pixel coords, at least on windows it looks like it does:

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#5716

Should nsIntPoint nsEventStateManager::GetMouseCoords() just be returning (innerWidth/2, innerHeight/2)?
Comment 4 Chris Pearce (:cpearce) 2012-06-14 21:12:52 PDT
Created attachment 633389 [details] [diff] [review]
Patch 1 v1: Coords relative to widget, not screen.

The problem in brief is that we're using the widget's bounds in screen coords to calculate the mouse position and movement deltas, but the mouse/dom events and widget synthetic dispatch always assume the coords are in widget coords (i.e. offset from the widget origin, not the primary screen's origin).

Specifically, nsEventStateManager::GetMouseCoords(bounds) is being used to calulate the center of the window containing the mouse event, but GetMouseCoords() is passed screenBounds (bounds in screen coords) and using that to seed its calculations causing the calculated mouse position to be in screen coords. We're dispatching synthetic mouse events to the screen coords, but the synthetic dispatch expects widget coords, causing the problem.

Changes in this patch:
* Change the window center calculation to return an offset relative to the widget's origin. This fixes the base problem.
* Remove the "if (aEvent->refPoint == aEvent->lastRefPoint) aEvent->refPoint = sLastRefPoint;" case in nsEventStateManager::GenerateMouseEnterExit. In my testing this branch was only taken when aEvent->refPoint == sLastRefPoint, so it has no effect.
* Save the mouse position in widget coords before going into pointer lock in nsEventStateManager::mPreLockPoint (previously it was saved in screen coords).
* Synthesize mouse move to nsEventStateManager::mPreLockPoint when exiting, so mouse returns to same position upon pointer lock exit (previously mPreLockPoint was write only, and sLastScreenPoint was used for this purpose, except sLastScreenPoint is in screen coords, so it would behave incorrectly on secondary monitors).
* Set nsEventStateManager::sLastRefPoint before entering/exiting pointer lock so that the synthetic mouse move fired when entering/exiting doesn't report movement as ((mouse position before lock) - (center of window)) as it did previously. (If that's not learn, play with http://pearce.org.nz/fullscreen/pointerlock-position.html and you should see what I mean)
Comment 5 Chris Pearce (:cpearce) 2012-06-14 21:22:47 PDT
Created attachment 633393 [details] [diff] [review]
Patch 2 v1: Add comments and a little cleanup.

While figuring out how pointer lock worked I added comments, removed some unused code, and simplified nsDOMUIEvent::GetMovementPoint(). This should make it easier to understand the pointer lock code in future.

Note the nsDOMUIEvent::GetMovementPoint() code was converting from widget to screen to app units then back to pixels and then diffing, whereas we can instead simply just subtract the event's refPoint from the previous to get the delta. Much simpler this way.
Comment 6 Olli Pettay [:smaug] 2012-06-15 03:01:50 PDT
Comment on attachment 633389 [details] [diff] [review]
Patch 1 v1: Coords relative to widget, not screen.

>-          aEvent->widget->SynthesizeNativeMouseMove(aEvent->lastRefPoint);
>+        nsIntPoint center = GetWidgetCenter(aEvent->widget);
>+        aEvent->lastRefPoint = center;
>+        // If this mouse move doesn't finish at the center of the widget,
>+        // dispatch a synthetic mouse move to return the mouse back to the
>+        // center.
>+        if (aEvent->refPoint != center) {
>+          aEvent->widget->SynthesizeNativeMouseMove(center);
>         }
Why do we want to center in widget, and not in the web page?
If browser chrome takes lots of space, the center can be over chrome.
Comment 7 Chris Pearce (:cpearce) 2012-06-18 17:45:51 PDT
Created attachment 634254 [details] [diff] [review]
Patch 1 v2: Coords relative to widget, not screen.
Comment 8 Chris Pearce (:cpearce) 2012-06-18 17:50:41 PDT
Created attachment 634256 [details] [diff] [review]
Patch 1 v3: Coords relative to widget, not screen.

Ooops, some of the changes from Patch 2 made it into this one. 

The new patch finds the center of the inner content area, rather than the center of the outer window/widget.
Comment 9 Chris Pearce (:cpearce) 2012-06-18 18:04:34 PDT
Created attachment 634259 [details] [diff] [review]
Patch 2 v2: Add comments and a little cleanup.

Rebased on new patch 1.
Comment 10 Olli Pettay [:smaug] 2012-06-19 10:58:20 PDT
Comment on attachment 634256 [details] [diff] [review]
Patch 1 v3: Coords relative to widget, not screen.


>+// Returns the center point of the window's inner content area.
>+// This is in widget coordinates, i.e. relative to the widget's top
>+// left corner, not in screen coordinates.
>+static nsIntPoint
>+GetWindowInnerRectCenter(nsPIDOMWindow* aWindow,
>+                         nsIWidget* aWidget,
>+                         nsPresContext* aContext)
>+{
>+  NS_ENSURE_TRUE(aWindow != nsnull, nsIntPoint(0,0));
>+  NS_ENSURE_TRUE(aWidget != nsnull, nsIntPoint(0,0));
>+  NS_ENSURE_TRUE(aContext != nsnull, nsIntPoint(0,0));

NS_ENSURE_TRUE(aWindow && aWidget && aContext, nsIntPoint(0,0));
Comment 11 Olli Pettay [:smaug] 2012-06-19 11:13:24 PDT
(In reply to Chris Pearce (:cpearce) from comment #5)
> Note the nsDOMUIEvent::GetMovementPoint() code was converting from widget to
> screen to app units then back to pixels and then diffing, whereas we can
> instead simply just subtract the event's refPoint from the previous to get
> the delta. Much simpler this way.
I don't understand this. The result should be in CSS pixels. With your patch, what guarantees that?
(Perhaps I'm just missing something.)
Comment 12 JP Rosevear [:jpr] 2012-06-19 13:26:00 PDT
Not needed for b2g.
Comment 13 Chris Pearce (:cpearce) 2012-06-19 16:38:26 PDT
(In reply to Olli Pettay [:smaug] from comment #11)
> (In reply to Chris Pearce (:cpearce) from comment #5)
> > Note the nsDOMUIEvent::GetMovementPoint() code was converting from widget to
> > screen to app units then back to pixels and then diffing, whereas we can
> > instead simply just subtract the event's refPoint from the previous to get
> > the delta. Much simpler this way.
> I don't understand this. The result should be in CSS pixels. With your
> patch, what guarantees that?
> (Perhaps I'm just missing something.)

Ah yes, my mistake. You're correct, I was converting to dev pixels, not CSS pixels. I'll fix that.

The change I'm trying to make here is to remove the unnecessary conversion from widget offset to screen coords. Let me update the patch again...
Comment 14 Chris Pearce (:cpearce) 2012-06-19 17:00:30 PDT
Created attachment 634661 [details] [diff] [review]
Patch 2 v3: Add comments and a little cleanup.
Comment 15 Sheila Mooney 2012-06-20 13:10:50 PDT
Since it's tagged as a games P2, we are not going to track this.
Comment 18 Martin Best (:mbest) 2012-07-04 15:16:06 PDT
Can we are get this on Aurora (fx15)?
Comment 19 Chris Pearce (:cpearce) 2012-07-04 15:58:30 PDT
Comment on attachment 634256 [details] [diff] [review]
Patch 1 v3: Coords relative to widget, not screen.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Mouse/pointer lock (bug 633602)
User impact if declined: Pointer lock reports incorrect mouse movement values when the mouse is locked on a secondary monitor. Games etc won't work. FF15 is our big bang HTML5 Games Are Awesome Release, so it would be good to get this in FF15.
Testing completed (on m-c, etc.): Local testing, and it's been on m-c for weeks.
Risk to taking this patch (and alternatives if risky): Pretty low risk.
String or UUID changes made by this patch: None.
Comment 20 Alex Keybl [:akeybl] 2012-07-05 17:20:20 PDT
Comment on attachment 634256 [details] [diff] [review]
Patch 1 v3: Coords relative to widget, not screen.

[Triage Comment]
Low risk, and in support of a big bang :). Approved for Aurora 15.
Comment 21 Chris Pearce (:cpearce) 2012-07-05 20:29:49 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/079ad5847a2e
Comment 22 Scoobidiver (away) 2012-07-16 00:54:12 PDT
Target Milestone is for m-c.
Comment 23 Chris Pearce (:cpearce) 2012-07-16 11:06:45 PDT
This is fixed in mozilla 15, so why shouldn't the target milestone be 15?

Or is target milestone supposed to be for the release in which it first was pushed to m-c?
Comment 24 Scoobidiver (away) 2012-07-16 11:14:38 PDT
(In reply to Chris Pearce (:cpearce) from comment #23)
> Or is target milestone supposed to be for the release in which it first was
> pushed to m-c?
Yes.

Note You need to log in before you can comment on or make changes to this bug.