931040 - [10.9] CSS system fonts are displayed incorrectly as Helvetica under OSX 10.9

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Mike de Boer [:mikedeboer]]

UA: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0

When I inspect ANY element in DOM inspector with a Nightly that DOES not show this behavior I see:

font-family: Lucida Grande

Now, with a fresh build of m-c/ Nightly I see:

font-family: .Lucida Grande UI

I don't have a regression-window yet. I'll see if I have time to do so.

Comment 1

[Mike de Boer [:mikedeboer]]

Oh, and notice the dot `.` in front of the font name.

Comment 2

[Mike de Boer [:mikedeboer]]

Alright, this is the current situation:

I couldn't find a regression window. Not with mozregression or by locally updating + clobber + build.
At a certain point I couldn't get m-c to build anymore for revisions prior to Oct 16.

What I DID find is that the regression is only noticeable with builds run with |./mach run|. Ain't that weird? In other words, I keep updating my UX nightly and it keeps showing me the correct font.

Steven, does this information ring any bell for you? If not, do you know someone for whom it might?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

This could be due to how you are building and running your build. Historically, I've noticed issues if I build using |mach build| and then run using |./obj-dir/dist/bin/firefox.exe|. Somehow stale libraries get referenced and the contents of the Browser Debugger shows out-of-date code. I also noticed this issue if I used pymake to compile the binary and then ran the binary with |mach run|. I no longer mix these two and my issues have gone away.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

I'll bet this doesn't happen on other versions of OS X, and is related to bug 931426.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] from comment #3)
> This could be due to how you are building and running your build.
> Historically, I've noticed issues if I build using |mach build| and then run
> using |./obj-dir/dist/bin/firefox.exe|. Somehow stale libraries get
> referenced and the contents of the Browser Debugger shows out-of-date code.
> I also noticed this issue if I used pymake to compile the binary and then
> ran the binary with |mach run|. I no longer mix these two and my issues have
> gone away.

Well, this issue happens to me only on OSX using the standard flow of |./mach build| and |./mach run|.

Comment 6

[Mike de Boer [:mikedeboer]]

And yes, it's OSX 10.9 Mavericks only.

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Steven Michaud from comment #4)
> I'll bet this doesn't happen on other versions of OS X, and is related to
> bug 931426.

Might be related, but please note that this I only see this in Main UI elements (thus way more visible). In fact, if you are able to reproduce it locally, this might be easier to figure out & debug re. bug 931426.

But for me the most interesting bit is that this only occurs for local builds!
(No difference if ran with `./mach run` or `objdir/dist/UXDebug.app/Contents/MacOS/firefox-bin`).

Comment 8

[Steven Michaud [:smichaud] (Retired)]

In both this bug and bug 931426 Firefox (under certain circumstances) chooses a "wrong font".  That's the underlying issue that (I think) makes them related.

Comment 9

[Timothy Nikkel (:tnikkel)]

Is the version of the mac os x sdk used different? Last time I checked official builds use sdk v10.6.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Timothy Nikkel (:tn) from comment #9)
> Is the version of the mac os x sdk used different? Last time I checked
> official builds use sdk v10.6.

Fine point! I use the latest Xcode (5.0.1), which can't be the one used by our build systems. I don't have a machine ready with the 10.6 SDK lying around to test this though...

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Steven Michaud from comment #8)
> In both this bug and bug 931426 Firefox (under certain circumstances)
> chooses a "wrong font".  That's the underlying issue that (I think) makes
> them related.

I suspect they're not directly related, except in that they're both triggered by changes in the collection of fonts shipped with the OS.

Bug 931426 is probably a result of Firefox getting incorrect style descriptors for some of the faces in the newly-extended Helvetica Neue family. I'm not yet sure whether this is caused by anomalies in the fonts themselves, something about how the Cocoa font manager handles them, or a bug in Gecko code.

In this case, I think ".Lucida Grande UI" must be a new variant of Lucida Grande that Apple is using specifically as a default UI font, but not exposing for normal content usage (hence the leading dot, which will prevent it being listed in font menus, etc). Most likely the APIs we use to query Cocoa for the default fonts for various elements may return this font, even though it's probably not in our normal font list.

