Closed Bug 832524 Opened 11 years ago Closed 11 years ago

Menu items are misaligned on high DPI

Categories

(Firefox :: Theme, defect)

20 Branch
All
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 23
Tracking Status
firefox22 + fixed
firefox23 --- fixed

People

(Reporter: edwardsgreg, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(9 files)

Attached image mainmenuaurora20.png
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130114 Firefox/20.0
Build ID: 20130114042016

Steps to reproduce:

Set Windows's DPI scale to something high like 150% or 200%.
Set the devPixelsPerPx pref to -1.0.
Open the main menu or a context menu.


Actual results:

Items with icons are positioned further right than items without icons. Additionally, on native-style menus (like context menus), icons overflow the little groove on the menu.


Expected results:

All menu items should be aligned and icons should not overflow UI chrome.
Blocks: win-hidpi
Component: Untriaged → Theme
Hardware: x86_64 → All
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130218 Firefox/20.0

I can confirm this issue on Firefox 19.0 RC (buildID: 20130215130331), latest Nightly (buildID: 20130219031055) and latest Aurora (buildID: 20130218042018), by setting the DPI to 200% and devPixelsPerPx from about:config to -1.0, as stated in comment 0.
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: bogdan.maris
Currently, it looks like there's some CSS-px vs device-pixel confusion going on: we load an icon that is 16x16 pixels, which is its intended CSS-pixel size, but which equates to 20, 24 or 32 device pixels (at 125%, 150% or 200% scale). But then we render the image at that size in *CSS* pixels, which means that it becomes increasingly oversized (and blurry, due to upscaling) as the resolution increases.

So if we could constrain the icons in the menuitems to be displayed at 16x16 CSS pixels, that would help here. I'm not sure how to do that, though, as they're being rendered via list-style-image, which AFAIK doesn't offer any way to control the image size.

Ideally, we'd also have larger versions of the images, corresponding to higher resolutions; but we'd still need to constrain their size to 16x16 CSS pixels, otherwise that would just make this problem even worse.
From what I can tell, the icons are always 16x16 CSS pixels. The problem is that the groove's position is based on device pixels, and menu items without icons are aligned to the groove.

Additionally, the groove width doesn't scale proportionately to DPI in Windows, and we're borrowing the values Windows gives us. It's a design oversight on Microsoft's part.

I think the correct approach is to fix it in two places:
1. Change the widget so that the groove width is always at least 16 CSS pixels.
2. Change the menu style so that iconless menu items are still indented 16 CSS pixels.

The second part is optional but would make the style more stable and resilient to similar bugs in the future.
Hmm, on looking more carefully, I think you're right - it's not that the icons are too big, but the item indents (and groove position) are not scaled properly.

And I see that we already have at least some of the menu icons designed at larger sizes, so we can substitute them and get better-looking results. I've confirmed that swapping in a larger icon does -not- result in it being drawn bigger; it scales to the same size, so we get a sharper image, as desired.
This is getting a bit messy. AFAICT, the root of the problem is that we rely on the metrics of the checkbox (or radio-button), as seen in the Options sub-menu, for example, to determine the width of the "margin" area of the menu.

However, as DPI increases, Windows does not scale the checkbox linearly; it increases slightly in size from 100% to 125%, but then at 150% and even 200%, it uses the same-size checkbox. So on a 192dpi screen, the checkbox is smaller in relation to the text size than it was on a 96dpi screen.

On the other hand, when menu items have -icons- (rather than checkboxes) in that position, the icons -do- scale linearly with DPI, so that they stay the same size relative to the text. And that's why they overflow the available space in our menus, which was based on the checkbox.

It looks like ideally, at high dpi, the menu margin would be larger for a menu with icons than it is for a menu that only has (potential) checkboxes. This is what I see in the Desktop right-click menu (which has icons for Screen Resolution, Gadgets, Personalize) and its View and Sort By submenus (no icons, only checkboxes).

It doesn't look like our current code can readily support this, however (nor is it clear what would happen if an icon were added dynamically to a menu that previously didn't have one, nor exactly how a checkbox would appear on an item in a menu that -also- has icons for some items). I think that as a first step, at least, we need to simply ensure that the margin is large enough for icons as they'll appear at the current DPI; this will mean that on high-dpi displays, checkboxes will get a bit more margin than they really need, but that's much better than the current situation.
WPF apps scale in a much more sane way. Scaling the margin proportionately in all cases is what they do, and I think we would be much better off doing the same.

