Closed Bug 1278818 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

47 Branch
x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 - wontfix
firefox48 + verified
firefox49 + verified
firefox50 + verified

People

(Reporter: thomas_scheffler, Assigned: jfkthame)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86
Component: Untriaged → Widget: Cocoa
Product: Firefox → Core
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.
Flags: needinfo?(jfkthame)
Keywords: regression
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.
Flags: needinfo?(thomas_scheffler)
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...)
Flags: needinfo?(jfkthame) → needinfo?(bogdan.maris)
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...
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
(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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(thomas_scheffler)
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.
Here are some stats about users that have monitors with multiple scale factors:
383 out of 11569 sessions matched. (3.31%)
(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.
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.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
Blocks: 890156
Duplicate of this bug: 1269802
Attachment #8761795 - Flags: review?(VYV03354)
Attachment #8761795 - Flags: review?(VYV03354) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/358018e43d8da2517c3600c6172f129c1e65ada0
Bug 1278818 - Convert window coordinates to desktop pixels before passing to ScreenForRect. r=emk
https://hg.mozilla.org/mozilla-central/rev/358018e43d8d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
As I see, Firefox 50 is scheduled to be released in November. Any chance this bug is fixed in let's say Firefox 48?
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.
Flags: needinfo?(bogdan.maris)
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
Attachment #8761795 - Flags: approval-mozilla-beta?
Attachment #8761795 - Flags: approval-mozilla-aurora?
Tracking 50+ for this fullscreen regression.
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!
Flags: needinfo?(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.
Flags: needinfo?(jfkthame)
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
Attachment #8761795 - Flags: approval-mozilla-beta?
Attachment #8761795 - Flags: approval-mozilla-beta+
Attachment #8761795 - Flags: approval-mozilla-aurora?
Attachment #8761795 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.