1278818 - Firefox 47 does not stay on the monitor when video is switched to fullscreen

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Thomas Scheffler]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160604131506

Steps to reproduce:

- Mac OS X 10.11.5
- Multi monitor setup is required (happens on FullHD and Retina of MacBook 15 or tripple Setup: FullHD, MacBook, Thunderbolt Monitor).
- Open a youtube video and switch to fullscreen.



Actual results:

- Firefox swaps to another monitor, I think the one with the largest logical resolution.
It's not like the video is just displayed on the wrong monitor, but Firefox with all tabs change places and goes to the largest monitor, which is really annoying.


Expected results:

Up to Firefox 46 switching to fullscreen displayed the video on the same monitor that was used to display the video in embedded form.

Comment 1

[Loic]

It could be an issue of bug 1240533 introduced in FF47.

Jonathan, do you have an idea or could you NI? someone from QA with OSX/dual monitor to try to reproduce this issue, please.

Comment 2

[Loic]

Thomas, could you use mozregression to find a regression range in FF47, please.

See http://mozilla.github.io/mozregression/ for details.
Run the command "mozregression --good=46" then copy here the final pushlog provided by the tool.

Comment 3

[Jonathan Kew [:jfkthame]]

This could also be a regression from something in bug 890156 and its various followups; although that bug was primarily about Windows, some of the code involved is cross-platform so OS X regressions are possible.

Bogdan, can you reproduce this? (I'll also try locally soon...)

Comment 4

[Thomas Scheffler]

mozregression did not find it. So I tried a new profile. FF 47 does not show the issue with a fresh profile. I will try to track down the setting or extension that causes this...

Comment 5

[Thomas Scheffler]

The problem is also present in safe mode and cannot be reproduced by copying pref.js to a fresh profile.
I noticed the "-p" option of mozregression. Sadly I was not able to track it down with "--good=46" but luckly with "--good=45"

here are the last lines of "mozregression --good=45 --profile-persistence=reuse -p ~/Library/Application\ Support/Firefox/Profiles/o1wih9wm.default-1403109554591/":

22:47.57 INFO: Using local file: /var/folders/9b/qczgfng51ggg26_63vvlwlpc0000gp/T/tmpP3oqNl/a9f9b36c1a2e--mozilla-inbound--firefox-46.0a1.en-US.mac.dmg (downloaded in background)
22:47.57 INFO: Running mozilla-inbound build built on 2016-01-13 10:35:31.221000, revision a9f9b36c
23:12.10 INFO: Launching /private/var/folders/9b/qczgfng51ggg26_63vvlwlpc0000gp/T/tmpqXJcQa/Nightly.app/Contents/MacOS/firefox
23:12.11 INFO: application_buildid: 20160113004331
23:12.11 INFO: application_changeset: a9f9b36c1a2eec7626e6b749e46ab0a8bf3323e2
23:12.11 INFO: application_name: Firefox
23:12.11 INFO: application_repository: https://hg.mozilla.org/integration/mozilla-inbound
23:12.11 INFO: application_version: 46.0a1
Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): bad
23:47.74 INFO: Narrowed inbound regression window from [98756a36, ecd38844] (3 revisions) to [98756a36, a9f9b36c] (2 revisions) (~1 steps left)
23:47.74 INFO: Oh noes, no (more) inbound revisions :(
23:47.74 INFO: Last good revision: 98756a36223c1a2b51cd0368736b728429038caf
23:47.74 INFO: First bad revision: a9f9b36c1a2eec7626e6b749e46ab0a8bf3323e2
23:47.74 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=98756a36223c1a2b51cd0368736b728429038caf&tochange=a9f9b36c1a2eec7626e6b749e46ab0a8bf3323e2

Comment 6

[Loic]

(In reply to Thomas Scheffler from comment #4)
> mozregression did not find it. So I tried a new profile. FF 47 does not show
> the issue with a fresh profile. I will try to track down the setting or
> extension that causes this...

Thanks for the regression range. Yes, Mozreg is easy to use, you can specify the path of a profile (with custom settings) and of course, you can run the test from any date or version number or revision string (after 2009, I guess). ;)

It's probably a regression from bug 890156 even there is:
Jonathan Kew — Bug 1239007 - Replace nsIntRect by strongly-typed rects in a couple of nsCocoaUtils functions. r=mstange

Comment 7

[Jonathan Kew [:jfkthame]]

It looks like this happens when Firefox is on a hi-dpi (retina) display, and there's another monitor to the right (e.g. an external screen connected to a retina MacBook). (I don't think it matters whether the additional display is hi- or low-dpi.)

Reproducing the bug is also somewhat dependent on the size/position of the original browser window; if you reduce the size of the window, and move it to the left of the original screen, it'll probably fullscreen correctly on the same display, but enlarge the window and it will go to the other display.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Here are some stats about users that have monitors with multiple scale factors:
383 out of 11569 sessions matched. (3.31%)

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Here are some stats about users that have monitors with multiple scale
> factors:
> 383 out of 11569 sessions matched. (3.31%)

These are mac users with monitors with multiple scales.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Convert window coordinates to desktop pixels before passing to ScreenForRect

AFAICT, the problem here arises from nsBaseWidget::GetWidgetScreen() passing the device-pixel window coords to ScreenForRect, which actually wants global desktop pixels. So this should fix the behavior. We'll need to test across other platforms as well, though, given that the change is in shared nsBaseWidget code.

Comment 11

[Jonathan Kew [:jfkthame]]

There's a try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=665725c627a4.

Bogdan, if you could test this (across Mac/Win/Linux with multiple-display configurations) that would be great -- thanks.

Comment 13

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/358018e43d8da2517c3600c6172f129c1e65ada0
Bug 1278818 - Convert window coordinates to desktop pixels before passing to ScreenForRect. r=emk

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/358018e43d8d

Comment 15

[Thomas Scheffler]

As I see, Firefox 50 is scheduled to be released in November. Any chance this bug is fixed in let's say Firefox 48?

Comment 16

[Bogdan Maris, Desktop QA]

Sorry for the late reply I was on PTO.
I was able to reproduce the initial issue on a MacBook Pro and 4k display using Firefox 47 beta 4 and verified that using latest Nightly 50.0a1 the issue is fixed.

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8761795 [details] [diff] [review]
Convert window coordinates to desktop pixels before passing to ScreenForRect

Approval Request Comment
[Feature/regressing bug #]: 890156

[User impact if declined]: Firefox may unexpectedly move to a different monitor when video is toggled to fullscreen.

[Describe test coverage new/current, TreeHerder]: tested manually on multi-screen configs

[Risks and why]: pretty low risk - patch is only relevant to multi-screen systems; worst-case is that it could lead to us finding the wrong screen in certain configurations, but that's exactly the type of problem we're aiming to fix here

[String/UUID change made/needed]: n/a

Comment 18

[Marcia Knous [:marcia]]

Tracking 50+ for this fullscreen regression.

Comment 19

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

Hi Jonathan, do you consider this fix safe enough to include in a 47 dot release? I would prefer to let this one be wontfix'd for 47 and ship it in Fx48. Please let me know if you have any concerns. Thanks!

Comment 20

[Jonathan Kew [:jfkthame]]

I'd consider it safe enough to include... the only area of functionality it could possibly regress would be the same as we're aiming to fix here, choosing the screen where a window should be placed. But this is a small and well-contained patch, it's confirmed to fix the issue we had, and no new problems have been reported since it landed, so I'd be comfortable shipping it even in a dot release.

On the other hand, I also think the problem here is sufficiently non-critical -- it's annoying, for the small minority of users who run into it, but it's not crippling -- that leaving it as WONTFIX in 47 and taking the fix on beta48 would also be OK with me; we can live with it for a few more weeks.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Per comment #19 & #20

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8761795 [details] [diff] [review]
Convert window coordinates to desktop pixels before passing to ScreenForRect

Let's try that in 48.
Should be in 48 beta 3

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/357a488bdca4

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/165d10d41d9a

Comment 25

[Bogdan Maris, Desktop QA]

Also verified as fixed using Firefox 48 beta 9 and latest DevEdition 49.0a2 across platforms (Mac OS X 10.11.2 - Macbook with retina and 4k monitor -, Windows 10 64-bit and Ubuntu 14.04 64-bit using 4k monitor and full HD just to see if nothing was broken).