Last Comment Bug 877203 - Replace Open Sans with Clear Sans
: Replace Open Sans with Clear Sans
Status: RESOLVED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: Readability (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: Firefox 27
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on: 883997
Blocks: 877774 877783
  Show dependency treegraph
 
Reported: 2013-05-29 08:46 PDT by Ian Barlow (:ibarlow)
Modified: 2015-02-03 11:44 PST (History)
22 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.35 MB, patch)
2013-05-29 10:22 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch (1.35 MB, patch)
2013-05-29 11:16 PDT, Brad Lassey [:blassey] (use needinfo?)
jfkthame: review-
Details | Diff | Splinter Review
patch for reftest 4 (3.81 KB, patch)
2013-06-13 14:49 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for reftest 3 (2.59 KB, patch)
2013-06-13 19:29 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for reftest 2 (36.94 KB, patch)
2013-06-13 19:30 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for reftest 1 (2.87 KB, patch)
2013-06-13 20:58 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for reftest 4 (1.83 KB, patch)
2013-06-14 05:00 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for reftest 4 (3.11 KB, patch)
2013-06-14 05:02 PDT, Brad Lassey [:blassey] (use needinfo?)
dholbert: review+
Details | Diff | Splinter Review
patch for reftest 1 (2.90 KB, patch)
2013-06-14 13:46 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for reftest 3 (2.67 KB, patch)
2013-06-14 13:46 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for reftest 2 (38.24 KB, patch)
2013-06-14 13:47 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for reftest 1 (2.86 KB, patch)
2013-06-14 14:16 PDT, Brad Lassey [:blassey] (use needinfo?)
dholbert: review+
Details | Diff | Splinter Review
patch for reftest 3 (3.49 KB, patch)
2013-06-14 15:08 PDT, Brad Lassey [:blassey] (use needinfo?)
dholbert: review-
Details | Diff | Splinter Review
patch for reftest 2 (36.86 KB, patch)
2013-06-14 15:25 PDT, Brad Lassey [:blassey] (use needinfo?)
dholbert: review-
Details | Diff | Splinter Review
consistently use pixel-rounded values for maxAscent/Descent in the FT2 font metrics (1.71 KB, patch)
2013-06-17 11:30 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
annotate unreliable reftest text/pre-line-1 as fuzzy on Android (1.43 KB, patch)
2013-06-17 11:31 PDT, Jonathan Kew (:jfkthame)
dholbert: review+
Details | Diff | Splinter Review
New font files with updated licensing information (1.04 MB, application/x-zip-compressed)
2013-07-08 06:47 PDT, Ian Barlow (:ibarlow)
no flags Details
script to strip 'kern' tables and add "modified by Mozilla" notes to the fonts (1.15 KB, application/x-sh)
2013-07-17 07:23 PDT, Jonathan Kew (:jfkthame)
no flags Details
clearsans-1.00.zip (3.79 MB, application/zip)
2013-09-24 06:05 PDT, Ian Barlow (:ibarlow)
no flags Details
pt 1 - replace Open Sans fonts with Clear Sans v1.00 (with stripped kern tables) (765.82 KB, patch)
2013-10-22 02:08 PDT, Jonathan Kew (:jfkthame)
blassey.bugs: review+
Details | Diff | Splinter Review
pt 2 - update CSS in mobile/android to refer to Clear Sans (5.76 KB, patch)
2013-10-22 02:09 PDT, Jonathan Kew (:jfkthame)
blassey.bugs: review+
Details | Diff | Splinter Review
pt 3 - update font prefs for Android to refer to Clear Sans (7.00 KB, patch)
2013-10-22 02:11 PDT, Jonathan Kew (:jfkthame)
blassey.bugs: review+
Details | Diff | Splinter Review
pt 1 - replace Open Sans fonts with Clear Sans v1.00 (with stripped kern tables). (765.92 KB, patch)
2013-10-23 08:22 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
pt 2 - remove obsolete/redundant font names from CSS in mobile/android. (5.68 KB, patch)
2013-10-23 08:23 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
pt 3 - update font prefs for Android to refer to Clear Sans. (9.12 KB, patch)
2013-10-23 08:25 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review

Description Ian Barlow (:ibarlow) 2013-05-29 08:46:53 PDT
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.
Comment 1 Sriram Ramasubramanian [:sriram] 2013-05-29 10:07:56 PDT
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!!)
Comment 2 Ian Barlow (:ibarlow) 2013-05-29 10:08:43 PDT
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.
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2013-05-29 10:22:11 PDT
Created attachment 755477 [details] [diff] [review]
patch
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2013-05-29 11:16:48 PDT
Created attachment 755509 [details] [diff] [review]
patch

