Closed Bug 1302168 Opened 5 years ago Closed 3 years ago

Windows 10 maximized window's menu bar is not on top edge of screen, window has ~3 pixel border, when changing the screen DPI scaling while Firefox is open

Categories

(Core :: Widget: Win32, defect, P2)

50 Branch
Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 + wontfix
firefox51 + wontfix
firefox52 + wontfix
firefox-esr60 --- wontfix
firefox53 + wontfix
firefox54 + wontfix
firefox55 - wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: cpeterson, Assigned: agashlin)

References

Details

(Keywords: polish, regression, Whiteboard: tpi:+)

Attachments

(3 files, 1 obsolete file)

Attached image menu-bar-screenshot.png
[Tracking Requested - why for this release]:

Jonathan, I bisected this regression from Windows DPI bug 1270954. It sounds like a dupe of bug 1289143, but that bug has already been fixed on Nightly and Aurora.

STR:
1. Maximize Firefox window.
2. Press the ALT key to open the menu bar.
3. Move the mouse pointer to the top edge of the screen.
4. Try to click one of the menu bar names.

RESULT:
Nothing happens. There is a ~3 pixel border around the maximized window between the top edge of the screen and the menu bar names.

See the attached (zoomed) screenshot of my menu bar.

I am running Windows 10 Anniversary Update on the Insider Preview "Slow Ring" channel.
Flags: needinfo?(jfkthame)
Maybe because we don't consider per-monitor non-client metrics yet?
https://dxr.mozilla.org/mozilla-central/source/widget/windows/nsNativeThemeWin.cpp#1664-1668

Windows 10 Anniversary Update added GetSystemMetricsForDpi function for this.
Version: unspecified → 50 Branch
Yes, we should look into adopting the new GetSystemMetricsForDpi (and/or SystemParametersInfoForDpi) APIs to get the various metrics we need, rather than the old non-dpi-aware functions. We currently try to scale the legacy metrics according to the monitor we're running on, but perhaps that isn't always done correctly.

