Closed Bug 1123654 Opened 9 years ago Closed 9 years ago

File picker button-text on Windows 8.1 uses a font and font-size that make the ellipsis look like an underscore

Categories

(Core :: Layout: Form Controls, defect)

x86
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox40 --- fixed

People

(Reporter: sawrubh, Assigned: jfkthame)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

The button for input type="file" has the button-text as "Browse_" on Windows, while on "Browse..." on Linux.

a) I think there should be a consistency between the text on different OS.
b) The underscore at the end of the button-text on Windows doesn't make much sense.
I think this is wontfix, but I'm not a domain expert. bz, can we use a different font here (like IE, or something) without breaking the web?
Component: Untriaged → Layout: Form Controls
Flags: needinfo?(bzbarsky)
Product: Firefox → Core
Summary: File picker button-text has an underscore → File picker button-text on Windows 8.1 uses a font and font-size that make the ellipsis look like an underscore
Version: unspecified → Trunk
Web compat for layout of file inputs is poor, so we have some freedom here.

That said, we don't do anything special with the font size of font family of file inputs.  It's the default font buttons use.

So the important question: do you see the same problem with just:

  data:text/html,<input type="button" value="Browse&%23x2026;">
Flags: needinfo?(bzbarsky) → needinfo?(saurabhanandiit)
It looks like the issue here is that we're using the "Microsoft Sans Serif" font, which I assume is coming from the GetStockObject(DEFAULT_GUI_FONT) call at [1]; in this font, the ellipsis character (…) is rather narrow and tightly-spaced, and ends up looking like an underscore.

In addition, it turns out that with a non-English system locale, things may get even worse: with the system locale set to Thai, the "Browse…" button displays as "Browse~" instead (because the DEFAULT_GUI_FONT gets mapped to an old Thai-codepage font).

Using the standard Win8 UI font (Segoe UI) would look a lot better. Maybe there's a more modern Windows API we should be using that would return this? I'd be a bit reluctant to simply hard-code it, as users may have customized their Windows theme to prefer something else, and we should also respect that if possible.

I note from a quick MSDN search that GetStockObject(DEFAULT_GUI_FONT) has been deprecated for many years.[2] Looks like we have some code that's overdue for an update... we should probably be using either one of the LOGFONTs returned by SystemParametersInfoW(SPI_GETNONCLIENTMETRICS,...), or perhaps just "MS Shell Dlg 2".

