[10.11] Some UI system fonts are wrong (Helvetica instead of San Francisco, makes menu items look wonky)

VERIFIED FIXED in Firefox 43

Status

()

defect
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: mstange, Assigned: jtd)

Tracking

(Blocks 2 bugs)

Trunk
mozilla44
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43+ verified, firefox44+ verified)

Details

(Whiteboard: gfx-noted)

Attachments

(11 attachments, 2 obsolete attachments)

50.35 KB, image/png
Details
33.51 KB, image/png
Details
48.99 KB, image/png
Details
40 bytes, text/x-review-board-request
jtd
: review+
Details
40 bytes, text/x-review-board-request
jtd
: review+
Details
38.35 KB, patch
mstange
: review+
Details | Diff | Splinter Review
228.85 KB, image/png
Details
62.37 KB, image/png
Details
50.95 KB, image/png
Details
93.37 KB, image/png
Details
31.20 KB, patch
mstange
: review+
Details | Diff | Splinter Review
In the recent 10.11 Betas, many pieces of UI text in Firefox have been using the wrong font.

I looked into what happens: [NSFont menuBarFontOfSize:0.0] returns a font with familyName .AppleSystemUIFont, which is not in gfxMacPlatformFontList::mSystemFontFamilies because CTFontManagerCopyAvailableFontFamilyNames() doesn't list it. Once that has been worked around, the next issue is that MacOSFontEntry::GetFontRef() returns null because CGFontCreateWithFontName(".AppleSystemUIFont") returns null. This too can be worked around by using CTFontCopyGraphicsFont((CTFontRef)[NSFont fontWithName:psname size:0], nullptr).

