Closed Bug 458634 Opened 16 years ago Closed 10 years ago

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

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: tempbz123, Assigned: jfkthame)

References

Details

Attachments

(5 files, 1 obsolete file)

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.
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.
Confirming on Mac OS X.
Status: UNCONFIRMED → NEW
Component: General → Layout: Text
Ever confirmed: true
OS: Windows Vista → All
Product: Firefox → Core
QA Contact: general → layout.fonts-and-text
Hardware: x86 → All
Version: unspecified → Trunk
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
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.
Depends on: 1015603
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.
Attachment #8428416 - Flags: review?(jdaggett)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Try build: https://tbpl.mozilla.org/?tree=Try&rev=ecf9031f8f3e; reftest orange on Win8 and B2G is expected.
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.
(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.
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.
Attachment #8429175 - Flags: review?(jdaggett)
Attachment #8429218 - Flags: review?(jdaggett)
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.
Attachment #8429392 - Flags: review?(jdaggett)
Attachment #8429218 - Attachment is obsolete: true
Attachment #8429218 - Flags: review?(jdaggett)
Blocks: 961558
Attachment #8429392 - Flags: review?(jdaggett) → review+
Attachment #8429175 - Flags: review?(jdaggett) → review+
Attachment #8428416 - Flags: review?(jdaggett) → review+
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)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: