1514501 - Forcing fullscreen may choose wrong monitor on Win7

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

The fullscreen fix in bug 1482920 doesn't properly handle selecting the screen to use when forcing the window dimensions if DPI scaling is enabled in Windows 7. Bug 1513066 addresses scaling the size of the window but not scaling the coordinates used in selecting a monitor.

Comment 1

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Steps to reproduce:

1. Configure 3 screens side-by-side, 1600x1200, 150% text size (DPI scaling), this is probably most easily done in VirtualBox.
2. Put a window on screen 2
3. Press F11 to fullscreen

Expected result:
Fullscreen on window 2

Actual result:
Fullscreen on window 3

This is because I'm passing the layout device pixels from Windows directly to ScreenForRect, which expects desktop pixels.

The fix is to divide by GetDesktopToDeviceScale() first.

---

While I'm in here, I noticed that the Shift + Windows + Left/Right Arrow hotkey (to move a window between screens) was behaving badly with DPI scaling on Win7 due to how we're rounding back and forth between layout device and desktop pixels (between setting Screen::mRectDisplayPix and nsWindow::Resize()), this causes fullscreen windows to be the wrong size by a pixel, and when it won't fit on the target screen (by some heuristic) Windows won't move the window with the hotkey:

1. Configure two 1600x1200 screens side-by-size, set text size to 150%
2. Open a window on screen 2, not maximized (in release 64.0 if you start on screen 1 it will fail in step 4, unless maximized in which case it will fail on step 5)
3. F11 to switch to fullscreen on screen 2
4. shift + windows key + left arrow to cycle to screen 1
5. repeat step 4 to switch back to screen 2

I'm including in this patch just using GetRect() directly instead of GetRectDisplayPix() and scaling to get the monitor size in layout device pixels, which was used in bug 1513066.
---

An old issue with the hotkey is bug 1475683, when maximized and fullscreen Shift+Win+Arrow will fail to move the window back to the primary monitor. No regression but I had hoped this would fix that, no such luck.

Comment 2

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1514501 - Fix screen selection and scaling for fullscreen

I needed to scale the layout device pixels coming from Windows before passing them to ScreenForRect(). Also, I'm using GetRect() directly instead of GetRectDisplayPix() * scale now, to avoid an unnecessary double scale & round which was making fullscreen windows off by one pixel in many cases.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/741ded543228
Fix screen selection and scaling for fullscreen r=jmathies

Comment 4

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/741ded543228

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance. Let's get QA looking at this too.

Comment 6

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 9034051 [details]
Bug 1514501 - Fix screen selection and scaling for fullscreen

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1482920

User impact if declined: On Win7, with multiple monitors (might need at least 3) and dpi scaling, when Firefox goes fullscreen it will do so on the wrong monitor.

Is this code covered by automated tests?: Unknown

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): It is a focused change which directly addresses the oversight that caused the regression. I've checked the issue on Nightly, reproducible in 20190109092644, fixed in 20190110893854.

String changes made/needed:

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9034051 [details]
Bug 1514501 - Fix screen selection and scaling for fullscreen

[Triage Comment]
Fixes Firefox going fullscreen on the wrong monitor for some users under certain circumstances. Approved for 65.0b11.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/694ef5b65db8

Comment 9

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce this issue using Firefox 65.0b4 on Windows 7 32bit VM.

This issue is verified fixed using Firefox 66.0a1 (BuildId:20190114014511) and Firefox 65.0b11 (provided in comment 8) on Windows 7 32bit VM.