(I think it would be very cool if we could port the entire Windows Desktop widget over to WPF but that sounds like a very big job.)
This ensures the margin in the menus is wide enough for icons, not only for checkboxes. With this, normal menus in the menubar and popup menus all seem to work OK at various resolutions.

The problem that remains is with the special Firefox application menu (which is used if the main menubar is turned off). The menu items align properly, but the dividing line in the submenus is still misplaced. This is because it is produced separately by the CSS in browser/themes/windows/browser.css, which renders the line using custom CSS gradients(!) The following patch will fix that.
Assignee: nobody → jfkthame
Attachment #730911 - Flags: review?(jmathies)
This adjusts the position of the line in submenus of the Firefox app menu to account for the resolution-dependent scaling. Tested and tuned for 125%, 150% and 200% scale factors; it may be slightly less than perfectly-aligned at other (non-standard) scale factors.

(Using such a CSS gradient to produce this dividing line seems an inherently problematic and fragile approach, but I didn't want to try completely re-implementing it here; that's left as a future exercise.)
Attachment #730913 - Flags: review?(jmathies)
Extra bonus patch :) ... I found that we've got larger versions of the Privacy and Add-ons icons already in the tree, so this patch plugs those in for >100% scale factors. Mostly just to test that we can do this successfully, but it also makes those two icons look -much- better on hidpi systems.

We'll still need larger versions of lots of other icons...
Attachment #730916 - Flags: review?(jmathies)
Tryserver build with these patches for testing:
https://hg.mozilla.org/try/rev/75fbea6d2dc8
Uhh, sorry... a more useful link:
https://tbpl.mozilla.org/?tree=Try&rev=75fbea6d2dc8
Comment on attachment 730913 [details] [diff] [review]
part 2 - adjust position of the gutter dividing line in the appmenu

