Mouse cursor teleports itself to lower-right screen corner when using Pointer Lock API with GDK scaling

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: another.code.996, Assigned: xidorn)

Tracking

(Depends on 1 bug, Blocks 1 bug)

44 Branch
mozilla48
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: dom-triaged)

Attachments

(2 attachments)

Reporter

Description

3 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160126104524

Steps to reproduce:

I'm using Arch Linux x86_64 with KDE Plasma 5.

I visit some pages with some Pointer Lock API features (like http://substack.net/projects/voxel-forest/ , https://kripken.github.io/BananaBread/cube2/index.html or https://mdn.github.io/pointer-lock-demo/ ) and give permission to use it.


Actual results:

The mouse cursor immediately teleports itself to the lower-right screen corner, and if I try to move the cursor inside the Firefox window it teleports to the same location (the lower-right screen corner) again.
If (like for https://kripken.github.io/BananaBread/cube2/index.html ) the webpage goes fullscreen everything goes crazy, I assume that's because the application is constantly receiving fake input from the mouse.


Expected results:

The mouse cursor don't teleport itself and the actual mouse input is forwarded to the application.
Reporter

Updated

3 years ago
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Component: Untriaged → Event Handling
Product: Firefox → Core
The BananaBread demo seems to just go into fullscreen for me and works fine on Windows.

Chris, do you know about our Linux implementation of Pointer Lock?
Flags: needinfo?(cpearce)
Whiteboard: dom-triaged
Our pointer lock implementation was produced by some Seneca college students. I have done some maintenance on it in the distant past. I think Xidorn (or Olli) might be able to help now, since Pointer Lock is pretty coupled with the Fullscreen API.
Flags: needinfo?(cpearce)
Xidorn, any thoughts?
Flags: needinfo?(quanxunzhen)
Assignee

Comment 4

3 years ago
Yeah, I can reproduce this issue on my Linux Mint VM. Pointer lock goes crazy there... I'll investigate this issue.
Assignee

Updated

3 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 5

3 years ago
What I actually see is the pointer jumps much farther than expected, but not teleported to lower-right screen corner as descripted in comment 0. So probably a different issue. But the investigation about the code could help figuring out the reason.

Basically, pointer lock works in this way:
whenever a mouse move happens, the pointer is moved to the center of the window via nsIWidget::SynthesizeNativeMouseMove;
the event generated due to the call to SynthesizeNativeMouseMove above is then suppressed.
This whole process can be found in EventStateManager.cpp [1].

So for this issue, I guess there is some unit conversion mistake, which makes SynthesizeNativeMouseMove constantly try to move the pointer to an incorrect position (the lower-right corner of screen, in this case).

Karl, could you have a look at this issue? I suppose this is an issue inside the GTK widget rather than the DOM code as it works well anywhere else. (And it is actually impossible for me to reproduce this bug due to a different issue.)

[1] https://dxr.mozilla.org/mozilla-central/rev/af6356a3e8c56036b74ba097395356d9c6e6c5a3/dom/events/EventStateManager.cpp#4230-4257
Flags: needinfo?(quanxunzhen) → needinfo?(karlt)
Assignee

Comment 6

3 years ago
Also, Dario, are you using any special DPI setting on your system or Firefox? I suspect that could be the reason of this issue.
Flags: needinfo?(another.code.996)
Reporter

Comment 7

3 years ago
Yes, having a screen resolution of 3840x2160 (4K, HiDPI), I have set the following environment variables:
export QT_DEVICE_PIXEL_RATIO=2
export GDK_SCALE=2
export GDK_DPI_SCALE=0.5

Also, I should mention that Firefox on Arch Linux is compiled with --enable-default-toolkit=cairo-gtk3
Flags: needinfo?(another.code.996)
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #5)
> Basically, pointer lock works in this way:
> whenever a mouse move happens, the pointer is moved to the center of the
> window via nsIWidget::SynthesizeNativeMouseMove;
> the event generated due to the call to SynthesizeNativeMouseMove above is
> then suppressed.
> This whole process can be found in EventStateManager.cpp [1].

It seems the synthesized event is distinguished from other events pending in
the queue by its position being the centre, which may match the wrong event if the pointer happened to be moved back to the centre, but I doubt that is the cause
of this bug.

It seems that only the first of possibly several pending synthesized events is
suppressed.  I wonder what effect that has on the
 previous_mousemove_refPoint - current_mousemove_refPoint
logic, but that's probably not the primary cause of this bug.

> So for this issue, I guess there is some unit conversion mistake, which
> makes SynthesizeNativeMouseMove constantly try to move the pointer to an
> incorrect position (the lower-right corner of screen, in this case).

Yes, GdkScaleFactor() needs to be considered when converting from
LayoutDevicePixel to GDK pixels.
https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/widget/gtk/nsWindow.cpp#6780

DevicePixelsToGdkPointRoundDown() may be good enough, or using doubles could provide sub-GDK-pixel accuracy.
Blocks: 975919
Component: Event Handling → Widget: Gtk
Flags: needinfo?(karlt)
Summary: Mouse cursor teleports itself to lower-right screen corner when using Pointer Lock API → Mouse cursor teleports itself to lower-right screen corner when using Pointer Lock API with GDK scaling
Assignee

Comment 9

3 years ago
(In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> Yes, GdkScaleFactor() needs to be considered when converting from
> LayoutDevicePixel to GDK pixels.
> https://dxr.mozilla.org/mozilla-central/rev/
> d848a5628d801a460a7244cbcdea22d328d8b310/widget/gtk/nsWindow.cpp#6780
> 
> DevicePixelsToGdkPointRoundDown() may be good enough, or using doubles could
> provide sub-GDK-pixel accuracy.

The issue is, the center of the window could be at a sub-GDK-pixel position. When that happen, rounding down would make the position constantly move towards top and/or left.

gdk_display_warp_pointer accepts gint, so it doesn't seem to me using doubles could help anything here :/
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #9)
> The issue is, the center of the window could be at a sub-GDK-pixel position.
> When that happen, rounding down would make the position constantly move
> towards top and/or left.

We have code to anticipate similar things elsewhere:
https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/view/nsView.cpp#251

Displays are usually even numbers of pixels and scale factors are usually either 1 or 2, but other scenarios are possible.

> gdk_display_warp_pointer accepts gint, so it doesn't seem to me using
> doubles could help anything here :/

Ah, OK.
Assignee

Comment 11

3 years ago
There exists some hack code to workaround full screen mode's 1px toolbar toggler. I'd like to remove that workaround as part of this bug, so make it depend on bug 729011.
Depends on: 729011
Assignee

Updated

3 years ago
Blocks: 1254037
Assignee

Comment 12

3 years ago
It seems we would also need to make the puppet widget support RoundsWidgetCoordinatesTo for e10s.
Assignee

Updated

3 years ago
Depends on: 1255319
For what it's worth, this bug is almost certainly the reason various historic games hosted at the Internet Archive are either crippled or unplayable.  The bug description matches the behavior of https://archive.org/details/msdos_Where_in_the_World_is_Carmen_Sandiego_Enhanced_1989 not working if I attempt to use a mouse with it (thereby invoking pointer lock).  And as far as I can tell, https://archive.org/details/msdos_Oregon_Trail_Deluxe_The_1992 *requires* mouse support to move past the title screen, making the game wholly unplayable with this bug.

The fairly high visibility of the Internet Archive games, and this bug's making pointer lock effectively unusable, suggests this bug should probably be prioritized to some degree.
Assignee

Comment 14

3 years ago
I already have patch to make it work on non-e10s. But there are several issues around e10s, including:
1. RoundsWidgetCoordinatesTo is not available on the puppet widget (which can be worked around via using GetDefaultScale)
2. the boundary of the puppet widget is not aligned to RoundsWidgetCoordinatesTo, which causes some calculation errors

I don't really see how the second issue has any trivial workaround.

Also, it seems to me our current approach of supporting pointerlock for e10s is very inefficient. We do all the logic in the content process, and then redirect the synthetic mouse move via IPC to the parent process for every real mouse move.

I think this should be fixed as well, and fixing this would make the two issues above irrelevant.

Given that I'm going to fix it only for non-e10s in this bug, and file a bug for changing the e10s support of pointerlock.
Comment on attachment 8728889 [details]
MozReview Request: Bug 1244546 part 2 - Align the center point for pointerlock to meet widget's requirement.

https://reviewboard.mozilla.org/r/39185/#review35987

::: dom/events/EventStateManager.cpp
(Diff revision 1)
> -// mode (see bug 799523 comment 35, and bug 729011). Using integer CSS pix

I assume you tested both these bugs with the patch.

::: dom/events/EventStateManager.cpp:4179
(Diff revision 1)
> -    aContext->CSSPixelsToDevPixels(innerY - cssScreenY + innerHeight / 2));
> +  auto point = LayoutDeviceIntPoint(rect.x + rect.width / 2,

Why not 
LayoutDeviceIntPoint point(params)
Attachment #8728889 - Flags: review?(bugs) → review+
Assignee

Comment 18

3 years ago
https://reviewboard.mozilla.org/r/39185/#review35987

> I assume you tested both these bugs with the patch.

I've fixed bug 729011 to ensure this change is safe :)

IIUC, that doesn't really have impact on normal usecase of pointer lock.

> Why not 
> LayoutDeviceIntPoint point(params)

Ah... I somehow forgot this option...
Attachment #8728888 - Flags: review?(karlt) → review+
Comment on attachment 8728888 [details]
MozReview Request: Bug 1244546 part 1 - Apply proper unit conversion for SynthesizeNativeMouseEvent.

https://reviewboard.mozilla.org/r/39183/#review36119
Assignee

Comment 20

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/560ae259d391cd537ca004cbc0c3caebcc6223fb
Bug 1244546 part 1 - Apply proper unit conversion for SynthesizeNativeMouseEvent. r=karlt

https://hg.mozilla.org/integration/mozilla-inbound/rev/04ba63cdb181cf7efba193c1455e3ab506a699cc
Bug 1244546 part 2 - Align the center point for pointerlock to meet widget's requirement. r=smaug
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #18)
> > Why not 
> > LayoutDeviceIntPoint point(params)
> 
> Ah... I somehow forgot this option...
Rule of thumb: "don't use auto" ;) (and then there are those few cases when using it isn't totally crazy.)
Assignee

Comment 22

3 years ago
(In reply to Olli Pettay [:smaug] from comment #21)
> (In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #18)
> > > Why not 
> > > LayoutDeviceIntPoint point(params)
> > 
> > Ah... I somehow forgot this option...
> Rule of thumb: "don't use auto" ;) (and then there are those few cases when
> using it isn't totally crazy.)

Actually when I was writing this code, I thought: oh, sorry smaug, I'm selling you auto again :P
Assignee: nobody → quanxunzhen

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/560ae259d391
https://hg.mozilla.org/mozilla-central/rev/04ba63cdb181
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.