657864 - rounding errors in (normal) line heights from font metrics

Status: ASSIGNED

(Core :: Layout: Text and Fonts, defect)

Description

[Karl Tomlinson (:karlt)]

I'll attach a reftest that uses a font with a known line-height to measure the line-height used by layout.  To keep things simple, the ascents and descents in the font are less than the line-height, and both the hhea and OS/2 line-heights are equal.

The em-size and line-height of the font in the test are both integer multiples of pixel sizes, so results are predictable, but ascents and descents are not integer to try out the computations.

It passes with gfxFT2FontBase.
Line height is 1 pixel too large with gfxDWriteFont.
Line height is 1 pixel too small with gfxGDIFont.
Line height is 4 pixels too large with gfxMacFont.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: reftest

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: DWrite part 1: be more careful about rounding metrics so as to preserve line-heights

Based on FT2 code, the key here is to round the line-height to nearest pixel rather than the leading (which is currently rounded up).

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: DWrite part 2: consider precision when rounding metrics up

A change to double precision variables in the previous patch introduced a rounding error caught by the ahem-metrics-1 reftest.

When rounding to nearest pixel above, we need to consider the precision of our calculations to avoid rounding up what is already pixel aligned.

Comment 4

[Karl Tomlinson (:karlt)]

Try build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b16abe234e10/try-win32/

Comment 5

[Karl Tomlinson (:karlt)]

Checked-in the reftest:
http://hg.mozilla.org/mozilla-central/rev/a884b23f7d16

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to Karl Tomlinson (:karlt) from comment #0)

> It passes with gfxFT2FontBase.
> Line height is 1 pixel too large with gfxDWriteFont.
> Line height is 1 pixel too small with gfxGDIFont.
> Line height is 4 pixels too large with gfxMacFont.

The 1-pixel discrepancies with DWrite and GDI can plausibly be attributed to rounding issues (and the patches here ought to fix the DW case), but the 4-pixel discrepancy with gfxMacFont is more worrying - it suggests that we are using a completely different basis for line-height metrics in this case.

One thing I notice in the markA-lineheight1500.ttf font is that the sTypo* metrics in the OS/2 table do not match the other ascent and descent metrics. This doesn't account for the Mac discrepancy, but it does seem like a potential pitfall with the current version of the test font.

I'd really like to do some further investigation of metrics on the various platforms before we make any firm decisions here.

Comment 7

[Karl Tomlinson (:karlt)]

I'm not aware of any reason why the sTypo* ascent and ascent should match hhea or win metrics.  I'd expect them to normally be different, but this font is a little different from typical latin-centric fonts in that the glyph-based metrics are smaller than the typo metrics.

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Karl Tomlinson (:karlt) from comment #7)
> I'm not aware of any reason why the sTypo* ascent and ascent should match
> hhea or win metrics.  I'd expect them to normally be different, but this
> font is a little different from typical latin-centric fonts in that the
> glyph-based metrics are smaller than the typo metrics.

Yes, in "normal" fonts the sTypo* metrics will often be different from the others, but I think for this special testing font we should probably ensure they're the same, to eliminate one potential source of discrepancies between backends.

Comment 9

[Karl Tomlinson (:karlt)]

Part of the point of the test is to check that line height metrics, not glyph bound metrics are used to determine line height.  If necessary, perhaps we could add further test fonts to check that other variations in non-line-height metrics don't affect the line-height.  For diagnosis of the discrepancies, another font could easily be used, if that helps.

Comment 10

[Phoenix]

Review and progress stalled?

Comment 11

[Jonathan Kew [:jfkthame]]

It hasn't been a high priority, that's all. Is this causing significant issues? As per comment 6, we definitely have some discrepancies, but I think it'd be best to address this across all platforms in a uniform way if possible.

Comment 12

[Phoenix]

