Menu arrows are very large in Windows 8.1 with DPI above 100%

VERIFIED FIXED in Firefox 31

Status

()

Core
Widget: Win32
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: streetwolf, Assigned: jimm)

Tracking

(Blocks: 1 bug)

24 Branch
mozilla32
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 wontfix, firefox28 wontfix, firefox29- wontfix, firefox30 affected, firefox31 verified, firefox32 verified)

Details

Attachments

(6 attachments, 7 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 798111 [details]
Large arrows on Context Menus.

Running Windows 8.1 Pro x64 RTM.  I would understand if you don't want to touch this yet.

This problem does not exist in IE11 or any other application so far.
(Reporter)

Comment 1

5 years ago
I run at 120 DPI.  When I ran at 96 DPI the arrows looked fine.  It seems to be another HiDPI issue.

Updated

5 years ago
Blocks: 844604
Component: Menus → Themes
Product: Firefox → Toolkit
Summary: Context menu arrows are very large in Windows 8.1 → Menu arrows are very large in Windows 8.1

Comment 2

5 years ago
I can also reproduce this at 150% DPI.

Comment 3

5 years ago
Two other bugs on this have also been filed:
Bug 916480
Bug 928720

Updated

5 years ago
Summary: Menu arrows are very large in Windows 8.1 → Menu arrows are very large in Windows 8.1 with DPI above 100%

Updated

5 years ago
Duplicate of this bug: 928720

Updated

5 years ago
Duplicate of this bug: 916480

Comment 6

5 years ago
(In reply to Josh Tumath from comment #2)
> I can also reproduce this at 150% DPI.

It even shows up at the relatively low increase of 110%.

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 26 Branch → 24 Branch

Updated

5 years ago
OS: Windows 8 → Windows 8.1

Comment 7

5 years ago
So who is gonna work on it now? it's just a little job :-)

Comment 8

5 years ago
I have the same thing at DPI 125%. When I set it to 125%, everything else and every other browser (being Google Chrome and IE) works fine, all text is nice, no fuzzyness, but mozilla menu buttons are huge and out of texture, as can be seen in the provided screenshot:
http://abload.de/img/lollusgksnd.png

At least I am not the only one with it, yet it can become a bit too "dirty" to look at. 

PS. DPI 100% just won't cut it on a 1920x180 display.

Comment 9

5 years ago
Is someone aware of that at Mozilla? Has it been identified as a Firefox or Win 8.1 issue?

Comment 10

5 years ago
I just updated from Windows 8.1 Preview to Windows 8.1 and have these unsightly arrow icons in both Mozilla Firefox 25.0 Beta 10 and Mozilla Thunderbird 24.0.1 at 125% DPI. Both were cleanly installed after the 8.1 upgrade wiped all of my applications. I didn't see this issue in the Developer Preview with the same versions of these Mozilla applications installed.

Installing the latest AMD graphics drivers and Samsung monitors drivers didn't help.

Updated

5 years ago
Duplicate of this bug: 933096
See Also: → bug 934389

Comment 12

5 years ago
Why anyone try to fix that display issue?
Requesting tracking-firefox29 because this happens even with default display settings (Windows 8.1 Pro, Firefox 26.0 and 29.0a1) and looks pretty ugly.
tracking-firefox29: --- → ?
Created attachment 8350215 [details] [diff] [review]
WIP

This patch makes most of the arrows slightly smaller, but they are still slightly too big, especially their width, and some (such as arrows next to bookmark folders) are still way too big.
Component: Themes → Widget: Win32
Product: Toolkit → Core
Created attachment 8350219 [details]
before/after screenshots with WIP patch
Created attachment 8350228 [details]
WIP 2

This is closer to the correct size, although it still looks badly scaled and I still have one item with a giant arrow (as in the previous screenshot).  I don't really know what I'm doing; could someone who understands the native theme code take a look at this, please?
Attachment #8350215 - Attachment is obsolete: true
Attachment #8350228 - Flags: feedback?(jmathies)

Comment 17

5 years ago
Just got a new laptop with windows 8.1 and I also have this problem. Running a 14 inch FHD display is not an option...

Any scaling other than 100% makes the menu triangles all pixelated and it could be my impression but the toolbar buttons seem slightly blurred.

Comment 18

5 years ago
Created attachment 8351397 [details]
Bookmarks toolbar favicon vs Tab vaficon

I concur on the bookmarks toolbar favicons looking blurry: this is especially interesting as the corresponding tab favicons are sometimes sharper. Here is a screen grab of the Google favicon in the Bookmarks toolbar on top and on a tab at the bottom.
(Please let me know if this should be split into a separate bug)
Comment on attachment 8350228 [details]
WIP 2

You attached a wrong file.
Attachment #8350228 - Attachment is patch: false
Attachment #8350228 - Attachment mime type: text/plain → image/png
Created attachment 8351498 [details] [diff] [review]
WIP 2

Oops!  Here's the correct file.
Attachment #8350228 - Attachment is obsolete: true
Attachment #8350228 - Flags: feedback?(jmathies)
Attachment #8351498 - Flags: feedback?(jmathies)
(Assignee)

Comment 21

5 years ago
Comment on attachment 8351498 [details] [diff] [review]
WIP 2

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

If generally it looks right I'm fine with this change. This code has a lot of tweaks like this in it. Any idea why the folders still display wrong?

::: widget/windows/nsNativeThemeWin.cpp
@@ -2289,5 @@
>        return NS_OK;
>  
>      case NS_THEME_MENUARROW:
> -      aResult->width = 26;
> -      aResult->height = 16;

This is definitely bad!

@@ +2292,5 @@
> +    {
> +      *aIsOverridable = false;
> +      double scaleFactor = nsIWidget::DefaultScaleOverride();
> +      if (scaleFactor <= 0.0) {
> +        scaleFactor = gfxWindowsPlatform::GetPlatform()->GetDPIScale();

You can grab this value from WinUtils now.
Attachment #8351498 - Flags: feedback?(jmathies) → feedback+
Should I request review on this patch, or do you want us to investigate the bad scaling further before landing anything?
Flags: needinfo?(jmathies)
tracking-firefox29: ? → +
(Assignee)

Comment 23

5 years ago
(In reply to Matt Brubeck (:mbrubeck) from comment #22)
> Should I request review on this patch, or do you want us to investigate the
> bad scaling further before landing anything?

I'm ok with that I suppose. File a follow up on the folder issue I guess?
Flags: needinfo?(jmathies)
Created attachment 8368188 [details] [diff] [review]
patch
Assignee: nobody → mbrubeck
Attachment #8351498 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8368188 - Flags: review?(jmathies)
(Assignee)

Updated

4 years ago
Attachment #8368188 - Flags: review?(jmathies) → review+
Blocks: 966017
Created attachment 8368277 [details]
before/after screenshots with latest patch

I was just giving this some final testing when I noticed that it altered the padding or positioning of the arrows, even when running with 100% scale (except, again, for the arrow next to a bookmark folder).  We should probably fix this before landing this patch.
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
status-firefox30: --- → affected

Comment 26

4 years ago
(In reply to Matt Brubeck (:mbrubeck) from comment #25)
> Created attachment 8368277 [details]
> before/after screenshots with latest patch
> 
> I was just giving this some final testing when I noticed that it altered the
> padding or positioning of the arrows, even when running with 100% scale
> (except, again, for the arrow next to a bookmark folder).  We should
> probably fix this before landing this patch.

Matt, do you think you'll have time to look at this this week? It'd be nice if we got this kind of stuff for Australis's release as well. :-)

Comment 27

4 years ago
(comment 26)
Flags: needinfo?(mbrubeck)
(In reply to :Gijs Kruitbosch from comment #26)
> Matt, do you think you'll have time to look at this this week? It'd be nice
> if we got this kind of stuff for Australis's release as well. :-)

I can spend some time looking at this this week, but as I said above, this really needs someone who understands this code better.  I'm still at the "try random things and see what happens" stage, so I'm not sure I'm on the path to a solution.
Flags: needinfo?(mbrubeck)
(Assignee)

Comment 29

4 years ago
I can take this now.
Assignee: mbrubeck → jmathies
(Assignee)

Updated

4 years ago
Attachment #8368188 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8351397 - Attachment is obsolete: true
(Assignee)

Comment 30

4 years ago
I don't understand why this doesn't "just work" on high-dpi systems. For 96 dpi, we get a glyph size of 9x9, and the typical widget rect we render to is 25 x 23. In this scenario, we pass the widget rect to DrawThemeBackground and it renders the arrow correctly. It's centered both vertically and horizontally, and has the proper edge padding so the arrow doesn't press up againt the menu border. The arrow is rendered at 9x9. 

On higher dpi systems, we get completely different rendering behavior. At 150%, glyph size is 14 x 13, which is expected. Our widget rect is around 33 x 32. But when we ask the theme library to render to the widget rect we get an arrow that fills the rect and is stretched.

I need to put together a small test app to isolate this from firefox, see if we're messing things up or if the problem is in the theme lib.
(Assignee)

Comment 31

4 years ago
This is definitely our fault. In a test app everything renders properly. My guess is this has something to do with the hdc we hand into the DrawThemeBackground routine.

Updated

4 years ago
Duplicate of this bug: 985713
(Assignee)

Comment 33

4 years ago
(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #31)
> This is definitely our fault. In a test app everything renders properly. My
> guess is this has something to do with the hdc we hand into the
> DrawThemeBackground routine.

Interesting, the test app I was using didn't have dpi awareness set. Once I set that, I got the same scaled bitmap.  The theme library doesn't do any adjusting or fitting in this case.
(Assignee)

Comment 34

4 years ago
Created attachment 8394218 [details] [diff] [review]
fix v.1
(Assignee)

Updated

4 years ago
Duplicate of this bug: 966017
(Assignee)

Updated

4 years ago
Blocks: 820679
(Assignee)

Updated

4 years ago
Attachment #8394218 - Flags: review?(tabraldes)
Comment on attachment 8394218 [details] [diff] [review]
fix v.1

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

I don't have a hidpi device to test these changes with, but r+ as long as:
  1. This has been tested on hidpi and non-hidpi devices to ensure that things look right
  2. The review comments are addressed (I don't care as much about the nits, but the 2 non-nits should be addressed)

::: widget/windows/nsNativeThemeWin.cpp
@@ +1811,5 @@
> +    // Firefox some menu items provide the full height of the item to us, in
> +    // others our widget rect is the exact dims of our arrow glyph. So check
> +    // for the right width here and adjust vertical positioning if needed.
> +    if (widgetWidth > glyphSize.cx) {
> +      centeredRect.top +=

Here we're checking the relative widths of our widget and glyph, then adjusting the vertical spacing. Is that intentional, or should we be checking the relative heights instead?

@@ +1812,5 @@
> +    // others our widget rect is the exact dims of our arrow glyph. So check
> +    // for the right width here and adjust vertical positioning if needed.
> +    if (widgetWidth > glyphSize.cx) {
> +      centeredRect.top +=
> +        static_cast<uint32_t>(floor((widgetHeight - glyphSize.cy) * .5));

nit: Could this be simplified to:
  (widgetHeight - glyphSize.cy) / 2;

@@ +1819,5 @@
> +    // I'm using the width of the arrow glyph for the right-side padding.
> +    // AFAICT there doesn't appear to be a theme constant we can query
> +    // for this value.
> +    centeredRect.left = widgetRect.right - (glyphSize.cx * 2);
> +    centeredRect.right = widgetRect.right - glyphSize.cx;

nit: This could be slightly more readable as:
  centeredRect.right = widgetRect.right - glyphSize.cx;
  centeredRect.left = centeredRect.right - glyphSize.cx;

Also, the name `centeredRect` is misleading: We're not creating a centered rect, we're creating a "right-padded but vertically centered" rect. Maybe call it `resultRect` or something similar.

@@ +2450,5 @@
> +    case NS_THEME_MENUARROW:
> +    {
> +      // Use the width of the arrow glyph as padding. See the drawing
> +      // code for details.
> +      aResult->width *= 2;

The previous code also set aResult->height, should we still be doing that?
Attachment #8394218 - Flags: review?(tabraldes) → review+
(Assignee)

Comment 38

4 years ago
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #37)
> Comment on attachment 8394218 [details] [diff] [review]
> fix v.1
> 
> Review of attachment 8394218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have a hidpi device to test these changes with, but r+ as long as:
>   1. This has been tested on hidpi and non-hidpi devices to ensure that
> things look right

Yes, it's been testing on a surface pro.

> 
> ::: widget/windows/nsNativeThemeWin.cpp
> @@ +1811,5 @@
> > +    // Firefox some menu items provide the full height of the item to us, in
> > +    // others our widget rect is the exact dims of our arrow glyph. So check
> > +    // for the right width here and adjust vertical positioning if needed.
> > +    if (widgetWidth > glyphSize.cx) {
> > +      centeredRect.top +=
> 
> Here we're checking the relative widths of our widget and glyph, then
> adjusting the vertical spacing. Is that intentional, or should we be
> checking the relative heights instead?

Yes you're right, that should be the height. Actually I don't think the check is needed now that I look at it since if widgetHeight  and glyphSize.cy are equal we'll just += 0.

> > +    case NS_THEME_MENUARROW:
> > +    {
> > +      // Use the width of the arrow glyph as padding. See the drawing
> > +      // code for details.
> > +      aResult->width *= 2;
> 
> The previous code also set aResult->height, should we still be doing that?

This code is in a different place. The original hardcoded width/height and then returned. Now we query the theme library for width/height, then after that increase the widget by a factor of 2 for the padding.
(Assignee)

Comment 39

4 years ago
Created attachment 8395037 [details] [diff] [review]
fix v.2
Attachment #8394218 - Attachment is obsolete: true
Attachment #8395037 - Flags: review?(jmathies)
(Assignee)

Updated

4 years ago
Attachment #8395037 - Flags: review?(jmathies) → review+
(Assignee)

Comment 40

4 years ago
status update here - this patch causes some positioning problems in our rtl widget rendering routine. I'm still trying to track down what's going on. Part of the problem involves a negative rect origin for certain widgets, which may be a different bug. Once I sort that out I'll land this.
Tim / Brian, since this is tracking 29 can one of you two take this?
Flags: needinfo?(tabraldes)
Flags: needinfo?(netzen)
with Jim away on PTO that is
Tomorrow I'll be fixing this m-bc perm fail on beta:
bug 990261


But after that I can take a look.  Tim feel free to work on it in the meantime if you aren't working on something else.
Flags: needinfo?(netzen)
I'm not sure exactly what's wrong with the patch.  

I tried with the force RTL extension and everything seems to work correctly.

I also tried pushing to try, but the tests all pass:
https://tbpl.mozilla.org/?tree=Try&rev=2cec066240b9

I guess some negative values are being passed to " rtl widget rendering routine" I'm not sure if that means RTL code in sNativeThemeWin.cpp or /nsWindow.cpp.

Unfortunately I haven't done any RTL related fixes before so I don't know it well.
(Assignee)

Comment 45

4 years ago
If you apply this and look at an rtl build, you'll notice arrows aren't left side padded. The rtl rendering function isn't designed to handle inner rects (it uses the raw bitmap dims in its calculations) which throws things off. 

If no one can look into it lets untrack on this for 29. I'll finish it up when I get back.
FYI, we are to build Firefox 29 beta6 today. If ready, we can accept it in beta 7 (thursday) or beta 8 (in a week).
Otherwise, yeh, we will have to untrack it.
This has been around for a while now and isn't ready in time for FF29, we can look at an uplift nomination when the patch is ready but there's no reason to keep tracking this as it's obviously not impacting a lot of users negatively.
tracking-firefox29: + → -
Didn't get to this one in time :/

Clearing needinfo
Flags: needinfo?(tabraldes)
(Assignee)

Comment 49

4 years ago
Created attachment 8411980 [details] [diff] [review]
fix v.3

Ok with a little pre-tweaking of the render rect I was able to get DrawThemeBGRTLAware to act the way I want. More generally I think DrawThemeBGRTLAware is kind of broken, we've been moving away from using it for quite some time. There are currently three calls to it, all of which could handle their own rendering of rtl glyphs without a lot of effort. Those changes are outside the scope of this bug so I'm not messing with it here.
Attachment #8395037 - Attachment is obsolete: true
Attachment #8411980 - Flags: review?(tabraldes)
(Assignee)

Updated

4 years ago
Assignee: jmathies → nobody
Status: ASSIGNED → NEW

Comment 50

4 years ago
Can also confirm this definetly happens on Firefox 28.0
(Reporter)

Comment 51

4 years ago
I really can't believe you let this one go for so long.  It is a show stopper IMO.  I've seen lesser problems solved in a day.

Comment 52

4 years ago
Indeed @streetwolf there are quite a few articles about this bug that I'm surprised no one has picked up on it before.

See: 
http://techreport.com/blog/25797/high-ppi-support-in-windows-8-1-still-not-so-great
http://www.hanselman.com/blog/LivingAHighDPIDesktopLifestyleCanBePainful.aspx

Firefox otherwise has otherwise very good High-DPI support, far superior to that of Chrome and this is the only small thing that let's it down and makes it look un-polished really :(
Hey guys, sorry we weren't able to fix this sooner.  We hit a number of technical snags (as you can see above).  But a fix is awaiting review now and should land soon.  For now, please remember https://bugzilla.mozilla.org/page.cgi?id=etiquette.html and try not to add further comments unless they are adding information needed to help get the fix landed.  Thanks!

Comment 54

4 years ago
Is this fix being applied to Thunderbird as well?
(In reply to johnsc301 from comment #54)
> Is this fix being applied to Thunderbird as well?

Yes, the patch fixes code that's shared by both Thunderbird and Firefox.
(Assignee)

Comment 56

4 years ago
Yep, this is almost there, and we can uplift once it's baked for a week on mc. Tim's back from PTO today for the final review as well.
Comment on attachment 8411980 [details] [diff] [review]
fix v.3

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

I still can't test this but it looks good as long as glyphs are always smaller than our widget. Also please clean up the `renderRect.top` calculation.

::: widget/windows/nsNativeThemeWin.cpp
@@ +1794,5 @@
> +    // Firefox some menu items provide the full height of the item to us, in
> +    // others our widget rect is the exact dims of our arrow glyph. Adjust the
> +    // vertical position by the added space, if any exists.
> +    renderRect.top +=
> +      static_cast<uint32_t>(floor((widgetHeight - glyphSize.cy) / 2.0));

`widgetHeight` is an `int32_t`, `glyphSize.cy` is a `LONG` [1] which is basically also a `int32_t` [2], and `renderRect.top` is also a `LONG` [3]. I believe this line could be rewritten as:
  (widgetHeight - glyphSize.cy) / 2);

i.e. Remove the call to `floor`, divide by `2` instead of `2.0`, and remove the cast to `uint32_t`.

Can the glyph ever be bigger than the widget? If so, this code might run into issues.

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/dd145106%28v=vs.85%29.aspx
[2] http://msdn.microsoft.com/en-us/library/cc230343.aspx
[3] http://msdn.microsoft.com/en-us/library/windows/desktop/dd162897%28v=vs.85%29.aspx
Attachment #8411980 - Flags: review?(tabraldes) → review+
(Assignee)

Comment 58

4 years ago
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #57)
> Comment on attachment 8411980 [details] [diff] [review]
> fix v.3
> 
> Review of attachment 8411980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still can't test this but it looks good as long as glyphs are always
> smaller than our widget. Also please clean up the `renderRect.top`
> calculation.
> 
> ::: widget/windows/nsNativeThemeWin.cpp
> @@ +1794,5 @@
> > +    // Firefox some menu items provide the full height of the item to us, in
> > +    // others our widget rect is the exact dims of our arrow glyph. Adjust the
> > +    // vertical position by the added space, if any exists.
> > +    renderRect.top +=
> > +      static_cast<uint32_t>(floor((widgetHeight - glyphSize.cy) / 2.0));
> 
> `widgetHeight` is an `int32_t`, `glyphSize.cy` is a `LONG` [1] which is
> basically also a `int32_t` [2], and `renderRect.top` is also a `LONG` [3]. I
> believe this line could be rewritten as:
>   (widgetHeight - glyphSize.cy) / 2);
> 
> i.e. Remove the call to `floor`, divide by `2` instead of `2.0`, and remove
> the cast to `uint32_t`.

updated per comments.

> Can the glyph ever be bigger than the widget? If so, this code might run
> into issues.

That would be unexpected since we request a widget size that fits the glyph in GetMinimumWidgetSize.
(Assignee)

Comment 60

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/a31f762698bb
Target Milestone: --- → mozilla32
(Assignee)

Updated

4 years ago
Assignee: nobody → jmathies
https://hg.mozilla.org/mozilla-central/rev/a31f762698bb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 62

4 years ago
Comment on attachment 8411980 [details] [diff] [review]
fix v.3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not a regression, caused by lack of good high dpi support on Windows.
User impact if declined: cosmetic anomaly
Testing completed (on m-c, etc.): yes, couple days
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Lets up this up to aurora to shorten the path to release by six weeks.
Attachment #8411980 - Flags: approval-mozilla-aurora?
status-firefox27: affected → wontfix
status-firefox28: affected → wontfix
status-firefox29: affected → wontfix
status-firefox31: --- → affected
status-firefox32: --- → fixed
Comment on attachment 8411980 [details] [diff] [review]
fix v.3

Any reason why you don't ask for an uplift to beta?
Attachment #8411980 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 64

4 years ago
I agree. If it's just a cosmetic anomaly, why not push it to Beta?
(Assignee)

Comment 65

4 years ago
I suppose I could. On aurora rtl localizers will have a chance to see the change. Since there are two code paths there and I can only test using the force rtl addin I'm curious if any of our testers spot any problems. If nothing comes up I'll request beta approval.
(Assignee)

Updated

4 years ago
Keywords: verifyme
Reproduced in nightly 2014-05-04.
Verified fixed 32.0a1 (2014-05-20), Win 8.1 x64
Status: RESOLVED → VERIFIED
status-firefox32: fixed → verified
Verified fixed FF 31b2, Win 8.1 x64.
status-firefox31: fixed → verified
Keywords: verifyme
Created attachment 8443445 [details]
bookmark toolbar button arrows at 150% dpi

As a remaining issue, I see the bookmark toolbar button arrows are blurry. Say if you think this should be filed separately. This is not a regression of this bug.
(Assignee)

Comment 70

4 years ago
(In reply to Paul Silaghi, QA [:pauly] from comment #69)
> Created attachment 8443445 [details]
> bookmark toolbar button arrows at 150% dpi
> 
> As a remaining issue, I see the bookmark toolbar button arrows are blurry.
> Say if you think this should be filed separately. This is not a regression
> of this bug.

Thanks! This is covered by bug 878288.

Comment 71

4 years ago
Created attachment 8450161 [details]
firefox30_menu_arrows

the arrows of the menu are still very large.
They are fixed only in Firefox 31 and above. Until 31 is released in couple of weeks, you can use Firefox 31 beta http://www.mozilla.org/en-US/firefox/channel/#beta

Comment 73

4 years ago
Thank you.

Comment 74

4 years ago
Thank you! The new version works great! :)
You need to log in before you can comment on or make changes to this bug.