Closed Bug 897009 Opened 11 years ago Closed 11 years ago

Update the OS with new Fonts

Categories

(Firefox OS Graveyard :: General, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:hd+, firefox26 fixed, b2g18 wontfix, b2g-v1.1hd fixed, b2g-v1.2 fixed)

RESOLVED FIXED
1.2 FC (16sep)
blocking-b2g hd+
Tracking Status
firefox26 --- fixed
b2g18 --- wontfix
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed

People

(Reporter: padamczyk, Assigned: mwu)

References

(Depends on 1 open bug)

Details

(Whiteboard: visual design, visual-tracking)

Attachments

(13 files, 10 obsolete files)

1.47 MB, application/zip
Details
352 bytes, text/html
jfkthame
: review+
Details
1.46 MB, application/zip
Details
5.23 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
352 bytes, text/html
jfkthame
: review+
Details
178.05 KB, image/png
Details
843 bytes, patch
mounir
: review+
Details | Diff | Splinter Review
475 bytes, patch
jfkthame
: review+
Details | Diff | Splinter Review
1.28 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.24 MB, application/zip
Details
217.49 KB, application/zip
Details
352 bytes, text/html
Details
8.57 KB, patch
Details | Diff | Splinter Review
Attached file Fira Sans.zip (obsolete) —
Please remove any instances of Feura and replace them with Fira (this is the final and official name). Attached is the final version of the font in various weights and italics, as well as monospaced. Please include them all.

I'd prefer OFT over TTF for the final font if the rendering is the same.

Can you please update master, v.1.2 and v.1.1 HD with these fonts?
(In reply to Patryk Adamczyk [:patryk] UX from comment #0)
> Created attachment 779731 [details]
> Fira Sans.zip
> 
> Please remove any instances of Feura and replace them with Fira (this is the
> final and official name). Attached is the final version of the font in
> various weights and italics, as well as monospaced. Please include them all.
> 
> I'd prefer OFT over TTF for the final font if the rendering is the same.
> 
> Can you please update master, v.1.2 and v.1.1 HD with these fonts?

Note - to patch 1.1 HD, you need a hd+ here to do that.
master == v1.2, so I'll get that done. 1.1HD will require hd+ blocking - I'll request.

Now that we have monospaced moz fonts, are we switching default monospaced fonts? We're currently using Source Code Pro. If we switch, do we want to remove Source Code Pro?
blocking-b2g: --- → hd?
Flags: needinfo?(padamczyk)
From a quick look at the files in the zip archive, I have a couple questions/concerns about issues that I think we should at least consider, prior to deeming these "final" fonts and shipping them.

a) The .ttf and .otf (CFF) versions of the fonts have been given distinct names, even though they're clearly intended to be alternate implementations (using different outline format) of exactly the same font - same design, same glyph inventory, same OpenType features, etc. This means they will not be interchangeable, as one would normally expect for a pair of .ttf / .otf versions of a font; documents/stylesheets will need to know which font format is in use, and choose the appropriate name. I think it would be more usual to have the same font/family names across both implementations.

b) There are duplicate copies of the Zcaron (Ž) and zcaron (ž) glyphs in the C1 Control Characters range at U+008E and U+009E respectively. Why? These shouldn't be present, AFAICS.

c) The glyphs encoded at U+22C6 (STAR OPERATOR) and U+25AF (WHITE VERTICAL RECTANGLE) are clearly incorrect for the Unicode characters in question.

d) There are four "Mozilla logo" glyphs, looking like they're intended for
Argh, sorry, fumbling fingers! Continuing...

d) There are four "Mozilla logo" glyphs, some looking like they're intended for multi-colored overlay use, encoded at U+272A..272D in the Dingbats block. This is inappropriate: those character codes are assigned to well-defined symbols that are NOT these glyphs. If we want special Mozilla logo glyphs in the fonts, they should be encoded at Private Use Area codepoints.