[1] http://hg.mozilla.org/mozilla-central/annotate/540077a30866/widget/windows/nsLookAndFeel.cpp#l591
[2] http://blogs.msdn.com/b/oldnewthing/archive/2005/07/07/436435.aspx
(In reply to Boris Zbarsky [:bz] from comment #2)
> Web compat for layout of file inputs is poor, so we have some freedom here.
> 
> That said, we don't do anything special with the font size of font family of
> file inputs.  It's the default font buttons use.
> 
> So the important question: do you see the same problem with just:
> 
>   data:text/html,<input type="button" value="Browse&%23x2026;">

They look exactly the same on my (en-gb/en-us-configured) win8 system.


(In reply to Jonathan Kew (:jfkthame) from comment #3)
> It looks like the issue here is that we're using the "Microsoft Sans Serif"
> font, which I assume is coming from the GetStockObject(DEFAULT_GUI_FONT)
> call at [1]; in this font, the ellipsis character (…) is rather narrow and
> tightly-spaced, and ends up looking like an underscore.

<snip>

> I note from a quick MSDN search that GetStockObject(DEFAULT_GUI_FONT) has
> been deprecated for many years.[2] Looks like we have some code that's
> overdue for an update... we should probably be using either one of the
> LOGFONTs returned by SystemParametersInfoW(SPI_GETNONCLIENTMETRICS,...), or
> perhaps just "MS Shell Dlg 2".
> 
> [1]
> http://hg.mozilla.org/mozilla-central/annotate/540077a30866/widget/windows/
> nsLookAndFeel.cpp#l591

The code right above the bit you linked uses those calls:

http://hg.mozilla.org/mozilla-central/annotate/540077a30866/widget/windows/nsLookAndFeel.cpp#l541

but the comments claim that it depends "on which stock font we want".

Experimenting with using font: message-box; or the other values that do use SystemParameterInfo shows that they do indeed do a better job of the ellipsis. They are, however, also all smaller (I think about 1px) than the font we currently use. It doesn't look like any of the values matches the current one for size. :-\
With:

data:text/html;charset=utf-8,<input type%3D"file"><br>%0D%0A<input type%3D"button" value%3D"Browse%26%23x2026%3B">%0D%0A

the two browse buttons look the same, on my Windows 7 system Nightly.
Flags: needinfo?(saurabhanandiit)
Experimental patch to replace the use of DEFAULT_GDI_FONT with something less deprecated. It's not obvious to me exactly what the most appropriate mappings for all the various system font constants should be, so perhaps further tweaks will be wanted, but here's a starting point for testing. There's a tryserver build with this patch at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b9df22ec03c. (The reftest oranges there are minor subpixel issues that we'll fix if we want to take this forward.)
(In reply to :Gijs Kruitbosch from comment #4)
> Experimenting with using font: message-box; or the other values that do use
> SystemParameterInfo shows that they do indeed do a better job of the
> ellipsis. They are, however, also all smaller (I think about 1px) than the
> font we currently use. It doesn't look like any of the values matches the
> current one for size. :-\

To some extent, this may be because the glyphs in Microsoft Sans Serif are in general slightly larger than those in Tahoma, for the same nominal font size. (I haven't checked whether the LOGFONT returned for lfMessageFont actually has the same size as DEFAULT_GUI_FONT or not... it may be there's also a difference there.)

I'm not sure this is necessarily a bad thing, anyhow; if switching to newer API gives us fonts that match what's widely used by other applications, that ought to be acceptable even if they're somewhat different than what we used to get.
OS: Linux → Windows 8.1
Hardware: x86_64 → x86
Comment on attachment 8552435 [details] [diff] [review]
Replace use of [deprecated] GetStockObject(DEFAULT_GDI_FONT) with newer API; results in use of Tahoma in place of Microsoft Sans Serif in various contexts

:jimm, see discussion above. The proposed patch here will have the effect of switching from Microsoft Sans Serif to Tahoma in a bunch of UI elements; is that a change we can happily take?
Attachment #8552435 - Flags: feedback?(jmathies)
Comment on attachment 8552435 [details] [diff] [review]
Replace use of [deprecated] GetStockObject(DEFAULT_GDI_FONT) with newer API; results in use of Tahoma in place of Microsoft Sans Serif in various contexts

(In reply to Jonathan Kew (:jfkthame) from comment #8)
> Comment on attachment 8552435 [details] [diff] [review]
> Replace use of [deprecated] GetStockObject(DEFAULT_GDI_FONT) with newer API;
> results in use of Tahoma in place of Microsoft Sans Serif in various contexts
> 
> :jimm, see discussion above. The proposed patch here will have the effect of
> switching from Microsoft Sans Serif to Tahoma in a bunch of UI elements; is
> that a change we can happily take?

Absolutely. There are parts of look and feel that really old, and we rarely have time to update it.

Note your patch has a unused hGDI in GetSysFontInfo.
Attachment #8552435 - Flags: feedback?(jmathies) → feedback+
Cleaned up, and now with fuzz adjustments for reftests that are affected by the change of default fonts. Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dab62122171. Do we want some kind of UX review here before landing such a change, or should we just push it out in Nightly and see how it goes?
Attachment #8553000 - Flags: review?(jmathies)
Attachment #8552435 - Attachment is obsolete: true
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Just noticed a typo in the commit message: s/DEFAULT_GDI_FONT/DEFAULT_GUI_FONT/. I'll fix that prior to landing.
Attachment #8553000 - Flags: review?(jmathies) → review+
OK, let's see how this goes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3815de49f8
Target Milestone: --- → mozilla38
Needed because the newer Windows APIs will result in us using different fonts for UI elements.
Attachment #8553660 - Flags: review?(jmaher)
So apparently we need to whitelist fonts that may be accessed by this test? That seems like a recipe for fragility; the slightest change in the content of the test, in the configuration of the test systems, and/or the browser's font preferences may affect the exact set of fonts that get touched.
Comment on attachment 8553660 [details] [diff] [review]
pt2 - Whitelist SegoeUI font files for talos mtio

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

thanks, we have a slight coordination issue here.  We had to back out a talos update yesterday due to orange on pgo.  Can you wait a bit before needing to deploy this?  Otherwise we would need to back out all the talos changes rather than fixing them in the next couple of days.
Attachment #8553660 - Flags: review?(jmaher) → review+
Yes, this can wait a few days if that makes things easier.
I am going to push to try to very all is well, then will handle updating talos.  Unfortunately the try push will need to wait for a pgo windows build - the backout wasn't clean.
talos will be updated in bug 1123852, waiting on review
Depends on: 1123852
Comment on attachment 8553000 [details] [diff] [review]
Replace use of [deprecated] GetStockObject(DEFAULT_GDI_FONT) with newer API; results in use of Tahoma in place of Microsoft Sans Serif in various contexts

>-  memcpy(name, ptrLogFont->lfFaceName, LF_FACESIZE*sizeof(char16_t));
>-  aFontName = name;
>+    memcpy(name, ptrLogFont->lfFaceName, LF_FACESIZE*sizeof(char16_t));
>+    aFontName = name;
[Huh, why the copy? name is the same size as lfFaceName.]
https://hg.mozilla.org/mozilla-central/rev/cf2fb09e09b6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 1194055
Blocks: 1195682
Depends on: 1196325
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: