We have an opportunity to be among the first to use a brand new open source typeface, commissioned by Intel and designed by Monotype, called Clear Sans. Having spent the last couple of weeks using it in a test build, we in UX feel that it fulfills all our necessary criteria for web content typefaces (beautiful design, comfortable readability, strong character support that is comparable to Open Sans). Overall, we have actually found Clear Sans to be a much more comfortably legible typeface than Open Sans, so we are proposing to replace it as the default sans serif font. Let's land this in Nightly to get some more people looking at it.

Are we going to use this for UI also? I'm starting to see few apps coming up with custom fonts (FB messenger, I'm looking at you!!)

Let's start with web / Reader content for now. I'm not opposed to trying it in our UI as well, but let's handle that as a separate discussion.

removes Open Sans font files from the APK

How's the final package size variance?

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/637055f71289 - given the size and scope (30 failures across all four reftest chunks, some of them clearly line-height and others not clear at all what's wrong), you'll probably want to spend some time talking to someone who knows about text layout about them.

If you're changing the reader mode styles to use this new font, can you file a follow-up to update the string in the new reader mode text style popup?

inbound push was here: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8f4682511308 Failures: R1: https://tbpl.mozilla.org/php/getParsedLog.php?id=23573834&tree=Mozilla-Inbound R2: https://tbpl.mozilla.org/php/getParsedLog.php?id=23573691&tree=Mozilla-Inbound R3: https://tbpl.mozilla.org/php/getParsedLog.php?id=23573751&tree=Mozilla-Inbound R4: https://tbpl.mozilla.org/php/getParsedLog.php?id=23573514&tree=Mozilla-Inbound The R3 failures don't make a ton of sense to me: REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.155:30037/tests/layout/reftests/svg/sizing/inline--float-left--01.xhtml | image comparison (==), max difference: 77, number of differing pixels: 51 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.155:30037/tests/layout/reftests/svg/sizing/inline--float-right--01.xhtml | image comparison (==), max difference: 77, number of differing pixels: 51 Last I checked 51 was less than the max of 77.

Similar for the first failure in R1 now that I go back and look: REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.162:30151/tests/layout/reftests/bugs/179596-1b.html | image comparison (==), max difference: 255, number of differing pixels: 121 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.162:30151/tests/layout/reftests/bugs/371041-1.html | image comparison (==), max difference: 255, number of differing pixels: 16808 Again, 121 is less than 255

In R2: REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/411059-1.html | image comparison (==), max difference: 255, number of differing pixels: 2612 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/421436-1a.html | image comparison (==), max difference: 255, number of differing pixels: 72 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/421436-1b.html | image comparison (==), max difference: 255, number of differing pixels: 72 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-1c-ltr.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-2f-ltr.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-3f-ltr.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-1c-rtl.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-2f-rtl.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-3f-rtl.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-1c-ltr-insets.html | image comparison (==), max difference: 53, number of differing pixels: 23 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-2f-ltr-insets.html | image comparison (==), max difference: 53, number of differing pixels: 23 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-3f-ltr-insets.html | image comparison (==), max difference: 53, number of differing pixels: 23 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-1c-rtl-insets.html | image comparison (==), max difference: 49, number of differing pixels: 21 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-2f-rtl-insets.html | image comparison (==), max difference: 49, number of differing pixels: 21 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-3f-rtl-insets.html | image comparison (==), max difference: 49, number of differing pixels: 21 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/495385-1c.html | image comparison (==), max difference: 255, number of differing pixels: 1566 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/495385-1d.html | image comparison (==), max difference: 255, number of differing pixels: 1566 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/495385-1e.html | image comparison (==), max difference: 255, number of differing pixels: 1566 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/495385-1f.html | image comparison (==), max difference: 255, number of differing pixels: 1566 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/507187-1.html | image comparison (==), max difference: 255, number of differing pixels: 121

In R2: REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/411059-1.html | image comparison (==), max difference: 255, number of differing pixels: 2612 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/421436-1a.html | image comparison (==), max difference: 255, number of differing pixels: 72 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/421436-1b.html | image comparison (==), max difference: 255, number of differing pixels: 72 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-1c-ltr.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-2f-ltr.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-3f-ltr.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-1c-rtl.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-2f-rtl.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-3f-rtl.html | image comparison (==), max difference: 49, number of differing pixels: 22 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-1c-ltr-insets.html | image comparison (==), max difference: 53, number of differing pixels: 23 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-2f-ltr-insets.html | image comparison (==), max difference: 53, number of differing pixels: 23 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-3f-ltr-insets.html | image comparison (==), max difference: 53, number of differing pixels: 23 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-1c-rtl-insets.html | image comparison (==), max difference: 49, number of differing pixels: 21 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-2f-rtl-insets.html | image comparison (==), max difference: 49, number of differing pixels: 21 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/428810-3f-rtl-insets.html | image comparison (==), max difference: 49, number of differing pixels: 21 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/495385-1c.html | image comparison (==), max difference: 255, number of differing pixels: 1566 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/495385-1d.html | image comparison (==), max difference: 255, number of differing pixels: 1566 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/495385-1e.html | image comparison (==), max difference: 255, number of differing pixels: 1566 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/495385-1f.html | image comparison (==), max difference: 255, number of differing pixels: 1566 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30214/tests/layout/reftests/bugs/507187-1.html | image comparison (==), max difference: 255, number of differing pixels: 121 15 of the 20 failures should be passes according to what's logged. and for completeness, R4: REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30206/tests/layout/reftests/svg/svg-integration/clipPath-html-04.xhtml | image comparison (==), max difference: 74, number of differing pixels: 50 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30206/tests/layout/reftests/svg/svg-integration/clipPath-html-04-extref.xhtml | image comparison (==), max difference: 74, number of differing pixels: 50 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30206/tests/layout/reftests/svg/svg-integration/clipPath-html-05.xhtml | image comparison (==), max difference: 82, number of differing pixels: 140 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30206/tests/layout/reftests/svg/svg-integration/clipPath-html-05-extref.xhtml | image comparison (==), max difference: 82, number of differing pixels: 140 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30206/tests/layout/reftests/text/pre-line-1.html | image comparison (==), max difference: 255, number of differing pixels: 3737 REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30206/tests/layout/reftests/text/subpixel-lineheight-1a.html | image comparison (==), max difference: 247, number of differing pixels: 1153 here 2 of the 6 failures should be passes

Any progress here? Would be great to get this into Nightly for some more testing asap.

(In reply to Brad Lassey [:blassey] from comment #8) > inbound push was here: > https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8f4682511308 > > Failures: > R1: > https://tbpl.mozilla.org/php/getParsedLog.php?id=23573834&tree=Mozilla- > Inbound > R2: > https://tbpl.mozilla.org/php/getParsedLog.php?id=23573691&tree=Mozilla- > Inbound > R3: > https://tbpl.mozilla.org/php/getParsedLog.php?id=23573751&tree=Mozilla- > Inbound > R4: > https://tbpl.mozilla.org/php/getParsedLog.php?id=23573514&tree=Mozilla- > Inbound > > The R3 failures don't make a ton of sense to me: > REFTEST TEST-UNEXPECTED-FAIL | > http://10.250.49.155:30037/tests/layout/reftests/svg/sizing/inline--float- > left--01.xhtml | image comparison (==), max difference: 77, number of > differing pixels: 51 > REFTEST TEST-UNEXPECTED-FAIL | > http://10.250.49.155:30037/tests/layout/reftests/svg/sizing/inline--float- > right--01.xhtml | image comparison (==), max difference: 77, number of > differing pixels: 51 > > Last I checked 51 was less than the max of 77. That's not what it means; those are two different things, not numbers to be compared with each other. The "max difference" is the maximum difference in pixel color-component values between the test and the reference (i.e., how different are the colors?), and the number of differing pixels is the total number of individual pixels that have any difference. So in this case, there are 55 pixels that differ, and the greatest difference in any color component is 77 (in the range 0-255).

Comment on attachment 755509 [details] [diff] [review] patch Review of attachment 755509 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/fonts/Makefile.in @@ +19,5 @@ > + ClearSans-Bold.ttf \ > + ClearSans-Italic.ttf \ > + ClearSans-MediumItalic.ttf \ > + ClearSans-Regular.ttf \ > + ClearSans-Thin.ttf \ The list of files here doesn't match the set of actual font files included in the patch - there are two copies of ClearSans-Italic.ttf listed here, and it's missing ClearSans-Light and ClearSans-Medium. (It appears there are five weights of the "upright" font, but only three weights of the italic style - is that correct?) I'd suggest sorting this list, either alphabetically or by style and weight, to make it easier to see what's here.

Dan or David, it seems silly to me that changing the default font should break reftests. How would you feel about including a font in our reftest profile and specifying it as the font to test against (by overriding the "font.name.(sans-)serif.*" prefs in the profile)? That would probably involve regenerating all of the reference images, so how would one go about doing that?

(In reply to Brad Lassey [:blassey] from comment #15) > Dan or David, it seems silly to me that changing the default font should > break reftests. It does, but there are plenty of reasons why it might. > How would you feel about including a font in our reftest > profile and specifying it as the font to test against (by overriding the > "font.name.(sans-)serif.*" prefs in the profile)? It frightens me a bit to be testing something different from what we're shipping, for something as user-visible as the default font. > That would probably > involve regenerating all of the reference images, so how would one go about > doing that? There are no reference images to regenerate, unless I missed something. We (generally) have an HTML testcase and an HTML reference case, and we render them both when we run the reftest. So if we did change the default font in a reftest profile, then both the testcase and the reference case would render with that font.

Reftests don't use "pregenerated" reference images, they compare a testcase and a reference that are expected to render the same. Normally changing the default font should cause both the testcase and the reference to change in the same way. (If that weren't the case, you'd be seeing failures on virtually all tests that involve text, not just in a few dozen.) I looked at a few of the failures here, and it seemed like many of them are related to line-height rounding issues, leading to slight discrepancies in the positioning of the text (or of other elements that are positioned relative to lines of text). This may indicate some fragility in the font-metrics and/or layout code (there are a couple of open bugs that may be relevant to this - e.g. bug 442139), though it sometimes turns out the tests are actually making invalid assumptions about text metrics.

Thanks guys, I misunderstood what was going on here.

(In reply to Aaron Train [:aaronmt] from comment #5) > How's the final package size variance? This will add about 400K to the size of the final APK, by the looks of it. (Though if we're uncomfortable with that, we could look into options for reducing it.)

(In reply to Ian Barlow (:ibarlow) from comment #0) > We have an opportunity to be among the first to use a brand new open source > typeface, commissioned by Intel and designed by Monotype, called Clear Sans. What's the license involved here? The fonts themselves seem to just have a standard Monotype copyright notice and Monotype EULA link.

(In reply to Brad Lassey [:blassey] from comment #21) > Created attachment 762331 [details] [diff] [review] > patch for reftest 4 try build with that patch https://tbpl.mozilla.org/?tree=Try&rev=1b79f7aa0203

Comment on attachment 762331 [details] [diff] [review] patch for reftest 4 It seems bad to change a test whose name is "subpixel-lineheight" to have an explicit 50px line-height. (It seems worth more investigation of the other issues here too, but it probably doesn't need to block landing.)

Comment on attachment 762331 [details] [diff] [review] patch for reftest 4 >+++ b/layout/reftests/svg/svg-integration/reftest.list >-fuzzy-if(true,140,70) == clipPath-html-05.xhtml clipPath-html-05-ref.xhtml # Bug 776089 >-fuzzy-if(true,140,70) == clipPath-html-05-extref.xhtml clipPath-html-05-ref.xhtml # Bug 776089 >+fuzzy-if(Android,74,50) == clipPath-html-04.xhtml clipPath-html-04-ref.xhtml >+fuzzy-if(Android,74,50) == clipPath-html-04-extref.xhtml clipPath-html-04-ref.xhtml >+fuzzy-if(true,140,140) == clipPath-html-05.xhtml clipPath-html-05-ref.xhtml # Bug 776089 >+fuzzy-if(true,140,140) == clipPath-html-05-extref.xhtml clipPath-html-05-ref.xhtml # Bug 776089 Change "fuzzy-if(true," to just "fuzzy(", while you're here. Also, it'd be worth filing a bug on the new fuzziness (for the 04 tests), and mentioning it inline there. >diff --git a/layout/reftests/text/pre-line-1-ref.html b/layout/reftests/text/pre-line-1-ref.html >--- a/layout/reftests/text/pre-line-1-ref.html >+++ b/layout/reftests/text/pre-line-1-ref.html >@@ -3,6 +3,7 @@ > <head> > <style> > span { background:yellow; } >+body { line-height: 50px; } Why 50px? This makes the test taller than my laptop-screen, which is probably bad. Probably worth making the line-height approximately equal to the font-size. Maybe set "font-size: 10px; line-height: 12px;" or something like that? >diff --git a/layout/reftests/text/reftest.list b/layout/reftests/text/reftest.list >--- a/layout/reftests/text/reftest.list >+++ b/layout/reftests/text/reftest.list >@@ -25,7 +25,7 @@ HTTP(..) load ligature-with-space-1.html > fails-if(cocoaWidget||winWidget) HTTP(..) == lineheight-metrics-1.html lineheight-metrics-1-ref.html # bug 657864 > == lineheight-percentage-1.html lineheight-percentage-1-ref.html > skip-if(B2G) == long-1.html long-ref.html >-== pre-line-1.html pre-line-1-ref.html >+fuzzy-if(Android,255,44) == pre-line-1.html pre-line-1-ref.html What's causing the remaining fuzziness, after the line-height tweak? Worth filing a followup bug on that, and annotating it here. (Also, I imagine the exact amount of fuzziness will change after you make the font-size/line-height tweak I suggested above, so you probably want to see how much fuzziness (if any) we actually need now. >diff --git a/layout/reftests/text/subpixel-lineheight-1a.html b/layout/reftests/text/subpixel-lineheight-1a.html >--- a/layout/reftests/text/subpixel-lineheight-1a.html >+++ b/layout/reftests/text/subpixel-lineheight-1a.html >@@ -7,6 +7,7 @@ > body { > text-rendering: optimizeLegibility; > font-size: 14.5px; >+ line-height: 50px; As dbaron said, probably worth avoiding mucking with the line-height of this test, since it appears to be testing line-height. I'd prefer you just tweak the existing (already-huge!) fuzzy-if annotation for this test. (It's currently labeled as having 603 fuzzy pixels --"fuzzy-if((Android||B2G),231,603)") http://mxr.mozilla.org/mozilla-central/source/layout/reftests/text/reftest.list#53

(In reply to Jonathan Kew (:jfkthame) from comment #20) > (In reply to Ian Barlow (:ibarlow) from comment #0) > > We have an opportunity to be among the first to use a brand new open source > > typeface, commissioned by Intel and designed by Monotype, called Clear Sans. > > What's the license involved here? The fonts themselves seem to just have a > standard Monotype copyright notice and Monotype EULA link. The fonts are open source, with an Apache 2.0 License

(In reply to Daniel Holbert [:dholbert] from comment #24) > I'd prefer you just tweak the existing (already-huge!) fuzzy-if annotation > for this test. (It's currently labeled as having 603 fuzzy pixels > --"fuzzy-if((Android||B2G),231,603)") > http://mxr.mozilla.org/mozilla-central/source/layout/reftests/text/reftest. > list#53 Actually, I take that back -- it looks like there are ~3737 fuzzy pixels after this patch, which is an order of magnitude higher than the existing annotation... Probably better to just split the existing "fuzzy-if" annotation into: fails-if(Android) fuzzy-if(B2G,231,603) and then file a followup and CC karl, who added this test back in bug 605043.

try run with patches for all the retest failures. Still failing reftest 1 though. https://tbpl.mozilla.org/?tree=Try&rev=f5a5c6df7007

with these 4 patches reftests are passing locally. Here's a try run to confirm: https://tbpl.mozilla.org/?tree=Try&rev=d27c32d09de0

It seems like these patches all (or mostly) add "line-height: 50px", which makes a lot of these tests several times taller than they otherwise were. (pushing potentially-important reftest content off of the reftest snapshot, especially on small screens) Is there any reason you picked 50px in particular? I'd prefer that you set the line-height to a more normal-sized value (and set font-size while you're at it, to make sure you don't end up with overlapping lines), as I noted in comment 24.

(In reply to Ian Barlow (:ibarlow) from comment #25) > (In reply to Jonathan Kew (:jfkthame) from comment #20) > > (In reply to Ian Barlow (:ibarlow) from comment #0) > > > We have an opportunity to be among the first to use a brand new open source > > > typeface, commissioned by Intel and designed by Monotype, called Clear Sans. > > > > What's the license involved here? The fonts themselves seem to just have a > > standard Monotype copyright notice and Monotype EULA link. > > The fonts are open source, with an Apache 2.0 License I think there needs to be some documentation of this to accompany the files we actually check in to our tree. The TTF files themselves just contain a copyright notice: "Font software Copyright (c) 2012 Intel Corporation. All rights reserved." and a license link: "http://www.monotypeimaging.com/html/license.aspx" that points to a standard Monotype EULA for proprietary fonts. If they're actually being distributed (by Intel/Monotype, and then by us) under an Apache 2.0 license, this needs to be clarified within our distribution. Also, is there an "upstream" location where we can interact with this as an open-source project? (Just searching for "Clear Sans font" or similar isn't very helpful.)

(In reply to Daniel Holbert [:dholbert] from comment #30) > It seems like these patches all (or mostly) add "line-height: 50px", which > makes a lot of these tests several times taller than they otherwise were. > (pushing potentially-important reftest content off of the reftest snapshot, > especially on small screens) > > Is there any reason you picked 50px in particular? No, I just pulled a number out of the air. > I'd prefer that you set > the line-height to a more normal-sized value (and set font-size while you're > at it, to make sure you don't end up with overlapping lines), as I noted in > comment 24. Will do. Just wanted to get all the tests passing before and into a known good state before going back to address review comments.

(In reply to Jonathan Kew (:jfkthame) from comment #31) > > The fonts are open source, with an Apache 2.0 License > > I think there needs to be some documentation of this to accompany the files > we actually check in to our tree. The TTF files themselves just contain a > copyright notice: > > "Font software Copyright (c) 2012 Intel Corporation. All rights reserved." > > and a license link: > > "http://www.monotypeimaging.com/html/license.aspx" > > that points to a standard Monotype EULA for proprietary fonts. If they're > actually being distributed (by Intel/Monotype, and then by us) under an > Apache 2.0 license, this needs to be clarified within our distribution. > > Also, is there an "upstream" location where we can interact with this as an > open-source project? (Just searching for "Clear Sans font" or similar isn't > very helpful.) Let me ask the people who provided the typeface to us and get back to you. Clear Sans hasn't actually been released to the world yet which is probably why you can't find it. Stay tuned. In the mean time can we still try to land this in Nightly to get more people trying it?

(In reply to Brad Lassey [:blassey] from comment #32) > Will do. Just wanted to get all the tests passing before and into a known > good state before going back to address review comments. Keep in mind that in some cases, this large line-height might've been pushing the "interesting part" of the reftest off the bottom of the screen, and therefore passing trivially (since the potentially-mismatching part wasn't part of the canvas that got compared). If the reftests start failing again when you switch to a lower line-height value, that could be why. (Hopefully they'll still pass, though. :))

Comment on attachment 762626 [details] [diff] [review] patch for reftest 4 >+++ b/layout/reftests/svg/svg-integration/reftest.list >+fuzzy-if(true,140,140) == clipPath-html-05.xhtml clipPath-html-05-ref.xhtml # Bug 776089 >+fuzzy-if(true,140,140) == clipPath-html-05-extref.xhtml clipPath-html-05-ref.xhtml # Bug 776089 Replace "fuzzy-if(true," with "fuzzy(", per beginning of comment 24. >+++ b/layout/reftests/text/reftest.list >+fails-if(Android) fuzzy-if(B2G,231,603) == subpixel-lineheight-1a.html subpixel-lineheight-1b.html Please file a followup on this failing test (w/ a dependency on this bug here) and mention it at the end of this line. r=me with that.

Actually, after reviewing https://developer.mozilla.org/en-US/docs/Web/CSS/line-height , I remembered that the preferred way to set line-height is with a unitless value, which is interpreted as a multiple of the font-size. So in general, rather than specifying "font-size: 10px; line-height: 12px", you should just be able to do "line-height: 1.2", which will be evaluated as a multiple of whatever the current font-size is.

(In reply to Ian Barlow (:ibarlow) from comment #35) > Let me ask the people who provided the typeface to us and get back to you. > Clear Sans hasn't actually been released to the world yet which is probably > why you can't find it. > > Stay tuned. In the mean time can we still try to land this in Nightly to get > more people trying it? I'm not an authority on licensing issues, but FWIW, my interpretation - given that the metadata in the font files themselves does NOT indicate any permission to distribute - would be that we shouldn't be landing these fonts in a public tree such as mozilla-central (which amounts to a form of distribution) or pushing them out in Nightly (which is unquestionably distribution) without an accompanying statement of the license terms that are applicable. (If the font family "hasn't actually been released to the world yet" I have to wonder whether the owners would be comfortable with us pre-empting such a release by distributing it through our channels. I guess the key question is whether they HAVE passed the font to us under an Apache 2.0 license - which would grant us the right to distribute, etc - or have they expressed their INTENTION to release the fonts under an Apache license at some future point, in which case we may not CURRENTLY have any right to distribute, even if they've shared the fonts with us prior to such a release actually happening.)

now using unit-less line height

unitless line-heights. Strangely, changing this to unitless line-heights caused a 1 pixel horizontal line of slightly lighter blue at the border of the div's 1 pixel border and it's fill. Corrected for this with a fuzzy-if. That didn't occur with the previous patch.

Comment on attachment 762912 [details] [diff] [review] patch for reftest 1 The reftest failures in these two Reftest-1 tests might both be real gecko rounding bugs, but they're orthogonal to what the tests are trying to check, and your patch is minimal enough (and doesn't have an impact on what the tests are trying to check) that I'm OK with it. [Ideally it'd be nice to keep a second copy of these tests, in their original form, marked as "fails-if(Android)" and file a followup bug on figuring out what's going on. But in these cases I don't think that's high-priority enough that anyone would get to it soon, so it's not really high-priority enough for me to gate my r+ on it.]

again, unitless. I'm seeing failures on the rtl comparisons to about:blank locally, though those didn't fail on my last try push. Pushing to try again to confirm.

Comment on attachment 762947 [details] [diff] [review] patch for reftest 3 (In reply to Brad Lassey [:blassey] from comment #44) > Strangely, changing this to unitless line-heights caused a 1 pixel > horizontal line of slightly lighter blue at the border of the div's 1 pixel > border and it's fill. It is almost exactly what occurred in the failure, though; so the line-height "fix" isn't really helping here, unfortunately. :-/ Good news, though -- there's some CSS hack we can use to fix this. If you add shape-rendering="crisp-edges" to the SVG, it *should* fix this. If that doesn't work, then there's another incantation -- but I'm pretty sure that's it. Basically what's happening is, the SVG's <rect> is ending up at a fractional pixel-value (why, I'm not sure -- probably due to some quirk of the font's default size or something, so after N lines, that's just the y-position we're at), and we gladly render it at that fractional pixel-value, with a fuzzy edge. Whereas, we render things like borders and backgrounds at pixel-snapped positions, IIRC, so the reference case doesn't have the fuzzy line. So: please swap this out for shape-rendering="crisp-edges" assuming that works.

(In reply to Daniel Holbert [:dholbert] from comment #47) > It is almost exactly what occurred in the failure, though; so the > line-height "fix" isn't really helping here, unfortunately. :-/ (that is to say -- the single lighter blue line that you're correcting with "fuzzy" is essentially what was causing the original test-failure for this reftest, in comment 8)

I'm a little uneasy about how many of these test-failures are related to bullets on empty lines being positioned a fraction of a pixel too low. (The first failure in reftest-1, and the majority of the failing tests in reftest-2, at least.) I think that might be a real bug, and we're hitting it in enough of these tests that it merits a dedicated followup bug, I think. (Might still be fine to paper over it with "line-height", but we should at least track it somewhere.) Could you file a followup with a list of the bullet-related reftests that are being tweaked here? Hopefully someone (maybe a contributor) can dive in and see what's going wrong there (especially after we're shipping the font, at which point maybe this will be reproducible on desktop if you use the same font).

Comment on attachment 762954 [details] [diff] [review] patch for reftest 2 So -- I'm trying to understand what might be causing the failures before r+'ing a line-height paper-over, to be sure we're not covering up any bugs in gecko and/or the new font. >+++ b/layout/reftests/bugs/411059-1.html This failure is very odd. AFAICT, the only visual difference here is that the third chunk (<p class="letterspace"> in the testcase) renders slightly taller than the corresponding chunk in the reference case, "<p><span class="space">a</span></p>". And the only source-file differences in the testcase vs. the reference case are: (a) they have different whitespace characters (tab in testecase, spaces in reference case) (b) they have different horizontal-spacing properties (letter-spacing:2px in testcase, padding: 0 2px in reference case) I don't think either of those should affect the vertical size of that chunk, but one of them apparently is affecting it. I slightly wonder if some of this font's whitespace characters are taller (in terms of their effect on the line-height) than others...? (e.g. if the tab character is extra tall) Is that possible? If not, I don't understand what could be causing the rendering difference here. >diff --git a/layout/reftests/bugs/421436-1a.html b/layout/reftests/bugs/421436-1a.html >--- a/layout/reftests/bugs/421436-1a.html Here, too, we've got some weirdness w/ "how tall is whitespace" -- this last three lines of this test compare the height of "character, blank line, character" to the height of "character, visibility:hidden character, character". It seems bizarre to me that these wouldn't match, but they don't -- the snapshot shows the testcases (with the "true" blank line) being shorter than the reference (which has a visibility:hidden character). I don't know enough about text layout to know what could be causing this and how serious of a bug (in gecko or in the font) it might be. I should probably defer to jfkthame about whether fonts can do anything weird to trigger this sort of thing, and how concerned we should be. >diff --git a/layout/reftests/bugs/428810-1-ltr-insets-ref.html b/layout/reftests/bugs/428810-1-ltr-insets-ref.html >--- a/layout/reftests/bugs/428810-1-ltr-insets-ref.html >+++ b/layout/reftests/bugs/428810-1-ltr-insets-ref.html > <div style="margin-left: 32px; display: list-item;"> As noted in comment 49, these bullet-rendering tests probably merit a followup (w/ test author CC'd), at least. >+++ b/layout/reftests/bugs/495385-1-ref.html Sorry... I don't understand what's going wrong in the 495385-1*.html tests. It looks like every test (c, d, and e) that ends with "Hello</body>" has a slightly _taller_ border on the div than every file that has a newline between Hello and </body>. (-ref, a, and b) That newline is the only operative difference that I can see towards the ends of the source-HTML in 495385-1c.html vs. 495385-1-ref.html which could conceivably have an impact on the positioning of that bottom border. But, I don't understand why they'd have that impact, unless the font is choosing to render lines-without-whitespace as shorter Seems likely to be a bug. This is a weird enough symptom that I'd lean towards marking these tests as fails-if(Android) and filing a followup on them, rather than papering over with line-height, in this case. Again, I'm curious if jfkthame or anyone else has any thoughts about how a font could be triggering this difference. >diff --git a/layout/reftests/bugs/507187-1.html b/layout/reftests/bugs/507187-1.html >--- a/layout/reftests/bugs/507187-1.html >+++ b/layout/reftests/bugs/507187-1.html >@@ -1,1 +1,1 @@ >-<body><ul><li><a></a></li><li><a></a></li><li><a></a></li><li><a></a></li></ul></body></html> >+<body style="line-height:1.2;"><ul><li><a></a></li><li><a></a></li><li><a></a></li><li><a></a></li></ul></body></html> (see above RE bullet-rendering tests)

needinfo=jfkthame to see if he has any thoughts on the things mystifying me in comment 50. (To see the mis-renderings, use the TBPL link from comment 9, click R3, and then click "open reftest analyzer")

(In reply to Daniel Holbert [:dholbert] from comment #50) > Comment on attachment 762954 [details] [diff] [review] > patch for reftest 2 > > So -- I'm trying to understand what might be causing the failures before > r+'ing a line-height paper-over, to be sure we're not covering up any bugs > in gecko and/or the new font. No worries, I'd like to understand the failures as well. We're just solidly into layout nuances that I don't understand at this point, so I've resorted to throwing changes at it until it passes.

(In reply to Daniel Holbert [:dholbert] from comment #50) > >+++ b/layout/reftests/bugs/411059-1.html > > This failure is very odd. AFAICT, the only visual difference here is that > the third chunk (<p class="letterspace"> in the testcase) renders slightly > taller than the corresponding chunk in the reference case, "<p><span > class="space">a</span></p>". And the only source-file differences in the > testcase vs. the reference case are: > (a) they have different whitespace characters (tab in testecase, spaces in > reference case) > (b) they have different horizontal-spacing properties (letter-spacing:2px > in testcase, padding: 0 2px in reference case) > > I don't think either of those should affect the vertical size of that chunk, > but one of them apparently is affecting it. I slightly wonder if some of > this font's whitespace characters are taller (in terms of their effect on > the line-height) than others...? (e.g. if the tab character is extra tall) > Is that possible? If not, I don't understand what could be causing the > rendering difference here. I don't see how that could be it; the line-height should be based only on the font-wide line-spacing metrics exposed through nsFontMetrics. I checked the ClearSans font to confirm that its <space> glyph doesn't have a (spurious) huge glyph bounding box that might affect anything, and it doesn't have a <tab> glyph at all. (IIRC, that shouldn't matter as we don't attempt to draw <tab> as a normal printable character anyway.) I'll attempt to reproduce some of these failures locally, with extra logging or even a debug build, to try and understand what's going on. Leaving needinfo? on myself for now, pending further investigation...

On my Nexus tablet, the line-height discrepancy we see in 421436-1, for example, seems to be dependent on the font size: at 16px (i.e. the default), I get the same discrepancy as we see on TBPL, but at 14px or 18px the testcase and reference match as expected. This supports the theory that we're hitting some kind of inconsistency in how rounding (of line heights, or at a lower level, of the font metrics that they depend on). We could presumably hack around the specific test failures by changing the font size, but that would be just as likely to cause fresh breakage for some other font, so it really isn't a solution. I note that the problem does -not- occur on Firefox desktop/OS X if I install and use the Clear Sans font there. (The font-metrics code is platform-specific, so this isn't too surprising.) Time to look more closely at the metrics we're computing in the FT2 backend.

It looks like the problem arises when the code at http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFT2Utils.cpp#120 replaces the maxAscent and/or maxDescent value we got from FreeType's ascender/descender fields, which give us pixel-rounded values derived from the hhea table metrics, with a slightly larger fractional-pixel value based on the OS/2 table. In particular, depending how the rounding works out, we may replace -one- of the values, but not the other, with the result that we have a rounded maxAscent but a non-rounded maxDescent, or vice versa. I'm experimenting with a patch to apply rounding to the values here, to maintain better consistency with the values we use when these conditionals are false. In initial local testing, at least, this seems to prevent the discrepancy we've been running into with Clear Sans. I've pushed a tryserver job (https://tbpl.mozilla.org/?tree=Try&rev=b65f54e96e30) to see if this resolves some of the orangeness.

With this font-metrics fix (and *without* the various reftest patches above), the only test failure I'm seeing is in text/pre-line-1.html. The failure there is a discrepancy between the width of the background we paint for a <span> that is broken by a forced <br>, and the background we paint for its containing <div>. (It's actually the reference rather than the testcase itself that shows the glitch.) This issue is not mobile-specific; I can reproduce it in the same testcase on OS X desktop by installing and using the Clear Sans font, and I can also reproduce with other fonts (e.g. the Mac's standard Helvetica) if I adjust font sizes to get it "just right" - it's dependent on the precise size of the font, and on the horizontal position within the page. Looks like a pixel-rounding issue where we're not treating the <span> and its containing <div> in a consistent manner. Given that this is a pre-existing bug that just happens to be tickled by the Clear Sans font at 16px, I propose to just annotate it as fuzzy and file a separate bug to investigate it further (if we care sufficiently...it doesn't look too critical to me).

Comment on attachment 755509 [details] [diff] [review] patch Marking this as r-, because I don't think it should land in Nightly as-is, until we've addressed the issues in comment 14, and most importantly the license issue (comment 31, comment 42).

Comment on attachment 763689 [details] [diff] [review] annotate unreliable reftest text/pre-line-1 as fuzzy on Android r=me on the pre-line-1 reftest annotation

Comment on attachment 762954 [details] [diff] [review] patch for reftest 2 [r-minusing the reftest-2 patch, since I'm not comfortable with it as-is per comment 50, and it sounds like jfkthame's platform fix makes it unnecessary.]

Comment on attachment 763688 [details] [diff] [review] consistently use pixel-rounded values for maxAscent/Descent in the FT2 font metrics Obsoleting this patch as I've moved it to a separate bug 883997, marked as blocking this one.

Oops - I failed to change the bug number in the commit message when moving the platform patch to bug 883997 (see comment 62), and hence this bug got resolved prematurely when it merged from inbound. Re-opening; sorry about the churn. :(

Another issue here that we should fix before landing these fonts is that they're seriously bloated. The .ttf files are around 300K apiece, of which roughly half is a huge 'kern' table; e.g. listing the tables in ClearSans-Regular.ttf shows: tag checksum length offset ---- ---------- ------- ------- GPOS 0x0bd6756f 29928 276916 GSUB 0xb139c116 146 306844 LTSH 0x9478b8fa 883 3992 OS/2 0xf6085e33 96 456 VDMX 0x733b7aa0 1504 4876 cmap 0x4fbf32f1 2342 21416 cvt 0x0b781cf1 476 26616 fpgm 0x95c07f00 2384 23760 gasp 0x00070007 12 276904 glyf 0x4e71c82d 70436 28852 hdmx 0x0a1e99b7 15036 6380 head 0x01201a75 54 332 hhea 0x119907b4 36 388 hmtx 0x0a728851 3438 552 kern 0x1b1237de 163266 99288 loca 0x053047c6 1760 27092 maxp 0x05c90364 32 424 name 0x753776e0 6267 262556 post 0x31ad905d 8079 268824 prep 0xd82d684f 470 26144 The 'kern' table is redundant as far as Gecko is concerned, as the 'GPOS' table also has a 'kern' feature, which is what we'll actually use. (AFAICT, it contains the same kerning data, but in a -far- more compact form, as it can make use of glyph classes rather than listing every pair individually.) The legacy 'kern' table is presumably intended for older applications that don't support the OpenType layout features, but only the simple pair-kerning data; but for Firefox, it serves no purpose. Furthermore, the 'kern' table is actually invalid according to the OpenType spec (and will be dropped from the file if processed through OTS, for example). It attempts to define some 27208 kerning pairs, but the subtable format it is using cannot hold more than about 10923 before the subtable length field will overflow. So two issues, really: the upstream project needs to address the fact that they're creating a broken 'kern' table; but for Gecko, we can simply strip that table from the file altogether with no loss of functionality.

Jonathan, can you either strip the kern table from these fonts? or better yet provide instructions on how to do it? Ian, I think we should get fonts with the proper licensing before committing them. Can you handle that?

Ok. Sent out an email a few days ago, just waiting to hear back. Stay tuned.

(In reply to Brad Lassey [:blassey] from comment #66) > Jonathan, can you either strip the kern table from these fonts? or better > yet provide instructions on how to do it? Yes - I have a patch that strips them, but if we end up getting fresh copies with updated license info, it won't apply any more so I won't post it yet, pending clarification of that aspect. FTR, the procedure I used to strip the kern tables from the .ttf fonts: cd mobile/android/fonts/ for f in ClearSans*.ttf; do ttx -x kern $f; rm $f; ttx `basename -s .ttf $f`.ttx; done rm *.ttx (This uses the TTX tool, from http://sourceforge.net/projects/fonttools/.)

Just got a new set of files from Intel, they are attached here. Jonathan can you take a look at the licensing info in the files and see if they look more appropriate to you?

Looks OK to me. The files still carry Intel's copyright notice: "Font software Copyright (c) 2012 Intel Corporation. All rights reserved." but in addition they now contain a license statement: "Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0. Unless required by applicable law or agreed to in writing, Licensor provides the Work (and each Contributor provides its Contributions) on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied, including, without limitation, any warranties or conditions of TITLE, NON-INFRINGEMENT, MERCHANTABILITY, or FITNESS FOR A PARTICULAR PURPOSE. You are solely responsible for determining the appropriateness of using or redistributing the Work and assume any risks associated with Your exercise of permissions under this License. See the License for the specific language governing permissions and limitations under the License." and a link to the Apache site for the full license text: http://www.apache.org/licenses/LICENSE-2.0.html So AFAICT that should be fine; cc'ing Gerv to double-check that we're OK landing the files into our tree, given that licensing info. I notice the files still contain the huge (and invalid) 'kern' table, as noted in comment 65, so we'll be wanting to remove this before shipping them. The Apache license permits this, but requires us to "cause any modified files to carry prominent notices stating that You changed the files", so we'll need to make sure we abide by that clause when modifying them.

Ping Gerv? ^

Yep, no problem landing files with that licence statement. You can add "Modified by Mozilla" to the end of the field that contains the licensing statement. For bonus points, put "Modified by Mozilla to remove redundant <kern> table" or whatever it is you do, but that's not necessary. (Happy to see even more open source font goodness :-) Gerv

pushed the 3 reftest patches, the clear sans patch and the new fonts with cleaned up font tables to try: https://tbpl.mozilla.org/?tree=Try&rev=32efcd64b67e fingers crossed

I don't think you need those older reftest patches any more (since bug 883997 fixed the metrics-rounding code); only the one for text/pre-line-1 should be needed.

cool, pushed just that then: https://tbpl.mozilla.org/?tree=Try&rev=93b1a6f00a37

Don't forget that before landing, we'll also need to add a statement noting that we have modified the fonts, as required by the Apache license - see comment 70 (last para) and comment 72.

How would one do that?

By modifying the relevant entries in the <name> table when the font is dumped to .ttx format, before re-compiling it into the new .ttf. If you're not comfortable doing that, I could work up a script to update them tomorrow.

Here's a simple shell script (using ttx and perl) to add appropriate notes to the font 'name' tables at the same time as stripping the unwanted 'kern' tables. Just run this in a directory with all the ClearSans-*.ttf files and it should do the job.

However, IIRC your tryserver runs from comments 73 and 75 (apparently no longer accessible, since try was reset) seemed to be showing some new failures, so we'll need to look into those.

added your script, ran it and set the results to try: https://tbpl.mozilla.org/?tree=Try&rev=9191545191f3

Yup, new round of failures: REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.157:30059/tests/layout/reftests/svg/text-language-00.xhtml | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.157:30059/tests/layout/reftests/svg/text-language-01.xhtml | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.170:30269/tests/layout/reftests/bugs/421234-1.html | image comparison (==), max difference: 255, number of differing pixels: 86 REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.170:30269/tests/layout/reftests/bugs/488685-1.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.170:30269/tests/layout/reftests/canvas/text-font-lang.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | http://10.250.49.166:30216/tests/layout/reftests/bugs/289480.html#top | image comparison (==), max difference: 8, number of differing pixels: 1042

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #82) > Yup, new round of failures: > REFTEST TEST-UNEXPECTED-PASS | > http://10.250.49.170:30269/tests/layout/reftests/bugs/488685-1.html | image > comparison (==) That one's easy to address, at least. :) It's currently marked as "fails-if(Android)" -- looks like we can drop that.

The text-language failures appear to show that we're ignoring the "lang" attribute when picking fonts (i.e. we render each line identically, when we should render them differently due to their different "lang" values). Per bug 546813 comment 8, it sounds like this could indicate "lack of appropriate fonts"...?

Hi all, just an update that it looks like Intel and Monotype plan to change the name of this typeface from Clear Sans to something else before they release it officially. It would be great to still land what we have in Nightly to get people using it, but just a heads up that there will be an updated font file that replaces the one we have, with a new name.

Attached is the final version of the font files. Turns out they are actually still calling it Clear Sans, heh. Let's ship it!

Looks like these files still contain the humungous 'kern' table; let's not forget to strip that (using the script from comment 79, or similar) before we ship these.

One more friendly ping to push this along -- can someone take these new fonts (and strip out the kern table) and get them ready to land into Nightly? Thanks! :)

This fixes up the references to Open Sans in mobile CSS; also corrects a couple of places where the wrong name is used for Charis.

And this updates the default font preferences. Tryserver run is at https://tbpl.mozilla.org/?tree=Try&rev=4f4582e16dd2; we may well need to fix up a reftest or two before this can actually land safely.

Comment on attachment 820215 [details] [diff] [review] pt 2 - update CSS in mobile/android to refer to Clear Sans Review of attachment 820215 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/aboutReader.js @@ -84,4 @@ > value: "serif", > linkClass: "serif" }, > { name: fontTypeSample, > - description: gStrings.GetStringFromName("aboutReader.fontTypeOpenSans"), these were intentionally specific, but I do think that Ian wants to move to Clear Sans (and Serif is Charis, though may not always be). I'll need-info to him to confirm. ::: mobile/android/locales/en-US/chrome/aboutReader.properties @@ +14,3 @@ > # These are the names of the fonts that are used. > +aboutReader.fontTypeSerif=Charis SIL Compact > +aboutReader.fontTypeSansSerif=Clear Sans same here ::: mobile/android/themes/core/aboutFeedback.css @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > body { > -moz-text-size-adjust: none; > + font-family: "Clear Sans",sans-serif; should this just be sans-serif to pick up the default?

Ian, do you want reader, about:feedback and our error pages to use the default serif/sans-serif?

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #92) > Comment on attachment 820215 [details] [diff] [review] > pt 2 - update CSS in mobile/android to refer to Clear Sans > > Review of attachment 820215 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/chrome/content/aboutReader.js > @@ -84,4 @@ > > value: "serif", > > linkClass: "serif" }, > > { name: fontTypeSample, > > - description: gStrings.GetStringFromName("aboutReader.fontTypeOpenSans"), > > these were intentionally specific, but I do think that Ian wants to move to > Clear Sans (and Serif is Charis, though may not always be). I'll need-info > to him to confirm. Presumably these properties were originally created when we were just shipping a couple of faces for Reader mode, but not using them for general content. It'd be hard to justify the bloat of shipping a full set of (Clear Sans) fonts for web content and then a -separate- (Open Sans) set for reader mode. So if we're removing Open Sans from the package, a property called fontTypeOpenSans (which will necessarily have to refer to some other family) would seem... odd. > ::: mobile/android/themes/core/aboutFeedback.css > @@ +3,5 @@ > > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > body { > > -moz-text-size-adjust: none; > > + font-family: "Clear Sans",sans-serif; > > should this just be sans-serif to pick up the default? Yes, that would make sense. There's no real reason to repeat the specific name here.

Pushed a new tryserver job with the fonts unified into a single family: https://tbpl.mozilla.org/?tree=Try&rev=bf27cf8a159c Also removed the (redundant) font names from the mobile CSS, as suggested in comment 92. I'll wait to see how this goes before attaching updated patches; I suspect the same couple of reftest failures will still show up, but I'd like to see the try results before adding fuzz.

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #93) > Ian, do you want reader, about:feedback and our error pages to use the > default serif/sans-serif? Yes please -- assuming you mean Clear Sans as the default sans, and Charis as the default serif.

Updated font files to all belong to the Clear Sans family. Carrying forward r=blassey.

Updated mobile-CSS patch to remove redundant font names. Carrying forward r=blassey.

And updated patch 3 with fuzz to cover the two reftests that fail with the Clear Sans fonts. Carrying forward r=blassey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/16ea1b03d8fe https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8f868d1abe https://hg.mozilla.org/integration/mozilla-inbound/rev/440b13668e56

https://hg.mozilla.org/mozilla-central/rev/16ea1b03d8fe https://hg.mozilla.org/mozilla-central/rev/7e8f868d1abe https://hg.mozilla.org/mozilla-central/rev/440b13668e56

This landing caused a 2x regression in robopan, and a small improvement in tcheck2: https://groups.google.com/d/topic/mozilla.dev.tree-management/kDrS7-5lPjk/discussion We should investigate whether this is a real perf change, or if the new font just changed the layout on these pages in a way that the benchmark is sensitive to.

(In reply to Matt Brubeck (:mbrubeck) from comment #102) > This landing caused a 2x regression in robopan, and a small improvement in tcheck2: Now on Aurora: Subject: <Regression> Mozilla-Aurora - Robocop Pan Benchmark - Android 2.2 (Native) - 153% Message-ID: <20131031154247.9EE9410411F@cruncher.srv.releng.scl3.mozilla.com> Content-Type: text/plain; charset="us-ascii" Regression: Mozilla-Aurora - Robocop Pan Benchmark - Android 2.2 (Native) - 153% increase ----------------------------------------------------------------------------------------- Previous: avg 51165.517 stddev 6677.712 of 12 runs up to revision 15ced10dfc22 New : avg 129316.917 stddev 13952.989 of 12 runs since revision 60c6fd67470e Change : +78151.400 (153% / z=11.703) Graph : http://mzl.la/19V3YyB Changeset range: http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=15ced10dfc22&tochange=60c6fd67470e