(In reply to Jonathan Kew (:jfkthame) from comment #11)
> Is this causing significant issues?
It looks like that this causing incorrect rendering in SeaMonkey Preferences (see https://bug868495.bugzilla.mozilla.org/attachment.cgi?id=745243), it may be workarounded in SM code but it is always better to remove the source of the problem.

Comment 13

[Jonathan Kew [:jfkthame]]

I'd like to get this finished up and landed (sorry it's languished for so long!), and have been looking into the issues with GDI and OS X metrics.

Karl's patches for DWrite look fine, but when I tried rebasing them to current trunk and pushing a tryserver job, I'm seeing test failures in mochitest-metro-chrome:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_context_menu_tests.js | Top position is 212, expected between 220 and 230
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_context_menu_tests.js | Top position is 173, expected between 175 and 190
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_context_menu_tests.js | Top position is 94, expected between 95 and 110
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_context_menu_tests.js | Top position is 94, expected between 110 and 125
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_frame_content.js | selection test - Got waistcoat, expected started
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_frame_content.js | runTests: Task failed - Error: Timed out waiting for condition to be true at waitForCondition@chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/head.js:426
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_frame_content.js | selection test - Got .
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_frame_textarea.js | Y position is 654.2000122070312, expected between 675 and 690
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_frame_textarea.js | Y position is 654.2000122070312, expected between 675 and 690

(See https://tbpl.mozilla.org/?tree=Try&rev=86f5c4a0d5b3)

It looks to me like these tests are making fragile assumptions about default line-height, and where things end up on screen as a result; the patch here will make line-height slightly smaller in some cases, and this is resulting in failures.

:jimm, could you (or someone) take a look at these and see what you think? A possible fix here is for us to simply change the tests to update their "expected" results. Is that reasonable, or are the coordinates specified in the tests based on something more than empirical observation of what's currently happening?

Comment 14

[Jim Mathies [:jimm]]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> I'd like to get this finished up and landed (sorry it's languished for so
> long!), and have been looking into the issues with GDI and OS X metrics.
> 
> Karl's patches for DWrite look fine, but when I tried rebasing them to
> current trunk and pushing a tryserver job, I'm seeing test failures in
> mochitest-metro-chrome:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/
> browser_context_menu_tests.js | Top position is 212, expected between 220
> and 230
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/
> browser_context_menu_tests.js | Top position is 173, expected between 175
> and 190
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/
> browser_context_menu_tests.js | Top position is 94, expected between 95 and
> 110
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/
> browser_context_menu_tests.js | Top position is 94, expected between 110 and
> 125
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/
> browser_selection_frame_content.js | selection test - Got waistcoat,
> expected started
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/
> browser_selection_frame_content.js | runTests: Task failed - Error: Timed
> out waiting for condition to be true at
> waitForCondition@chrome://mochitests/content/metro/browser/metro/base/tests/
> mochitest/head.js:426
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/
> browser_selection_frame_content.js | selection test - Got .
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/
> browser_selection_frame_textarea.js | Y position is 654.2000122070312,
> expected between 675 and 690
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/
> browser_selection_frame_textarea.js | Y position is 654.2000122070312,
> expected between 675 and 690
> 
> (See https://tbpl.mozilla.org/?tree=Try&rev=86f5c4a0d5b3)
> 
> It looks to me like these tests are making fragile assumptions about default
> line-height, and where things end up on screen as a result; the patch here
> will make line-height slightly smaller in some cases, and this is resulting
> in failures.
> 
> :jimm, could you (or someone) take a look at these and see what you think? A
> possible fix here is for us to simply change the tests to update their
> "expected" results. Is that reasonable, or are the coordinates specified in
> the tests based on something more than empirical observation of what's
> currently happening?

Yeah that seems fine. If you want to post a rollup of the patches that caused this I can update the tests.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: use more consistent and properly-rounded line-height metrics for GDI and Mac font backends.

This is based on the WIP patch from bug 598900. It switches us to using the same 'sfnt' metrics (where available) on GDI and Mac, and incorporates similar rounding to the DWrite patch. With this, the line-height test works consistently across platforms, and the Cambria Math problem from bug 598900 is also resolved.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: reftest updates to take account of line-height rounding fixes.

This updates a few reftests that are affected by the line-height changes, according to tryserver results.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: [DWrite pt1+pt2] be more careful about rounding metrics so as to preserve line-heights

Here's the DWrite patch, rebased to trunk; this is the only piece relevant to Win8/Metro.

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8369407 [details] [diff] [review]
[DWrite pt1+pt2] be more careful about rounding metrics so as to preserve line-heights

Marking this r+, it's really Karl's original two patches to round the DWrite metrics carefully; I just rebased and folded them together.

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: use more consistent and properly-rounded line-height metrics for GDI and Mac font backends.

Tidied up the GDI & Mac patch slightly; no functional change. Note that bug 947650 will extend this with a special-case for math fonts.

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: update Windows Metro tests affected by changes to line spacing.

Here are some tweaks for the metro tests that seem to make everything pass on tryserver: https://tbpl.mozilla.org/?tree=Try&rev=e74c1b4c262b. The changes are totally ad-hoc: just figured out by experiment what works. I fear these tests will remain pretty fragile, liable to break in the event of any changes to the system's default fonts, etc.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 8371448 [details] [diff] [review]
update Windows Metro tests affected by changes to line spacing.

retriggers look good.

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8369405 [details] [diff] [review]
reftest updates to take account of line-height rounding fixes.

>+fuzzy-if(winWidget,255,50) == multiscripts-1.html multiscripts-1-ref.html

255 is a lot of fuzz.  Can you link to a failing try run, or explain what is
happening here, please?
Would adding an explicit, larger line-height to the <body>s of these tests
make them pass?

>+             font-family: arial, sans-serif;
>+          }
>+          div {
>+              position: fixed;
>+              top: 9.8px;

>-  <div style="position:fixed; left:100px; top:9.6px; font-family: serif">

>+              position: fixed;
>+              top: 10.2px;

>-  <div style="position:fixed; left:100px; top:10.4px; font-family: serif">

Why require Arial, remove the serif test, and make the positions closer to
integer?

Hopefully the serif test is no longer necessary with all the tests at
different sizes, but the positions change suggests a bug, and what was the
issue with the font that was not arial?

I didn't know that css3-fonts is proposing a change to case-insensitive family
matching.  I don't see gfxUserFontSet, at least, doing case-insensitive
matching, unless the style system is case-folding somewhere.

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to Karl Tomlinson (:karlt) from comment #22)
> Comment on attachment 8369405 [details] [diff] [review]
> reftest updates to take account of line-height rounding fixes.
> 
> >+fuzzy-if(winWidget,255,50) == multiscripts-1.html multiscripts-1-ref.html
> 
> 255 is a lot of fuzz.  Can you link to a failing try run, or explain what is
> happening here, please?

See the WinXP reftest runs in https://tbpl.mozilla.org/?tree=Try&rev=c78f8e24815d. (There are some other failures in that job as well, but this shows the multiscripts-1 issue.) The first testcase on the third line ends up with one pixel less of descent, for a total of 50 pixels that differ.

It fails only on WinXP, so I guess we could make the annotation more narrowly targeted; but it's not simply DWrite vs GDI, as Win7 unaccelerated doesn't fail. Presumably there's a difference between the font versions on XP vs Win7, but it didn't seem important enough to spend more time on.

> Would adding an explicit, larger line-height to the <body>s of these tests
> make them pass?

Perhaps; I could experiment a bit...

> 
> >+             font-family: arial, sans-serif;
> >+          }
> >+          div {
> >+              position: fixed;
> >+              top: 9.8px;
> 
> >-  <div style="position:fixed; left:100px; top:9.6px; font-family: serif">
> 
> >+              position: fixed;
> >+              top: 10.2px;
> 
> >-  <div style="position:fixed; left:100px; top:10.4px; font-family: serif">
> 
> Why require Arial,

This is for the sake of OS X, where the default sans font is Helvetica, and the Mac's Helvetica ends up running afoul of bug 832313. By preferring Arial, we avoid that and allow this test to behave as intended.

> remove the serif test,

Likewise, Times on OS X runs into bug 832313. I expect we could work around that similarly if desired.

> and make the positions closer to
> integer?

So that the rounding to device pixels still behaves in the intended way on a Retina screen.

> 
> Hopefully the serif test is no longer necessary with all the tests at
> different sizes, but the positions change suggests a bug, and what was the
> issue with the font that was not arial?
> 
> I didn't know that css3-fonts is proposing a change to case-insensitive
> family
> matching.  I don't see gfxUserFontSet, at least, doing case-insensitive
> matching, unless the style system is case-folding somewhere.

Hasn't it always been case-insensitive? I thought we applied lowercasing to family names everywhere before using them as lookup keys.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to Jonathan Kew (:jfkthame) from comment #23)
> See the WinXP reftest runs in
> https://tbpl.mozilla.org/?tree=Try&rev=c78f8e24815d. (There are some other
> failures in that job as well, but this shows the multiscripts-1 issue.) The
> first testcase on the third line ends up with one pixel less of descent, for
> a total of 50 pixels that differ.

OK, thanks.  I filed bug 970622 on that.

The proposed fuzz is fine.  Can you reference 970622, please?

> This is for the sake of OS X, where the default sans font is Helvetica, and
> the Mac's Helvetica ends up running afoul of bug 832313. By preferring
> Arial, we avoid that and allow this test to behave as intended.

Can you note that in the test, please?

> > remove the serif test,
> 
> Likewise, Times on OS X runs into bug 832313. I expect we could work around
> that similarly if desired.

I don't mind.

> 
> > and make the positions closer to
> > integer?
> 
> So that the rounding to device pixels still behaves in the intended way on a
> Retina screen.

I didn't know we ran tests on Retina, but I'd like to keep the test tighter on
1dppx platforms.  Can you use @media (min-resolution: 1dppx) and @media
(min-resolution: 2dppx) to set the value appropriately for the platform,
please?

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to Karl Tomlinson (:karlt) from comment #24)

> I didn't know we ran tests on Retina, but I'd like to keep the test tighter
> on
> 1dppx platforms.  Can you use @media (min-resolution: 1dppx) and @media
> (min-resolution: 2dppx) to set the value appropriately for the platform,
> please?

We don't (yet) run them on Retina as part of automation, but we'd like to be able to in due course - and many of us do run them locally, so tests that assume 1dppx and fail at higher resolution can be an annoyance. But yes, we can make this resolution-aware; I'll update it accordingly.

Comment 26

[Jonathan Kew [:jfkthame]]

Attachment: reftest updates to take account of line-height rounding fixes.

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 8369639 [details] [diff] [review]
use more consistent and properly-rounded line-height metrics for GDI and Mac font backends.

Sorry, I marked the wrong patch here.

I haven't looked through all the details here yet.
One thing I want to avoid is introducing bug 643781 by using ceil on other platforms, too.

Comment 28

[Karl Tomlinson (:karlt)]

Comment on attachment 8369639 [details] [diff] [review]
use more consistent and properly-rounded line-height metrics for GDI and Mac font backends.

There seem to be a few changes here that are not directly related to line
height rounding.  These would be easier to review in separate patches, but even
with several changes in one patch, the changeset comment still needs to list
exactly what is changing.

Can you make separate patches for the x glyph measurement and size adjust
refactoring in gfxGDIFont font, please, and explain the motivation for the
size adjust changes?

Attachment 520939 [details] observes that IE is using the equivalent of GDI metrics when
using DWrite with old fonts.  We should not be changing our GDI code to start
using ascent/descent metrics rounded in a way that we know do not match.  It
is our DWrite code that is inconsistent (bug 657864).  In order to replace the
GDI code with code that gets metrics directly from the SFNT tables, we need to
investigate how GDI does rounding to apply that to the SFNT table values.

Also, changing from os/2 win metrics to hhea metrics on GDI is going to make
GDI more different from DWrite and other browsers on Windows, for the sake of
making it more similar to Mac.  Consider Tahoma, for example, with
sTypoAscender = 1566 and sTypoLineGap = 59, but ascent = 2049 and lineGap = 0.

Windows systems will usually have fonts intended for Windows, and so the win
metrics are more likely to be correct (if the typo metrics are not requested
at least).  Font authors knew that win metrics were used on Windows and hhea
on Mac, so, if they have set different values, then they wanted different
behavior.

The remaining comments here are mostly about the Mac changes, because I didn't
analyse all the changes to the GDI code.

>+    // Round up maxAscent/Descent to whole pixels; take account of precision
>+    // of mFUnitsConvFactor to avoid rounding up by a full pixel.
>+    // (See karlt's patch for DWrite backend in bug 657864.)
>+    const gfxFloat precision = 1.0 / (1 << 23);
>+    aMetrics.maxAscent = ceil(ascent * (1.0 - precision));
>+    aMetrics.maxDescent = ceil(descent * (1.0 - precision));

>-static double
>-RoundToNearestMultiple(double aValue, double aFraction)
>-{
>-    return floor(aValue/aFraction + 0.5) * aFraction;
>-}
>-
> void gfxFont::CalculateDerivedMetrics(Metrics& aMetrics)
> {
>-    aMetrics.maxAscent =
>-        ceil(RoundToNearestMultiple(aMetrics.maxAscent, 1/1024.0));
>-    aMetrics.maxDescent =
>-        ceil(RoundToNearestMultiple(aMetrics.maxDescent, 1/1024.0));

The (1 << 23) maths I used in the DWrite patch was making a conservative
change to the code, but is not something that other platforms should be moving
towards, if we want consistent behaviour with other browsers.
(Probably rounding maxHeight to nearest and then selecting integer maxAscent
and maxDescent to sum to that would be good approach, but it depends what GDI
is doing.)  I don't know where 1024 came from, but changing from the old code
with 1024 to 1<<23 (even though the numbers are not directly comparable) would
be a step in wrong direction, so please leave something equivalent to the old
code until we know what to do for GDI.

>+    aMetrics.emHeight = floor(GetAdjustedSize() + 0.5);

http://www.w3.org/TR/css3-fonts/#font-style-matching says "Further
computations, e.g., by ‘em’ values in other properties, are based on the
‘font-size’ value that is used, not the one that is specified."
There is no change to the behaviour on Mac here, but there would be a change
if other platforms start using this code, so can you identify this in a
comment at least, please?

The GDI code was inconsistent about whether it used metrics.tmHeight -
metrics.tmInternalLeading or mAdjustedSize, but it looked like emHeight was
correct before.  InitMetricsFromSfntTables() will need to get this right
before GDI can use this code.

>+    aMetrics.internalLeading =
>+        std::max(aMetrics.maxHeight - aMetrics.emHeight, 0.0);
>+
>+    lineHeight = std::max(floor(lineHeight + 0.5), aMetrics.maxHeight);
>+    aMetrics.externalLeading =
>+        lineHeight - aMetrics.internalLeading - aMetrics.emHeight;

If maxHeight < emHeight and lineHeight < emHeight, then internalLeading is 0
and externalLeading is -ve.  

http://www.microsoft.com/typography/otspec/recom.htm gives formulae for
internalLeading and externalLeading on Windows, which I assume is where these
concepts come from and what GDI does, that would make internalLeading -ve and
externalLeading 0.
Limiting internalLeading to >= 0 may be sensible, but I suspect layout
will not handle -ve externalLeading well.

This code was probably fine before because few fonts would have had a -ve
hhea->lineGap, but the introduction of typo metrics makes such scenarios more
likely.  It's probably easiest to leave the typo metrics changes for a
separate patch.

>-    aMetrics.maxHeight = aMetrics.maxAscent + aMetrics.maxDescent;
>-
>-    if (aMetrics.maxHeight - aMetrics.emHeight > 0.0) {
>-        aMetrics.internalLeading = aMetrics.maxHeight - aMetrics.emHeight;
>-    } else {
>-        aMetrics.internalLeading = 0.0;
>-    }
>-
>-    aMetrics.emAscent = aMetrics.maxAscent * aMetrics.emHeight
>-                            / aMetrics.maxHeight;
>-    aMetrics.emDescent = aMetrics.emHeight - aMetrics.emAscent;

Metrics from InitMetricsFromPlatform still need maxHeight, internalLeading,
emAscent, and emDescent, or did I miss where they are calculated?

In gfxMacFont::InitMetricsFromPlatform()

>-    mMetrics.maxAscent = ::CTFontGetAscent(ctFont);
>-    mMetrics.maxDescent = ::CTFontGetDescent(ctFont);
>+    mMetrics.maxAscent = ceil(::CTFontGetAscent(ctFont));
>+    mMetrics.maxDescent = ceil(::CTFontGetDescent(ctFont));

Again, bug 643781 indicates that this is not the appropriate rounding strategy
if we want consistency.

Comment 29

[Khaled Hosny]

(In reply to Karl Tomlinson (:karlt) from comment #28)
> Windows systems will usually have fonts intended for Windows, and so the win
> metrics are more likely to be correct (if the typo metrics are not requested
> at least).  Font authors knew that win metrics were used on Windows and hhea
> on Mac, so, if they have set different values, then they wanted different
> behavior.

Is this true in with the widespread use of webfonts? I tend to think that for a web browser having a consistent cross-platform behavior is more important than respecting the incorrect behavior of some platform(s).

Comment 30

[Karl Tomlinson (:karlt)]

(In reply to Khaled Hosny from comment #29)
> (In reply to Karl Tomlinson (:karlt) from comment #28)
> > Windows systems will usually have fonts intended for Windows, and so the win
> > metrics are more likely to be correct (if the typo metrics are not requested
> > at least).  Font authors knew that win metrics were used on Windows and hhea
> > on Mac, so, if they have set different values, then they wanted different
> > behavior.
> 
> Is this true in with the widespread use of webfonts?

I don't know.  Many web fonts are of poor quality, as seen by the hinting instructions used with GDI, for example.  There is a limit to what we can do with bad fonts.  There may be an argument for having different behaviour for web and system fonts, but then there is an inconsistency between those situations.
The best thing we can do IMO is follow the spec, so that font authors who read the spec get what they expect.

> I tend to think that
> for a web browser having a consistent cross-platform behavior is more
> important than respecting the incorrect behavior of some platform(s).

Which behavior is "incorrect"?
Is using win metrics for line height really more incorrect than using hhea metrics for line height?  Each has a line gap value, and each are described as "platform-specific metrics" "constrained by backward compatibility requirements".
Are you advocating using only the typo metrics, even when USE_TYPO_METRICS is not set?

If fonts start setting USE_TYPO_METRICS, then we won't have this inconsistency, but we have this backward compat issue unfortunately, as well as compat with other browsers.

Comment 31

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #30)
> Each has a line gap value

Sorry, I was wrong here.

Win metrics don't specify a line gap, but there is a recommended formula for BTBD including win metrics for windows.

(In reply to Karl Tomlinson (:karlt) from comment #28)
> Consider Tahoma, for example, with
> sTypoAscender = 1566 and sTypoLineGap = 59, but ascent = 2049 and lineGap =
> 0.

And I was confused here too, so please ignore this.
Tahoma has matching hhea and win metrics.

Perhaps there is an argument for ignoring win metrics and using only hhea.

Has there been some analysis of Windows core web fonts to see what difference there would be if switching from win to hhea?

Would it be possible to switch DWrite over at the same time as GDI?
(This need not be in the same patch many of the changes here.)

Comment 32

[Khaled Hosny]

(In reply to Karl Tomlinson (:karlt) from comment #30)
> (In reply to Khaled Hosny from comment #29)
> > (In reply to Karl Tomlinson (:karlt) from comment #28)
> > > Windows systems will usually have fonts intended for Windows, and so the win
> > > metrics are more likely to be correct (if the typo metrics are not requested
> > > at least).  Font authors knew that win metrics were used on Windows and hhea
> > > on Mac, so, if they have set different values, then they wanted different
> > > behavior.
> > 
> > Is this true in with the widespread use of webfonts?
> 
> I don't know.  Many web fonts are of poor quality, as seen by the hinting
> instructions used with GDI, for example.  There is a limit to what we can do
> with bad fonts.  There may be an argument for having different behaviour for
> web and system fonts, but then there is an inconsistency between those
> situations.
> The best thing we can do IMO is follow the spec, so that font authors who
> read the spec get what they expect.
> 
> > I tend to think that
> > for a web browser having a consistent cross-platform behavior is more
> > important than respecting the incorrect behavior of some platform(s).
> 
> Which behavior is "incorrect"?

Using usWinAscent and usWinDescent to compute line height.

> Is using win metrics for line height really more incorrect than using hhea
> metrics for line height?  Each has a line gap value, and each are described
> as "platform-specific metrics" "constrained by backward compatibility
> requirements".
> Are you advocating using only the typo metrics, even when USE_TYPO_METRICS
> is not set?

Yes, because this what the spec says:
http://www.microsoft.com/typography/otspec/os2.htm#wa

“… Some applications use this value to determine default line spacing. This is strongly discouraged. The typographic ascender, descender and line gap fields in conjunction with unitsPerEm should be used for this purpose.”

Comment 33

[Karl Tomlinson (:karlt)]

(In reply to Khaled Hosny from comment #32)
> (In reply to Karl Tomlinson (:karlt) from comment #30)
> > Are you advocating using only the typo metrics, even when USE_TYPO_METRICS
> > is not set?
> 
> Yes, because this what the spec says:
> http://www.microsoft.com/typography/otspec/os2.htm#wa
> 
> “… Some applications use this value to determine default line spacing. This
> is strongly discouraged. The typographic ascender, descender and line gap
> fields in conjunction with unitsPerEm should be used for this purpose.”

That is what I wanted to do also when I filed bug 402473.
I fear there may be a number of changes required to layout if we change maxAscent/Descent values to what are really emAscent/Descent, but now that we already do that for some fonts with DWrite, perhaps some of those issues have been resolved.  I'm not sure because layout positioning choices change slowly due to compat issues.

Starting with typo metrics when USE_TYPO_METRICS is set sounds like a good step in the right direction.

Comment 34

[Elbart]

Chrome 37 is set to come with DirectWrite enabled, and it doesn't show the line-height problems of Firefox, as far as I noticed.