Closed Bug 987872 (Fira) Opened 10 years ago Closed 10 years ago

Add New Version of Fira Sans & Mono

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.0M+, b2g-v2.0 wontfix, b2g-v2.0M fixed, b2g-v2.1 fixed)

Status:
RESOLVED FIXED
Milestone:
2.0 S6 (18july)
Project Flags:
blocking-b2g 2.0M+
Tracking Flags:
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed

People

(Reporter: padamczyk, Assigned: mwu)

References

Details

(Whiteboard: ux-tracking, visual design, visual-tracking, bokken, [2.0S+])

User Story

This change will include the new version of Fira Sans & Mono with various glyph additions / fixes as well as new weights.

Attachments

(9 files, 4 obsolete files)

Browser bookmarks list - Before
(deleted), image/png
Details
Browser bookmarks list - After
(deleted), image/png
Details
Lock screen - Before
(deleted), image/png
Details
Lock screen - After
(deleted), image/png
Details
Fira Mono width test
(deleted), image/png
Details
Update font name in gecko, v3
(deleted), patch
jfkthame
: review+
Details | Diff | Splinter Review
(PR) Update to Fira 3.109
(deleted), text/x-github-pull-request
Details
[PR] update to Fira 3.109 and fix glyph advances, line heights
(deleted), text/x-github-pull-request
mwu
: review+
Details
[PR] Update to 3.109 on v2.0m
(deleted), text/x-github-pull-request
Details 
      No description provided.
Blocks: 940397
Blocks: 2.0-visual-refresh
No longer blocks: 940397
User Story: (updated)
Whiteboard: ux-tracking, visual design, visual-tracking, bokken
Assignee: nobody → padamczyk
Blocks: 940397
Alias: Fira
Blocks: 944418
What's the version number of the fonts for this update?
Family names and OS/2 usWeightClass values for the updated fonts:

family: Fira Sans Two (20)
family: Fira Sans Four (40)
family: Fira Sans Eight (80)

family: Fira Sans Hair (100)
family: Fira Sans Thin (150)
family: Fira Sans UltraLight (200)
family: Fira Sans ExtraLight (250)
family: Fira Sans Light (300)
family: Fira Sans Book (350)

family: Fira Sans (400)
family: Fira Sans Medium (500)

family: Fira Sans SemiBold (600)
family: Fira Sans ExtraBold (800)
family: Fira Sans Heavy (900)
family: Fira Sans Ultra (950)

Is this the family arrangement we intend to use in the production version of these fonts? What's the subset that we actually intend to ship on devices?
Do we need all of these? More fonts = slower startup && more memory. Please select carefully and test the impact. Thanks!
Patryk, please advise if we should block 2.0 on this/if this is critical to making the rest of the visual refresh work. Thanks!
Flags: needinfo?(padamczyk)
This is critical for v.2.1, since Building Blocks was pushed out of v.2.0.
The new font changes will will address some font bugs.
And we will only ship 9 weights not all 15, just want to cover CSS weight values 100 - 900.
Flags: needinfo?(padamczyk)
I've received 3.104 and will be preparing the necessary changes.
Assignee: padamczyk → mwu
A patch for gecko will also be necessary since the font name has changed.
Attached patch Update font name in gecko (obsolete) (deleted) — — Splinter Review 
Will request review once locally tested.

I didn't switch Greek over to Fira - it can be done if we think the support is sufficiently complete.
Looking at the new fonts, I notice there seems to have been a change in the behavior of the 'frac' (automatic fractions) feature, and I wondered whether this was intended.

Previously, in an example like

  data:text/html,<div style="font-family:Fira Sans OT;
                  -moz-font-feature-settings:'frac';">fraction test 1 3/4</div>

the 'frac' feature would replace the space glyph within "1 ¾" with a narrow space.frac glyph, so that the fraction stays much closer to the preceding number.

The new 3.104 font doesn't do this any longer; the space is left unchanged.

If this is a deliberate change in functionality, perhaps it's fine, but I'd like to understand the decision to change it. Or if it's an accidental omission, how did that happen and can we get it fixed?
Comment on attachment 8424341 [details] [diff] [review]
Update font name in gecko

This seems to work.
Attachment #8424341 - Flags: review?(jfkthame)
Attachment #8424339 - Flags: review?(jfkthame)
Attached image Browser bookmarks list - Before (deleted) — 

    
    

    
  

      
      
      
      

        Attached image
          
        Browser bookmarks list - After (deleted)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached image
          
        Lock screen - Before (deleted)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached image
          
        Lock screen - After (deleted)
        — 
      

    


  

    
    

    
  
There's a difference in line height. It also appears to be italicized where it wasn't before.

    
    

    
  
