1304692 - [Mac] PointerLock doesn't work properly while in Full Screen on MacBook Pro Retina

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Camelia Badau [:cbadau], Desktop Test Engineering]

[Affected versions]:
- Firefox 50 Beta 1

[Affected platforms]:
- Mac OS X 10.11 (MacBook Pro Retina)

[Steps to reproduce]:
1. Navigate to one of the following games/demos: https://kripken.github.io/BananaBread/wasm-demo/index.html, http://substack.net/projects/voxel-forest/, http://media.tojicode.com/q3bsp/. 
2. From Hamburger menu, select "Full Screen" . 
3. Click on the demo and start navigating/playing.
 

[Expected result]:
- There are no issues encountered while playing/navigating. 

[Actual result]:
- The game/demo doesn't work properly while in Full Screen. After clicking on the demo/game (to lock the pointer), the game/demo is blocked in a position and can't be moved from there.  

[Regression range]:
- It is not a regression, it reproduces since the feature landed in Firefox (Nightly 50.0a1: 2016-07-10).

[Additional notes]:
- The issue is reproducible only on MacBook Pro Retina 10.11.6 -> Graphics: Intel Iris Pro 1536 MB 

- The issue is NOT reproducible on Mac OS X 10.11 -> graphics: NVIDIA GeForce GT 640M
- The issue is NOT reproducible on iMac (21.5-inch, Mid 2011) 10.10.5 -> graphics: AMD Radeon HD 6750M 512 MB

Comment 1

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Andrew, SoftVision found this bug during 50.b1 sign off testing. Could you please help find an owner to investigate? PointerLock API is a feature tracking for release 50.

Comment 2

[Andrew Overholt [:overholt]]

This is Xidorn's baby.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

What? It's system version related and hardware related? I'm afraid I probably don't have device for debugging this... I only have a 2013 15-inch rMBP with OS X 10.10...

FWIW, the PointerLock API has been implemented since long ago. 50 just unprefix the API. I believe it is not a regression, so it has been existing for quite long... (Probably since 10.11 released?)

If this issue is graphics card related, I would also guess it is some bug (maybe os bug) in the graphics part... PointerLock doesn't really involve graphics things. The only possible related thing is hiding cursor...

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

cbadau, is that related to e10s? If you open a non-e10s window and put it into fullscreen, does it have the same issue?

(FWIW, I observe another issue on OS X with pointerlock in fullscreen with e10s enabled, which seems to have the same symptom as Linux HiDPI issue descibed in bug 1255538.)

Comment 5

[Camelia Badau [:cbadau], Desktop Test Engineering]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> cbadau, is that related to e10s? If you open a non-e10s window and put it
> into fullscreen, does it have the same issue?
> 
> (FWIW, I observe another issue on OS X with pointerlock in fullscreen with
> e10s enabled, which seems to have the same symptom as Linux HiDPI issue
> descibed in bug 1255538.)

I can't tested on a non-e10s window because the "Open [Non-]e10s Window" option was removed from Menu (see bug 1003313). 
I've manually disabled e10s (set browser.tabs.remote.autostart.2 to false) and with e10s disabled, the issue is NOT reproducible.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Let me confirm, is the problem that, the position would move upwards at beginning, and then get stuck at the upmost point?

If you try using https://mdn.github.io/pointer-lock-demo/, the red ball would go upwards consistently in that case, right?

If yes, I probably know what the problem is, and have a potential fix now. But that still needs some more investigation.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1304692 - Make puppet widget get coordinate rounding from parent.

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

Comment 8

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

Comment on attachment 8795168 [details]
Bug 1304692 - Make puppet widget get coordinate rounding from parent.

https://reviewboard.mozilla.org/r/81296/#review79922

Do we end up updating the rounding value when window is moved to another display?

::: dom/ipc/TabChild.h:577
(Diff revision 1)
>      nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(webNav);
>      return GetFrom(docShell);
>    }
>  
>    virtual bool RecvUIResolutionChanged(const float& aDpi,
> +                                       const int32_t& aRounding,

So here, rounding is the 2nd param...

::: dom/ipc/TabParent.cpp:1034
(Diff revision 1)
>      TryCacheDPIAndScale();
>      // If mDPI was set to -1 to invalidate it and then TryCacheDPIAndScale
>      // fails to cache the values, then mDefaultScale.scale might be invalid.
>      // We don't want to send that value to content. Just send -1 for it too in
>      // that case.
> -    Unused << SendUIResolutionChanged(mDPI, mDPI < 0 ? -1.0 : mDefaultScale.scale);
> +    Unused << SendUIResolutionChanged(

...but here it is sent as 3rd param

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8795168 [details]
Bug 1304692 - Make puppet widget get coordinate rounding from parent.

https://reviewboard.mozilla.org/r/81296/#review79922

I searched mDefaultScale in the touched files, and added rounding with it, so I suppose we do update the rounding value as far as scale value is updated. Isn't that supposed to be done via UIResolutionChanged message?

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8795168 [details]
Bug 1304692 - Make puppet widget get coordinate rounding from parent.

https://reviewboard.mozilla.org/r/81296/#review79922

> ...but here it is sent as 3rd param

Ah... my bad.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8795168 [details]
Bug 1304692 - Make puppet widget get coordinate rounding from parent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81296/diff/1-2/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8795168 [details]
Bug 1304692 - Make puppet widget get coordinate rounding from parent.

https://reviewboard.mozilla.org/r/81296/#review79922

I tested that the rounding value is updated as expected when window is moved between monitors.

Comment 13

[Camelia Badau [:cbadau], Desktop Test Engineering]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6)
> Let me confirm, is the problem that, the position would move upwards at
> beginning, and then get stuck at the upmost point?
> 
> If you try using https://mdn.github.io/pointer-lock-demo/, the red ball
> would go upwards consistently in that case, right?
> 
> If yes, I probably know what the problem is, and have a potential fix now.
> But that still needs some more investigation.

Yes, that's the issue.

Comment 14

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

Comment on attachment 8795168 [details]
Bug 1304692 - Make puppet widget get coordinate rounding from parent.

https://reviewboard.mozilla.org/r/81296/#review79980

::: dom/ipc/TabChild.cpp:2952
(Diff revision 2)
>  void
> +TabChild::GetWidgetRounding(int32_t* aRounding)
> +{
> +  *aRounding = -1;
> +  if (!mRemoteFrame) {
> +    return;

Do we really want to return -1 here. I think we want 1, so that PuppetWidget's code end up using 1 and not -1

::: dom/ipc/TabChild.cpp:2960
(Diff revision 2)
> +    *aRounding = mRounding;
> +    return;
> +  }
> +
> +  // Fallback to a sync call if needed.
> +  SendGetWidgetRounding(aRounding);

Do we actually end up doing this sync call? Hopefully not normally at least. Please test that opening a new tab doesn't trigger this case.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for Fx50 based on SV recommendation.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8795168 [details]
Bug 1304692 - Make puppet widget get coordinate rounding from parent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81296/diff/2-3/

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8795168 [details]
Bug 1304692 - Make puppet widget get coordinate rounding from parent.

https://reviewboard.mozilla.org/r/81296/#review79980

> Do we really want to return -1 here. I think we want 1, so that PuppetWidget's code end up using 1 and not -1

Probably makes sense, although it doesn't really matter, since -1 is effectively the same as 1 when used as a rounding factor :)

> Do we actually end up doing this sync call? Hopefully not normally at least. Please test that opening a new tab doesn't trigger this case.

I tested it via adding a MOZ_ASSERT_UNREACHABLE before this statement, and it was never reached, so supposingly it is never called.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/853d4b8a9fde
Make puppet widget get coordinate rounding from parent. r=smaug

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/853d4b8a9fde

Comment 20

[Julien Cristau [:jcristau]]

This landed in nightly a couple of weeks ago, and is tracked for fx 50.  Xidorn, if you think this is ready to be uplifted would you mind filing the request for aurora/beta?

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

I'm not confident on uplifting this. Let's have it ride the train.

I think it is an issue with e10s, but not a new issue as itself.

Comment 22

[Julien Cristau [:jcristau]]

Wontfix for fx 50 per comment 21.