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

RESOLVED FIXED in Firefox 40

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: sawrubh, Assigned: jfkthame)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
mozilla38
x86
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.

Comment 1

3 years ago
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)
(Assignee)

Comment 3

3 years ago
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

Comment 4

3 years ago
(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. :-\
(Reporter)

Comment 5

3 years ago
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)
(Assignee)

Comment 6

3 years ago
Created 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

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.)
(Assignee)

Comment 7

3 years ago
(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.
(Assignee)

Updated

3 years ago
OS: Linux → Windows 8.1
Hardware: x86_64 → x86
(Assignee)

Comment 8

3 years ago
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 9

3 years ago
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+
(Assignee)

Comment 10

3 years ago
Created 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

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)
(Assignee)

Updated

3 years ago
Attachment #8552435 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 11

3 years ago
Just noticed a typo in the commit message: s/DEFAULT_GDI_FONT/DEFAULT_GUI_FONT/. I'll fix that prior to landing.

Updated

3 years ago
Attachment #8553000 - Flags: review?(jmathies) → review+
(Assignee)

Comment 12

3 years ago
OK, let's see how this goes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3815de49f8
Target Milestone: --- → mozilla38
Backed out for talos xperf failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5edf734c837

https://treeherder.mozilla.org/logviewer.html#?job_id=5787735&repo=mozilla-inbound
(Assignee)

Comment 14

3 years ago
Created attachment 8553660 [details] [diff] [review]
pt2 - Whitelist SegoeUI font files for talos mtio

Needed because the newer Windows APIs will result in us using different fonts for UI elements.
Attachment #8553660 - Flags: review?(jmaher)
(Assignee)

Comment 15

3 years ago
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+
(Assignee)

Comment 17

3 years ago
Yes, this can wait a few days if that makes things easier.
https://hg.mozilla.org/build/talos/rev/1f1ad592a768
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
(Assignee)

Comment 21

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e5250c6d353
(Assignee)

Comment 22

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2fb09e09b6
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
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Depends on: 1150896

Updated

2 years ago
Depends on: 1194055
Blocks: 1195682
Depends on: 1196325
Depends on: 1208297
You need to log in before you can comment on or make changes to this bug.