840315 - Change default serif and sans serif fonts on gonk

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Michael Wu [:mwu]]

New default serif should be Claris.
New default sans serif should be MozTT.

Comment 1

[Dietrich Ayala (:dietrich)]

Requesting info from Product team re the blockingness of this change.

Comment 2

[Chris Lee [:clee]]

Who is making this request?  UX?

Comment 3

[Peter Dolanjski [:pdol]]

Patrick, is this a UX requirement?

Comment 4

[Patryk Adamczyk [:patryk] UX]

Yes. 

Charis is the serif we use in Firefox for Android. So would be good to unify this. Currently our default serif is Droid I believe. 

And we're still using Open Sans in some places. MozTT is default, it will be replaced by "Feura Sans" in the next few weeks once we get the final cut from the vendor. This will happen in the Leo timeframe.

Comment 5

[Michael Wu [:mwu]]

Attachment: Switch to Charis

This is just a simple s/Droid Serif/Charis SIL Compact/ over the gonk section of the pref file. I didn't do the change for MozTT since we're changing that font's name.

Comment 6

[Michael Wu [:mwu]]

Apparently the android prefs don't use Charis everywhere for serif. Our copy of Droid Serif was removed in bug 834244 though, so there's not much of a choice.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 722998 [details] [diff] [review]
Switch to Charis

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

This will cause a regression for Greek text (see comment below), in that the serif/sans-serif distinction will no longer exist.

Not marking as r+ yet, as I think we need to explicitly discuss whether that's acceptable, or what to do about it.

::: modules/libpref/src/init/all.js
@@ +3168,5 @@
>  // some entries could probably be cleaned up.
>  
>  // ar
>  
> +pref("font.name.serif.el", "Charis SIL Compact");

This doesn't really make sense - Charis does not support Greek, so for Android we deliberately left the Greek preference as Droid Serif.

If you're completely removing Droid Serif, then there will no longer be a serif Greek face available on the device, AFAIK. Maybe that's OK - we'll fall back to using the sans-serif - but it's a decision that should at least be consciously taken, not just an accidental outcome here.

@@ +3173,5 @@
>  pref("font.name.sans-serif.el", "Droid Sans");
>  pref("font.name.monospace.el", "Droid Sans Mono");
>  pref("font.name-list.sans-serif.el", "Roboto, Droid Sans");
>  
> +pref("font.name.serif.he", "Charis SIL Compact");

Charis doesn't support Hebrew, but neither did Droid Serif AFAIK, so there's no real loss here - I expect we'll just fall back to the sans font.

@@ +3178,5 @@
>  pref("font.name.sans-serif.he", "Droid Sans");
>  pref("font.name.monospace.he", "Droid Sans Mono");
>  pref("font.name-list.sans-serif.he", "Droid Sans Hebrew, Droid Sans");
>  
> +pref("font.name.serif.ja", "Charis SIL Compact");

Although Charis also doesn't support Japanese (or Korean or Chinese), for these our practice seems to be that we -expect- fallback to find the appropriate font, and we want to define the face that will be used for Latin-script words that occur within the CJK content. So here I think this is OK.

@@ +3189,4 @@
>  pref("font.name.sans-serif.ko", "Droid Sans");
>  pref("font.name.monospace.ko", "Droid Sans Mono");
>  
> +pref("font.name.serif.th", "Charis SIL Compact");

Same comment as for Hebrew.

(We should look at providing additional fonts for these scripts, too. But that would be a separate bug.)

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Michael Wu [:mwu] from comment #6)
> Apparently the android prefs don't use Charis everywhere for serif. Our copy
> of Droid Serif was removed in bug 834244 though, so there's not much of a
> choice.

It was removed already? Hmm, in that case you're presumably in a somewhat broken state right now, where most "serif" text will end up falling back to the sans-serif face.

Comment 9

[Michael Wu [:mwu]]

Patryk, what do you think? (comment 7)

Comment 10

[Patryk Adamczyk [:patryk] UX]

Thanks for the additional info.
Can we do the following. Charis default. Droid Serif would be the fall back for CJK?

Comment 11

[Dylan Oliver [:doliver]]

triage: blocking- / tracking+

Comment 12

[Michael Wu [:mwu]]

This should be blocking leo. It goes with 834244 which is blocking. Both need to land to have the correct effect.