Hmm, there must be something about the style properties in the font files that's not quite right, or is confusing Gecko somehow. I'll take a look and see if I can figure that out.

And as for the line height, I think we're going to need to adjust that like we did with the previous version.

    
    

    
  
I only see italicized text at certain weights - 100, 200, 300, 500.

    
    

    
  
In a number of the files, the fsSelection field in the 'OS/2' table is not set correctly. Bit 0 of that field is supposed to be set for italic faces, but in most cases it's missing. (Only FiraSans-Italic.otf and FiraSans-BoldItalic.otf have it set.)

At the weights where this is missing, it's basically a crapshoot whether you'll get the "regular" or the "italic" version, as they have identical attributes for our font-selection process.

(The italic style should also be reflected in the macStyle field of the 'head' table, though I think it's the OS/2 bit that is more important for our purposes here.)

    
    

    
  
Comment on attachment 8424339 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/moztt/pull/45

Clearing review since we need another update.

        Attachment #8424339 -
        Flags: review?(jfkthame)

    
    

    
  
Another thing I just found - fontconfig doesn't think Fira Mono is a monospaced font. It looks like there's a few glyphs that aren't the same width as others, which prevents fontconfig from identifying it as a monospaced font. Probably not a problem for our use in FirefoxOS, though.

    
    

    
  
Actually, using Fira Mono in Firefox also shows an issue with the spacing. Things don't seem to line up.

    
    

    
  
Re: Comment 22
I will let Ralph know :S

Latest bug fix:
_ aogonec uni0105 nicer connection
_ outline corrections concerning some interpolation glitches
_ web font formats added (beta version only)

Mono:
http://dev.carrois.com/wordpress/wp-content/uploads/downloads/fira_3_1/FiraMono3106.zip

Sans:
http://dev.carrois.com/wordpress/wp-content/uploads/downloads/fira_3_1/FiraSans3106.zip

    
    

    
  
Can you outline what are the issues that Ralph can fix in this releases?
So far I can identify the following:
Comment 18: Is this still an issue in v.3.106?

This won't gate shipment but should be fixed...
Comment 21: Mono glyphs are not all the same width. Do we know which ones?
Flags: needinfo?(mwu)
Flags: needinfo?(jfkthame)

    
    

    
  
(In reply to Patryk Adamczyk [:patryk] UX from comment #24)
> Can you outline what are the issues that Ralph can fix in this releases?
> So far I can identify the following:
> Comment 18: Is this still an issue in v.3.106?
> 

Yup. I just added checked the new release and most of the UI is still italicized.

> This won't gate shipment but should be fixed...
> Comment 21: Mono glyphs are not all the same width. Do we know which ones?

I have a screenshot. This only seems to affect desktop.. things seem to behave OK on the device.
Flags: needinfo?(mwu)

    
    

    
  

      
      
      
      

        Attached image
          
        Fira Mono width test (deleted)
        — 
      

    


  
Tested on Linux desktop. Test page at https://people.mozilla.org/~mwu/monotest.html . The issue isn't seen on a Flame device.

    
    

    
  
There's 40 or so characters in each row. The rows that stick out stick out by about 40 pixels, so it sounds like some widths are being rounded up one pixel. Different rows stick out at different sizes.

    
    

    
  
(In reply to Patryk Adamczyk [:patryk] UX from comment #24)
> Can you outline what are the issues that Ralph can fix in this releases?
> So far I can identify the following:
> Comment 18: Is this still an issue in v.3.106?
> 
> This won't gate shipment but should be fixed...
> Comment 21: Mono glyphs are not all the same width. Do we know which ones?

I reported a couple of Fira Mono issues recently at:
  https://github.com/mozilla/Fira/issues/35
  https://github.com/mozilla/Fira/issues/36
Are you monitoring issues filed there?

(Sorry, it's not been at all clear to me where current Fira issues are primarily/best tracked. Bugzilla? Github? Etherpad? Email?!)

We also need to address the line-height issue: see comment 15, and https://github.com/mozilla/Fira/pull/37. Either we should get Ralph to adjust the "master" sources so that the generated fonts have the desired line metrics, or we need to systematically adjust this every time we import them into FxOS.

(If the latter, we also need to consider the question of which version will be deployed on websites and distributed to others: the "original" with Ralph's line metrics, or the version that we ship in FxOS?)
Flags: needinfo?(jfkthame)

    
    

    
  
(In reply to Michael Wu [:mwu] from comment #27)
> There's 40 or so characters in each row. The rows that stick out stick out
> by about 40 pixels, so it sounds like some widths are being rounded up one
> pixel. Different rows stick out at different sizes.

This might be because the isFixedPitch flag is not set (in the OS/2 table and within the CFF font data), and therefore when grid-fitting the glyphs, FreeType doesn't know that they're supposed to all remain the same width. Then it could round different widths differently if it's giving priority to maintaining stem widths, sidebearings, etc. instead.

    
    

    
  
Comment on attachment 8430291 [details]
Fira Mono width test

I wonder if this is a linux thing? I do recall seeing a few linux distros in the past and their type handing wasn't too great. If it works ok on device, perhaps we can ignore this for now. I will let Ralph know however.

    
    

    
  
> I reported a couple of Fira Mono issues recently at:
>   https://github.com/mozilla/Fira/issues/35
>   https://github.com/mozilla/Fira/issues/36
> Are you monitoring issues filed there?

Yes I scanned through these, and had Ralph address most of the reasonable ones. Lets keep all the issues on Github since they are easier to find.
I will talk to Ralph about 35 & 36


As far as the line height. I just want to make sure I understand what is going on? in v.2 of the font did we use the line heights provided in the master font or did we adjust them? I believe the master font should have the desired line heights for our OS.

    
    

    
  
(In reply to Patryk Adamczyk [:patryk] UX from comment #31)
> As far as the line height. I just want to make sure I understand what is
> going on? in v.2 of the font did we use the line heights provided in the
> master font or did we adjust them? I believe the master font should have the
> desired line heights for our OS.

We adjusted the line height in v2. https://bugzilla.mozilla.org/show_bug.cgi?id=897009#c53

    
    

    
  
Also see https://bugzilla.mozilla.org/show_bug.cgi?id=897009#c61

It was adjusted to workaround issues in Gecko.

    
    

    
  
http://dev.carrois.com/wordpress/downloads/fira_3_1/FiraFonts3107.zip

More bugs fixed, and Ralph things he knows what is causing the height inconsistencies, apparently this version has a fix.
_ uni0400 & uni0450 (small & capital/sc cyrillic i with grave)
_ uni040D & uni045D (small & capital/sc cyrillic ie with grave)
_ Mono: now set as isFixedPitch
_ Mono: panose information added
_ Mono: asterisk * uni002A now on x-height and a little bit larger
_ UFO source files
_ fixed interpolation glitches
Flags: needinfo?(mwu)

    
    

    
  
Most of the Italic faces in Fira Sans still don't have the proper style bits set, in either the macStyle field of the 'head' table or the fsSelection field of the OS/2 table. So the problem described in comment 15 and following is still going to happen.

    
    

    
  
(Filed this as https://github.com/mozilla/Fira/issues/40, in case it's getting overlooked here.)

    
    

    
  
The new version of Fira Mono fixes all my issues with inconsistent widths on Linux. The line heights also look correct now on the device.

However, Italicized versions of Fira Sans are still being used when the normal version is expected. (comment 35, comment 18)
Flags: needinfo?(mwu)

    
    

    
  
Great. Hopefully Ralph can address this in tomorrow's update v.3.108

    
    

    
  
Looks like a number of things have been fixed here, including the unwanted-Italics problem mentioned in preceding comments - great!

(I haven't tested this on a device, but the style bits look correct now so I believe it'll work.)

One remaining issue: I see a bunch of glyphs with negative advance widths, when they should be zero-width. Filed https://github.com/mozilla/Fira/issues/42 on this.

    
    

    
  
Everything checks out here, and I don't know of any real showstoppers now, so I'll post an updated PR.

I think the line height might actually be a tiny bit shorter now, FWIW.

    
    

    
  
Comment on attachment 8424339 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/moztt/pull/45

Updated to Fira 3.108.

        Attachment #8424339 -
        Flags: review?(jfkthame)

    
    

    
  
There's one other issue that I just noticed - Fira 3.1 comes with an updated OFL license in Licence_OFL11.txt, but I'm not sure how to interpret it. It looks like a bit of a pseudodiff between OFL 1.0 and 1.1, but it also adds a reserved font name.

    
  
Blocks: 1021271

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Update font name in gecko, v2 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Unbitrotted.

        Attachment #8424341 -
        Attachment is obsolete: true

        Attachment #8424341 -
        Flags: review?(jfkthame)

        Attachment #8435274 -
        Flags: review?(jfkthame)

    
    

    
  
Was 3.107 also suppose to fix line heights in Fira Mono? It sounds like Fira Mono might be too high still.
Flags: needinfo?(padamczyk)

    
    

    
  
A) Yes by 3.107 Sans and Mono should have the same line heights. They don't? If not, I'll get Ralph to spit out a v.3.109 with the fix.
B) Is v.3.108 in the OS now? Want to make sure we can get it in for v.2
Flags: needinfo?(padamczyk)

    
  
Flags: needinfo?(mwu)

    
    

    
  
3.108 is ready to land as soon Jonathan reviews the PR and gecko patch. Requesting 2.0 blocking to make sure this can get in.
blocking-b2g: --- → 2.0?
Flags: needinfo?(mwu)

    
    

    
  
I'm not really comfortable with taking the update until the issue of glyphs with negative "advance" width is resolved (https://github.com/mozilla/Fira/issues/42). Can we get a 3.109 with this bug fixed?

    
    

    
  
(In reply to Patryk Adamczyk [:patryk] UX from comment #46)
> A) Yes by 3.107 Sans and Mono should have the same line heights. They don't?
> If not, I'll get Ralph to spit out a v.3.109 with the fix.

Someone reported that Mono was too high compared to the line height hacked version we had for v.2. I'm using Fira Mono as my monospaced font right now and the line height does seem to be a bit high.

    
    

    
  
Is this an enhancement request? or feature? Please use feature b2g flag for approvals.
Flags: needinfo?(swilkes)

    
    

    
  
Earlier, I was told to remove the 2.0 flag so that's why it's not on here. This is part of the 2.0 Visual Refresh. Flagging Patryk for any additional notes here.
Flags: needinfo?(swilkes) → needinfo?(padamczyk)

    
    

    
  
Patryk, this bug appears to have morphed in terms of what issue it is actually about. Can you speak to:
* Whether this is still a *2.1 but not a 2.0 blocker* as it initially was?
* Whether the size issues are 2.0 blockers?

    
    

    
  
This bug does 3 things:
+ fix localization issues
+ adds further polish to the fonts
+ adds more font features

I am not sure why this bug has been de-scoped. I believe its ready to be integrated into v.2.0.
Flags: needinfo?(padamczyk)

    
    

    
  
(In reply to Patryk Adamczyk [:patryk] UX from comment #53)
> This bug does 3 things:
> + fix localization issues
> + adds further polish to the fonts
> + adds more font features
> 
> I am not sure why this bug has been de-scoped. I believe its ready to be
> integrated into v.2.0.

Based on bugs comments this reads as a really nice-to-have for 2.0 but I don't see any comments saying this will block the release if not fixed. In this case, i would recommend seeking approval-gaia-2.0 to get this uplifted on 2.0 rather than block on this and depending on risk/reward and where we are in the release timeline we could approve this.
blocking-b2g: 2.0? → ---

    
    

    
  
Jonathan & Michael can we try to get this into master sooner than later? 3.108 seems good. I'd like to see this make it into v.2 if possible. Thanks.

Bhavana, the big fix really the redesign of over 20 characters in Cyrillic and Greek languages which we've gotten various bugs / complaints over. Its a matter of localization. It might not be a very important carrier centric fix, but its important to our users that consume those languages. There are also new font styles that we're dependent on in the headers and home screen.  Should have been clearer.
Flags: needinfo?(mwu)
Flags: needinfo?(jfkthame)
Flags: needinfo?(bbajaj)

    
    

    
  
As mentioned before, I'm only waiting for review. I'm ok with 3.108, but Jonathan wants the negative advance width issue to be fixed.

BTW this doesn't change our default Greek language font - I can provide a patch for that if desired.
Flags: needinfo?(mwu)

    
    

    
  
(In reply to Patryk Adamczyk [:patryk] UX from comment #55)
> Jonathan & Michael can we try to get this into master sooner than later?
> 3.108 seems good. I'd like to see this make it into v.2 if possible. Thanks.

See comment 48; in 3.108, the italic faces have glyphs with negative "advance" width, which I fear could have unexpected consequences for layout. We should get this fixed before taking the update.

If we can't get a corrected version (3.109) from Ralph, I guess we could hack the fonts locally to fix this, but I've been assuming it should be fixed upstream.

    
    

    
  
(In reply to Patryk Adamczyk [:patryk] UX from comment #55)
> Jonathan & Michael can we try to get this into master sooner than later?
> 3.108 seems good. I'd like to see this make it into v.2 if possible. Thanks.
> 
> Bhavana, the big fix really the redesign of over 20 characters in Cyrillic
> and Greek languages which we've gotten various bugs / complaints over. Its a
> matter of localization. It might not be a very important carrier centric
> fix, but its important to our users that consume those languages. There are
> also new font styles that we're dependent on in the headers and home screen.
> Should have been clearer.

Thanks for the context! I understand where you coming from and given this missed the 2.0 boat and keeping the reward in mind, I would recommend to request approval to get these landed on 2.0 once these land on master/central and stick there. Depending on the risk and reward(which clearly is high as you state) and where we are in the release cycle you should be able to get approval.
Flags: needinfo?(bbajaj)

    
    

    
  
Patryk, is there a prospect of a negative-widths fix from Ralph sometime soon, or would you like me to do a locally-hacked version of 3.108?
Flags: needinfo?(padamczyk)
Flags: needinfo?(jfkthame)

    
    

    
  
I talked to Ralph and he's aiming for next week. I am fine with either you hacking or just landing Fira Sans and we create another bug for Mono. If there is a chance for this to get into v.2, I'd opt for it.
Flags: needinfo?(padamczyk) → needinfo?(jfkthame)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Update font name in gecko, v3 (deleted)
        — 
          — Splinter Review
      

    


  
More unbitrotting.

        Attachment #8435274 -
        Attachment is obsolete: true

        Attachment #8435274 -
        Flags: review?(jfkthame)

        Attachment #8439416 -
        Flags: review?(jfkthame)

    
    

    
  

      
      
      
      

        Attached file
          
        link to github PR to update Fira fonts (obsolete) (deleted)
        — 
      

    


  
I have not yet built and tested this on a device, so handle with care. :)

This should replace the v2 Fira fonts with those from the 3.108 archive, modified to fix the glyphs that had negative advance width.

AFAICS, the Fira Sans and Fira Mono fonts here should have the same default line-height; but again, I have not yet tested this.

        Attachment #8439487 -
        Flags: review?(mwu)
Flags: needinfo?(jfkthame)

    
    

    
  
Comment on attachment 8439416 [details] [diff] [review]
Update font name in gecko, v3

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

This looks fine; but ideally its landing in Gecko should be coordinated with the new fonts landing in moztt.

        Attachment #8439416 -
        Flags: review?(jfkthame) → review+

    
  

        Attachment #8424339 -
        Flags: review?(jfkthame)

    
    

    
  
Comment on attachment 8439487 [details]
link to github PR to update Fira fonts

Tested. Seems to render OK.

        Attachment #8439487 -
        Flags: review?(mwu) → review+

    
    

    
  
This is somewhat hard to coordinate - I don't know of an easy way to atomically land both parts. So, this is what I'm gonna do:

1. Land the moztt repo update.
2. Wait for mirroring to git.mozilla.org.
3. Wait for an update to b2g-inbound to pick up the change.
4. Land the gecko change to b2g-inbound.

At step 3, there's probably going to be some tests failing, which will be fixed by step 4.

    
    

    
  
Reverting for reftest failures. As before, I'm doing this in two parts.

https://github.com/mozilla-b2g/moztt/commit/c510babaf88dfa2cfe2c202afb2649ee124569af

These are the failing tests:

R10:

REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/first-letter/329069-3.html | image comparison (==), max difference: 211, number of differing pixels: 81
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/first-letter/617869-1.html | image comparison (==), max difference: 255, number of differing pixels: 872
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-align-self-baseline-horiz-3.xhtml | image comparison (==), max difference: 63, number of differing pixels: 96
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-dyn-insertAroundDiv-1.xhtml | image comparison (==), max difference: 180, number of differing pixels: 83
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-dyn-insertAroundSpan-1.xhtml | image comparison (==), max difference: 180, number of differing pixels: 83
06-13 03:12:37.864 711 711 I Gecko : REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/first-letter/329069-3.html | image comparison (==), max difference: 211, number of differing pixels: 81
06-13 03:15:32.045 711 711 I Gecko : REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/first-letter/617869-1.html | image comparison (==), max difference: 255, number of differing pixels: 872
06-13 03:21:06.914 711 711 I Gecko : REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-align-self-baseline-horiz-3.xhtml | image comparison (==), max difference: 63, number of differing pixels: 96
06-13 03:24:43.445 711 711 I Gecko : REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-dyn-insertAroundDiv-1.xhtml | image comparison (==), max difference: 180, number of differing pixels: 83
06-13 03:25:15.844 711 711 I Gecko : REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-dyn-insertAroundSpan-1.xhtml | image comparison (==), max difference: 180, number of differing pixels: 83

R11:

REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/forms/input/number/number-significant-fractional-digits.html | image comparison (==), max difference: 3, number of differing pixels: 9
06-13 03:10:49.691 718 718 I Gecko : REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/forms/input/number/number-significant-fractional-digits.html | image comparison (==), max difference: 3, number of differing pixels: 9

R17:

REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/svg/fallback-color-04.svg | image comparison (==), max difference: 19, number of differing pixels: 1
06-13 03:31:14.044 695 695 I Gecko : REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/svg/fallback-color-04.svg | image comparison (==), max difference: 19, number of differing pixels: 1

There are also unexpected passes:

R4:

REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8888/tests/layout/reftests/bidi/922530-1.html | image comparison (==)
06-13 02:53:04.037 702 702 I Gecko : REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8888/tests/layout/reftests/bidi/922530-1.html | image comparison (==)

R7:

REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8888/tests/layout/reftests/bugs/593243-2.html | image comparison (==)
06-13 03:17:48.050 715 715 I Gecko : REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8888/tests/layout/reftests/bugs/593243-2.html | image comparison (==)

    
    

    
  
Most of the interesting failures are in R10.

> R10:
> 
> REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/first-letter/329069-3.html |
> image comparison (==), max difference: 211, number of differing pixels: 81
> REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/first-letter/617869-1.html |
> image comparison (==), max difference: 255, number of differing pixels: 872

These two fail due to differences in line height.

The tests changes the style of the first letter via :first-letter, and the reference changes the style of the first letter via a <span>. The line height of the reference page with the <span> is higher than the test.

> REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-align-self-
> baseline-horiz-3.xhtml | image comparison (==), max difference: 63, number
> of differing pixels: 96

Another difference in line height - the reference has a taller colored background box than the test.

> REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-dyn-
> insertAroundDiv-1.xhtml | image comparison (==), max difference: 180, number
> of differing pixels: 83
> REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-dyn-
> insertAroundSpan-1.xhtml | image comparison (==), max difference: 180,
> number of differing pixels: 83

The right/bottom of the text is rendered differently. 

> 06-13 03:12:37.864 711 711 I Gecko : REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/first-letter/329069-3.html |
> image comparison (==), max difference: 211, number of differing pixels: 81
> 06-13 03:15:32.045 711 711 I Gecko : REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/first-letter/617869-1.html |
> image comparison (==), max difference: 255, number of differing pixels: 872
> 06-13 03:21:06.914 711 711 I Gecko : REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-align-self-
> baseline-horiz-3.xhtml | image comparison (==), max difference: 63, number
> of differing pixels: 96
> 06-13 03:24:43.445 711 711 I Gecko : REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-dyn-
> insertAroundDiv-1.xhtml | image comparison (==), max difference: 180, number
> of differing pixels: 83
> 06-13 03:25:15.844 711 711 I Gecko : REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/flexbox/flexbox-dyn-
> insertAroundSpan-1.xhtml | image comparison (==), max difference: 180,
> number of differing pixels: 83
> 

Not sure what happened in these tests - the reftest analyzer didn't give me something useful.

> R11:
> 
> REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/forms/input/number/number-
> significant-fractional-digits.html | image comparison (==), max difference:
> 3, number of differing pixels: 9
> 06-13 03:10:49.691 718 718 I Gecko : REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/forms/input/number/number-
> significant-fractional-digits.html | image comparison (==), max difference:
> 3, number of differing pixels: 9
> 

The right edge of the text is rendered differently.

> R17:
> 
> REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/svg/fallback-color-04.svg | image
> comparison (==), max difference: 19, number of differing pixels: 1
> 06-13 03:31:14.044 695 695 I Gecko : REFTEST TEST-UNEXPECTED-FAIL |
> http://10.0.2.2:8888/tests/layout/reftests/svg/fallback-color-04.svg | image
> comparison (==), max difference: 19, number of differing pixels: 1
> 

An apparently random pixel in the output is rendered differently. Looks like there's green text on a green background.

> There are also unexpected passes:
> 
> R4:
> 
> REFTEST TEST-UNEXPECTED-PASS |
> http://10.0.2.2:8888/tests/layout/reftests/bidi/922530-1.html | image
> comparison (==)
> 06-13 02:53:04.037 702 702 I Gecko : REFTEST TEST-UNEXPECTED-PASS |
> http://10.0.2.2:8888/tests/layout/reftests/bidi/922530-1.html | image
> comparison (==)
> 

I think we're actually passing this now.

> R7:
> 
> REFTEST TEST-UNEXPECTED-PASS |
> http://10.0.2.2:8888/tests/layout/reftests/bugs/593243-2.html | image
> comparison (==)
> 06-13 03:17:48.050 715 715 I Gecko : REFTEST TEST-UNEXPECTED-PASS |
> http://10.0.2.2:8888/tests/layout/reftests/bugs/593243-2.html | image
> comparison (==)

I have no idea what's going on here. The bug was WONTFIX'd.
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-]

    
  

        Attachment #8424339 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        (PR) Update to Fira 3.109 (deleted)
        — 
      

    


  

        Attachment #8439487 -
        Attachment is obsolete: true

        Attachment #8447798 -
        Flags: review?(jfkthame)

    
    

    
  
(In reply to Michael Wu [:mwu] from comment #71)
> Created attachment 8447798 [details]
> (PR) Update to Fira 3.109

This still appears to result in increased default line spacing that we (presumably) don't want.

Testcase comparing the current (2.001) fonts with this 3.109 version:
http://people.mozilla.org/~jkew/tests/firatest/t.html

I'll do some comparisons of what's actually present in the font metrics...

    
    

    
  
It doesn't actually seem too bad on device. It does seem a bit taller than before, but it's not ridiculously tall like the first time.

However, I feel like something in the font metrics is also causing the reftest failures..

    
    

    
  
Looking at the testcase above in the Inspector, I see a reported line-height (with a 32px font-size) of 38.5px using the existing 2.001 font, and 45.3px using 3.109.

Comparing the metrics fields in the fonts, in 2.001 we have an expected line-height of 1200 em-units, regardless of whether this is based on 'hhea' or 'OS/2' metrics. (Different platforms/apps unfortunately rely on different fields in the font - hence it's generally a good idea to ensure they're consistent with each other.) That would equate to 38.4px for a 32px font; the 0.1px discrepancy in the Inspector is presumably a result of rounding somewhere along the way.

In 3.109, the metrics fields are set to give a line-height of 1400 em-units, which would equate to 44.8px; again, what we're actually getting is close to this, modulo some rounding of the components of the total value.

(All this is based on testing on OS X desktop; the testcase looks very similar on an actual FxOS device, though the precise details of metrics rounding will vary slightly for the different font back-ends and device resolution.)

Based on the metrics in the fonts, then, we'd expect the 3.109 version to increase default line-heights everywhere by about 17% (1400/1200), and that fits with how the example looks on screen. While it's not a huge change - most of the time it'd probably look fine - I suspect that (a) it's responsible for at least some test failures, and (b) any apps that don't explicitly set a fixed line-height will tend to display less information per screen, which is probably a bad thing.

So I think we should fix this (i.e. revert to the metrics we had in our version of 2.001) to minimize the disruption of the update. We could consider taking a global change of the default line-height at some point, to match the upstream Fira release, but that's a change we'd need to examine carefully, and probably lots of Gaia apps, etc., would need style adjustments to accompany it.

    
    

    
  


  
This incorporates your commit from PR#49, plus fixes for the negative glyph advances (https://github.com/mozilla/Fira/issues/42) and for the increased line height discussed above.

Not tested in an actual device build yet.

        Attachment #8447965 -
        Flags: review?(mwu)

    
    

    
  
(In reply to Jonathan Kew (:jfkthame) from comment #74)
> Based on the metrics in the fonts, then, we'd expect the 3.109 version to
> increase default line-heights everywhere by about 17% (1400/1200), and that
> fits with how the example looks on screen. While it's not a huge change -
> most of the time it'd probably look fine - I suspect that (a) it's
> responsible for at least some test failures, and (b) any apps that don't
> explicitly set a fixed line-height will tend to display less information per
> screen, which is probably a bad thing.
> 

I actually fixed a bunch of tests that were sensitive to large line heights during the last font update. I wouldn't expect many more tests left that are sensitive, unless we enabled a bunch more tests since the last time.

> So I think we should fix this (i.e. revert to the metrics we had in our
> version of 2.001) to minimize the disruption of the update. We could
> consider taking a global change of the default line-height at some point, to
> match the upstream Fira release, but that's a change we'd need to examine
> carefully, and probably lots of Gaia apps, etc., would need style
> adjustments to accompany it.

I'm expecting many Gaia apps to require adjustment regardless, but I agree on minimizing the amount of disruption. Many parts of gaia appear to be using the lightest weight. I checked one case, and that was using "lighter" as its font weight. Not sure if gecko is doing the right thing there. If we get approval to backport, I'll probably trim all the new weights out.

    
    

    
  
(In reply to Michael Wu [:mwu] from comment #76)

> I'm expecting many Gaia apps to require adjustment regardless, but I agree
> on minimizing the amount of disruption. Many parts of gaia appear to be
> using the lightest weight. I checked one case, and that was using "lighter"
> as its font weight.

Ugh... yeah, if they use "font-weight:lighter", where the inherited weight is "normal", then they'll get the lightest face (CSS weight 100). That's what the current CSS-Fonts spec says.[1] Probably not what they wanted, if we previously only had a "light" but not an "ultralight" face.

> Not sure if gecko is doing the right thing there.

It's right according to the spec. I'm not sure I actually agree with the way the lighter/bolder behavior is currently specced, but that's a different discussion.

As far as Gaia is concerned, they'd probably be better off using an explicit weight such as "font-weight:300" where they want a somewhat-lighter-than-default face.

> If we get approval to backport, I'll probably trim all the new weights out.

That would be the safest approach, certainly.

[1] http://dev.w3.org/csswg/css-fonts/#font-weight-prop

    
  

        Attachment #8447798 -
        Flags: review?(jfkthame)

    
    

    
  
Comment on attachment 8447965 [details]
[PR] update to Fira 3.109 and fix glyph advances, line heights

No obvious problems when tested on the emulator.

Going to run this through reftests locally.

        Attachment #8447965 -
        Flags: review?(mwu) → review+

    
    

    
  
Things look mostly ok testing locally, so I'm trying this again.

https://github.com/mozilla-b2g/moztt/commit/96cdde4b5b5d8d3785b36c3c68cd746aff3005cc

This will be followed up by a patch on b2g-inbound to update font names when everything is mirrored.

    
    

    
  
https://hg.mozilla.org/integration/b2g-inbound/rev/184277751313

I also marked a bidi test as passing on B2G.

    
  
Blocks: 1032659

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/184277751313
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED

    
    

    
  
Should we request uplift approval here for 2.0?

    
    

    
  
(In reply to Kevin Grandon :kgrandon from comment #82)
> Should we request uplift approval here for 2.0?

If we do that, we'll need to take care to minimize any potential styling disruption, as per comments 76 and 77.

Also, I suspect there may be an issue with updating the installed fonts on existing devices; see bug 1032874. We should be cautious about any uplift or wider deployment until we're sure we understand how that works.
Depends on: 1032874

    
    

    
  
Comment on attachment 8447965 [details]
[PR] update to Fira 3.109 and fix glyph advances, line heights

Flagging Patryk on ui-review? here. Please make sure to flag UX people for ui-review? on any patches with user-facing changes. Thanks!

        Attachment #8447965 -
        Flags: ui-review?(padamczyk)

    
    

    
  
(In reply to Stephany Wilkes from comment #84)
> Comment on attachment 8447965 [details]
> [PR] update to Fira 3.109 and fix glyph advances, line heights
> 
> Flagging Patryk on ui-review? here. Please make sure to flag UX people for
> ui-review? on any patches with user-facing changes. Thanks!

There is *way* too many patches to ux-review everything, but something as important as a font makes sense.

    
    

    
  
Comment on attachment 8447965 [details]
[PR] update to Fira 3.109 and fix glyph advances, line heights

Given that the font update has been driven by the UX side, asking for ui-review is silly.

        Attachment #8447965 -
        Flags: ui-review?(padamczyk)

    
  
blocking-b2g: --- → 2.0M?

    
    

    
  

      
      
      
      

        Attached file
          
        [PR] Update to 3.109 on v2.0m (deleted)
        — 
      

    


  

    
    

    
  
according to triage result, 2.0M+, tracking flag 2.0M affected.
blocking-b2g: 2.0M? → 2.0M+

          status-b2g-v2.0M:
          --- → affected

    
    

    
  
Dolphin will need this solution, [2.0S+]
Whiteboard: ux-tracking, visual design, visual-tracking, bokken → ux-tracking, visual design, visual-tracking, bokken, [2.0S+]

    
    

    
  
Bug 1032659 also needs to be uplifted to fix certain gaia text being too light with the new font.

    
  
blocking-b2g: --- → 2.0M+

          status-b2g-v2.0M:
          --- → affected

    
  
Blocks: Woodduck

    
    

    
  
Josh, I think 2.0m is not affected. Is any other issue?
Flags: needinfo?(jocheng)

    
    

    
  
(In reply to Kai-Zhen Li [:seinlin] from comment #94)
> Josh, I think 2.0m is not affected. Is any other issue?

Hi Kai-Zhen,
Per https://bugzilla.mozilla.org/show_bug.cgi?id=1092073#c14
Michael mentioned we should also backport following 2 bugs to 2.0m - bug 987872 and bug 1032659.
Flags: needinfo?(jocheng)
See Also:  → https://bugzilla.mozilla.org/show_bug.cgi?id=1059213

    
  
Flags: needinfo?(kli)

    
    

    
  
[Blocking Requested - why for this release]:

Hi Michael,
Could you help to raise approval request for 2.0? We need new Fira font per your comment of https://bugzilla.mozilla.org/show_bug.cgi?id=1126641#c37?
Thanks!
blocking-b2g: 2.0M+ → 2.0?
Flags: needinfo?(mwu)

    
    

    
  
Hi Bhavana,
We need this new Fira font as pre-req for bug 1126641. This is to fix issue that Fulah UI missing characters.
Thank you!
Flags: needinfo?(bbajaj)
blocking-b2g: 2.0? → 2.0+
Flags: needinfo?(bbajaj)

    
    

    
  
Ping RE: b2g32 approval requests?

          status-b2g-v2.0:
          --- → affected

          status-b2g-v2.1:
          --- → fixed
Target Milestone: --- → 2.0 S6 (18july)

    
    

    
  
2.0M is the right supporting branch to land this to avoid this issue and for the required launch, so switching the approval flag.

Josh, please make sure 2.0M sheriffs uplift this
blocking-b2g: 2.0+ → 2.0M+

    
  
Flags: needinfo?(mwu)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: