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

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect)

Description

[Markus Stange [:mstange]]

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

Comment 1

[Markus Stange [:mstange]]

Attachment: Firefox context menu with wrong font

Comment 2

[Markus Stange [:mstange]]

Attachment: Finder context menu with correct font

Comment 3

[Markus Stange [:mstange]]

Attachment: Firefox context menu with correct font

Comment 4

[John Daggett (:jtd)]

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

Comment 5

[Markus Stange [:mstange]]

Should we land a workaround now? We can still remove it if Apple changes things.

Comment 6

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1201318 - Use nsAutoReleasePool from nsCocoaUtils.h. r?jtd

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.

Comment 7

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1201318 - Factor out AddFamily. r?jtd

Bug 1201318 - Factor out AddFamily. r?jtd

Comment 8

[Markus Stange [:mstange]]

Attachment: 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 9

[Markus Stange [:mstange]]

Attachment: 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 workaround 10.11 weirdness. r?jtd

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8656367 [details]
MozReview Request: Bug 1201318 - Factor out AddFamily. r?jtd

Bug 1201318 - Factor out AddFamily. r?jtd

Comment 11

[Markus Stange [:mstange]]

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 12

[Markus Stange [:mstange]]

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

Comment 13

[John Daggett (:jtd)]

Chromium has fixed a similar, related problem:
https://code.google.com/p/chromium/issues/detail?id=521034

Comment 14

[John Daggett (:jtd)]

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...

Comment 15

[Markus Stange [:mstange]]

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?

Comment 16

[John Daggett (:jtd)]

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

Comment 17

[Markus Stange [:mstange]]

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

Comment 18

[John Daggett (:jtd)]

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.

Comment 19

[John Daggett (:jtd)]

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.

Comment 20

[Cam]

(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/

Comment 21

[Markus Stange [:mstange]]

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.

Comment 22

[John Daggett (:jtd)]

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

Comment 24

[Markus Stange [:mstange]]

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.

Comment 25

[(not currently active) Ted Mielczarek]

Does this mean we need to take some form of the patch in bug 1170986?

Comment 26

[John Daggett (:jtd)]

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

Comment 27

[Markus Stange [:mstange]]

(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?

Comment 28

[John Daggett (:jtd)]

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

Comment 29

[Markus Stange [:mstange]]

(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?

Comment 30

[John Daggett (:jtd)]

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

Comment 31

[Markus Stange [:mstange]]

Oh, I see, thanks. I was getting confused by the size/weight-specific font names like -apple-system-headline.

Comment 32

[John Daggett (:jtd)]

Attachment: patch, rework system font handling under OSX

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.

Comment 33

[Markus Stange [: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.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9bf6dbf9b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/76f93328931b
https://hg.mozilla.org/integration/mozilla-inbound/rev/39f705a5cb70

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a9bf6dbf9b3
https://hg.mozilla.org/mozilla-central/rev/76f93328931b
https://hg.mozilla.org/mozilla-central/rev/39f705a5cb70

Comment 36

[Markus Stange [:mstange]]

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

Comment 37

[John Daggett (:jtd)]

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

Comment 38

[John Daggett (:jtd)]

Attachment: file menu under 10.11 after patch, safari vs. nightly

Comment 39

[John Daggett (:jtd)]

Attachment: context menu under 10.11, safari vs. nightly

Difference in similar menu item width is likely bug 1213280.

Comment 40

[John Daggett (:jtd)]

Attachment: url bar under OSX 10.11 after patch, nightly vs. safari

Here too, the width issue is likely bug 1213280.

Comment 41

[John Daggett (:jtd)]

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

Comment 42

[John Daggett (:jtd)]

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

Comment 43

[John Daggett (:jtd)]

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

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8670760 [details] [diff] [review]
patch, rework system font handling under OSX

landed before the merge.

Comment 47

[blaguskida15]

Attachment: 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

Comment 48

[u279076]

(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?

Comment 49

[blaguskida15]

Are there any temporary workarounds I can use in the meantime?

Comment 50

[John Daggett (:jtd)]

(In reply to blaguskida15 from comment #49)
> Are there any temporary workarounds I can use in the meantime?

Sorry, no workaround.

Comment 51

[John Daggett (:jtd)]

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

Comment 52

[Sylvestre Ledru [:Sylvestre]]

Redirecting to Liz as she is the owner of 43.

Comment 53

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 54

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8656367 [details]
MozReview Request: Bug 1201318 - Factor out AddFamily. r?jtd

Please uplift to beta.

Comment 55

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8656366 [details]
MozReview Request: Bug 1201318 - Use nsAutoReleasePool from nsCocoaUtils.h. r?jtd

Please uplift to beta.

Comment 56

[Carsten Book [:Tomcat]]

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!

Comment 57

[John Daggett (:jtd)]

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

Comment 58

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking for 43 and 44, mostly to make sure we land this in 43.

Comment 60

[John Daggett (:jtd)]

Attachment: patch, rework system font handling under OSX (for beta)

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

Comment 61

[John Daggett (:jtd)]

Comment on attachment 8687783 [details] [diff] [review]
patch, rework system font handling under OSX (for beta)

Approval Request Comment - see last comment

Comment 62

[Markus Stange [:mstange]]

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?

Comment 63

[John Daggett (:jtd)]

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

Comment 64

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Should this uplift wait to make sure that it lands alongside your fix from bug 1213280?

Comment 65

[John Daggett (:jtd)]

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

Comment 66

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8687783 [details] [diff] [review]
patch, rework system font handling under OSX (for beta)

Rebased for beta, system font fix.

Comment 67

[Carsten Book [:Tomcat]]

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!

Comment 68

[John Daggett (:jtd)]

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.

Comment 69

[Mihai Boldan, Desktop QA [:mboldan]]

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

Comment 71

[j.j.]

Is it correct that this bug implements "font-family: -apple-system" for web content in Firefox 43 on MAC OS X?