>     background-image: linear-gradient(to left, #f1f5fb 26px, ThreeDLightShadow 26px,
>                                       ThreeDLightShadow 27px, ThreeDHighlight 27px,
>                                       ThreeDHighlight 28px, #f1f5fb 28px);

How about replacing that with:

>     background-image: linear-gradient(to left, ThreeDLightShadow 0,
>                                       ThreeDLightShadow 1px, ThreeDHighlight 1px,
>                                       ThreeDHighlight 2px, #f1f5fb 2px);
>     background-position: 26px 0;
Well, that's how the code is currently written; I didn't aim to change anything about the design or implementation approach, just tweak it for the resolution changes.

As for that change - I can't quite decide whether I like it or not. It slightly simplifies the gradient specification, but OTOH it seems like a trickier approach; it relies on the repeating behavior of the background to effectively "wrap around" and fill in the background color -before- the dividing line, as I understand it, but that's a somewhat non-obvious "trick".

And we'd still need the separate specifications of the gradient for each resolution, so it doesn't really simplify anything overall.
(In reply to Greg Edwards from comment #0)
> Set Windows's DPI scale to something high like 150% or 200%.
> Set the devPixelsPerPx pref to -1.0.
> Open the main menu or a context menu.
> 
> Actual results:
> 
> Items with icons are positioned further right than items without icons.
> Additionally, on native-style menus (like context menus), icons overflow the
> little groove on the menu.

I can't reproduce this on a Surface Pro running Nightly. AFAICT I have the settings right, is there something I'm missing here?
Attached image blurry icons
I see blurry icons, but I don't see any positioning problems.
What's the default resolution scaling there? If it's only 125% (i.e. 120dpi), you might need to increase it to at least 150% or even 200% for the positioning problems to show up.

How about the dividing line in the Bookmarks or History submenus (not shown in your screenshot)? I suspect it'll be misplaced, even at 125%, though the problem becomes more marked as the resolution increases.
Actually, your screenshot -does- show a very, very slight positioning problem - but it's only a fraction of a pixel at this scale, so it's easy to overlook. If you zoom in and compare the rendering of the "N" in "New Private Window" vs "New Tab", you can see that the one with the icon is shifted a fraction of a pixel to the right, leading to different subpixel antialiasing, although they would be expected to render identically.
Comment on attachment 730913 [details] [diff] [review]
part 2 - adjust position of the gutter dividing line in the appmenu

I'm not qualified to review css related changes in theme code. Moving this over to dao.
Attachment #730913 - Flags: review?(jmathies) → review?(dao)
Attachment #730916 - Flags: review?(jmathies) → review?(dao)
Comment on attachment 730911 [details] [diff] [review]
part 1 - ensure the menu gutter is wide enough for icons, not just for checkboxes

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

This code looks fine. I haven't messed with GetGutterSize so I'm not 100% on how this value is used and how layout handles rendering these items. Since this is a simple theme change lets land it and see what the side effect are. We can always back it out.
Attachment #730911 - Flags: review?(jmathies) → review+
Here's a comparison of the app menu and submenu as rendered by current Nightly (left) and the tryserver build (right), at the four "common" resolution settings of 96, 120, 144 and 192 dpi.

Problems in the Nightly version:
- misalignment depending on presence of icon (visible at 144dpi, glaring at 192)
- mispositioned gutter dividing rule in the submenu (increasingly bad as resolution increases)
- also, the rule becomes fatter, although the horizontal item serparators don't
- blurry icons

The try build fixes all these (only the Privacy and Addons icons are fixed, as those are the assets I found ready-made).
(In reply to Jonathan Kew (:jfkthame) from comment #14)
> As for that change - I can't quite decide whether I like it or not. It
> slightly simplifies the gradient specification, but OTOH it seems like a
> trickier approach; it relies on the repeating behavior of the background to
> effectively "wrap around" and fill in the background color -before- the
> dividing line, as I understand it, but that's a somewhat non-obvious "trick".

You could use background-repeat:repeat-y and set a background color.

> And we'd still need the separate specifications of the gradient for each
> resolution, so it doesn't really simplify anything overall.

Why? Are pixel values for background-position not DPI aware?
(In reply to Jonathan Kew (:jfkthame) from comment #21)
> Created attachment 731133 [details]
> comparison of app menu rendering at different resolutions, Nightly vs Try
> build

The right side looks great!
(In reply to Dão Gottwald [:dao] from comment #22)
> (In reply to Jonathan Kew (:jfkthame) from comment #14)
> > As for that change - I can't quite decide whether I like it or not. It
> > slightly simplifies the gradient specification, but OTOH it seems like a
> > trickier approach; it relies on the repeating behavior of the background to
> > effectively "wrap around" and fill in the background color -before- the
> > dividing line, as I understand it, but that's a somewhat non-obvious "trick".
> 
> You could use background-repeat:repeat-y and set a background color.
> 
> > And we'd still need the separate specifications of the gradient for each
> > resolution, so it doesn't really simplify anything overall.
> 
> Why? Are pixel values for background-position not DPI aware?

They are, but there are two issues that mean we still need separate per-resolution properties:

(a) The width of the menu margin does not scale linearly with dpi - at low dpi, the (potential) checkbox enforces a minimum width, and then as dpi increases (but the checkbox does not scale in proportion), the (potential) icon scales more than the checkbox and takes over as the determining factor; moreover, the margins of the checkbox or icon do not scale along with its size.

(b) The desired width of the rule (expressed in CSS px, as part of the gradient spec) needs to become -smaller- as the dpi increases, so that it will remain a -fixed- thickness in device pixels (which is what the standard menu separators do, as seen in content popup menus, for example).

These two issues are the reasons why the vertical dividing rule in the Bookmarks submenu (a) becomes increasingly misplaced, even in relation to the menu items with icons (see the left-hand images in attachment 731133 [details]), and (b) becomes fatter (see especially 192dpi) and blurry (see 144dpi), instead of remaining crisp like the horizontal dividers.
(In reply to Jonathan Kew (:jfkthame) from comment #24)
> (a) The width of the menu margin does not scale linearly with dpi - at low
> dpi, the (potential) checkbox enforces a minimum width, and then as dpi
> increases (but the checkbox does not scale in proportion), the (potential)
> icon scales more than the checkbox and takes over as the determining factor;
> moreover, the margins of the checkbox or icon do not scale along with its
> size.

And just to clarify, these are based on metrics we're getting from the Windows theme code. If we implemented completely proportional scaling, as per WPF (see comment 7), some of this would become simpler - but that would be a major overhaul that I don't think we should attempt here in this bug, if at all.
Pushed pt1 (nsNativeThemeWin code change) to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40cde6c631d3

Please leave the bug open when merging to m-c, as the CSS patches are still awaiting review.
Whiteboard: [leave open]
Dão, ping re the CSS changes here - it would be good to get them landed to go along with the gutter-width fix so that hidpi users get a tidier app menu.
Attachment #730916 - Flags: review?(dao) → review+
Comment on attachment 730913 [details] [diff] [review]
part 2 - adjust position of the gutter dividing line in the appmenu

This is nasty, but luckily this code won't exist for much longer...
Attachment #730913 - Flags: review?(dao) → review+
Nasty indeed, but at least it works for now. I'll be glad if it disappears!

https://hg.mozilla.org/integration/mozilla-inbound/rev/70176237e018
https://hg.mozilla.org/integration/mozilla-inbound/rev/9da2aa52affc
Whiteboard: [leave open]
Target Milestone: --- → Firefox 23
Comment on attachment 730913 [details] [diff] [review]
part 2 - adjust position of the gutter dividing line in the appmenu

Note that the code fix in part 1 here is already on aurora-22, but the CSS patches were not reviewed in time for the merge.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 844604 (Windows HiDPI support)

User impact if declined: ugly Firefox application menu on HiDPI Windows

Testing completed (on m-c, etc.): tested locally with various dpi scale factors; landed on inbound

Risk to taking this patch (and alternatives if risky): minimal risk, just adjusting CSS for HiDPI menu styling

String or IDL/UUID changes made by this patch: none
Attachment #730913 - Flags: approval-mozilla-aurora?
Comment on attachment 730916 [details] [diff] [review]
part 3 - use higher-res icons when available

[Approval Request Comment]

As above.
Attachment #730916 - Flags: approval-mozilla-aurora?
We'll wait to approve till this has a day on Nightly.
Attachment #730913 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #730916 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Not quite fixed. On 192DPI, menu items that expand have thick borders, but menu items that don't have thin borders.
(My preference is for the thick borders ^^)
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Also, checks and similar decorations are left justified in the margin space but would look better if centred.

(Windows does left justify them but I disagree from an aesthetic perspective, especially when mixed with full-sized icons. Right click the Back button for a good example of how bad this looks.)

(Sorry for the double post.)
Attached image Back button misalgnment
Shows misalignment between bullet decoration and icons. Check decoration behaves in the same way.
I just noticed in my screenshot that expanding menu items also have more vertical height.
Greg: thanks for the testing. Can you file that as a new bug, and mark it as "blocking" this one? It's much better to track variants in new bugs rather than re-opening old ones.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Also, note that even in FF19/96dpi, I see a difference between "expanding" and normal menu items in the app menu. They are styled differently (presumably by design?), including having different heights. So that's not specific to hi-dpi, nor related to the fix here.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: