925833 - Support system fonts

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Simulator, defect, P2)

Description

[(not reading bugmail) Nick Desaulniers [:\n]]

System fonts such as Fira Sans ship as part of Gonk.  Since an app can use 'Fira Sans' on device, but not in the simulator, there's a difference in appearance when developing in the simulator vs device.  This issue doesn't feel all too different from only being able to support the same codecs as the desktop platform, but filing this issue as a heads up.

Comment 1

[Alexandre Poirot [:ochameau]]

As a big part of the localization task on phones is to prevent text overflow... that issue is especially important for localizers as the overflow elipsis depends a lot on the font.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Use the right fonts in the Simulator

I think this is our best hope to improve the Simulator's fonts without expending enormous effort on this issue.

By default, B2G desktop / Simulator get the same font prefs as Firefox Desktop.  With this patch, I override them to be the same as the font prefs shipped on Gonk.

This (of course) doesn't make the fonts magically appear, although it does change the default family from "serif" to "sans-serif", which is in itself much better looking.

To get the rest of the benefits, Simulator users will need to install the various fonts, which can be found in moztt[1].  So, we'll need some docs, etc. explaining what to do to get the "improved" font experience.  Perhaps we can even try to detect they are missing... but that can happen later.  Specifically, installing the following covers most cases:

* Charis SIL Compact
* Fira Sans
* Fira Mono

For some history, the related PR[2] for this explored injecting the needed fonts via CSS @font-face, but it can only cover cases where the font family is named explicitly, which is now quite rare for Gaia.

I also considered making some kind of build script to extract the proper values from the source file (modules/libpref/src/init/all.js), but they don't change that frequently.

[1]: https://github.com/mozilla-b2g/moztt
[2]: https://github.com/mozilla/r2d2b2g/issues/353

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8398854 [details] [diff] [review]
Use the right fonts in the Simulator

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

Instead of copy pasting, could you "if GONK or FXOS_SIMULATOR" in all.js?
I think it would be "#if defined(MOZ_WIDGET_GONK) || defined(FXOS_SIMULATOR)"
but please verify as there is always something wrong with preprocessed conditions...

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Use the right fonts in the Simulator (prefs version)

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8399522 [details] [diff] [review]
Use the right fonts in the Simulator (prefs version)

I considered using more ifdefs as well, but wasn't sure if it would make all.js too confusing...  Anyway, this approach works too, let me know if it seems better.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8399522 [details] [diff] [review]
Use the right fonts in the Simulator (prefs version)

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

Have you trippled checked that it doesn't break font on device?
(I'm really scared with preprocessed ifdef now)

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Totally agree that the ifdef version is scarier!  I'm doing a device build right now to make sure it's safe.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Okay, I've confirmed that device builds look right too.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=7c468ae3dd94

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Spoke with bdahl about pdf.js's approach to fonts.  They also use a @font-face approach for embedded fonts, but that works in their case, since they are introduce new fonts and don't rely on the system font names.

Try finally looks good.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73c640ec6139

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73c640ec6139

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8399522 [details] [diff] [review]
Use the right fonts in the Simulator (prefs version)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Needed for FxOS 1.4 simulator.  The 1.4 simulator will be built from Aurora, like FxOS 1.4 itself.
User impact if declined: The simulator will display different fonts than real devices, so it would be a less realistic representation of the device.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low, just changes pref values.
String or IDL/UUID changes made by this patch: None.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8399522 [details] [diff] [review]
Use the right fonts in the Simulator (prefs version)

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

::: modules/libpref/src/init/all.js
@@ +3171,5 @@
>  // print_extra_margin enables platforms to specify an extra gap or margin
>  // around the content of the page for Print Preview only
>  pref("print.print_extra_margin", 0); // twips
>  
> +# ANDROID

This #-line isn't correct! Use // for comments in all.js, please.

@@ +3468,5 @@
>  pref("font.default.x-tibt", "serif");
>  pref("font.size.variable.x-tibt", 16);
>  pref("font.size.fixed.x-tibt", 13);
>  
> +# ANDROID || FXOS_SIMUALTOR

As above.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jonathan Kew (:jfkthame) from comment #14)
> Comment on attachment 8399522 [details] [diff] [review]
> Use the right fonts in the Simulator (prefs version)
> 
> Review of attachment 8399522 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpref/src/init/all.js
> @@ +3171,5 @@
> >  // print_extra_margin enables platforms to specify an extra gap or margin
> >  // around the content of the page for Print Preview only
> >  pref("print.print_extra_margin", 0); // twips
> >  
> > +# ANDROID
> 
> This #-line isn't correct! Use // for comments in all.js, please.
> 
> @@ +3468,5 @@
> >  pref("font.default.x-tibt", "serif");
> >  pref("font.size.variable.x-tibt", 16);
> >  pref("font.size.fixed.x-tibt", 13);
> >  
> > +# ANDROID || FXOS_SIMUALTOR
> 
> As above.

I agree the "#" comment style is odd, but I did not introduce it...  I was just following the local style of this file.  If you look at the file before my changes, such comments were already there.

If we'd like to clean it up, let's do that in bug 993137, or file a new bug to clean up the comments.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/00962671d272