See also bug 1301944, which sounds like a possibly-related issue.
See Also: → 1301944
Tracking 50/51+ for this regression as it is a common operation using Windows.
(In reply to Jonathan Kew (:jfkthame) from comment #2)
> We currently try to scale the legacy
> metrics according to the monitor we're running on, but perhaps that isn't
> always done correctly.

IIRC, some of them are backed out because they didn't agree with Windows non-client dpi metrics. (e.g. bug 1256731.)  I think we should (partially) revert bug 1256731 because now Windows respects per-monitor dpi scaling if EnableNonClientDpiScaling is available.
Blocks: 1256731
(In reply to Masatoshi Kimura [:emk] from comment #4)
> (In reply to Jonathan Kew (:jfkthame) from comment #2)
> > We currently try to scale the legacy
> > metrics according to the monitor we're running on, but perhaps that isn't
> > always done correctly.
> 
> IIRC, some of them are backed out because they didn't agree with Windows
> non-client dpi metrics. (e.g. bug 1256731.)  I think we should (partially)
> revert bug 1256731 because now Windows respects per-monitor dpi scaling if
> EnableNonClientDpiScaling is available.

Or use new APIs for border metrics instead of depending on imprecise calculation.
btw, I am able to reproduce this bug with a display scaling factor of 200% but not 225%.
(In reply to Chris Peterson [:cpeterson] from comment #6)
> btw, I am able to reproduce this bug with a display scaling factor of 200%
> but not 225%.

How are you setting this?

With a default scaling factor I'm unable to reproduce this in 50 on Windows 10 (what I think is the stock release).

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Yes, we should look into adopting the new GetSystemMetricsForDpi (and/or
> SystemParametersInfoForDpi) APIs to get the various metrics we need, rather
> than the old non-dpi-aware functions.

To me it feels pretty late in the 50 cycle to try to change what APIs we use for this stuff but maybe it's not a big deal?
Flags: needinfo?(cpeterson)
Attached image Display_settings.jpg
(In reply to Andrew Overholt [:overholt] from comment #7)
> (In reply to Chris Peterson [:cpeterson] from comment #6)
> > btw, I am able to reproduce this bug with a display scaling factor of 200%
> > but not 225%.
> 
> How are you setting this?

Open Windows' Display settings and then adjust the "Change the size of text, apps, and other items". Please see the attached screenshot.
Flags: needinfo?(cpeterson)
Depending on your screen resolution, you may need to try different scaling factors and possibly restart Firefox for it to recognize the new scaling factor.
(In reply to Andrew Overholt [:overholt] from comment #7)
> (In reply to Jonathan Kew (:jfkthame) from comment #2)
> > Yes, we should look into adopting the new GetSystemMetricsForDpi (and/or
> > SystemParametersInfoForDpi) APIs to get the various metrics we need, rather
> > than the old non-dpi-aware functions.
> 
> To me it feels pretty late in the 50 cycle to try to change what APIs we use
> for this stuff but maybe it's not a big deal?

No, I wouldn't want to attempt that for 50; it'd be for Nightly, and maybe (depending how complex it looks) we'd consider uplifting to Aurora, but I don't think the issue here would justify landing a potentially-risky change like that on Beta.
Flags: needinfo?(jfkthame)
Priority: -- → P2
Whiteboard: tpi:+
we are a week away from RC, too late, this is now a wontfix for 50.
This is a pretty annoying usability regression that will ship in 50. Will we be able to fix it for 51? The Beta 51 cycle starts just next week, but it will be longer than usual and Release 51 won't ship until January 24.
huh, I can't reproduce this I would have thought I would have been able to. I'm on Win10 Anniversary Edition with a 4K monitor set to 200% scaling. But when I maximize the window and hit the Alt key, I can select any menu with my cursor at the top edge of the screen. I tried on 50.1 and Nightly.

Anyway, given earlier comments about uplift risk, I'm setting 51 to wontfix.
I can still reproduce this problem if I change the display scale (from 225% to 200%) while Firefox is running. The Display Settings Control Panel says "Some apps won't respond to scaling changes until you sign out." If I quit and restart Firefox, then the tab scaling problem is fixed (without needing to sign out as suggested by the Display Settings Control Panel). So the problem appears to be that Firefox is not responding dynamically to display scale changes while running.
Updating the summary per comment #15.

Chris, was it always only broken in this case (when updating dpi settings while Firefox was open) or did part of this bug effectively get fixed? I'm asking because maybe whatever fixed that bit might still want uplift.
Flags: needinfo?(cpeterson)
Summary: Windows 10 maximized window's menu bar is not on top edge of screen, window has ~3 pixel border → Windows 10 maximized window's menu bar is not on top edge of screen, window has ~3 pixel border, when changing the screen dpi while Firefox is open
(In reply to :Gijs from comment #16)
> Chris, was it always only broken in this case (when updating dpi settings
> while Firefox was open) or did part of this bug effectively get fixed? I'm
> asking because maybe whatever fixed that bit might still want uplift.

I retested 49 and it has the same behavior as 50-53: display scaling changes are not handled correctly until Firefox is restarted. So there is no bug fix that could be uplifted; I just better understand the STR.
Flags: needinfo?(cpeterson)
Summary: Windows 10 maximized window's menu bar is not on top edge of screen, window has ~3 pixel border, when changing the screen dpi while Firefox is open → Windows 10 maximized window's menu bar is not on top edge of screen, window has ~3 pixel border, when changing the screen DPI scaling while Firefox is open
[Tracking Requested - why for this release]: this is truly horrible when you encounter this

This is always broken for me using an external monitor and mixed display zoom settings. One set to 175% the other set to 125%. The only way I don't get a bar is to use the same zoom setting on the two displays. This does not look good as they have different pixel densities.
Sounds like this has a bad impact for many users. Jim can you help find an owner to investigate?
Flags: needinfo?(jmathies)
Andrew, maybe you can help? Thanks!
Flags: needinfo?(overholt)
Jim is in a better place to help than I am.
Flags: needinfo?(overholt)
Does this correct itself if you restart the browser?
Flags: needinfo?(cpeterson)
(In reply to Jim Mathies [:jimm] from comment #23)
> Does this correct itself if you restart the browser?

Yes. More info and STR in comment 15.
Flags: needinfo?(cpeterson)
Flags: needinfo?(jmathies)
I don't think it seems reasonable to expect users to restart the browser when they plug in an external monitor. 

Jim, are you intending to wontfix this bug? No one is assigned and we've been wontfixing it for 5 months. 
Do you want to ask QE to test if it's still an issue on current nightly?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #25)
> I don't think it seems reasonable to expect users to restart the browser
> when they plug in an external monitor. 
> 
> Jim, are you intending to wontfix this bug? No one is assigned and we've
> been wontfixing it for 5 months. 
> Do you want to ask QE to test if it's still an issue on current nightly?

Valid bug, it's just corner case so not a huge priority. User's don't tweak dpi settings often and running multiple monitors with different dpis is not a common use case for us. We should keep this around until we find some time to work on it.
Flags: needinfo?(jmathies)
We can still take a patch here but I'm going to let this fall off the regression triage.
I don't feel the need to track this for 55, if a fix is ready and deemed low risk, please nominate for uplift to Beta55.
Keywords: polish
I'm getting this behavior on FF 57.0.3 & Win10, and it's driving me nuts.  When the window is on the laptop screen & maximized, I can click on the tab tops.  When it's on the secondary monitor, there's a 1 pixel space at the top that prevents tab selecting, but allows dragging the window.  I.e., it behaves as if it wasn't maximized.

Restarting FF doesn't fix it.  Setting the display scaling on the second monitor to > 100% *does* fix it, however - irrespective of the laptop screen setting.  Setting both the laptop and external monitor to 100% scaling does *not* fix it.  Fiddling with userChrome.css did not seem to fix it.

w.r.t. the previous comment about mixed-scale display setups: I would argue that now that HDPI laptops are becoming common, mixed-scale display setups are, too.  My external monitor is 1920x1200, but my laptop does 2560x1440.  Having the same display scaling on both of them just isn't feasible.
See Also: → 1434110
See Also: 1434110
Duplicate of this bug: 1434110
I'm getting this behavior on Firefox Dev 62.0b6 (64-bit) & Win10. Restarting FF doesn't fix it. Only logging out, and back in with a 96dpi "main monitor", fixes it.

I'm also using a HiDPI laptop with mixed-scale display setup. My external monitor *and* laptop are 1920x1200, but my laptop screen is only 60% the size of my external monitor.
This gap seems to be equal to:

(GetSystemMetrics      (SM_CYFRAME)      + GetSystemMetrics      (SM_CXPADDEDBORDER)) -
(GetSystemMetricsForDpi(SM_CYFRAME, dpi) + GetSystemMetricsForDpi(SM_CYPADDEDBORDER, dpi)

This also holds when the top of the tabs are cut off (negative gap).

Unsurprisingly this seems related to non-client DPI scaling (bug 1270954, bug 1354020). If I remove PerMonitorV2 in the manifest and WinUtils::NonClientDpiScalingDefWindowProc, this issue goes away. I'm not sure what we're supposed to be doing differently, though. I'm attaching a patch with a hack based on the above.
Comment on attachment 9005791 [details] [diff] [review]
Compensate for non-client scaling

There is a specific hack in nsNativeThemeWin::GetWidgetPadding to compensate for a hack in nsWindow::UpdateNonClientMargins that includes the border padding in the window. One reflected per-monitor DPI but the other did not, thus the offset noted in comment 32.
Attachment #9005791 - Attachment is obsolete: true
I tried taking out the GetSystemMetricsForDpi calls, to avoid using those in just one place, but using the same kind of scaling as in UpdateNonClientMargins was causing serious discrepancies.

This really calls for using GetSystemMetricsForDpi at least throughout all the non-client area calculations, since adding up the components scaled by GetSystemMetricsForDpi vs. adding them up and then scaling and rounding is going to give different values in general. But I think this patch should work for now until we decide to catch up on that. See also bug 1408328 which shows this issue with the rest of the window frame.
Assignee: nobody → agashlin
Status: NEW → ASSIGNED
Comment on attachment 9006307 [details]
Bug 1302168: Fix maximized caption height with per-monitor DPI

Jim Mathies [:jimm] has approved the revision.
Attachment #9006307 - Flags: review+
Pushed by agashlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57a2d6865e17
Fix maximized caption height with per-monitor DPI r=jmathies
https://hg.mozilla.org/mozilla-central/rev/57a2d6865e17
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Duplicate of this bug: 1485281
Adam, is that a good candidate for a beta uplift or should we let it ride the trains?
Flags: needinfo?(agashlin)
I don't see a great reason to uplift, it's an old fairly minor issue. And the world of multi-dpi is complicated enough it's safer to stick with the trains.
Flags: needinfo?(agashlin)
Duplicate of this bug: 1496778
You need to log in before you can comment on or make changes to this bug.