Closed Bug 1304692 Opened 3 years ago Closed 3 years ago

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

Categories

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

50 Branch
x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 + wontfix
firefox52 --- fixed

People

(Reporter: cbadau, Assigned: xidorn)

Details

Attachments

(1 file)

[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
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.
Flags: needinfo?(overholt)
This is Xidorn's baby.
Flags: needinfo?(overholt) → needinfo?(xidorn+moz)
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...
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.)
Flags: needinfo?(xidorn+moz) → needinfo?(camelia.badau)
(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.
Flags: needinfo?(camelia.badau)
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.
Flags: needinfo?(camelia.badau)
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
Attachment #8795168 - Flags: review?(bugs) → review-
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 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 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.
(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.
Flags: needinfo?(camelia.badau)
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.
Attachment #8795168 - Flags: review?(bugs) → review+
Tracked for Fx50 based on SV recommendation.
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.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/853d4b8a9fde
Make puppet widget get coordinate rounding from parent. r=smaug
Assignee: nobody → xidorn+moz
https://hg.mozilla.org/mozilla-central/rev/853d4b8a9fde
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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?
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.