Closed Bug 911409 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: Widget: Win32, defect)

24 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 - wontfix
firefox30 --- affected
firefox31 --- verified
firefox32 --- verified

People

(Reporter: streetwolf, Assigned: jimm)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 7 obsolete files)

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.
I run at 120 DPI.  When I ran at 96 DPI the arrows looked fine.  It seems to be another HiDPI issue.
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
I can also reproduce this at 150% DPI.
Two other bugs on this have also been filed:
Bug 916480
Bug 928720
Summary: Menu arrows are very large in Windows 8.1 → Menu arrows are very large in Windows 8.1 with DPI above 100%
Duplicate of this bug: 928720
Duplicate of this bug: 916480
(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%.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 26 Branch → 24 Branch
So who is gonna work on it now? it's just a little job :-)
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.
Is someone aware of that at Mozilla? Has it been identified as a Firefox or Win 8.1 issue?
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.
Duplicate of this bug: 933096
See Also: → 934389
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.
Attached patch WIP (obsolete) — Splinter Review
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
Attached image WIP 2 (obsolete) —
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)
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.
Attached image Bookmarks toolbar favicon vs Tab vaficon (obsolete) —
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
Attached patch WIP 2 (obsolete) — Splinter Review
Oops!  Here's the correct file.
Attachment #8350228 - Attachment is obsolete: true
Attachment #8350228 - Flags: feedback?(jmathies)
Attachment #8351498 - Flags: feedback?(jmathies)
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)
(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)
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → mbrubeck
Attachment #8351498 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8368188 - Flags: review?(jmathies)
Attachment #8368188 - Flags: review?(jmathies) → review+
Blocks: 966017
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.
(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 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)
I can take this now.
Assignee: mbrubeck → jmathies
Attachment #8368188 - Attachment is obsolete: true
Attachment #8351397 - Attachment is obsolete: true
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.
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.
Duplicate of this bug: 985713
(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.
Attached patch fix v.1 (obsolete) — Splinter Review
Duplicate of this bug: 966017
Blocks: win-hidpi
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+
(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.
Attached patch fix v.2 (obsolete) — Splinter Review
Attachment #8394218 - Attachment is obsolete: true
Attachment #8395037 - Flags: review?(jmathies)
Attachment #8395037 - Flags: review?(jmathies) → review+
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.
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.
Didn't get to this one in time :/

Clearing needinfo
Flags: needinfo?(tabraldes)
Attached patch fix v.3Splinter Review
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: jmathies → nobody
Status: ASSIGNED → NEW
Can also confirm this definetly happens on Firefox 28.0
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.
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!
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.
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+
(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.
https://hg.mozilla.org/integration/fx-team/rev/a31f762698bb
Target Milestone: --- → mozilla32
Assignee: nobody → jmathies
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?
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+
I agree. If it's just a cosmetic anomaly, why not push it to Beta?
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.
Keywords: verifyme
Reproduced in nightly 2014-05-04.
Verified fixed 32.0a1 (2014-05-20), Win 8.1 x64
Status: RESOLVED → VERIFIED
Verified fixed FF 31b2, Win 8.1 x64.
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.
(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.
Attached image 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
Thank you.
Thank you! The new version works great! :)
You need to log in before you can comment on or make changes to this bug.