Comment 13

[Michael Wu [:mwu]]

So you want to add Droid Serif back?

I think the main concern was not having serif for Greek, and whether we're ok with that or if we want to put Droid Serif back so we can have serif Greek.

Comment 14

[Dietrich Ayala (:dietrich)]

It's not clear why this is blocking in the other bug, but transferring blocking because of the dependency. We need to figure out how to consolidate these changes into the feature requirements for a given release.

Comment 15

[Patryk Adamczyk [:patryk] UX]

MWU for v.1.1 we plan on having Greek support, so lets add Droid Serif as a fall back in that case.

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 722998 [details] [diff] [review]
Switch to Charis

OK, if the plan is to bring back Droid Serif, this patch should be updated to retain that as the pref font for Greek.

Comment 17

[Michael Wu [:mwu]]

Attachment: Switch to Charis where possible

This adds back droid serif for greek.

Not requesting review since we're suppose to be getting a new moztt font this week, so I'll update this when we get that new font.

Comment 18

[Michael Wu [:mwu]]

Attachment: Update default Gonk fonts

This switches most of our fonts over.

1. Droid Sans -> Feura Sans
2. Droid Sans Mono -> Source Code Pro
3. Droid Serif -> Claris SIL Compact (except for greek)

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 732125 [details] [diff] [review]
Update default Gonk fonts

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

::: modules/libpref/src/init/all.js
@@ +3175,5 @@
>  
>  // ar
>  
>  pref("font.name.serif.el", "Droid Serif");
> +pref("font.name.sans-serif.el", "Feura Sans");

From looking at the Feura Sans fonts, they appear to be Latin-only; no support for Greek.

As such, it'd probably make more sense for this to be Roboto.

@@ +3217,3 @@
>  
> +pref("font.name.serif.x-cyrillic", "Charis SIL Compact");
> +pref("font.name.sans-serif.x-cyrillic", "Feura Sans");

Again, there's no Cyrillic in Feura Sans, so probably better to use Roboto here.

