Closed Bug 925833 Opened 6 years ago Closed 6 years ago

Support system fonts

Categories

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

defect

Tracking

(firefox29 wontfix, firefox30 fixed, firefox31 fixed)

RESOLVED FIXED
1.4 S6 (25apr)
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: nick, Assigned: jryans)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

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.
Priority: -- → P2
Whiteboard: [simulator]
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.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Component: Developer Tools: App Manager → Simulator
Product: Firefox → Firefox OS
Whiteboard: [simulator]
Keywords: dev-doc-needed
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
Attachment #8398854 - Flags: review?(poirot.alex)
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...
Attachment #8398854 - Flags: review?(poirot.alex) → review+
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.
Attachment #8399522 - Attachment description: Use the right fonts in the Simulator. r=ochameau → Use the right fonts in the Simulator (prefs version)
Attachment #8399522 - Flags: review?(poirot.alex)
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)
Attachment #8399522 - Flags: review?(poirot.alex) → review+
Attachment #8398854 - Attachment is obsolete: true
Totally agree that the ifdef version is scarier!  I'm doing a device build right now to make sure it's safe.
Okay, I've confirmed that device builds look right too.
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/73c640ec6139
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 992210
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.
Attachment #8399522 - Flags: approval-mozilla-aurora?
Attachment #8399522 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
(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.
You need to log in before you can comment on or make changes to this bug.