Alon Zakai (:azakai) Reporter
	Description • 12 years ago

http://apps.playcanvas.com/playcanvas/scifi/latest

is an FPS demo. Press space to go to fullscreen. In that mode, the mouse has no effect on Linux, but people tell me it works on other platforms, so seems platform-specific.

	Alon Zakai (:azakai) Reporter
	Comment 1 • 12 years ago

Note that the mouse does seem to work while the permission dialog is shown.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 2 • 12 years ago

Are you running two monitors? And/or nvidia twin view?

	Alon Zakai (:azakai) Reporter
	Comment 3 • 12 years ago

Nothing fancy on my system, just a macbook running linux. No external monitors.

I have ATI and their binary drivers enabled.

bjacob said he also saw this bug, not sure what his setup is.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 4 • 12 years ago

I have the same issue on a Thinkpad W520, Ubuntu 12.04 64bit, NVIDIA.  No external monitor or pointing device.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 5 • 12 years ago

Nightly 2012-06-21-05-30-48 pointer lock doesn't work on either my primary or secondary monitor.
Nightly 2012-06-20-06-51-38 pointer locks works on my secondary monitor but not on my primary monitor.

Therefore I assume the regression range is:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c3190d715044&tochange=10e019421e6b

It seems it's a safe assumption that regressor is these changesets:

Wed Jun 20 16:38:48 2012 -0700
d047caf12f04 Chris Pearce — Bug 756936 - Minor tidy ups to pointer lock code. r=smaug
4be7471e7a95 Chris Pearce — Bug 756936 - Ensure MouseEvent.mozMovement{X,Y} values are correct on secondary monitors. r=smaug

	Martin Best (:mbest)
	Comment 6 • 12 years ago

It would be good to have this for fx16.

	Martin Best (:mbest)
	Comment 7 • 12 years ago

We are not sure on 15 and 16 status.  The regression seems in range of 16 so it seems likely that it's affected.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 8 • 12 years ago

We're already about to build our final RC for FF15, so not tracking for that.  Will track for 16, since this is a regression.

	Alex Keybl [:akeybl]
	Comment 9 • 12 years ago

Chris - can you investigate and suggest a backout, fix, or wontfix for FF16? This affects the bananabread demo.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 10 • 12 years ago

We should backout Bug 756936 on beta, and fix this bug on Aurora.

Bug 756936 fixes pointer lock in a few edge cases, notably when there is multiple monitors on Windows. AFAICT there's been no-one complaining about the issues Bug 756936 fixes, so I think we can live without Bug 756936's changes in release for another six weeks.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 11 • 12 years ago

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 756936
User impact if declined: Pointer lock won't work on Linux. However with this patch pointer lock won't work on secondary monitors (on Windows and Linux, maybe Mac too), but we had this behaviour in FF14 and no one complained (bug 756936 shipped in FF15, fixing pointer lock on secondary monitors but breaking linux pointer lock).
Testing completed (on m-c, etc.): mochitests pass locally. They passed before bug 756936 landed, so once we back it out they should still pass.
Risk to taking this patch (and alternatives if risky): Low, since this is a backout...
String or UUID changes made by this patch:  None.

	Alon Zakai (:azakai) Reporter
	Comment 12 • 12 years ago

