Note: There are a few cases of duplicates in user autocompletion which are being worked on.

add cap height support to nsFontMetrics

RESOLVED FIXED in Firefox 51

Status

()

Core
Layout: Text
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: jeremychen, Assigned: jeremychen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

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

MozReview-Commit-ID: JhiRCaCCq01
(Assignee)

Comment 2

11 months ago
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
Attachment #8781110 - Attachment description: add cap height support to nsFontMetrics on mac platform. → (wip) add cap height support to nsFontMetrics on mac platform.
Attachment #8781110 - Flags: feedback?(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 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.
Attachment #8781110 - Flags: feedback?(jfkthame)
(Assignee)

Comment 5

11 months ago
(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.
(Assignee)

Comment 6

11 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

11 months ago
mozreview-review
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.
(Assignee)

Updated

11 months ago
Attachment #8782371 - Flags: review?(jfkthame)
Attachment #8782372 - Flags: review?(jfkthame)
Attachment #8782373 - Flags: review?(jfkthame)
Attachment #8782374 - Flags: review?(jfkthame)
Attachment #8782375 - Flags: review?(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

11 months ago
mozreview-review
Comment on attachment 8782371 [details]
Bug 1293210 - add cap height support to nsFontMetrics.

https://reviewboard.mozilla.org/r/72554/#review70374
Attachment #8782371 - Flags: review?(jfkthame) → review+

Comment 15

11 months ago
mozreview-review
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
Attachment #8782372 - Flags: review?(jfkthame) → review+

Comment 16

11 months ago
mozreview-review
Comment on attachment 8782373 [details]
Bug 1293210 - get cap height from DirectWrite backend.

https://reviewboard.mozilla.org/r/72558/#review70378
Attachment #8782373 - Flags: review?(jfkthame) → review+

Comment 17

11 months ago
mozreview-review
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.
Attachment #8782374 - Flags: review?(jfkthame) → review+

Comment 18

11 months ago
mozreview-review
Comment on attachment 8782375 [details]
Bug 1293210 - get cap height from GDI backend.

https://reviewboard.mozilla.org/r/72562/#review70388
Attachment #8782375 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 19

11 months ago
(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.
(Assignee)

Comment 20

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

Comment 21

11 months ago
Pushed by jichen@mozilla.com:
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
(Assignee)

Updated

11 months ago
Blocks: 1296561

Comment 22

11 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.