I've filed the following bug with Apple about this:
rdar://22548869/ 
> Summary:
> NSFont has a few APIs to get system fonts, for example
> +[NSFont systemFontOfSize:] and +[NSFont menuBarFontOfSize:]. On 10.11, some of
> these APIs return fonts with familyName .SFNSText, and others return fonts with
> familyName .AppleSystemUIFont.
> 
> .SFNSText is working correctly.
> .AppleSystemUIFont however is working differently - it neither shows up in the
> list returned by CTFontManagerCopyAvailableFontFamilyNames(), nor is it
> accepted by CGFontCreateWithFontName. Both of the latter factors break system
> fonts in the Firefox UI.
> 
> Steps to Reproduce:
> NSFont* font = [NSFont menuBarFontOfSize:0.0];
> for (NSArray* face in [[NSFontManager sharedFontManager] availableMembersOfFontFamily:[font familyName]]) {
>   CGFontRef cgFont = CGFontCreateWithFontName((CFStringRef)[face objectAtIndex:0]);
>   assert(cgFont);
> }
> 
> Expected Results:
> Either .AppleSystemUIFont should be supported by both CGFontCreateWithFontName
> and CTFontManagerCopyAvailableFontFamilyNames, or all NSFont APIs should return
> fonts with familyName .SFNSText, or the functionality change should be
> documented as intentional so that we can work around it in Firefox.
> 
> Actual Results:
> CGFontCreateWithFontName returns null for .AppleSystemUIFont, and
> CTFontManagerCopyAvailableFontFamilyNames doesn't list it.
> 
> Version:
> 10.11 Beta 15A279b
(In reply to Markus Stange [:mstange] from comment #0)

> I looked into what happens: [NSFont menuBarFontOfSize:0.0] returns a font
> with familyName .AppleSystemUIFont, which is not in
> gfxMacPlatformFontList::mSystemFontFamilies because
> CTFontManagerCopyAvailableFontFamilyNames() doesn't list it. Once that has
> been worked around, the next issue is that MacOSFontEntry::GetFontRef()
> returns null because CGFontCreateWithFontName(".AppleSystemUIFont") returns
> null. This too can be worked around by using
> CTFontCopyGraphicsFont((CTFontRef)[NSFont fontWithName:psname size:0],
> nullptr).

Hmmm, that's obnoxious. The whole notion of "hidden" UI fonts seems sort of ill-conceived by Apple.
Should we land a workaround now? We can still remove it if Apple changes things.
Bug 1201318 - Use nsAutoReleasePool from nsCocoaUtils.h. r?jtd

The next patch will use nsCocoaUtils::GetStringForNSString, so I need to add the include to nsCocoaUtils.h anyway.
Attachment #8656366 - Flags: review?(jdaggett)
Bug 1201318 - Factor out AddFamily. r?jtd
Attachment #8656367 - Flags: review?(jdaggett)
Bug 1201318 - Add font families for system fonts in case those aren't listed in CTFontManagerCopyAvailableFontFamilyNames(). r?jtd
Attachment #8656368 - Flags: review?(jdaggett)
Bug 1201318 - Fall back to getting the CGFontRef from NSFont to workaround 10.11 weirdness. r?jtd
Attachment #8656369 - Flags: review?(jdaggett)
Whiteboard: gfx-noted
Comment on attachment 8656367 [details]
MozReview Request: Bug 1201318 - Factor out AddFamily. r?jtd

Bug 1201318 - Factor out AddFamily. r?jtd
Comment on attachment 8656368 [details]
MozReview Request: Bug 1201318 - Add font families for system fonts in case those aren't listed in CTFontManagerCopyAvailableFontFamilyNames(). r?jtd

Bug 1201318 - Add font families for system fonts in case those aren't listed in CTFontManagerCopyAvailableFontFamilyNames(). r?jtd
Comment on attachment 8656369 [details]
MozReview Request: Bug 1201318 - Fall back to getting the CGFontRef from NSFont to work around 10.11 weirdness. r?jtd

Bug 1201318 - Fall back to getting the CGFontRef from NSFont to work around 10.11 weirdness. r?jtd
Attachment #8656369 - Attachment description: MozReview Request: Bug 1201318 - Fall back to getting the CGFontRef from NSFont to workaround 10.11 weirdness. r?jtd → MozReview Request: Bug 1201318 - Fall back to getting the CGFontRef from NSFont to work around 10.11 weirdness. r?jtd
Assignee: nobody → mstange
Chromium has fixed a similar, related problem:
https://code.google.com/p/chromium/issues/detail?id=521034
Safari on 10.11 uses the magical Apple system font keywords rather than real font names:

http://people.mozilla.org/~jdaggett/tests/systemfontlist.html

I suspect that 'AppleSystemUIFont' is somehow an alias that changes based on the size of the font from xxx Text to xxx Display depending upon the size. I think we need to investigate what happens across sizes for these families...
I'm not really sure what to look for. Do you want to take over this bug?

I have a (possibly unrelated) question about changing font characteristics based on size. On HiDPI, I expect regular Cocoa apps to draw text with the size in logical screen units under a 2x scale transform, and intuitively I'd say that Gecko probably draws it with the size in physical screen units without a scale transform. Will that make a difference in rendering?
(In reply to Markus Stange [:mstange] from comment #15)
> I'm not really sure what to look for. Do you want to take over this bug?
> 
> I have a (possibly unrelated) question about changing font characteristics
> based on size. On HiDPI, I expect regular Cocoa apps to draw text with the
> size in logical screen units under a 2x scale transform, and intuitively I'd
> say that Gecko probably draws it with the size in physical screen units
> without a scale transform. Will that make a difference in rendering?

Yeah, I think I need to review the WWDC video (2013?) about "Dynamic Text" and maybe ping some people at Apple. It seems to me that the 'AppleSystemUIFont' may be what they're using and when a specific NSFont object is instantiated with a size, the choice of which underlying font to use is made at that point (e.g. .SFNSText or .SFNSDisplay). This is "optical sizing" and there are various, incompatible schemes for this across OS and fonts. Adobe has one (the 'size' OpenType feature which really shouldn't have been a feature) and so does Microsoft (the usLowerOpticalPointSize and usUpperOpticalPointSize additions to the OS/2 table).

Any idea about the priority of this? Seems like it should be relatively high but I'd just like to confirm that.
(In reply to John Daggett (:jtd) from comment #16)
> Any idea about the priority of this? Seems like it should be relatively high
> but I'd just like to confirm that.

I think it would be very embarrassing to ship with broken context menu and URL bar fonts when 10.11 is released, so yes, I think this needs to be a high priority. Some websites estimate the ship date of 10.11 to be at the end of September, so we need to be ready very soon.
Assignee: mstange → jdaggett
Markus, I'm going to take this one if you don't mind. As I suspected, it looks like this is much more complicated than simply figuring out how to look up private system fonts. The use of optical sizing means there are new API's that we should be using for system fonts.

The relevant presentation on 10.11 and the use of optical sizing:

https://developer.apple.com/videos/wwdc/2015/?id=804

If you look through the PDF, there's a section entitled "Font API Pitfalls" (p. 295) which includes points like:

  - Don’t Access Fonts with Private Names
  - Don’t Access System Font by Name
  - Don’t Draw System Font at a Different-Than-Nominal Point Size

The first two imply one should simply lookup and use the system font and never touch the family name. The last one means we shouldn't take a given system font and use it at a different size, the size needs to be part of the system font lookup, since the optical size rules will choose a different underlying font based on the size.

I think the solution here is to probably tweak the handling of UI fonts such that we pass a special system fontname (e.g. -apple-system-xxx?) down to the font system and have a special subclass of gfxMacFontFamily that deals with the details of the actual lookup. I haven't completely thought this through but it's clear from the WWDC slides that relying on family name lookups isn't going to work. The end result needs to be that any time a new gfxFont instance is created for a system font family, some form of systemFontOfSize: needs to get called to assure that the right font gets picked up.

Note that I think this will also clean up some of the nastiness associated with the iOS code on bug 1170986.
Blocks: 1170986
Comment on attachment 8656366 [details]
MozReview Request: Bug 1201318 - Use nsAutoReleasePool from nsCocoaUtils.h. r?jtd

Clearing out the review requests for now, as I think we need to investigate this a little more and do a different approach.
Attachment #8656366 - Flags: review?(jdaggett)
Attachment #8656367 - Flags: review?(jdaggett)
Attachment #8656368 - Flags: review?(jdaggett)
Attachment #8656369 - Flags: review?(jdaggett)
(In reply to Markus Stange [:mstange] from comment #17)
> (In reply to John Daggett (:jtd) from comment #16)
> > Any idea about the priority of this? Seems like it should be relatively high
> > but I'd just like to confirm that.
> 
> I think it would be very embarrassing to ship with broken context menu and
> URL bar fonts when 10.11 is released, so yes, I think this needs to be a
> high priority. Some websites estimate the ship date of 10.11 to be at the
> end of September, so we need to be ready very soon.

Yup, Sep 30. About as official as it can get: https://www.apple.com/osx/elcapitan-preview/
Firefox 41, which will be released on September 22nd, is currently in the RC stage, so any fix for this bug won't be accepted into it. That means that this bug won't be fixed by the time 10.11 is released.
Firefox 42 will be released on November 3rd.
(In reply to Markus Stange [:mstange] from comment #21)
> Firefox 41, which will be released on September 22nd, is currently in the RC
> stage, so any fix for this bug won't be accepted into it. That means that
> this bug won't be fixed by the time 10.11 is released.
> Firefox 42 will be released on November 3rd.

I'd like to get this fixed this week. Then we can see what is required/possible in terms of pushing it out. But I think FF42 is probably the likely earliest release.
Duplicate of this bug: 1192119
Here's Apple's response to the bug report I filed:

> This is the new reality in supporting the SF font. The system font is now a
> collection of virtual fonts that might not show up in user interface related
> list.
> 
> You need to migrate to CoreText-based APIs such as CTFontDescriptor.
Does this mean we need to take some form of the patch in bug 1170986?
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #25)
> Does this mean we need to take some form of the patch in bug 1170986?

Not precisely but it does mean we need to treat system fonts like this differently. I'm going to rework the LookAndFeel code to pass down a "special" name for system fonts, something along the lines of what Safari is doing with names like "-apple-system". The font family name lookup code will then do the lookup using CoreText/NSFont magic. Note that this includes doing a size-specific lookup when creating a gfxFont object, as different sizes will map to different underlying fonts due to the way optical sizing works. In other words, sizes less than 20pt will map to a "text" flavor of San Francisco and sizes larger will map to a "display" font. So we need different underlying gfxFontEntry objects for each.

I think the one outcome here is that the iOS code will actually become simpler with this distinction.

Still a little fuzzy on what CTFontDescriptor magic is needed, but it may just be that this "virtual font" logic only works under 10.11. Webkit code isn't much of a help as they simply use a bunch of private API's (naturally...).
(In reply to John Daggett (:jtd) from comment #26)
> I'm going to rework the LookAndFeel code to pass down a
> "special" name for system fonts, something along the lines of what Safari is
> doing with names like "-apple-system". The font family name lookup code will
> then do the lookup using CoreText/NSFont magic.

Why not make the LookAndFeel code pass down an NSFont pointer directly, instead of going through a magic string?
Blocks: 1211878
(In reply to Markus Stange [:mstange] from comment #27)

> Why not make the LookAndFeel code pass down an NSFont pointer directly,
> instead of going through a magic string?

That's a cross-platform API, the style system puts the name into the fontlist, which is in turn visible to authors. So better to do as Apple does and expose "-apple-system" as a magic font family name. In addition, the actual font under 10.11 will vary based on the size between .SF NS Text and .SF NS Display.
Attachment #8656366 - Flags: review+
Attachment #8656367 - Flags: review+
Attachment #8656368 - Flags: review-
Attachment #8656369 - Flags: review-
(In reply to John Daggett (:jtd) from comment #28)

I thought Apple's magic font names were analogous to our platform font names like menu, icon, message-box etc. Shouldn't those be the names that we put into the font list?
(In reply to Markus Stange [:mstange] from comment #29)
> (In reply to John Daggett (:jtd) from comment #28)
> 
> I thought Apple's magic font names were analogous to our platform font names
> like menu, icon, message-box etc. Shouldn't those be the names that we put
> into the font list?

They use the *same* font across *all* of those calls, the only variation is the size and weight that come back. It simplifies things to make this assumption, since the lookup from -apple-system ==> font family is much quicker if there's only one or two families to deal with.
Oh, I see, thanks. I was getting confused by the size/weight-specific font names like -apple-system-headline.
Rework handling of system fonts under OSX. The LookAndFeel code now calls through to gfx code to lookup the system font family/style. It uses the same "meta" system font name that Webkit uses, -apple-system. The FindFamily method now takes a gfxFontStyle pointer so that the size can be used to look up the underlying font family name when running under 10.11.

The code here assumes that (1) the system font family name is the same across UI font API calls and (2) that OSX 10.11 uses a combination of text/display font families for UI display. The handling of these families is hard-coded since there's no efficient API for determining this on the fly. But within debug code, both of these assumptions are confirmed so that we can flag when changes do occur in the future.
Attachment #8656368 - Attachment is obsolete: true
Attachment #8656369 - Attachment is obsolete: true
Attachment #8670760 - Flags: review?(mstange)
Comment on attachment 8670760 [details] [diff] [review]
patch, rework system font handling under OSX

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

Thanks!

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +647,5 @@
>      sFontManager = [NSFontManager sharedFontManager];
> +
> +#ifdef DEBUG
> +    // different system font API's always map to the same family under OSX, so
> +    // just assume that and omit a warning if that ever changes

omit -> emit

@@ +772,5 @@
>  }
>  
> +// System fonts under OSX may contain weird "meta" names but if we create
> +// a new font using just the Postscript name, the NSFont api returns an object
> +// with the actual real family name

Can you add one example of the different names here?

@@ +803,5 @@
> +    nsAutoString familyName, key;
> +    nsCocoaUtils::GetStringForNSString(textFamilyName, familyName);
> +    GenerateFontListKey(familyName, key);
> +    gfxFontFamily* family = mSystemFontFamilies.GetWeak(key);
> +    NS_ASSERTION(family, "null system text font family");

So the "real family name" is one that is listed by CTFontManagerCopyAvailableFontFamilyNames()?

@@ +819,5 @@
> +        mSystemDisplayFontFamily = CheckFamily(family);
> +    }
> +
> +#if DEBUG
> +    // confirm that the optical size switch is at 20.0

Thank you for this.

@@ +1081,5 @@
> +gfxFontFamily*
> +gfxMacPlatformFontList::FindFamily(const nsAString& aFamily, gfxFontStyle* aStyle)
> +{
> +    nsAutoString key;
> +    GenerateFontListKey(aFamily, key);

You're not using the variable "key" in this function.
Attachment #8670760 - Flags: review?(mstange) → review+
Comment on attachment 8670760 [details] [diff] [review]
patch, rework system font handling under OSX

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

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +653,5 @@
> +    if ([sysFamily compare:[[NSFont boldSystemFontOfSize:0.0] familyName]] != NSOrderedSame ||
> +        [sysFamily compare:[[NSFont controlContentFontOfSize:0.0] familyName]] != NSOrderedSame ||
> +        [sysFamily compare:[[NSFont menuBarFontOfSize:0.0] familyName]] != NSOrderedSame ||
> +        [sysFamily compare:[[NSFont toolTipsFontOfSize:0.0] familyName]] != NSOrderedSame) {
> +        NS_WARNING("system font types map to different font families -- please log a bug!!");

Hmm, I realized that I don't actually understand the purpose of this warning. Did you mean to use GetRealFamilyName here? Because this warning fires on my machine.

systemFont: .SF NS Text
boldSystemFont: .SF NS Text
controlContentFont: .AppleSystemUIFont
menuBarFont: .AppleSystemUIFont
toolTipsFont: .AppleSystemUIFont
Flags: needinfo?(jdaggett)
Depends on: 1213280
(In reply to Markus Stange [:mstange] from comment #36)
> > +    if ([sysFamily compare:[[NSFont boldSystemFontOfSize:0.0] familyName]] != NSOrderedSame ||
> > +        [sysFamily compare:[[NSFont controlContentFontOfSize:0.0] familyName]] != NSOrderedSame ||
> > +        [sysFamily compare:[[NSFont menuBarFontOfSize:0.0] familyName]] != NSOrderedSame ||
> > +        [sysFamily compare:[[NSFont toolTipsFontOfSize:0.0] familyName]] != NSOrderedSame) {
> > +        NS_WARNING("system font types map to different font families -- please log a bug!!");
> 
> Hmm, I realized that I don't actually understand the purpose of this
> warning. Did you mean to use GetRealFamilyName here? Because this warning
> fires on my machine.
> 
> systemFont: .SF NS Text
> boldSystemFont: .SF NS Text
> controlContentFont: .AppleSystemUIFont
> menuBarFont: .AppleSystemUIFont
> toolTipsFont: .AppleSystemUIFont

Argh, yes, these are effectively the same font for size: 0.0 so we should use GetRealFamilyName here. I'll log a new bug to fix this up.
Flags: needinfo?(jdaggett)
Depends on: 1214490
Difference in similar menu item width is likely bug 1213280.
Here too, the width issue is likely bug 1213280.
Comment on attachment 8656366 [details]
MozReview Request: Bug 1201318 - Use nsAutoReleasePool from nsCocoaUtils.h. r?jtd

Approval Request Comment
[Feature/regressing bug #]: latest OSX 10.11 doesn't use correct system font
[User impact if declined]: incorrect font will display in UI elements
[Describe test coverage new/current, TreeHerder]: stable on trunk
[Risks and why]: low risk, simply selects correct system font
[String/UUID change made/needed]: none
Attachment #8656366 - Flags: approval-mozilla-beta?
Attachment #8656366 - Flags: approval-mozilla-aurora?
Comment on attachment 8656367 [details]
MozReview Request: Bug 1201318 - Factor out AddFamily. r?jtd

Approval Request Comment
[Feature/regressing bug #]: latest OSX 10.11 doesn't use correct system font
[User impact if declined]: incorrect font will display in UI elements
[Describe test coverage new/current, TreeHerder]: stable on trunk
[Risks and why]: low risk, simply selects correct system font
[String/UUID change made/needed]: none
Attachment #8656367 - Flags: approval-mozilla-beta?
Attachment #8656367 - Flags: approval-mozilla-aurora?
Comment on attachment 8670760 [details] [diff] [review]
patch, rework system font handling under OSX

Approval Request Comment
[Feature/regressing bug #]: latest OSX 10.11 doesn't use correct system font
[User impact if declined]: incorrect font will display in UI elements
[Describe test coverage new/current, TreeHerder]: stable on trunk
[Risks and why]: low risk, simply selects correct system font
[String/UUID change made/needed]: none
Attachment #8670760 - Flags: approval-mozilla-beta?
Attachment #8670760 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1218421
Comment on attachment 8670760 [details] [diff] [review]
patch, rework system font handling under OSX

landed before the merge.
Attachment #8670760 - Flags: approval-mozilla-aurora?
Attachment #8656366 - Flags: approval-mozilla-aurora?
Attachment #8656367 - Flags: approval-mozilla-aurora?
Blocks: 1221263
The font in the address bar & context menu is getting worse. It's harder to read than ever.

I'm using Firefox 42.0 on OS X El Capitan 10.11.1
(In reply to blaguskida15 from comment #47)
> Created attachment 8683713 [details]
> Address Bar & Context Menu Font Issue
> 
> The font in the address bar & context menu is getting worse. It's harder to
> read than ever.
> 
> I'm using Firefox 42.0 on OS X El Capitan 10.11.1

As mentioned in bug 1218421 this has only been fixed in Firefox 44+. I doubt we'll fix this in Firefox 42 but we might be able to uplift this to Firefox 43 (currently in Beta).

John, what are the chances we can uplift this?
Are there any temporary workarounds I can use in the meantime?
(In reply to blaguskida15 from comment #49)
> Are there any temporary workarounds I can use in the meantime?

Sorry, no workaround.
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #48)

> John, what are the chances we can uplift this?

I've already nominated it for approval-beta!

Sylvestre, approval-beta ping.

This is actually pretty important as it will not only fix our UI fonts in 10.11 but it will also allow authors to actually *use* the system font. Because of the way it strangely uses hidden font names, this is currently difficult to do. The folks at Medium have been blogging about this and I've seen a draft where they explicitly say "can't do this with Firefox", which is something I'd like to be able to push back on.
Flags: needinfo?(sledru)
Redirecting to Liz as she is the owner of 43.
Flags: needinfo?(sledru) → needinfo?(lhenry)
Comment on attachment 8670760 [details] [diff] [review]
patch, rework system font handling under OSX

Let's uplift this to beta, we want the font to look good.
Flags: needinfo?(lhenry)
Attachment #8670760 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8656367 [details]
MozReview Request: Bug 1201318 - Factor out AddFamily. r?jtd

Please uplift to beta.
Attachment #8656367 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8656366 [details]
MozReview Request: Bug 1201318 - Use nsAutoReleasePool from nsCocoaUtils.h. r?jtd

Please uplift to beta.
Attachment #8656366 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
the 3rd patch does not apply clean to beta:

grafting 307340:39f705a5cb70 "Bug 1201318 - revise OSX system font handling. r=mstange"
merging gfx/thebes/gfxDWriteFontList.cpp
merging gfx/thebes/gfxDWriteFontList.h
merging gfx/thebes/gfxFcPlatformFontList.cpp
merging gfx/thebes/gfxFcPlatformFontList.h
merging gfx/thebes/gfxFontEntry.cpp
merging gfx/thebes/gfxGDIFontList.cpp
merging gfx/thebes/gfxMacPlatformFontList.mm
merging gfx/thebes/gfxPlatformFontList.cpp
warning: conflicts during merge.
merging gfx/thebes/gfxPlatformFontList.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')
merging gfx/thebes/gfxPlatformFontList.h
merging gfx/thebes/gfxPlatformMac.cpp
merging gfx/thebes/gfxTextRun.cpp
warning: conflicts during merge.
merging gfx/thebes/gfxTextRun.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

can you take a look, thanks!
Flags: needinfo?(jdaggett)
(In reply to Carsten Book [:Tomcat] from comment #56)
> the 3rd patch does not apply clean to beta:
> 
> grafting 307340:39f705a5cb70 "Bug 1201318 - revise OSX system font handling.
> r=mstange"
> merging gfx/thebes/gfxDWriteFontList.cpp
> merging gfx/thebes/gfxDWriteFontList.h
> merging gfx/thebes/gfxFcPlatformFontList.cpp
> merging gfx/thebes/gfxFcPlatformFontList.h
> merging gfx/thebes/gfxFontEntry.cpp
> merging gfx/thebes/gfxGDIFontList.cpp
> merging gfx/thebes/gfxMacPlatformFontList.mm
> merging gfx/thebes/gfxPlatformFontList.cpp
> warning: conflicts during merge.
> merging gfx/thebes/gfxPlatformFontList.cpp incomplete! (edit conflicts, then
> use 'hg resolve --mark')
> merging gfx/thebes/gfxPlatformFontList.h
> merging gfx/thebes/gfxPlatformMac.cpp
> merging gfx/thebes/gfxTextRun.cpp
> warning: conflicts during merge.
> merging gfx/thebes/gfxTextRun.cpp incomplete! (edit conflicts, then use 'hg
> resolve --mark')
> abort: unresolved conflicts, can't continue
> (use hg resolve and hg graft --continue)
> 
> can you take a look, thanks!

Yeah, the patch was done on top of changes for bug 1182361. I'm going to put together a tweaked patch for the beta branch.
Flags: needinfo?(jdaggett)
Tracking for 43 and 44, mostly to make sure we land this in 43.
Duplicate of this bug: 1218255
The original patch landed for this was done on top of a refactoring of the pref font handling code (bug 1182361). I'd rather not uplift the patches for that bug, so I've limited the changes here to a OSX-specific. Instead of modifying the parameters to the FindFamily method shared across platforms, this version creates a new, #ifdef OS_MACOSX version that uses a method, FindFamilyWithStyle, to achieve the same behavior. Other parts of the patch are unchanged.

Approval Request Comment
[Feature/regressing bug #]: system font is not correct under OSX 10.11
[User impact if declined]: menus and UI will appear slightly off
[Describe test coverage new/current, TreeHerder]: landed over a month ago on trunk, no problems report
[Risks and why]: reduced version of the patch landed on trunk/aurora
[String/UUID change made/needed]: none
Attachment #8687783 - Flags: review?(mstange)
Comment on attachment 8687783 [details] [diff] [review]
patch, rework system font handling under OSX (for beta)

Approval Request Comment - see last comment
Attachment #8687783 - Flags: approval-mozilla-beta?
Comment on attachment 8687783 [details] [diff] [review]
patch, rework system font handling under OSX (for beta)

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

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +653,5 @@
> +    if ([sysFamily compare:[[NSFont boldSystemFontOfSize:0.0] familyName]] != NSOrderedSame ||
> +        [sysFamily compare:[[NSFont controlContentFontOfSize:0.0] familyName]] != NSOrderedSame ||
> +        [sysFamily compare:[[NSFont menuBarFontOfSize:0.0] familyName]] != NSOrderedSame ||
> +        [sysFamily compare:[[NSFont toolTipsFontOfSize:0.0] familyName]] != NSOrderedSame) {
> +        NS_WARNING("system font types map to different font families -- please log a bug!!");

So you don't want to integrate a fix for bug 1214490 into this patch?
Attachment #8687783 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #62)

> So you don't want to integrate a fix for bug 1214490 into this patch?

I'm going to upload patches for this one along with bug 1213280 soonish, hopefully tomorrow.
Should this uplift wait to make sure that it lands alongside your fix from bug 1213280?
Flags: needinfo?(jdaggett)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #64)
> Should this uplift wait to make sure that it lands alongside your fix from
> bug 1213280?

It doesn't need to wait, this is the more crucial fix since without this we're choosing Helvetica Neue for UI instead of San Francisco. The fix for 1213280 will improve the spacing of text in San Francisco.
Flags: needinfo?(jdaggett)
Comment on attachment 8687783 [details] [diff] [review]
patch, rework system font handling under OSX (for beta)

Rebased for beta, system font fix.
Attachment #8687783 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
seems this has problems to apply to beta:

patching file gfx/thebes/gfxMacPlatformFontList.h
Hunk #2 FAILED at 73
1 out of 3 hunks FAILED -- saving rejects to file gfx/thebes/gfxMacPlatformFontList.h.rej
patching file gfx/thebes/gfxMacPlatformFontList.mm
Hunk #3 succeeded at 740 with fuzz 2 (offset 23 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh rework-mac-system-font-lookups-v3-beta.patch

John, could you take a look, thanks!
Flags: needinfo?(jdaggett)
Huh, wonder why that happened. The beta patch applies just fine on top of the other two patches for me.

Anywho, I went ahead and pushed these to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/5284eeaeb23e
https://hg.mozilla.org/releases/mozilla-beta/rev/b6f944763659
https://hg.mozilla.org/releases/mozilla-beta/rev/01ac7ddda781

Pinged lizzard and kwierso on IRC to let them know before landing.
Flags: needinfo?(jdaggett)
Flags: qe-verify+
Managed to reproduce this issue on Mac OS X 10.11.1 with the Firefox 42.0b1 (20150921151815) build.
The issue is no longer reproducible on the Mac OS X 10.11.1, using Firefox 43.0b9 (20151203163240) and FIrefox 44.0a2 (2015-12-08).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Duplicate of this bug: 1233280
Is it correct that this bug implements "font-family: -apple-system" for web content in Firefox 43 on MAC OS X?
You need to log in before you can comment on or make changes to this bug.