Closed Bug 1267636 Opened 8 years ago Closed 8 years ago

DevEdition theme and others cropped in menubar when switching to lowdpi monitor

Categories

(Core :: Widget: Win32, defect)

All
Windows
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 --- verified
firefox48 --- verified
firefox49 --- verified

People

(Reporter: bmaris, Assigned: jfkthame)

References

Details

(Keywords: regression)

Attachments

(2 files)

[Affected versions]:
- Firefox 47 beta 1
- Latest Developer Edition 48.0a2
- Latest Nightly 49.0a1

[Affected platforms]:
- Windows 10 64-bit

[Steps to reproduce]:
Prerequisits: hidpi and lowdpi monitors.
1. Start Firefox
2. Install DevEdition theme for Nightly and Beta. (https://addons.mozilla.org/en-US/firefox/addon/devedition-theme-enabler/)
3. Move Firefox from hidpi to lowdpi monitor.

[Expected result]:
- Theme is not altered in menubar.

[Actual result]:
- Theme is altered in menubar.
If theme is not installed, an increase of menubar can be seen when firefox is moved to lowdpi screen.

[Regression range]:
- This is a recent regression:

[2016-03-11, 2016-03-12]
Last good revision: 102886e9ac63b3fa33a6f1b394aea054abce2dfd (2016-03-11)
First bad revision: 946ed22cad04431c75ab5093989dfedf1bae5a3e (2016-03-12)

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=102886e9ac63b3fa33a6f1b394aea054abce2dfd&tochange=946ed22cad04431c75ab5093989dfedf1bae5a3e

Maybe caused by: 	
68cabbc58a91	Jonathan Kew — Bug 1249496 - Don't apply dpi-based scaling for window titlebar dimensions when on a secondary display, because windows doesn't scale it. r=emk

[Additional notes]:
- This happens only when moving FF from hidpi to lowdpi (hidpi being Main display).
- This also happens with other complete themes, eg: Lava Fox v2
- Windows 8.1 is not affected due to different windows theme, although the menubar is also increased on 8.1.
I thought this should have been addressed by bug 1256731... Jonathan, any idea what's going on here?
Blocks: 1249496, 1256731
Flags: needinfo?(jfkthame)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Handling the various theme metrics gets messy because while Windows doesn't (currently) scale the non-client area and controls according to per-monitor DPI, on Win10 we don't actually use the native Windows rendering of the controls. So while I don't really understand all the interactions going on here, the result of my experimentation across Win8.1 and Win10 with mixed DPI displays is that I think we should revert the GetMinimumWidgetSize part of bug 1249496, but only when running on Win10. This avoids the excess titlebar height that results in ugly DevEdition theme painting.

I've started a try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=33ddf38e6930. Bogdan, could you please test with this build when it's ready, and confirm whether it fixes the problem you were seeing here? Thanks.
Flags: needinfo?(jfkthame) → needinfo?(bogdan.maris)
Attachment #8753376 - Flags: review?(VYV03354)
Comment on attachment 8753376 [details] [diff] [review]
Adjust window titlebar metrics scaling on Win10 to improve the appearance of DevEdition and similar themes on a secondary lo-dpi display

Excellent.
Attachment #8753376 - Flags: review?(VYV03354) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4b7abddb9e13bf34c6b6a1865c449cbdaf7ec8
Bug 1267636 - Adjust window titlebar metrics scaling on Win10 to improve the appearance of DevEdition and similar themes on a secondary lo-dpi display. r=emk
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> I've started a try build at
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=33ddf38e6930.
> Bogdan, could you please test with this build when it's ready, and confirm
> whether it fixes the problem you were seeing here? Thanks.

I used this build and saw right from the start that when Firefox starts with a fresh profile the titlebar has some excess height but when moving the window, it automatically shrinks back to normal. This behavior can be seen on both lowdpi or hidpi.
Flags: needinfo?(bogdan.maris) → needinfo?(jfkthame)
Forgot to mention that after I move the window, I can't see any other issues related to this.
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #6)
> (In reply to Jonathan Kew (:jfkthame) from comment #3)
> > I've started a try build at
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=33ddf38e6930.
> > Bogdan, could you please test with this build when it's ready, and confirm
> > whether it fixes the problem you were seeing here? Thanks.
> 
> I used this build and saw right from the start that when Firefox starts with
> a fresh profile the titlebar has some excess height but when moving the
> window, it automatically shrinks back to normal. This behavior can be seen
> on both lowdpi or hidpi.

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #7)
> Forgot to mention that after I move the window, I can't see any other issues
> related to this.

I suspect this is bug 1273094.
Thanks Bogdan. It sounds like the issue you describe is probably bug 1273094, as Gijs says. To help us double-check, I've pushed a try build of mozilla-beta plus the patch here:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c0a17ef7353

When that's ready, we should be able to confirm that it resolves this problem and does _not_ have the other (bug 1273094) issue.
Flags: needinfo?(jfkthame) → needinfo?(bogdan.maris)
https://hg.mozilla.org/mozilla-central/rev/cb4b7abddb9e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8753376 [details] [diff] [review]
Adjust window titlebar metrics scaling on Win10 to improve the appearance of DevEdition and similar themes on a secondary lo-dpi display

Approval Request Comment
[Feature/regressing bug #]: 1249496

[User impact if declined]: ugly titlebar rendering for users with Win10, complete themes such as Dev Edition, mixed display resolutions, and Firefox running on a low-dpi secondary monitor

[Describe test coverage new/current, TreeHerder]: tested manually

[Risks and why]: minimal risk, as the patch is extremely localized in its effect (dpi scaling of titlebar metrics) and is restricted to Win10 only

[String/UUID change made/needed]: none
Attachment #8753376 - Flags: approval-mozilla-beta?
Attachment #8753376 - Flags: approval-mozilla-aurora?
Bogdan, could you verify this before we uplift it into beta? I will take it in aurora so that we improve the coverage
Comment on attachment 8753376 [details] [diff] [review]
Adjust window titlebar metrics scaling on Win10 to improve the appearance of DevEdition and similar themes on a secondary lo-dpi display

Improve dev edition l&f, taking it.
Attachment #8753376 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> Bogdan, could you verify this before we uplift it into beta?

Note: see comment 9 for the tryserver build of beta+patch, for verification.
Just checked using trybuild from comment 9 and I can't see the issue described in bug 1273094 anymore, it looks good.
Flags: needinfo?(bogdan.maris)
Comment on attachment 8753376 [details] [diff] [review]
Adjust window titlebar metrics scaling on Win10 to improve the appearance of DevEdition and similar themes on a secondary lo-dpi display

Fix was verified on a try build, Beta47+
Attachment #8753376 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Verified this is fixed on Windows 10 64-bit using Firefox 47 beta 8, latest Developer Edition 48.0a2, latest Nightly 49.0a1.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.