1293210 - add cap height support to nsFontMetrics

Status: RESOLVED FIXED

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

Description

[Jeremy Chen ([e-mail])]

Per https://drafts.csswg.org/css-inline/#sizing-initial-letters, we need cap height information to precisely calculate the font-size of a initial-letter. So, add cap height support to nsFontMetrics.

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: (wip) add cap height support to nsFontMetrics on mac platform.

MozReview-Commit-ID: JhiRCaCCq01

Comment 2

[Jeremy Chen ([e-mail])]

Comment on attachment 8781110 [details] [diff] [review]
(wip) add cap height support to nsFontMetrics on mac platform.

In this patch, I try to add cap height support on Mac platform first. I'm wondering:

1. Should I add support to os2 font in [1]?
2. Am I going to the wrong direction?
3. What kind of test should be added in this bug, so we could make sure the support of cap height is added properly?

Hi Jonathan, could you take a look and give me some feedback? Thank you.


[1] http://searchfox.org/mozilla-central/rev/9ec085584d7491ddbaf6574d3732c08511709172/gfx/thebes/gfxFont.cpp#3467

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #2)
> Comment on attachment 8781110 [details] [diff] [review]
> (wip) add cap height support to nsFontMetrics on mac platform.
> 
> In this patch, I try to add cap height support on Mac platform first. I'm
> wondering:
> 
> 1. Should I add support to os2 font in [1]?

Yes. In the great majority of cases, that's where we should be getting the metrics from. The platform-specific implementations serve as a fallback that should only be used if we have a font that a particular platform supports (such as Core Text on OS X) but where we can't find the data we need in standard OpenType tables.

> 2. Am I going to the wrong direction?

I'll put some comments inline in the patch...

> 3. What kind of test should be added in this bug, so we could make sure the
> support of cap height is added properly?

We don't generally have much testing at this low level of internal APIs. You can add capHeight to the (disabled) printf statements at the end of gfxMacFont::InitMetrics, for example, and enable this in a local build just to check that the values you're getting look reasonable; but beyond that, I'd most likely proceed to implement the actual layout feature that depends on this (i.e. initial-letter), and the reftests for that feature will have the effect of testing capHeight support. When it comes time to test initial-letter, you can test it with fonts such as Gentium where there is a significat difference between ascent and capHeight, to confirm that it is using the correct value.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8781110 [details] [diff] [review]
(wip) add cap height support to nsFontMetrics on mac platform.

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

::: gfx/thebes/gfxFont.cpp
@@ +3511,5 @@
>          // pick an arbitrary value that's better than zero
>          aMetrics.xHeight = aMetrics.maxAscent * DEFAULT_XHEIGHT_FACTOR;
>      }
>  
> +    // FIXME: should we pick an arbitrary value for capHeight?

Maybe, but I'm not sure it's worth doing anything more complex than what you have here. If we have a font that doesn't provide a capHeight value, I think using maxAscent will be a reasonable fallback.

@@ +3779,5 @@
>      metrics->zeroOrAveCharWidth = metrics->aveCharWidth;
>      metrics->maxHeight = metrics->maxAscent + metrics->maxDescent;
>      metrics->xHeight = metrics->emHeight / 2;
> +    // FIXME: should we assign an arbitrary value to metrics->capHeight?
> +    // metrics->capHeight = metrics->maxAscent;

It'd be good to set something as a "placeholder", although until we have real-world examples of usage to look at, we shouldn't worry too much about the details.

::: gfx/thebes/gfxMacFont.cpp
@@ +254,5 @@
>      }
>  
> +    if (mMetrics.capHeight == 0.0) {
> +        mMetrics.capHeight = ::CGFontGetCapHeight(mCGFont) * cgConvFactor;
> +    }

This should be OK, but normally we'd expect to find the value in the OS/2 table; this will be a rarely-used fallback.