removes Open Sans font files from the APK
Comment 5 Aaron Train [:aaronmt] 2013-05-29 11:21:30 PDT
How's the final package size variance?
Comment 6 Phil Ringnalda (:philor, back in August) 2013-05-29 23:38:16 PDT
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.
Comment 7 :Margaret Leibovic 2013-05-30 11:21:14 PDT
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?
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2013-05-30 19:59:12 PDT
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.
Comment 9 Brad Lassey [:blassey] (use needinfo?) 2013-05-30 20:00:12 PDT
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
Comment 10 Brad Lassey [:blassey] (use needinfo?) 2013-05-30 20:01:18 PDT
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
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2013-05-30 20:04:18 PDT
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
Comment 12 Ian Barlow (:ibarlow) 2013-06-10 06:24:04 PDT
Any progress here? Would be great to get this into Nightly for some more testing asap.
Comment 13 Jonathan Kew (:jfkthame) 2013-06-13 09:16:32 PDT
(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 14 Jonathan Kew (:jfkthame) 2013-06-13 09:33:09 PDT
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.
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2013-06-13 09:40:29 PDT
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?
Comment 16 Daniel Holbert [:dholbert] 2013-06-13 10:01:31 PDT
(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.
Comment 17 Jonathan Kew (:jfkthame) 2013-06-13 10:04:20 PDT
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.
Comment 18 Brad Lassey [:blassey] (use needinfo?) 2013-06-13 13:42:17 PDT
Thanks guys, I misunderstood what was going on here.
Comment 19 Jonathan Kew (:jfkthame) 2013-06-13 14:36:11 PDT
(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.)
Comment 20 Jonathan Kew (:jfkthame) 2013-06-13 14:37:32 PDT
(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.
Comment 21 Brad Lassey [:blassey] (use needinfo?) 2013-06-13 14:49:27 PDT
Created attachment 762331 [details] [diff] [review]
patch for reftest 4
Comment 22 Brad Lassey [:blassey] (use needinfo?) 2013-06-13 14:50:01 PDT
(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 23 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2013-06-13 15:01:13 PDT
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 24 Daniel Holbert [:dholbert] 2013-06-13 15:32:26 PDT
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
Comment 25 Ian Barlow (:ibarlow) 2013-06-13 15:33:25 PDT
(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
Comment 26 Daniel Holbert [:dholbert] 2013-06-13 15:54:19 PDT
(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.
Comment 27 Brad Lassey [:blassey] (use needinfo?) 2013-06-13 19:29:18 PDT
Created attachment 762456 [details] [diff] [review]
patch for reftest 3

try run with patches for all the retest failures. Still failing reftest 1 though. https://tbpl.mozilla.org/?tree=Try&rev=f5a5c6df7007
Comment 28 Brad Lassey [:blassey] (use needinfo?) 2013-06-13 19:30:09 PDT
Created attachment 762457 [details] [diff] [review]
patch for reftest 2
Comment 29 Brad Lassey [:blassey] (use needinfo?) 2013-06-13 20:58:07 PDT
Created attachment 762489 [details] [diff] [review]
patch for reftest 1

with these 4 patches reftests are passing locally. Here's a try run to confirm:
https://tbpl.mozilla.org/?tree=Try&rev=d27c32d09de0
Comment 30 Daniel Holbert [:dholbert] 2013-06-14 00:01:10 PDT
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.
Comment 31 Jonathan Kew (:jfkthame) 2013-06-14 02:08:21 PDT
(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.)
Comment 32 Brad Lassey [:blassey] (use needinfo?) 2013-06-14 04:21:53 PDT
(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.
Comment 33 Brad Lassey [:blassey] (use needinfo?) 2013-06-14 05:00:06 PDT
Created attachment 762625 [details] [diff] [review]
patch for reftest 4
Comment 34 Brad Lassey [:blassey] (use needinfo?) 2013-06-14 05:02:36 PDT
Created attachment 762626 [details] [diff] [review]
patch for reftest 4
Comment 35 Ian Barlow (:ibarlow) 2013-06-14 07:25:05 PDT
(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?
Comment 36 Daniel Holbert [:dholbert] 2013-06-14 12:57:11 PDT
(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 37 Daniel Holbert [:dholbert] 2013-06-14 13:02:32 PDT
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.
Comment 38 Daniel Holbert [:dholbert] 2013-06-14 13:32:56 PDT
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.
Comment 39 Brad Lassey [:blassey] (use needinfo?) 2013-06-14 13:46:00 PDT
Created attachment 762888 [details] [diff] [review]
patch for reftest 1
Comment 40 Brad Lassey [:blassey] (use needinfo?) 2013-06-14 13:46:51 PDT
Created attachment 762889 [details] [diff] [review]
patch for reftest 3
Comment 41 Brad Lassey [:blassey] (use needinfo?) 2013-06-14 13:47:39 PDT
Created attachment 762890 [details] [diff] [review]
patch for reftest 2
Comment 42 Jonathan Kew (:jfkthame) 2013-06-14 14:13:48 PDT
(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.)
Comment 43 Brad Lassey [:blassey] (use needinfo?) 2013-06-14 14:16:54 PDT
Created attachment 762912 [details] [diff] [review]
patch for reftest 1

now using unit-less line height
Comment 44 Brad Lassey [:blassey] (use needinfo?) 2013-06-14 15:08:32 PDT
Created attachment 762947 [details] [diff] [review]
patch for reftest 3

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 45 Daniel Holbert [:dholbert] 2013-06-14 15:09:35 PDT
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.]
Comment 46 Brad Lassey [:blassey] (use needinfo?) 2013-06-14 15:25:11 PDT
Created attachment 762954 [details] [diff] [review]
patch for reftest 2

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 47 Daniel Holbert [:dholbert] 2013-06-14 15:34:22 PDT
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.
Comment 48 Daniel Holbert [:dholbert] 2013-06-14 15:36:21 PDT
(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)
Comment 49 Daniel Holbert [:dholbert] 2013-06-14 15:53:50 PDT
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 50 Daniel Holbert [:dholbert] 2013-06-14 16:59:19 PDT
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)
Comment 51 Daniel Holbert [:dholbert] 2013-06-14 17:08:37 PDT
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")
Comment 52 Brad Lassey [:blassey] (use needinfo?) 2013-06-15 14:52:10 PDT
(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.
Comment 53 Jonathan Kew (:jfkthame) 2013-06-17 02:26:39 PDT
(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...
Comment 54 Jonathan Kew (:jfkthame) 2013-06-17 05:03:25 PDT
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.
Comment 55 Jonathan Kew (:jfkthame) 2013-06-17 07:19:46 PDT
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.
Comment 56 Jonathan Kew (:jfkthame) 2013-06-17 10:57:38 PDT
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 57 Jonathan Kew (:jfkthame) 2013-06-17 11:30:53 PDT
Created attachment 763688 [details] [diff] [review]
consistently use pixel-rounded values for maxAscent/Descent in the FT2 font metrics
Comment 58 Jonathan Kew (:jfkthame) 2013-06-17 11:31:31 PDT
Created attachment 763689 [details] [diff] [review]
annotate unreliable reftest text/pre-line-1 as fuzzy on Android
Comment 59 Jonathan Kew (:jfkthame) 2013-06-17 11:36:13 PDT
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 60 Daniel Holbert [:dholbert] 2013-06-17 12:03:48 PDT
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 61 Daniel Holbert [:dholbert] 2013-06-17 12:21:43 PDT
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 62 Jonathan Kew (:jfkthame) 2013-06-17 12:29:58 PDT
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.
Comment 63 Ed Morley [:emorley] 2013-06-18 04:00:49 PDT
https://hg.mozilla.org/mozilla-central/rev/1bd6b04a535f
Comment 64 Jonathan Kew (:jfkthame) 2013-06-18 04:30:39 PDT
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. :(
Comment 65 Jonathan Kew (:jfkthame) 2013-06-18 06:01:20 PDT
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.
Comment 66 Brad Lassey [:blassey] (use needinfo?) 2013-06-18 12:45:35 PDT
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?
Comment 67 Ian Barlow (:ibarlow) 2013-06-18 12:59:15 PDT
Ok. Sent out an email a few days ago, just waiting to hear back. Stay tuned.
Comment 68 Jonathan Kew (:jfkthame) 2013-06-18 13:12:13 PDT
(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/.)
Comment 69 Ian Barlow (:ibarlow) 2013-07-08 06:47:39 PDT
Created attachment 772062 [details]
New font files with updated licensing information

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?
Comment 70 Jonathan Kew (:jfkthame) 2013-07-08 07:25:19 PDT
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.
Comment 71 Ian Barlow (:ibarlow) 2013-07-15 02:49:53 PDT
Ping Gerv? ^
Comment 72 Gervase Markham [:gerv] 2013-07-15 07:54:34 PDT
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
Comment 73 Brad Lassey [:blassey] (use needinfo?) 2013-07-15 11:34:33 PDT
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
Comment 74 Jonathan Kew (:jfkthame) 2013-07-15 11:52:14 PDT
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.
Comment 75 Brad Lassey [:blassey] (use needinfo?) 2013-07-15 11:58:48 PDT
cool, pushed just that then:
https://tbpl.mozilla.org/?tree=Try&rev=93b1a6f00a37
Comment 76 Jonathan Kew (:jfkthame) 2013-07-15 12:47:13 PDT
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.
Comment 77 Brad Lassey [:blassey] (use needinfo?) 2013-07-15 12:49:17 PDT
How would one do that?
Comment 78 Jonathan Kew (:jfkthame) 2013-07-15 14:14:49 PDT
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.
Comment 79 Jonathan Kew (:jfkthame) 2013-07-17 07:23:27 PDT
Created attachment 777107 [details]
script to strip 'kern' tables and add "modified by Mozilla" notes to the fonts

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.
Comment 80 Jonathan Kew (:jfkthame) 2013-07-17 07:25:35 PDT
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.
Comment 81 Brad Lassey [:blassey] (use needinfo?) 2013-07-19 05:54:06 PDT
added your script, ran it and set the results to try:

https://tbpl.mozilla.org/?tree=Try&rev=9191545191f3
Comment 82 Brad Lassey [:blassey] (use needinfo?) 2013-07-22 08:25:18 PDT
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
Comment 83 Daniel Holbert [:dholbert] 2013-07-22 10:00:03 PDT
(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.
Comment 84 Daniel Holbert [:dholbert] 2013-07-22 10:06:37 PDT
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"...?
Comment 85 Ian Barlow (:ibarlow) 2013-08-06 06:42:54 PDT
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.
Comment 86 Ian Barlow (:ibarlow) 2013-09-24 06:05:01 PDT
Created attachment 809147 [details]
clearsans-1.00.zip

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

Let's ship it!
Comment 87 Jonathan Kew (:jfkthame) 2013-09-24 09:13:59 PDT
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.
Comment 88 Ian Barlow (:ibarlow) 2013-10-15 13:21:48 PDT
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! :)
Comment 89 Jonathan Kew (:jfkthame) 2013-10-22 02:08:26 PDT
Created attachment 820214 [details] [diff] [review]
pt 1 - replace Open Sans fonts with Clear Sans v1.00 (with stripped kern tables)
Comment 90 Jonathan Kew (:jfkthame) 2013-10-22 02:09:55 PDT
Created attachment 820215 [details] [diff] [review]
pt 2 - update CSS in mobile/android to refer to Clear Sans

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.
Comment 91 Jonathan Kew (:jfkthame) 2013-10-22 02:11:24 PDT
Created attachment 820216 [details] [diff] [review]
pt 3 - update font prefs for Android to refer to Clear Sans

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 92 Brad Lassey [:blassey] (use needinfo?) 2013-10-22 02:15:14 PDT
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?
Comment 93 Brad Lassey [:blassey] (use needinfo?) 2013-10-22 02:17:49 PDT
Ian, do you want reader, about:feedback and our error pages to use the default serif/sans-serif?
Comment 94 Jonathan Kew (:jfkthame) 2013-10-22 02:41:35 PDT
(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.
Comment 95 Jonathan Kew (:jfkthame) 2013-10-22 05:32:43 PDT
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.
Comment 96 Ian Barlow (:ibarlow) 2013-10-22 05:56:52 PDT
(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.
Comment 97 Jonathan Kew (:jfkthame) 2013-10-23 08:22:35 PDT
Created attachment 821035 [details] [diff] [review]
pt 1 - replace Open Sans fonts with Clear Sans v1.00 (with stripped kern tables).

Updated font files to all belong to the Clear Sans family. Carrying forward r=blassey.
Comment 98 Jonathan Kew (:jfkthame) 2013-10-23 08:23:24 PDT
Created attachment 821036 [details] [diff] [review]
pt 2 - remove obsolete/redundant font names from CSS in mobile/android.

Updated mobile-CSS patch to remove redundant font names. Carrying forward r=blassey.
Comment 99 Jonathan Kew (:jfkthame) 2013-10-23 08:25:14 PDT
Created attachment 821038 [details] [diff] [review]
pt 3 - update font prefs for Android to refer to Clear Sans.

And updated patch 3 with fuzz to cover the two reftests that fail with the Clear Sans fonts. Carrying forward r=blassey.
Comment 102 Matt Brubeck (:mbrubeck) 2013-10-25 09:28:46 PDT
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.
Comment 103 Geoff Brown [:gbrown] 2013-10-31 09:15:27 PDT
(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

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