(In reply to Alex Keybl [:akeybl] from comment #9)
> Chris - can you investigate and suggest a backout, fix, or wontfix for FF16?
> This affects the bananabread demo.

Hmm, I doublechecked and actually it looks like this does *not* affect other demos than the one reported here. BananaBread is ok, as is http://media.tojicode.com/q3bsp/ , I checked on 16, 17 and 18.

So the problem appears specific to the playcanvas demo for some unknown reason.

Given that, I would recommend not making any changes to aurora or beta. But it would be good to find out why that demo does not work on nightly even though others do (and since the demo does work in other browsers).

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 13 • 12 years ago

Hmmm, indeed my pointer lock demo at http://pearce.org.nz/fullscreen/pointerlock-position.html works ok in Linux, but *only* when I don't have a secondary monitor active. I'll cancel the approval request until we can figure out what's going on here...

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 14 • 12 years ago

I will fix the issue of pointer lock not working on Linux with multiple displays in bug 791121, and we'll leave this one for tracking down why http://apps.playcanvas.com/playcanvas/scifi/latest doesn't work with pointer lock on Linux.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 15 • 12 years ago

This is not Linux specific, PlayCanvas also doesn't work for me in [Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0].

Please disregard the regression range I reported in comment 5, that's a regression range for the problem with Linux and pointer lock with multiple monitors. I misunderstood the bug report here. Sorry about that.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 16 • 12 years ago

I take it back. After re-doing a regression range search on Windows using the PlayCanvas demo as a test case I have discovered that this bug has the same regression range I reported in comment 5, i.e. the regression range is 

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c3190d715044&tochange=10e019421e6b

Thus it's probably a regression from Bug 756936.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 17 • 12 years ago

Confirmed this with PlayCanvas on Linux too. It's definitely a regression from Bug 756936's 4be7471e7a95 changeset.

	Alex Keybl [:akeybl]
	Comment 18 • 12 years ago

Since this only affects a single demo (as opposed to pointer lock on Linux in general), we're no longer going to track for release.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 19 • 12 years ago

The problem with the PlayCanvas demo was caused by removing this if branch in 4be7471e7a95:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be7471e7a95#l1.65

With that if-branch resurrected, plus all calls to SynthesizeNativeMouseMove in screen coords, and with Windows' SynthesizeNativeMouseMove implementation altered to accept screen coords, pointer lock appears to work properly; the PlayCanvas demo works fine, on multiple monitors, and bug 782777 also seems to be fixed. Patches to follow...

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 20 • 12 years ago

I was wrong in changeset 4be7471e7a95 to switch to passing widget coords to nsIWidget::SynthesizeNativeMouseMove(). We should pass in screen coords. All the nsIWidget::Synthesize* methods are documented that they take screen coords, and the widget backends expect screen coords (except for Windows, which is a bug that I'm going to fix in the next patch). See Bug 791121 comment 2 for more info.

With this patch pointer lock now behaves correctly on non-Windows platforms with multiple monitors. This will break Windows pointer lock on secondary monitors without the next patch.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 21 • 12 years ago

In changeset 4be7471e7a95 I removed this branch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be7471e7a95#l1.65

I removed it because I was never able to observe it having any affect. Guess I was wrong.

Removing this branch caused the PlayCanvas demo to break.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 22 • 12 years ago

The other widget backends' implementation of SynthesizeNativeMouseMove() accept a coord in device pixels, but the Windows implementation is accepting it in widget coords (relative to the window's top left). All implementations should accept coords in the same coordinate system, otherwise cross platform code will be wrong.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 23 • 12 years ago

Comment on attachment 662500 [details] [diff] [review]
Patch 2 v1: Resurrect pointer lock refPoint restore on centering

Trying to figure out the reason for this...

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 24 • 12 years ago

Comment on attachment 662502 [details] [diff] [review]
Patch 3 v1: Make Windows nsWindow::SynthesizeNativeMouseEvent accept coords in screen coords, not widget coords

This needs a review from Windows widget peer.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 12 years ago

Comment on attachment 662500 [details] [diff] [review]
Patch 2 v1: Resurrect pointer lock refPoint restore on centering

Hmm, shouldn't we actually check that if
refPoint == sLastRefPoint and don't dispatch anything in that case.
We do want to dispatch 0,0 movement when mouse is back to center, right?

Re-ask review if I'm wrong here :)
The code needs some comments here.

	Jim Mathies [:jimm]
	Comment 26 • 12 years ago

Comment on attachment 662502 [details] [diff] [review]
Patch 3 v1: Make Windows nsWindow::SynthesizeNativeMouseEvent accept coords in screen coords, not widget coords

This has been this way since it landed in 09, so please run through try to confirm this doesn't break any tests.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 27 • 12 years ago

Green on Try, modulo random orange:
https://tbpl.mozilla.org/?tree=Try&rev=93b00a03d636

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 28 • 12 years ago

The previous "refPoint restore" code which I was resurrecting in my previous patch was confusing, so I changed the code to do what I *think* it meant to do, and what makes sense to me, and added I comments to explain how it works.

I explicitly look out for and cancel the "synthetic native" mouse event that we dispatch to re-center the pointer, and this fixes the PlayCanvas demo, and bug 782777 as well.

Explicitly tracking when we should cancel the mouse event also prevents us from cancelling mouse events that are targeted at the center of the window and which follow a pointer-lock re-centering mouse move (earlier versions of *this* patch had mochitests failures because the pointerlock mochitests dispatch synthetic mouse moves to the center of the window, I don't know if existing pointer lock test orange is/was caused by this).

Greenish: https://tbpl.mozilla.org/?tree=Try&rev=a2eed6362fc4

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 29 • 12 years ago

Comment on attachment 663914 [details] [diff] [review]
Patch 2 v2: Cancel synth centering pointer moves, comment code.

Why do we set sSynthCenteringPoint = center; after dispatching the
synth event?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 30 • 12 years ago

...and not before.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 31 • 12 years ago

So we know to expect a synthetic event, so that we can cancel the next event (which is dispatched asynchronously based on my testing). It can be set before dispatching I imagine.

Some of the mochitests were failing because they also dispatch synthetic events to the center of the window, and we were cancelling them.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 32 • 12 years ago

Setting sSynthCenteringPoint before the synthetic centering move dispatch.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 33 • 12 years ago

Thanks for reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/74a9ba7336cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f87f13f8da4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a88b3cd6f28d

	Ed Morley [:emorley]
	Comment 34 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/74a9ba7336cf
https://hg.mozilla.org/mozilla-central/rev/1f87f13f8da4
https://hg.mozilla.org/mozilla-central/rev/a88b3cd6f28d

	Paul Silaghi, QA [:pauly]
	Comment 35 • 10 years ago

Verified fixed 32.0a1 (2014-05-18), Ubuntu 13.04