@@ +256,5 @@
> +    if (mMetrics.capHeight == 0.0) {
> +        mMetrics.capHeight = ::CGFontGetCapHeight(mCGFont) * cgConvFactor;
> +    }
> +
> +    // apply font-size-adjust, and recalculate metrics

It's not correct to modify the code below in the way you've done here. This block exists to implement CSS font-size-adjust, which modifies the font size based on the xHeight of the face that has been chosen, and then (recursively) re-evaluates all the font metrics. But there's no comparable behavior related to the font's cap-height. So this change should just be removed, I think.

Comment 5

[Jeremy Chen ([e-mail])]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> > 1. Should I add support to os2 font in [1]?
> 
> Yes. In the great majority of cases, that's where we should be getting the
> metrics from. The platform-specific implementations serve as a fallback that
> should only be used if we have a font that a particular platform supports
> (such as Core Text on OS X) but where we can't find the data we need in
> standard OpenType tables.

Then, I shall focus on how to ensure that os2 could return correct cap height values.

> > 3. What kind of test should be added in this bug, so we could make sure the
> > support of cap height is added properly?
> 
> We don't generally have much testing at this low level of internal APIs. You
> can add capHeight to the (disabled) printf statements at the end of
> gfxMacFont::InitMetrics, for example, and enable this in a local build just
> to check that the values you're getting look reasonable; but beyond that,
> I'd most likely proceed to implement the actual layout feature that depends
> on this (i.e. initial-letter), and the reftests for that feature will have
> the effect of testing capHeight support. When it comes time to test
> initial-letter, you can test it with fonts such as Gentium where there is a
> significat difference between ascent and capHeight, to confirm that it is
> using the correct value.

Ok, I guess I might have to disable the fallback setting for cap height (in this wip patch) to do verification in my local build.

Thank you for the feedback.

Comment 6

[Jeremy Chen ([e-mail])]

I add some debug logs and run a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27086dba25a009cc5e08fed24347ea1192fb58fb

Although WinXP and Win8 are still running, we can tell that all backend libraries (FT2, GDI, DWrite) should be taken into considerations.

Linux and Android => FT2
Win7 => GDI
Win10 => DWrite
Mac => may sometimes use its system API as a fallback

Work on adding cap height support on rest of the backends.

Comment 7

[Jeremy Chen ([e-mail])]

Attachment: Bug 1293210 - add cap height support to nsFontMetrics.

Review commit: https://reviewboard.mozilla.org/r/72554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72554/

Comment 8

[Jeremy Chen ([e-mail])]

Attachment: Bug 1293210 - get cap height from platform APIs for non-sfnt fonts on Mac.

Review commit: https://reviewboard.mozilla.org/r/72556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72556/

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: Bug 1293210 - get cap height from DirectWrite backend.

Review commit: https://reviewboard.mozilla.org/r/72558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72558/

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Bug 1293210 - get cap height from FT2 backend.

Review commit: https://reviewboard.mozilla.org/r/72560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72560/

Comment 11

[Jeremy Chen ([e-mail])]

Attachment: Bug 1293210 - get cap height from GDI backend.

Review commit: https://reviewboard.mozilla.org/r/72562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72562/

Comment 12

[Jeremy Chen ([e-mail])]

Comment on attachment 8782371 [details]
Bug 1293210 - add cap height support to nsFontMetrics.

https://reviewboard.mozilla.org/r/72552/#review70200

From the try result listed in Comment 6, sometimes we may not get cap height info from os/2 table, even just using default font setting. I did some local test on my Mac, even just init the browser's XUL UI would lead Mac to fallback to its platform specific API to get font information. So, I assume that we should apply the fallback mechanism to all backends.

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #12)
> Comment on attachment 8782371 [details]
> Bug 1293210 - add cap height support to nsFontMetrics.
> 
> https://reviewboard.mozilla.org/r/72552/#review70200
> 
> From the try result listed in Comment 6, sometimes we may not get cap height
> info from os/2 table, even just using default font setting. I did some local
> test on my Mac, even just init the browser's XUL UI would lead Mac to
> fallback to its platform specific API to get font information.