e) The monospaced fonts contain ligatures for 'fi' and 'fl' that will be used by default (as they're part of the 'liga' Common Ligatures feature, which is applied automatically). This is inappropriate in a monospaced font. We don't want to find ligatures afflicting monospaced text in <pre> elements, etc.
Oh, and one more thing, while I'm on a roll here!

I wonder about the wisdom of the design chosen for the ampersand glyph (&) in the FiraSans-*Italic faces. I think it's sufficiently unusual (looks like it may be inspired by Ole Schäfer's "Fago" - though there are doubtless other similar examples out there) that it may be out of place in a font that's expected to be the main text and/or UI font on a consumer device. In this context, a font needs to be primarily legible and functional, but it should -avoid- drawing attention to its own design rather than to the actual content.

ISTM that a markedly unconventional glyph design like this may be appropriate when the focus is on the design for its own sake - it sits up and shouts "Hey, look at me! I'm quirky and interesting!" - but is out of place in the FxOS default font family. Indeed, I suspect there'd be a non-negligible number of users who, when confronted with that glyph, would be unsure what it represented, or would think it was a mistake.
Thanks Jonathan, I'll start an email thread with the font designer and get these technical issues sorted out.
Flags: needinfo?(padamczyk)
Patryk - Is this a required improvement for v1.1HD?

We would prefer to keep v1.1HD consistent with v1.1 but if this is recommended from UX's perspective I will HD+ this bug.

Thanks.
Flags: needinfo?(padamczyk)
Yes this should be for v.1.1 HD as it adds more language supports as well. I should be getting the new font with the updates jkew requested (comment 3-5) in the next few days. Once I attach it. It should be ready for integration into the builds.
Flags: needinfo?(padamczyk)
blocking-b2g: hd? → hd+
Attached file OFT.zip (obsolete) —
Open Type Fonts, please use these if we have all the features working.
Attachment #779731 - Attachment is obsolete: true
Attached file TTF.zip (obsolete) —
True Type Fonts
I attached both the OFT and TTF fonts, Erik fixed the glyph placement issues :jfkthame addressed except for A which was a design decisions which has the advantage of having both fonts installed at the time. And f, the ampersand which we will likely address in a future version.
Component: Gaia::System → General
Hi Michael,

Is this something you need to follow up here?

