458634 - Small caps do not look right with OpenType fonts that have the smcp feature

Status: RESOLVED FIXED

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

Description

[tempbz123]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US) AppleWebKit/525.13 (KHTML, like Gecko) Chrome/0.2.149.30 Safari/525.13
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; nb-NO; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3

Firefox seems to ignore small cap glyphs present in some OpenType fonts, like Cambria and the other new Windows Vista / Office 2007 fonts. Instead it scales capital letters to x-height, causing an inferior result which is thinner and more narrow than it should be.

Reproducible: Always

Steps to Reproduce:
<div style="font-family: Cambria; font-size: 40px; font-variant: small-caps;">
HeLlO,<br>wOrLd!
</div>

Actual Results:  
Capitals were scaled to x-height.

Expected Results:  
Use proper small cap glyphs from the font.

Comment 1

[tempbz123]

Attachment: Comparison between Firefox and correct rendering

Comment 2

[Jonathan Kew [:jfkthame]]

To fix this, we need to be able to detect and apply optional OpenType features, which we can't currently do using the text layout engines we're using. The Vista version of Uniscribe would support it but then we'd have backward compatibility issues.

Comment 3

[Henri Sivonen (:hsivonen)]

Confirming on Mac OS X.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Test case using Linux Libertine

Test case for demonstrating the problem on the Mac at least. Requires Linux Libertine and Linux Libertine Capitals (for reference) to be installed.
http://linuxlibertine.sourceforge.net/Libertine-EN.html

Comment 5

[Jonathan Kew [:jfkthame]]

Although we now use harfbuzz for (most) text rendering, which in principle gives us access to features such as 'smcp', it would still be hard to fix this within the current text code because small-caps is implemented by the text-frame code before we have performed font matching, and therefore we don't know what features are actually available.

The proposed refactoring of the small-caps implementation in bug 1015603, moving it to the gfxFont level, should make it possible to fix this properly.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: use OpenType 'smcp' feature to implement small-caps if available in the font.

This builds on the patch series in bug 1015603, and makes OpenType fonts with 'smcp' do the right thing for font-variant:small-caps. Note that this will cause reftests to fail on Win8 and B2G, as the default fonts on those platforms include real small-caps, but we have a reftest that assumes small-caps are simply uppercase letters reduced to 80%. So we'll need to update tests appropriately before we can land this. Oh, another thing: this is currently linked only to font-variant:small-caps, not to the font-variant-caps property, so I expect we'll want an additional connection there.

Comment 7

[Jonathan Kew [:jfkthame]]

Try build: https://tbpl.mozilla.org/?tree=Try&rev=ecf9031f8f3e; reftest orange on Win8 and B2G is expected.

Comment 8

[John Daggett (:jtd)]

Comment on attachment 8428416 [details] [diff] [review]
use OpenType 'smcp' feature to implement small-caps if available in the font.

>  bool
> +gfxFont::SupportsSmallCaps(int32_t aScript)
> +{
> +    if (mGraphiteShaper && gfxPlatform::GetPlatform()->UseGraphiteShaping()) {
> +        // we don't currently support real small caps via graphite
> +        return false;
> +    }
> +
> +    return GetFontEntry()->SupportsOpenTypeSmallCaps(aScript);
> +}

Hmmm, it seems to me a Graphite font can support a small-caps feature just as easily as an OpenType font. I'm not sure I see the need for the artificial distinction here.  If the font has a feature marked 'smcp' the real small-caps is used, otherwise fallback to synthesized small-caps.

Otherwise, looks fine.

One thing to note here is that this will need to be revised at some point to support the fallback behavior dictated for font-variant-caps, where petite-caps falls back to small-caps.  But that's best left for another bug.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #8)
> Comment on attachment 8428416 [details] [diff] [review]
> use OpenType 'smcp' feature to implement small-caps if available in the font.
> 
> >  bool
> > +gfxFont::SupportsSmallCaps(int32_t aScript)
> > +{
> > +    if (mGraphiteShaper && gfxPlatform::GetPlatform()->UseGraphiteShaping()) {
> > +        // we don't currently support real small caps via graphite
> > +        return false;
> > +    }
> > +
> > +    return GetFontEntry()->SupportsOpenTypeSmallCaps(aScript);
> > +}
> 
> Hmmm, it seems to me a Graphite font can support a small-caps feature just
> as easily as an OpenType font. I'm not sure I see the need for the
> artificial distinction here.  If the font has a feature marked 'smcp' the
> real small-caps is used, otherwise fallback to synthesized small-caps.

Yes, we should do that too, but it'll require a separate SupportsGraphiteSmallCaps function to check for the feature in the font, so I figured I'd leave that for a followup patch.

> One thing to note here is that this will need to be revised at some point to
> support the fallback behavior dictated for font-variant-caps, where
> petite-caps falls back to small-caps.  But that's best left for another bug.

Yup, I agree - there's still work to be done here, but this seems like a good place to start, and a useful step forward in its own right.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - use 'smcp' feature in Graphite fonts if available.

Here's the Graphite small-caps patch; tested with the current Charis SIL font. (Note that it won't work with older Charis releases from before they adopted the OpenType tag convention for feature IDs; people with such versions of the font will still get fake small-caps.) Still to come: reftests.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: reftests for real small-cap support.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: reftests for real small-cap support.

Minor update: fixed up the new XUL and canvas small-caps tests to avoid failure on Win8 when real small-caps are enabled. Try run in progress: https://tbpl.mozilla.org/?tree=Try&rev=52127f409933.

Comment 13

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74222d6482a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/83ce79fabdd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb7aec714906

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74222d6482a9
https://hg.mozilla.org/mozilla-central/rev/83ce79fabdd5
https://hg.mozilla.org/mozilla-central/rev/fb7aec714906

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f3b3cccc362

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8429392 [details] [diff] [review]
reftests for real small-cap support.

Minor late-breaking followup here -- the TTF file and its txt license were added with the an executable file-mode, by accident it seems:
  
>diff --git a/layout/reftests/fonts/sil/Charis-license.txt b/layout/reftests/fonts/sil/Charis-license.txt
>new file mode 100755
[...]
>diff --git a/layout/reftests/fonts/sil/CharisSIL-R.ttf b/layout/reftests/fonts/sil/CharisSIL-R.ttf
>new file mode 100755

(Mode 755 is executable.)


I pushed a trivial followup to mark these files as non-executable:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f3b3cccc362

(In retrospect, I should have added DONTBUILD in the commit message to skip an unnecessary build/test-run; oh well)

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f3b3cccc362