1302168 - 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

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

Attachment: 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.

Comment 1

[Masatoshi Kimura [:emk]]

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.

Comment 2

[Jonathan Kew [:jfkthame]]

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.

Comment 3

[Marcia Knous [:marcia]]

Tracking 50/51+ for this regression as it is a common operation using Windows.

Comment 4

[Masatoshi Kimura [:emk]]

(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.

Comment 5

[Masatoshi Kimura [:emk]]

(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.

Comment 6

[Chris Peterson [:cpeterson]]

btw, I am able to reproduce this bug with a display scaling factor of 200% but not 225%.

Comment 7

[Andrew Overholt [:overholt]]

(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?

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: 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.

Comment 9

[Chris Peterson [: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.

Comment 10

[Jonathan Kew [:jfkthame]]

(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.

Comment 11

[Andrew Overholt [:overholt]]

Thanks, Chris and Jonathan.

Comment 12

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

we are a week away from RC, too late, this is now a wontfix for 50.

Comment 13

[Chris Peterson [:cpeterson]]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[Chris Peterson [:cpeterson]]

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.

Comment 16

[:Gijs (he/him)]

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.

Comment 17

[Chris Peterson [:cpeterson]]

(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.

Comment 18

[Kevin Brosnan [Ex-Mozilla]]

[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.

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

https://www.reddit.com/r/firefox/comments/5nv021/clicking_top_of_windowtab/
https://www.reddit.com/r/firefox/comments/5m2oew/new_unclickable_area_on_top_of_tabs_its_different/
https://www.reddit.com/r/firefox/comments/5jy0tg/can_i_make_taps_extend_all_the_way_to_the_top_of/

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like this has a bad impact for many users. Jim can you help find an owner to investigate?

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrew, maybe you can help? Thanks!

Comment 22

[Andrew Overholt [:overholt]]

Jim is in a better place to help than I am.

Comment 23

[Jim Mathies [:jimm]]

Does this correct itself if you restart the browser?

Comment 24

[Chris Peterson [: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.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 26

[Jim Mathies [:jimm]]

(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.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

We can still take a patch here but I'm going to let this fall off the regression triage.

Comment 28

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

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.

Comment 29

[mitch deoudes]

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.

Comment 31

[nyanpasu64]

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.

Comment 32

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

Attachment: Compensate for non-client scaling

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 33

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

Attachment: Bug 1302168: Fix maximized caption height with per-monitor DPI

Comment 34

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

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.

Comment 35

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

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.

Comment 36

[Jim Mathies [:jimm]]

Comment on attachment 9006307 [details]
Bug 1302168: Fix maximized caption height with per-monitor DPI

Jim Mathies [:jimm] has approved the revision.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57a2d6865e17
Fix maximized caption height with per-monitor DPI r=jmathies

Comment 38

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/57a2d6865e17

Comment 40

[Pascal Chevrel:pascalc]

Adam, is that a good candidate for a beta uplift or should we let it ride the trains?

Comment 41

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

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.