Thanks.
Flags: needinfo?(mwu)
(In reply to Patryk Adamczyk [:patryk] UX from comment #8)
> Yes this should be for v.1.1 HD as it adds more language supports as well. I
> should be getting the new font with the updates jkew requested (comment 3-5)
> in the next few days. Once I attach it. It should be ready for integration
> into the builds.

If we're adding new language support, should that be consistent and in 1.1 as well as in 1.1 HD then?
If it can make the v.1.1 timelines, please lets add it in. But it seems like v.1.1 HD is the next viable release.
Blocks: 902390
(In reply to Wayne Chang [:wchang] from comment #12)
> Hi Michael,
> 
> Is this something you need to follow up here?
> 
> Thanks.

I'll Update the fonts once I receive the new files with licensing fixed - I believe that's the only issue left. We'll need to add the new fonts and also update the font lists in gecko prefs.
Flags: needinfo?(mwu)
Attached file TTF.zip (obsolete) —
With updated license info.
Attachment #786435 - Attachment is obsolete: true
Attached file OFT.zip (obsolete) —
With updated license info.
Attachment #786434 - Attachment is obsolete: true
ok new font files added, please submit them to master.
Flags: needinfo?(mwu)
Whiteboard: visual design → visual design, visual-tracking, sverd
Are you sure these are the updated versions? I checked several of the font files, and AFAICS they still say Mozilla Corporation (not Foundation), and refer to the Apache license. I thought that was supposed to be changed to the OFL, as per discussion in bug 832201.
Flags: needinfo?(mwu) → needinfo?(padamczyk)
Attached file Fira.zip (obsolete) —
You're right Jonathan, not sure what happened, looks like when I uploaded my previous attachment, it didn't overwrite the orginals.

Folders should have the date 2013.08.26
Attachment #795667 - Attachment is obsolete: true
Attachment #795668 - Attachment is obsolete: true
Flags: needinfo?(padamczyk)
Attached file Fira.zip
Attachment #796020 - Attachment is obsolete: true
Whiteboard: visual design, visual-tracking, sverd → visual design, visual-tracking,
Hey Michael, looks like editing the font is more complicated than I originally hoped.
Its vital to get the fonts into the builds for v.1.1 (SD or HD) and then we can always swap update them in v.1.2. The names will not change so its an easy fix.

We had the current font in the OS, which people have been ripping already for the last year. The current font (MozTT) doesn't have any licensing info. So I'd rather get the new version of the font into the OS for v.1.1 and fix countless localization issues.

It also frustrates me to no end seeing some text labels still displaying in Open Sans, I would love to get that fixed.
Flags: needinfo?(mwu)
Pointer to Github pull-request
Attachment #796957 - Flags: review?(jfkthame)
Patryk, can we delete Source Code Pro now? I'm planning to use Fira Mono as the default monospace. Also, do we need Source Sans Pro? It's not listed in any of our default font lists.
Flags: needinfo?(mwu) → needinfo?(padamczyk)
Attached patch Update default Gonk fonts (obsolete) — Splinter Review
I have a number of open questions about this default font list update.

1. Should we use the OTF versions?
2. Should we use Fira Sans where we had to use Roboto before? (is Cyrillic covered now?)
3. Do we still need Roboto?
Attachment #796965 - Flags: review?(jfkthame)
>>1. Should we use the OTF versions?
Yes I believe OTF is the better solution. But we had rendering issues but Jeff fixed them I believe.

>>2. Should we use Fira Sans where we had to use Roboto before? (is Cyrillic covered now?)
Yes Cyrillic is covered.

>>3. Do we still need Roboto?
No we don't need it.

>> Patryk, can we delete Source Code Pro now?
Yes
Flags: needinfo?(padamczyk)
Attached patch Update default Gonk fonts, v2 (obsolete) — Splinter Review
This enables Fira Sans for x-cyrillic and el, and also switches to the otf versions of the fonts.
Attachment #796965 - Attachment is obsolete: true
Attachment #796965 - Flags: review?(jfkthame)
Attachment #798569 - Flags: review?(jfkthame)
Attached the wrong patch.
Attachment #798569 - Attachment is obsolete: true
Attachment #798569 - Flags: review?(jfkthame)
Attachment #798571 - Flags: review?(jfkthame)
Attached file Fira-2.001-OFL.zip
Given that we're updating to the "final" font names at this time, I think we really should ship with the correct licensing info, to minimize confusion.

I'm attaching a zip file with the Fira 2.001 fonts patched to refer to the OFL (instead of Apache) license, and with MoCo changed to MoFo in the copyright strings. As I understand it, this should now be the correct metadata. So I think you should use these rather than the versions that still say MoCo and Apache.

(Aside: in the process of modifying the 'name' table strings, I have also stripped the 'DSIG' digital signature from the files. This is because the signature would no longer be valid, having modified the data. This has no effect on functionality. If we want to re-sign the fonts, we'd need to investigate the necessary tooling and certificates, but I don't see any real need for this.)
Comment on attachment 798571 [details] [diff] [review]
Update default Gonk fonts, v2 (correct patch)

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

Comment 26 suggests we'll be dropping Roboto from the build altogether; if that's the case, we should also remove all references to it from these font prefs. It's still present in a number of the font-list values here.
(In reply to Jonathan Kew (:jfkthame) from comment #30)
> Comment 26 suggests we'll be dropping Roboto from the build altogether; if
> that's the case, we should also remove all references to it from these font
> prefs. It's still present in a number of the font-list values here.

We can remove most of the Roboto fonts, but the build system is somewhat stubborn about installing Roboto Regular and Roboto Bold. Guess we can drop those references anyway.
Roboto removed
Attachment #798571 - Attachment is obsolete: true
Attachment #798571 - Flags: review?(jfkthame)
I've also updated the moztt PR to remove Roboto and not install TTF variants of Fira Sans/Mono.
Attachment #800243 - Flags: review?(jfkthame)
Ignore the outdated files in the PR - I forgot to update the files with your copies. I'll update it tomorrow.
Blocks: 825091
Comment on attachment 800243 [details] [diff] [review]
Update default Gonk fonts, v3

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

Looks fine to me, but be sure to run this (with the new fonts) through tryserver before actually landing. I suspect there's a strong possibility we'll see a few failures on overly-fragile tests, in which case we'll need to fix those up to avoid turning trees orange.
Attachment #800243 - Flags: review?(jfkthame) → review+
Patryk, do we still need Source Sans? I'm not sure we ever used that font.
Flags: needinfo?(padamczyk)
(In reply to Jonathan Kew (:jfkthame) from comment #35)
> Looks fine to me, but be sure to run this (with the new fonts) through
> tryserver before actually landing. I suspect there's a strong possibility
> we'll see a few failures on overly-fragile tests, in which case we'll need
> to fix those up to avoid turning trees orange.

Not sure if we can easily do that since the fonts are stored in moztt, and we don't have the ability to easily test changes outside the gecko repo. I might have to just land this and get tests fixed ASAP.
Hmm, that's too bad. In fact, I'd say it's a pretty serious gap in our dev/testing setup.

Maybe I'm being overly pessimistic, and it'll all go smoothly. But past experience suggests that changing all the fonts will usually surface at least a few test failures.
We had failures last time, and I'm not expecting anything different this time.

An alternative might be to only *add* fonts so we can test and land the new fonts settings, and then follow up with removing unused fonts.
Yes, that might help, as you'd then be able to use tryserver to iterate on prefs settings and test fixes.

It's still possible you could get new failures later when removing the "unused" fonts, if there's a testcase that has actually been hitting them via font fallback even though they're not mentioned explicitly in any prefs or styles. But that's much less likely than failures that show up when the prefs are changed to refer to the new fonts.
I've updated the pull request to add the new fonts and only remove MozTT, which we aren't using anymore.
Comment on attachment 796957 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/moztt/pull/14

As commented in the PR: this should work for testing purposes, allowing try builds with updated font prefs. (But we must make sure to follow up with removal of the old fonts from the list in fonts.mk before actually shipping a product, to avoid bloating the build.)

Not sure how you want to track that; maybe file a followup bug about removing the obsolete fonts, and mark it as blocking whatever release this lands in, so it doesn't get forgotten?
Attachment #796957 - Flags: review?(jfkthame) → review+
I'll just not close this bug until we remove the old fonts.
(In reply to Michael Wu [:mwu] from comment #36)
> Patryk, do we still need Source Sans? I'm not sure we ever used that font.

No we don't NEED it, I was hoping to use the monospaced version clock (and potentially some other apps) but now we have Fira Mono. So we can remove it. We were also thinking of allowing the user to switch fonts on the fly... but that might be atleast a few a versions out.
Flags: needinfo?(padamczyk)
Comment on attachment 801535 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/moztt/pull/15 (Remove unused fonts)

This PR removes the unused fonts. I'll merge after we switch the default fonts over.
Attachment #801535 - Flags: review?(jfkthame)
Jonathon, looks like three reftests failed. I can fix them. However, there's also a new failure on mochitest 2 - any idea what that's about? It looks like the same failure that we hit when we switched the gonk widget to Feura Sans in bug 870113 .
Flags: needinfo?(jfkthame)
And by fixing the reftests, I mean mark as failing. It seems like the difference comes from some 1px difference in the baseline/line height.
(In reply to Michael Wu [:mwu] from comment #50)
> And by fixing the reftests, I mean mark as failing. It seems like the
> difference comes from some 1px difference in the baseline/line height.

In the case of 488685, it looks like we've run into that before, and there's a suggested fix (see bug 488685 comment 28) that we haven't gotten around to implementing. Maybe give that a try?

The screenshot of flexbox-widget-flex-items-3.html looks like Fira is giving us very excessive default line-spacing (look at the two-line "Submit Query" item). I think we should look into that and possibly adjust something.
Flags: needinfo?(jfkthame)
(In reply to Michael Wu [:mwu] from comment #49)
> Jonathon, looks like three reftests failed. I can fix them. However, there's
> also a new failure on mochitest 2 - any idea what that's about? It looks
> like the same failure that we hit when we switched the gonk widget to Feura
> Sans in bug 870113 .

I wonder if this is also caused by the large line-spacing, if it affects where the various elements appear.

What does a simple paragraph of text (using default size, line-height, etc) look like with the Fira font? If the line spacing is very wide, we should try fixing that and then see what test issues remain.
Attached image line-height comparison
OK, so I tried this on Firefox desktop; the attached screenshot compares the default line-spacing we get with (from L to R) Fira Sans, Open Sans, Clear Sans and Roboto.

IMO, while the Roboto line height might be considered a bit tight, making for a rather cramped appearance, the Clear Sans text looks very comfortably spaced. OTOH, Fira Sans is excessively loose. This will significantly reduce the amount of text (or UI elements that size themselves according to the text within them) that will fit on the device screen.
https://tbpl.mozilla.org/?tree=Try&rev=2d2c5d9912f7 (some test changes and one test disabled)
Attached patch Fix test_focus_disabled.html (obsolete) — Splinter Review
So, I think what's going on here is that elements are overflowing (due to the large line height of this font) and we end up synthesizing mouse clicks on things that aren't visible. This patch makes the test consistently rehide elements so the element being tested is always visible.
Attachment #802012 - Flags: review?(mounir)
And this is the patch I meant to attach.
Attachment #802012 - Attachment is obsolete: true
Attachment #802012 - Flags: review?(mounir)
Attachment #802013 - Flags: review?(mounir)
display: block seems to fix things.
Attachment #802014 - Flags: review?(jfkthame)
The flexbox height wasn't large enough to contain the image input element after switching to the new font.
Attachment #802018 - Flags: review?(dholbert)
Comment on attachment 802018 [details] [diff] [review]
Fix flexbox-widget-flex-items-3

r=me

Bonus points if you make the same change in flexbox-widget-flex-items-1 and -2 (and their -ref files), since those test files are all the same as this one, with the only differences being things that are relevant to what we're trying to test. (and "height" isn't relevant to what we're trying to test here, so it'd be nice to have these files consistent on it.

No worries if you don't get to that, though.
Attachment #802018 - Flags: review?(dholbert) → review+
Comment on attachment 802014 [details] [diff] [review]
Fix 488685-1.html

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

Yup - if it works, let's do it.
Attachment #802014 - Flags: review?(jfkthame) → review+
It looks like the line-height issue we're seeing is not really how Fira is intended to render; it's designed to have a line height that is only very slightly looser than Roboto, for example (although IMO the result looks substantially less cramped). But the way the ascent/descent/line-spacing metrics are set up in the font files doesn't play well with the way Gecko computes default line heights, and as a result we end up with a much-too-large value.

So I think there's an issue here that we need to look into on the Gecko side, but that will require time, careful review and extensive testing - as reworking our font-metrics code will potentially affect the rendering of all text everywhere. (We have some open bugs in this area, e.g. 832313, 657864, and others that show up in a search such as https://bugzilla.mozilla.org/buglist.cgi?quicksearch=line%20height.)

Meanwhile, the quickest and simplest fix is to tweak how the line metrics are specified in the new fonts, so that Gecko will actually give the intended default line-height. I'll attach a copy of the fonts with such an adjustment; you might like to try these and review both the test results and the effect on UI and content rendering on the device.
Here's a copy of the fonts (both .otf and .ttf) that should render in Gecko with the designer-intended line height instead of with lots of extra spacing.
Oh, I bet the same issue applies to Fira Mono - I'll look into those as well.
Here's the corresponding version of Fira Mono.
Depends on: 914549
And I've filed bug 914549 on the Gecko bug whereby we're not respecting the proper line spacing metrics in the original version of the fonts.
Thanks Jonathan - Fira Sans looks much better with this line spacing. I'll submit a PR to update to these fonts.
Attachment #802198 - Attachment description: Pointer to Github pull request: https://github.com/mozilla-b2g/moztt/pull/16 → Pointer to Github pull request: https://github.com/mozilla-b2g/moztt/pull/16 (Fix line height)
Attachment #802198 - Flags: review?(jfkthame)
Attachment #801535 - Attachment description: Pointer to Github pull request: https://github.com/mozilla-b2g/moztt/pull/15 → Pointer to Github pull request: https://github.com/mozilla-b2g/moztt/pull/15 (Remove unused fonts)
Attachment #802013 - Flags: review?(mounir) → review+
Comment on attachment 802198 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/moztt/pull/16 (Fix line height)

I'll just merge this. Review doesn't make a whole lot of sense here.
Attachment #802198 - Flags: review?(jfkthame)
(In reply to Michael Wu [:mwu] from comment #68)
> Comment on attachment 802198 [details]
> Pointer to Github pull request: https://github.com/mozilla-b2g/moztt/pull/16
> (Fix line height)
> 
> I'll just merge this. Review doesn't make a whole lot of sense here.

Fair enough - I'd only be rubber-stamping my own work anyway!
Attachment #801535 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/integration/b2g-inbound/rev/78ddef44e871 Font update and test fixes landed. I also disabled the transformed-1.html test.
Whiteboard: visual design, visual-tracking, → visual design, visual-tracking, [leave open]
Screwed up the commit message. Meant to put mounir in the list instead of listing dholbert twice. Oh well.
Followup to mark 488685-1.html as now passing on Android:
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/2ba79e21c3ca
(Do you know if the test tweaks were actually still needed, after the line-spacing change mentioned in comment 66? Not a huge deal, just curious.)
(In reply to Daniel Holbert [:dholbert] from comment #74)
> (Do you know if the test tweaks were actually still needed, after the
> line-spacing change mentioned in comment 66? Not a huge deal, just curious.)

Don't know - the test fixes seemed generally good, so I checked them in anyway.
Font removal PR merged.

https://github.com/mozilla-b2g/moztt/pull/15
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: visual design, visual-tracking, [leave open] → visual design, visual-tracking
I had to put Roboto back since the lack of it was killing ICS builds.

https://github.com/mozilla-b2g/moztt/commit/61281cb89364d9132cfe3c50c5ebe4fcf9bb1f07
mwu, you said you would handle the uplift to v1.1hd, right?
Flags: needinfo?(mwu)
Target Milestone: --- → 1.2 FC (16sep)
Blocks: 919845
Created a new v1.1.0hd branch on moztt and cherrypicked the appropriate changes - https://github.com/mozilla-b2g/moztt/commit/46c6b5607aa83c0b975b77f930c80e4bdc71e0fb . This also includes the update to charis sil, since uplifting that makes uplifting everything else easier. We'll need to point the helix manifest to the new branch and make sure our partners switch to the new branch. Also, the actual font pref change needs to land, but I want to do that after I see things mirror to gitmo.
Flags: needinfo?(mwu)
Target Milestone: 1.2 FC (16sep) → ---
I hate bugzilla.
Target Milestone: --- → 1.2 FC (16sep)
https://github.com/mozilla-b2g/b2g-manifest/commit/7aad15b654a9cbc81d43f5db1d5148aaca50f9b3 switches the relevant devices on v1.1.0hd over to the v1.1.0hd branch on moztt.
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/211697c9ba2c

Hopefully the tests will be ok.
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Depends on: 914122
Depends on: 928026
I downloaded the Fira Fonts 3.108 today from http://www.carrois.com/fira-3-1/. The line height of Fira Mono (OTF) is enormous. I replaced Fira Mono with Fira Mono OT from FiraMono-fixed-lineheight.zip (comment 64), and I get 30% more lines in my editor.
You need to log in before you can comment on or make changes to this bug.