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

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Dario Ostuni]

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.

Comment 1

[Andrew Overholt [:overholt]]

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?

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 3

[Andrew Overholt [:overholt]]

Xidorn, any thoughts?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Also, Dario, are you using any special DPI setting on your system or Firefox? I suspect that could be the reason of this issue.

Comment 7

[Dario Ostuni]

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

Comment 8

[Karl Tomlinson (:karlt)]

(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.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

(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 :/

Comment 10

[Karl Tomlinson (:karlt)]

(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.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

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 15

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1244546 part 1 - Apply proper unit conversion for SynthesizeNativeMouseEvent.

Review commit: https://reviewboard.mozilla.org/r/39183/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39183/

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1244546 part 2 - Align the center point for pointerlock to meet widget's requirement.

Review commit: https://reviewboard.mozilla.org/r/39185/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39185/

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

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...

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 8728888 [details]
MozReview Request: Bug 1244546 part 1 - Apply proper unit conversion for SynthesizeNativeMouseEvent.

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

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(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.)

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

(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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/560ae259d391
https://hg.mozilla.org/mozilla-central/rev/04ba63cdb181