This will be the case for many of the fonts shipped with OS X, because Apple (often) does not include an OS/2 table at all in the fonts they ship with the OS. (After all, it's described as the "OS/2 and Windows specific metrics" table... the Apple text frameworks have never relied on it, AFAIK.)

> So, I assume
> that we should apply the fallback mechanism to all backends.

Such fonts will be very rare on non-Mac platforms; but yes, we should still include reasonable fallbacks for all backends.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8782371 [details]
Bug 1293210 - add cap height support to nsFontMetrics.

https://reviewboard.mozilla.org/r/72554/#review70374

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 8782372 [details]
Bug 1293210 - get cap height from platform APIs for non-sfnt fonts on Mac.

https://reviewboard.mozilla.org/r/72556/#review70376

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8782373 [details]
Bug 1293210 - get cap height from DirectWrite backend.

https://reviewboard.mozilla.org/r/72558/#review70378

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8782374 [details]
Bug 1293210 - get cap height from FT2 backend.

https://reviewboard.mozilla.org/r/72560/#review70386

::: gfx/thebes/gfxFT2Utils.cpp:180
(Diff revision 1)
> +    if (GetCharExtents('H', &extents) && extents.y_bearing < 0.0) {
> +        aMetrics->capHeight = -extents.y_bearing;
> +    } else {
> +        if (os2 && os2->sCapHeight && yScale > 0.0) {
> +            aMetrics->capHeight = os2->sCapHeight * yScale;
> +        } else {
> +            aMetrics->capHeight = aMetrics->maxAscent;
> +        }
> +    }

I'm not sure it's really a good idea to prefer measuring a glyph here, rather than just using the value from the os/2 table (if available), but I guess it's OK for now, following the example of x-height.

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8782375 [details]
Bug 1293210 - get cap height from GDI backend.

https://reviewboard.mozilla.org/r/72562/#review70388

Comment 19

[Jeremy Chen ([e-mail])]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #12)
> > Comment on attachment 8782371 [details]
> > Bug 1293210 - add cap height support to nsFontMetrics.
> > 
> > https://reviewboard.mozilla.org/r/72552/#review70200
> > 
> > From the try result listed in Comment 6, sometimes we may not get cap height
> > info from os/2 table, even just using default font setting. I did some local
> > test on my Mac, even just init the browser's XUL UI would lead Mac to
> > fallback to its platform specific API to get font information.
> 
> This will be the case for many of the fonts shipped with OS X, because Apple
> (often) does not include an OS/2 table at all in the fonts they ship with
> the OS. (After all, it's described as the "OS/2 and Windows specific
> metrics" table... the Apple text frameworks have never relied on it, AFAIK.)

Oh, I see. Thank you for the explanation. :-)

> > So, I assume
> > that we should apply the fallback mechanism to all backends.
> 
> Such fonts will be very rare on non-Mac platforms; but yes, we should still
> include reasonable fallbacks for all backends.

Comment 20

[Jeremy Chen ([e-mail])]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e20de33dbc2a

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/449a9db6d082
add cap height support to nsFontMetrics. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/985039d0c6b4
get cap height from platform APIs for non-sfnt fonts on Mac. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/9d0ffc202a0a
get cap height from DirectWrite backend. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/de23dcdc9860
get cap height from FT2 backend. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/d12e64e97af2
get cap height from GDI backend. r=jfkthame

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/449a9db6d082
https://hg.mozilla.org/mozilla-central/rev/985039d0c6b4
https://hg.mozilla.org/mozilla-central/rev/9d0ffc202a0a
https://hg.mozilla.org/mozilla-central/rev/de23dcdc9860
https://hg.mozilla.org/mozilla-central/rev/d12e64e97af2