427730 - Regression: [font-variant: small-caps] Font rendering in Firefox 3 has kerning problems

Status: VERIFIED FIXED

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

Description

[Alex Limi (:limi) — Firefox UX Team]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-us) AppleWebKit/523.15.1 (KHTML, like Gecko) Version/3.0.4 Safari/523.15
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5

When rendering the "Baskerville" font in a web page with Firefox 3 (beta 5), it screws up the font rendering. There is not enough space between letters, causing them to overlap.

This worked fine in Firefox 2, and works in all other browsers available on the Mac platform (Safari, Opera).

Reproducible: Always

Steps to Reproduce:
1. Go to http://limi.net using Firefox 3.

Actual Results:  
Notice how the letters in the headlines flow into each other.



Expected Results:  
I expect the letters to be spaced normally, the way it looks in Firefox 2, Opera and Safari.

Comment 1

[Alex Limi (:limi) — Firefox UX Team]

Attachment: Expected result, this is how the headline rendering looks in Firefox 2

Comment 2

[Alex Limi (:limi) — Firefox UX Team]

Attachment: Screenshot showing the rendering regression, notice how the letters in the headlines flow into each other

Comment 3

[philippe (part-time)]

The problem is with the use of font-variant: small-caps in those headlines. The kerning is quite tight indeed.

Comment 4

[philippe (part-time)]

Attachment: test case

test case from URL, with various fonts.

Comment 5

[John Daggett (:jtd)]

Attachment: screenshot, looks fine on windows

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008033105 Minefield/3.0pre

Comment 6

[John Daggett (:jtd)]

Attachment: testcase, with and without smallcaps

So it looks like the code in nsFontVariantTextRunFactory::RebuildTextRun is chopping the text run into separate text runs for upper and lower case and applying small caps by (1) uppercasing and (2) applying font-size: 80% to the original lowercase spans.  The equivalent set of spans renders fine so there must be something messing with the advances a bit too much.  I'm a bit confused why this seems to be mac only.

Comment 7

[Damon Sicore (:damons)]

+'ing this.  We don't want web developers to decide to not use small-caps.  Roc is going to take this on.

Comment 8

[Damon Sicore (:damons)]

Fixing priority.  P2.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase showing reduced advance width

An "F" gets a smaller advance width in small-caps mode. That is strange.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Found the bug. In gfxAtsuiFonts, we try to prevent weird RTL issues by appending "." as the last character of the text before we call ATSUI to get the layout, *if* the text is not known to be ASCII (so might contain RTL control characters, in which case we wrap the text in direction override characters).

Problem is, certain characters like 'F' form kerning pairs with ".", so the advance width we compute for 'F' is narrower than it should be!

Not sure how to fix this just yet. The only thing we need "." for is to ensure there is no trailing whitespace. So possibly we should only append the "." when there is trailing whitespace --- that wouldn't create kerning problems.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This turned out to be a bit more work than I thought.

The most important thing I'm doing here is adding a space before the "." we append to the text for our ATSU layout. That prevents kerning between the last character of the real text and the ".", which led to this bug.

But in the process of creating a reftest, I discovered that the advance width given to a character at the end of the layout (e.g., "T" in ASCII-mode where we don't need any wrapper characters) can be greater than the advance width given to the character when it's followed by a space. To get consistency along both the wrapped and not-wrapped paths, we have to add a trailing space for the not-wrapped path as well. This probably also fixes hitherto-undetected width inconsistencies based on which words were cached by gfxTextRunWordCache.

All this wrapping was getting confusing so I reorganized parameter names in various places to that we clearly identify a) the whole string including all wrappers b) the substring for which we should create the ATSU layout and c) the subrange of the textrun to which glyphs should be copied.

My reftest does not use small-caps, because I don't think we should constrain what small-caps actually does.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 315073 [details] [diff] [review]
fix

Looks fine here; would a ZWNJ be better than a space?  (Ignore me if that's a dumb question -- does a ZWNJ count as trailing whitespace, could you replace the " ." with a single ZWNJ?  Probably doesn't matter in practice..)

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure how ZWNJ would be treated.

Comment 14

[John Daggett (:jtd)]

Comment on attachment 315073 [details] [diff] [review]
fix

This fix definitely removes the ugly overlap in the Pl case but we effectively lose kerning at uppercase/lowercase boundaries.  It would be nice in the future for us to consider evaluating whether a font contains small-cap glyphs (e.g. Hoefler Text) and using those if possible.  One other way of doing this would be just to use '|' instead of '.' to assure that whitespace is not treated as trailing, that appears to give the desired results.

Comment 15

[John Daggett (:jtd)]

Attachment: testcase, small cap kerning examples

Note the poor kerning in "Far Away" in the small-caps case.

Comment 16

[John Daggett (:jtd)]

Attachment: screenshot, smallcap kerning before/after patch

Current trunk is on the left, with patch is on the right.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

As long as we're using this approach of uppercasing the lowercase characters and scaling down, we won't get true kerning across all the text.

I'm not sure why using '|' instead of '.' would produce any different results, given this patch. Or did you mean '|' instead of ' .'? That might produce better results in some cases but it's still not very smart.

I'm not sure how small-cap glyphs are accessed so I don't know how hard that is.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 315073 [details] [diff] [review]
fix

Fixes serious Mac text rendering regression. Reasonably low risk.

Comment 19

[John Daggett (:jtd)]

(In reply to comment #17)
> I'm not sure why using '|' instead of '.' would produce any different results,
> given this patch. Or did you mean '|' instead of ' .'? That might produce
> better results in some cases but it's still not very smart.

Sorry, I meant in place of this patch we could simply use '|' instead '.' as the character used to force ATSUI not to treat whitespace as trailing.  Sequences like F. T. P. Y. are probably going to pick up kerning pairs whereas F| T| P| Y| avoids this.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315073 [details] [diff] [review]
fix

a1.9=beltzner

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 22

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041704 Minefield/3.0pre. I verified using the site in the URL as well as running the various testcases.