(All the above is somewhat conjectural, as I haven't actually tried Mavericks yet.)

Comment 12

[Timothy Nikkel (:tnikkel)]

I believe one can push to try and change this line http://mxr.mozilla.org/mozilla-central/source/build/macosx/universal/mozconfig.common#15 to use the 10.7 sdk. I think 10.7 was the latest installed on the build slaves.

Comment 14

[Mike de Boer [:mikedeboer]]

Paul confirmed on IRC that the issue is not present when built with the 10.8 SDK.

Comment 16

[John Daggett (:jtd)]

(In reply to Steven Michaud from comment #4)
> I'll bet this doesn't happen on other versions of OS X, and is related to
> bug 931426.

It's not related to the font weight problems in bug 931426.

Comment 17

[John Daggett (:jtd)]

In Gecko, UI elements make use of CSS system fonts, which are defined in the 'font' shorthand CSS property.  For example:

  font: menu;

This says "use the font that's used by the system for menus".  On OSX there are a limited number of calls that help support this.  The code within nsLookAndFeel::GetFontImpl contains the mapping:

http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsLookAndFeel.mm#544

With OSX 10.9, Apple shipped a new retina-tuned default font since Lucida Grande was originally designed for lower resolution displays. When using any SDK version greater than 10.6, the NSFont calls used to map CSS system fonts to actual fonts now return the name of this new font:

OSX 10.8 [[NSFont systemFontOfSize] familyName] ==> returns Lucida Grande
OSX 10.9 [[NSFont systemFontOfSize] familyName] ==> returns .Lucida Grande UI

The '.' prefix here is Apple's convention for "hidden system fonts that should never appear in UI font lists".  Current Gecko code attempts to look up this hidden font and fails. This causes fallback to the default user content font (Helvetica) and hence the problem everyone is seeing here.

The problem is that we *don't* want to expose these hidden system fonts to user content via the name (via system font usage is fine).  Which means we probably want to move the handling of system fonts down into gfx. We've already discussed reworking the fontlist passed into gfxFontGroup to be one that distinguishes generic font families from other font family names.  I think it would make sense to also distinguish system fonts and doing the mapping to actual fonts within the platform font list rather than the widget code.  This would allow us to deal with mapping CSS system fonts to hidden system fonts in the OSX case.

Comment 18

[John Daggett (:jtd)]

Note: release and nightly builds are unaffected currently because they use SDK 10.6 which doesn't exhibit the problem, probably due to differences in the NSFont implementation.

Comment 19

[:Gijs (he/him)]

(In reply to John Daggett (:jtd) from comment #17)
> The problem is that we *don't* want to expose these hidden system fonts to
> user content via the name (via system font usage is fine).  Which means we
> probably want to move the handling of system fonts down into gfx. We've
> already discussed reworking the fontlist passed into gfxFontGroup to be one
> that distinguishes generic font families from other font family names.  I
> think it would make sense to also distinguish system fonts and doing the
> mapping to actual fonts within the platform font list rather than the widget
> code.  This would allow us to deal with mapping CSS system fonts to hidden
> system fonts in the OSX case.

OK. What's required to get traction on this? It's been a recurring issue when testing front-end things because the font also affects things like line height, text width, and therefore alignment and so on. Yes, everybody should just use mozconfigs that point to an older SDK, but in practice many people build without such a thing in their mozconfig, and so we end up with the current state of things, which is very sad-making.

If it's any help, I think I understand your explanation reasonably well, but I'm not sure what you mean by "move the handling of system fonts down into gfx". Currently all CSS font resolution is done in widget, as are other OS-specific CSS things (like -moz-windows-default-theme and such). Moving that logic (which I presume implies moving it for Windows and GTK (and whatever else we still support, if anything)) sounds like a lot of work, and I'm not 100% clear on why it'd be necessary*, but maybe I misunderstand what you mean?

* ie: why can't we "cheat" and make the actual usage/lookup of .Lucida Grande UI (presumably already in gfx/) just work?

Comment 20

[John Daggett (:jtd)]

The short answer is that system fonts are passed into gfx via the font family name. But we really *don't* want to expose this name to content, authors should *not* be able to put ".Lucida Grande" in their stylesheets.  It's meant to be a hidden system font, for which the Apple convention is to use the "." prefix.

To fix this what needs to happen is that the lookup of system font values needs to occur within gfx rather than widget code.  Within gfx we can handle these hidden fonts in a special way that doesn't expose them through the family name to general content.

Our production builds use the 10.6 SDK and unless there's a plan for obsoleting 10.6 I'm not sure this is terribly high priority. Would be nice to support these hidden system fonts eventually.

The workaround for developers is to use the 10.6 SDK, just as our production builds do.

Comment 21

[John Daggett (:jtd)]

(In reply to :Gijs Kruitbosch from comment #19)

> OK. What's required to get traction on this? It's been a recurring issue
> when testing front-end things because the font also affects things like line
> height, text width, and therefore alignment and so on. Yes, everybody should
> just use mozconfigs that point to an older SDK, but in practice many people
> build without such a thing in their mozconfig, and so we end up with the
> current state of things, which is very sad-making.

To put this another way, you shouldn't be testing with a configuration that's not what users will see.  I think the temporary fix may be to just hard code "Lucida Grande" for now so that developers using non-10.6 SDK's will be using what our production code will be using.

Comment 23

[Markus Stange [:mstange]]

This bug also affects Mac OS 10.10, regardless of what SDK we build with - i.e. our current Nightlies are affected on Yosemite. 10.10 uses the familyName ".Helvetica Neue DeskInterface" for system fonts.
Due to this bug we end up using the wrong font throughout the UI and it looks really bad, especially because the baseline of the used font seems to be wrong, which puts all labels a little too high inside the button, textbox or menu item.

John, can you or somebody else pick this up soon?

Alternatively, can you point me to the place where the lookup of the font based on its name fails, so I can try pumping the native NSFont* object from nsLookAndFeel::GetFontImpl to that place somehow?

Comment 24

[Jonathan Kew [:jfkthame]]

I don't currently have a 10.9 or 10.10 system to try this on, but what happens if you remove the code that filters out dot-prefixed family names in gfxMacPlatformFontList?

  http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxMacPlatformFontList.mm#665

This will presumably clutter our font family list with things we don't really want to expose, but perhaps we could filter those out at a different level if it enables the desired system fonts to work here.

Comment 25

[Markus Stange [:mstange]]

That did it! Everything looks much better now, and the baseline problem is fixed, too. (testing on 10.10)

Comment 26

[Jonathan Kew [:jfkthame]]

OK, but I assume you'll see various .-prefixed fonts if you look in the Preferences / Content font list, and they'll work if specified in CSS font-family lists, which we probably don't want. So we'll need to exclude these names in a couple of other places instead, if we don't filter them at font-list creation time.

jdaggett, do you think we should proceed along these lines, or do you have a better alternative in mind?

Comment 27

[Markus Stange [:mstange]]

Attachment: font list with that change

(In reply to Jonathan Kew (:jfkthame) from comment #26)
> OK, but I assume you'll see various .-prefixed fonts if you look in the
> Preferences / Content font list

Correct.

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: include hidden system fonts in the platform font list, but don't expose them in UI or to CSS font-family

Maybe something like this (untested) would do what we need.

Comment 29

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #26)
> OK, but I assume you'll see various .-prefixed fonts if you look in the
> Preferences / Content font list, and they'll work if specified in CSS
> font-family lists, which we probably don't want. So we'll need to exclude
> these names in a couple of other places instead, if we don't filter them at
> font-list creation time.
> 
> jdaggett, do you think we should proceed along these lines, or do you have a
> better alternative in mind?

I don't want to expose the .xxx names to content or to have them show up in the prefs fontlist!!!

I think what we can do is keep a pointer to the system font family around and for *system* font styles do an additional comparison to match it with the system font family (typically one per system lang).

I'll work on a patch tomorrow.

I again should emphasize that this is *not* the behavior users will see unless we change to using an SDK version that's newer for shipping builds. Not sure what we should do about the system font switching to Helvetica Neue in 10.10, I think we'll need to rev the SDK version by that point.

Comment 30

[Markus Stange [:mstange]]

(In reply to Jonathan Kew (:jfkthame) from comment #28)
> Created attachment 8472951 [details] [diff] [review]
> include hidden system fonts in the platform font list, but don't expose them
> in UI or to CSS font-family
> 
> Maybe something like this (untested) would do what we need.

I tested this patch. The gfxPlatformFontList.cpp change correctly stops the hidden fonts from showing up in the font lists in the preferences. However, the gfxFont.cpp change, while successfully stopping web pages from using the font, unfortunately also breaks Firefox UI fonts again.

Comment 31

[Jonathan Kew [:jfkthame]]

(In reply to Markus Stange [:mstange] from comment #30)
> (In reply to Jonathan Kew (:jfkthame) from comment #28)
> > Created attachment 8472951 [details] [diff] [review]
> > include hidden system fonts in the platform font list, but don't expose them
> > in UI or to CSS font-family
> > 
> > Maybe something like this (untested) would do what we need.
> 
> I tested this patch. The gfxPlatformFontList.cpp change correctly stops the
> hidden fonts from showing up in the font lists in the preferences. However,
> the gfxFont.cpp change, while successfully stopping web pages from using the
> font, unfortunately also breaks Firefox UI fonts again.

Oops, so that wasn't the right place to hack. If I get some time, I'll take another look; or maybe John will post a patch in the meantime.

Comment 32

[John Daggett (:jtd)]

Attachment: patch, add separate system font list

Allow hidden system fonts to be used for system fonts. Web content cannot access these and these fonts won't show up in any fontlist.

Comment 33

[Markus Stange [:mstange]]

Comment on attachment 8473517 [details] [diff] [review]
patch, add separate system font list

Perfect! Thanks for the fast response!

Comment 34

[John Daggett (:jtd)]

Great! Let me do some more testing Monday with this and then I'll tag it for review.

Comment 35

[John Daggett (:jtd)]

Attachment: patch, add separate system font list on OSX

Make the check OSX only so the only inefficiency here is the extra nsAutoPtr on the platform fontlist object and the extra parameter to FindFamily.

Comment 36

[Jonathan Kew [:jfkthame]]

Comment on attachment 8474368 [details] [diff] [review]
patch, add separate system font list on OSX

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

r=me, with minor suggestions below.

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +690,5 @@
> +            mFontFamilies.Put(familyName, familyEntry);
> +        } else {
> +            if (!mSystemFontFamilies) {
> +                mSystemFontFamilies =
> +                    new nsRefPtrHashtable<nsStringHashKey, gfxFontFamily>();

AFAIK, in practice there'll always be some hidden system fonts; so it'd make sense to create mSystemFontFamilies directly in InitFontList(), and not bother checking for its existence and creating if necessary here.

Oh, wait a minute. I think there's actually a bug in this patch as it stands: if the font list is rebuilt (e.g. because the user installs a new font while Firefox is running), you need to clear the old mSystemFontFamilies before rebuilding it.

Just doing an unconditional mSystemFontFamilies = new .... in InitFontList() should handle that, as the nsAutoPtr will delete any old version.

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +743,5 @@
>      }
>  
> +#if defined(XP_MACOSX)
> +    // for system font types allow hidden system fonts to be referenced
> +    if (mSystemFontFamilies && aUseSystemFonts) {

With mSystemFontFamilies created unconditionally (as suggested in comment on gfxMacPlatformFontList.mm), you won't need to check its existence here.

::: gfx/thebes/gfxPlatformFontList.h
@@ +126,5 @@
>                            const gfxFontStyle* aStyle);
>  
>      // TODO: make this virtual, for lazily adding to the font list
> +    virtual gfxFontFamily* FindFamily(const nsAString& aFamily,
> +                                      bool aUseSystemFonts = false);

While you're here, you could remove the obsolete TODO comment above.

@@ +293,5 @@
>      // canonical family name ==> family entry (unique, one name per family entry)
>      nsRefPtrHashtable<nsStringHashKey, gfxFontFamily> mFontFamilies;
>  
> +    // canonical family name ==> family entry (unique, one name per family entry)
> +    nsAutoPtr<nsRefPtrHashtable<nsStringHashKey, gfxFontFamily>> mSystemFontFamilies;

As the only use of this member is within #if defined(XP_MACOSX), you might as well make its declaration conditional as well.

(Really, I'd have preferred to move this entirely to the Mac subclass, and override FindFamily there to add the system font lookup. But it doesn't work well for such a FindFamily override to use the inherited method and then add to it, as it wants to insert its extra work in the middle. So I guess we'll live with the conditional-compilation version here.)

Comment 37

[John Daggett (:jtd)]

Pushed to inbound, with changes based on review comments
https://hg.mozilla.org/integration/mozilla-inbound/rev/241f31c1ad62

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/241f31c1ad62