Closed Bug 1242720 Opened 4 years ago Closed 4 years ago

Text sizing problem with mixed DPI screens

Categories

(Core :: Widget: Win32, defect)

Unspecified
Windows 10
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 + fixed

People

(Reporter: dcamp, Assigned: jfkthame)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached image screenshot
I have a surface book (3000x2000) that I connect to an external 1920x1200 display, using extended display.  The external monitor is set as the main display.

Text/app size is 100% on the external display, 200% on the laptop display.

While the external display is plugged in Firefox looks bad no matter which display it's on.  Text/etc is too big and gets clipped.
Blocks: 890156
Is this with latest Nightly, current Firefox release, or what?

Have you tweaked any prefs related to resolution (e.g. layout.css.devPixelsPerPx) at any point? (Do you get the same problem with a fresh profile?)
Flags: needinfo?(dcamp)
Latest nightly.  Happens with a fresh profile.
Flags: needinfo?(dcamp)
Hmm. If you log out and log back in to Windows *while the external display is connected* does that make any difference?
Flags: needinfo?(dcamp)
If I restart while the external display is connected, firefox looks fine on both displays.  But then if I unplug, text becomes unreasonably small on the laptop display.
Flags: needinfo?(dcamp)
(In reply to Dave Camp (:dcamp) from comment #4)
> If I restart while the external display is connected, firefox looks fine on
> both displays.  But then if I unplug, text becomes unreasonably small on the
> laptop display.

OK. So this appears to be caused by a Windows issue, AFAICT.

You can reproduce similar issues even on a single-monitor system in Win8.1 and later. If you dynamically change the scaling factor of the display, Windows ends up in a sort of half-updated state whereby it's using the new resolution scaling, but system metrics such as theme font sizes are not adjusted accordingly until you log out (a full restart is not needed, I believe) and back in.

On Win10, when changing the scaling factor in the Display settings, I get a message "Sign in again for the best experience: Some apps will look their best after you've signed out of Windows and signed in again." And in Win8.1, there's a message in the control panel to similar effect.

In the dual-monitor case, the trouble is that Windows theme metrics (e.g. font and common control sizes) are dependent on the scaling of the system's primary display. But when you connect/disconnect your external monitor, because you've configured it to be the main display (when it's available), you are in effect changing your system's primary scaling factor -- but you're not going through the Display settings or control panel to do so, and therefore even though you're ending up in that semi-updated state, you miss the warning that a logout/login is necessary "for applications to look their best". :(

(An illustration of the fact that Windows is getting in a slightly inconsistent state: when you change the resolution scale factor in Display settings, e.g. from 100% to 150%, the positioning of icons on the desktop adjusts slightly but the icons themselves do not scale to the new size. But then after logout/login, they do get scaled because the Windows state is now fully updated and consistent.)

Maybe we can somehow detect this semi-updated state of Windows after a primary-display resolution change but before a logout has fully refreshed things, and compensate for it in our code.... it's certainly an ugly problem.
Please revisit bug 890156 comment #127.
::GetDeviceCaps() returns the old value (because this function is designed for apps that do not support dynamic DPI change) and ::GetDpiForMonitor() returns the new value in the semi-updated state.
Ah, thanks - I hadn't realized they could differ in that situation. That may indeed give a way forward here.
This seems to make things better; thanks for pointing out the distinction.
Attachment #8713195 - Flags: review?(VYV03354)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=649865838936

:dcamp, if you'd like to try that build (once available), I think it should help with the worst of the issues you're describing.
Comment on attachment 8713195 [details] [diff] [review]
Use (non-dynamic) resolution from GetDeviceCaps when dealing with native-theme code that does not handle dynamic changes to system DPI

Review of attachment 8713195 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/WinUtils.cpp
@@ +525,5 @@
>  }
>  
> +// static
> +double
> +WinUtils::SystemScaleFactor()

Could you change the latter part of LogToPhysFactor so that it calls this function?

r=me with this change.
Attachment #8713195 - Flags: review?(VYV03354) → review+
(In reply to Masatoshi Kimura [:emk] from comment #10)
> > +// static
> > +double
> > +WinUtils::SystemScaleFactor()
> 
> Could you change the latter part of LogToPhysFactor so that it calls this
> function?

Sure, good suggestion.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5281172300f0d4afc55898cb581b42c7c4ff261b
Bug 1242720 - Use (non-dynamic) resolution from GetDeviceCaps when dealing with native-theme code that does not handle dynamic changes to system DPI. r=emk
https://hg.mozilla.org/mozilla-central/rev/5281172300f0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Weighing in a bit late, that try build did work for me.  Thanks!
Looks like my suggestion got lost.
Attachment #8713918 - Flags: review?(jfkthame)
It's a waste of time to divide by 96.0 everytime SystemScaleFactor() is called.
This function will be called from a performance-sensitive code path as bug 1240180 indicates.
Attachment #8713918 - Attachment is obsolete: true
Attachment #8713918 - Flags: review?(jfkthame)
Attachment #8713931 - Flags: review?(jfkthame)
(In reply to Masatoshi Kimura [:emk] from comment #15)
> Created attachment 8713918 [details] [diff] [review]
> Followup: reduce code duplication
> 
> Looks like my suggestion got lost.

Huh, that's weird. I definitely remember making the change! I must have messed up somehow and lost it, sorry.
Comment on attachment 8713931 [details] [diff] [review]
Pre-calculate the system scale

Review of attachment 8713931 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8713931 - Flags: review?(jfkthame) → review+
Comment on attachment 8713195 [details] [diff] [review]
Use (non-dynamic) resolution from GetDeviceCaps when dealing with native-theme code that does not handle dynamic changes to system DPI

Approval Request Comment
[Feature/regressing bug #]: 890156 (Windows per-monitor DPI support)

[User impact if declined]: possibility of badly-sized text in Firefox UI when system (primary) resolution is changed dynamically on Win8.1/Win10

[Describe test coverage new/current, TreeHerder]: tested locally, :dcamp confirmed that try build resolved his issue

[Risks and why]: Low risk, no effect except in multi-monitor/mixed-DPI setups.

[String/UUID change made/needed]: n/a
Attachment #8713195 - Flags: approval-mozilla-aurora?
Comment on attachment 8713931 [details] [diff] [review]
Pre-calculate the system scale

Approval Request Comment

See above. This is a followup for a review suggestion; should have been part of the original landing, but I messed up with HG and so :emk landed this separately.
Attachment #8713931 - Flags: approval-mozilla-aurora?
Marking affected for 46 and tracking since this is follow up work from bug 890145. 
If you end up backing out the work from 890145, should this also be backed out in 46?
Flags: needinfo?(jfkthame)
Comment on attachment 8713195 [details] [diff] [review]
Use (non-dynamic) resolution from GetDeviceCaps when dealing with native-theme code that does not handle dynamic changes to system DPI

Fix for recent regression, manual testing looks ok.
Attachment #8713195 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8713931 [details] [diff] [review]
Pre-calculate the system scale

Manual testing, recent regression, please uplift to aurora.
Attachment #8713931 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #23)
> Marking affected for 46 and tracking since this is follow up work from bug
> 890145. 

Just FTR, that's a typo: s/890145/890156/

> If you end up backing out the work from 890145, should this also be backed
> out in 46?

It shouldn't need to be. If we decide we're not ready to expose the per-monitor dpi feature on release, I anticipate we'll just disable it via the application manifest, without needing a wholesale backout of the code changes.
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #26)
> It shouldn't need to be. If we decide we're not ready to expose the
> per-monitor dpi feature on release, I anticipate we'll just disable it via
> the application manifest, without needing a wholesale backout of the code
> changes.

It will not fix non-Windows issues such as bug 1240533.
Backed out of aurora-46, see bug 890156 comment 168.
You need to log in before you can comment on or make changes to this bug.