(Font fallback would find it anyway, but I think it's better to set the default pref appropriately.)

Comment 20

[Jonathan Kew [:jfkthame]]

(And I guess if you do those changes, there's no point in having the associated name-list prefs.)

Comment 21

[Michael Wu [:mwu]]

Attachment: Update default Gonk fonts, v2

Review comments addressed.

Comment 22

[Jonathan Kew [:jfkthame]]

Comment on attachment 732406 [details] [diff] [review]
Update default Gonk fonts, v2

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

Looks OK to me - let's get it out there for some testing. I wonder whether differences in the font metrics will cause any surprises for app layouts that made too-rigid assumptions...?

(Is it planned that Feura will eventually cover more characters, such as Greek and Cyrillic? If so, this'll want updating as new ranges get supported, but for now it looks reasonable.)

Comment 23

[Michael Wu [:mwu]]

I've heard the plan is to add Greek and Cyrillic support. I'll make sure we get this list updated when that happens.

Comment 24

[Jonathan Griffin (:jgriffin)]

I've made a try run with a new emulator that has the new fonts, but doesn't have this gecko patch:  https://tbpl.mozilla.org/?tree=Try&rev=0b9a034efc3e

Comment 25

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #24)
> I've made a try run with a new emulator that has the new fonts, but doesn't
> have this gecko patch:  https://tbpl.mozilla.org/?tree=Try&rev=0b9a034efc3e

There are a group of mochitest test failure in this try job related to text; I'm guessing we may need the path from this bug to fix them?

Comment 26

[Michael Wu [:mwu]]

(In reply to Jonathan Griffin (:jgriffin) from comment #25)
> (In reply to Jonathan Griffin (:jgriffin) from comment #24)
> > I've made a try run with a new emulator that has the new fonts, but doesn't
> > have this gecko patch:  https://tbpl.mozilla.org/?tree=Try&rev=0b9a034efc3e
> 
> There are a group of mochitest test failure in this try job related to text;
> I'm guessing we may need the path from this bug to fix them?

Hmm, I don't understand these tests well enough to know for sure, but it's probably worth trying this patch and seeing what happens.

Comment 27

[Jonathan Griffin (:jgriffin)]

Will try that as soon as bug 858853 is fixed.

Comment 28

[Michael Wu [:mwu]]

(In reply to Jonathan Griffin (:jgriffin) from comment #27)
> Will try that as soon as bug 858853 is fixed.

This is a gecko patch, so we should be able to test without uploading a new emulator, no?

Bug 858853 is important and still needs to be fixed, of course.

Comment 29

[Jonathan Griffin (:jgriffin)]

Oh, right, I was thinking I'd have to build it into the emulator.

Comment 30

[Jonathan Griffin (:jgriffin)]

Pushed this patch with the new emulator to try: https://tbpl.mozilla.org/?tree=Try&rev=6247243a7b0b

Comment 31

[Michael Wu [:mwu]]

Attachment: hg patch ready patch (Update default Gonk fonts, v2)

This patch will need to be checked in at the same time we update the emulator.

Comment 32

[Jonathan Griffin (:jgriffin)]

try run was green; mwu you can land that emu patch in the try job at the same time as the gecko patch.

Comment 33

[Jonathan Griffin (:jgriffin)]

Attachment: Emulator update for font support,

Here's the emulator patch that needs to land at the same time as this; we already have a green try run for it.

Comment 34

[Michael Wu [:mwu]]

https://hg.mozilla.org/projects/birch/rev/e06db531baaa
https://hg.mozilla.org/projects/birch/rev/0f1658d37a86

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Seeing reftest failures on birch after this landed.

Comment 36

[Jonathan Griffin (:jgriffin)]

reftests are currently hidden most places (except birch).   mwu, do you want to back this out and see what needs to happen to fix the reftests, or should we just hide the reftests on birch again, and file a bug for follow-up?

Comment 37

[Ryan VanderMeulen [:RyanVM]]

No response here or on IRC. Backed out for now. FWIW, I'm inclined to back things out that cause new failures in the name of making this suite reliable. If we allow new failures to pile up, that's just going to make that job more difficult.
https://hg.mozilla.org/projects/birch/rev/150d71fa4e8a

Comment 38

[Jonathan Kew [:jfkthame]]

FWIW, I'd suggest letting this stick and filing followups; the test failures don't look too alarming to me.

R3: looks like a known intermittent (bug 590415).

R4: failure in layout/reftests/bugs/456147.xul - this seems odd, I don't know why the XUL and HTML versions differ. Worth investigating in a followup; there may be an unwanted difference in behavior here.

R5: "unexpected" PASS in layout/reftests/canvas/text-font-lang.html - this test is inherently tricky, as it depends on available fonts and font pref settings. I think we should just remove the fails-if(b2g) annotation (with the understanding that it might fail again some day if we make further changes to the font configuration).

R10: failure in layout/reftests/text/subpixel-lineheight-1a.html - I think this is not very robust on mobile. It's already marked fuzzy on android; I'd suggest marking it fuzzy on b2g as well.

(I see it's been backed out anyhow, though, while I was writing this...)

Comment 39

[Ryan VanderMeulen [:RyanVM]]

I'm fine with re-landing with annotations made as needed, but if we're going to hold birch to the same standards as inbound, backing out was the right call. BTW, I'm going to be un-hiding a subset of the B2G reftests and crashtests on m-c/inbound shortly. It appears that R1 and R6 are the only flaky ones left, so we should do our best to keep it that way.

Comment 40

[Michael Wu [:mwu]]

https://hg.mozilla.org/projects/birch/rev/8e4cbef11f0d
https://hg.mozilla.org/projects/birch/rev/3fdae5870de1

Relanded with tests annotated.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e4cbef11f0d
https://hg.mozilla.org/mozilla-central/rev/3fdae5870de1

Comment 42

[Ryan VanderMeulen [:RyanVM]]

I assume this is going to need b2g18-specific patches for uplift? Or am I safe using the m-c versions?

Comment 43

[Michael Wu [:mwu]]

Just uplifted all required font related changes into moztt. We'll need a manifest change so these changes can be picked up.

Comment 44

[Michael Wu [:mwu]]

b2g-manifest update for v1-train in https://github.com/mozilla-b2g/b2g-manifest/commit/8589ac6015d4108a77736b02eaf794657afcca11

Comment 45

[Michael Wu [:mwu]]

Attachment: Update default Gonk Fonts, v2 (for b2g18)

This replaces the fonts on both B2G and Android, but we're not making Android builds off b2g18 so it doesn't matter.

Comment 46

[Michael Wu [:mwu]]

Attachment: Emulator update for font support (for b2g18)

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d3df3a3445a0
https://hg.mozilla.org/releases/mozilla-b2g18/rev/064af1926654

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Backed out for timeouts.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d1b2e28b303f

https://tbpl.mozilla.org/php/getParsedLog.php?id=21879792&tree=Mozilla-B2g18

14:28:51     INFO - #####
14:28:51     INFO - ##### Running run-tests step.
14:28:51     INFO - #####
14:28:51     INFO - Running command: ['/home/cltbld/talos-slave/test/build/venv/bin/python', '/home/cltbld/talos-slave/test/build/tests/mochitest/runtestsb2g.py', '--adbpath', '/home/cltbld/talos-slave/test/build/emulator/b2g-distro/out/host/linux-x86/bin/adb', '--b2gpath', '/home/cltbld/talos-slave/test/build/emulator/b2g-distro', '--console-level', 'INFO', '--emulator', 'arm', '--logcat-dir', '/home/cltbld/talos-slave/test/build', '--remote-webserver', '10.0.2.2', '--run-only-tests', 'b2g.json', '--xre-path', '/home/cltbld/talos-slave/test/build/xre/bin', '--total-chunks', '9', '--this-chunk', '5', '--gecko-path', '/home/cltbld/talos-slave/test/build/application/b2g', '--busybox', '/home/cltbld/talos-slave/test/build/busybox', '--symbols-path', 'http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-b2g18-ics_armv7a_gecko/1366144626/b2g-18.0.en-US.eabi-arm.crashreporter-symbols.zip'] in /home/cltbld/talos-slave/test/build/tests/mochitest
14:28:51     INFO - Copy/paste: /home/cltbld/talos-slave/test/build/venv/bin/python /home/cltbld/talos-slave/test/build/tests/mochitest/runtestsb2g.py --adbpath /home/cltbld/talos-slave/test/build/emulator/b2g-distro/out/host/linux-x86/bin/adb --b2gpath /home/cltbld/talos-slave/test/build/emulator/b2g-distro --console-level INFO --emulator arm --logcat-dir /home/cltbld/talos-slave/test/build --remote-webserver 10.0.2.2 --run-only-tests b2g.json --xre-path /home/cltbld/talos-slave/test/build/xre/bin --total-chunks 9 --this-chunk 5 --gecko-path /home/cltbld/talos-slave/test/build/application/b2g --busybox /home/cltbld/talos-slave/test/build/busybox --symbols-path http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-b2g18-ics_armv7a_gecko/1366144626/b2g-18.0.en-US.eabi-arm.crashreporter-symbols.zip
14:28:51     INFO -  /home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/distribute-0.6.14-py2.6.egg/setuptools/command/install_scripts.py:3: UserWarning: Module manifestparser was already imported from /home/cltbld/talos-slave/test/build/tests/mochitest/manifestparser.py, but /home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages is being added to sys.path
14:28:51     INFO -    from pkg_resources import Distribution, PathMetadata, ensure_directory
14:33:35     INFO -  Traceback (most recent call last):
14:33:35     INFO -    File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/marionette/marionette.py", line 414, in start_session
14:33:35     INFO -      self.session = self._send_message('newSession', 'value')
14:33:35     INFO -    File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/marionette/marionette.py", line 310, in _send_message
14:33:35    ERROR -      raise TimeoutException(message='socket.timeout', status=ErrorCodes.TIMEOUT, stacktrace=None)
14:33:35    ERROR -  TimeoutException: socket.timeout

Comment 49

[Jonathan Griffin (:jgriffin)]

I can reproduce the issue which caused the backout on mozilla-b2g18.  Basically, the emulator fails to fully boot after updating gecko from mozilla-central (which is built into this emulator) to mozilla-b2g18.  This may be due to an incompatibility between master branch gaia and mozilla-b2g18 gecko.

We probably need to build a v1-train emulator for the mozilla-b2g18 branch, rather than trying to use a mozilla-central emulator.  I'll make a build for this.

Comment 50

[Michael Wu [:mwu]]

Another try. I clobbered the emulator build before this push.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/47055263bce3