1342315 - CSS letter-spacing is not working properly on "fi" ligature when using an AAT font on macOS

Status: RESOLVED FIXED

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

Description

[Jeremy Chen ([e-mail])]

Test case:

  data:text/html, <p style="letter-spacing: 1em;">justification</p>

Expect:
'f' and 'i' should be letter-spaced.

Actual:
'f' and 'i' are not letter-spaced.


According to https://drafts.csswg.org/css-text-3/#letter-spacing-property,

"When the effective spacing between two characters is not zero (due to either justification or a non-zero value of letter-spacing), user agents should not apply optional ligatures."

Which means, if a word is letter-spaced, an “fi” ligature should not be used as it will prevent even spacing of the text (see Example 17 in the specification).

This bug would also cause incorrect justification result for text-justify property, since text-justify shares the same spacing process code path.

Comment 1

[Jeremy Chen ([e-mail])]

Looks like Chrome also has this issue....
Test case in comment 0 looks even worse on Safari since "fic" are concatenated to one ligature.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

On Linux, both Firefox and Chrome seem fine.  On Windows, Firefox, Chrome, and Edge seem fine.  Is this Mac-only?

Comment 3

[Jonathan Kew [:jfkthame]]

Huh, this used to work (i.e. ligatures would be turned off when non-zero letter-spacing is in effect). We should identify when it regressed...

Comment 4

[Jonathan Kew [:jfkthame]]

On macOS, it works for me on Firefox release (51.0.1), but fails on Nightly. So a recent-ish regression.

Comment 5

[Jonathan Kew [:jfkthame]]

According to mozregression:

11:13.47 INFO: Last good revision: 44ab9ab869d54feae16cd084cafe36b5595833c4
11:13.47 INFO: First bad revision: 273ce98bb9e831b70d9ce2b34590859ea2ce80b8
11:13.47 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=44ab9ab869d54feae16cd084cafe36b5595833c4&tochange=273ce98bb9e831b70d9ce2b34590859ea2ce80b8

11:14.57 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1321031

So it looks like I broke this in bug 1321031. :(

What is perhaps even more sad is that we didn't have a reftest that caught it.

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #2)
> On Linux, both Firefox and Chrome seem fine.  On Windows, Firefox, Chrome,
> and Edge seem fine.  Is this Mac-only?

Did you check that the fonts involved actually use a ligature in the absence of letter-spacing? The default fonts on Linux and Windows may not include a ligature there... Times New Roman on Windows doesn't, certainly.

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> So it looks like I broke this in bug 1321031. :(

More precisely, it's broken by https://hg.mozilla.org/mozilla-central/rev/53365854908e.

Comment 8

[Jonathan Kew [:jfkthame]]

Note that this affects only AAT fonts (that we shape using Core Text); with OpenType fonts, we still disable ligatures correctly. So it is a Mac-only issue.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Don't inadvertently ignore font feature settings when creating a new CTFont

Oops, this was a trivial error - just failed to pass the feature descriptor through in the (usual) case where it's not a variation font.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Add reftests to check that we disable ligatures when letter-spacing is in effect

Adding a reftest that would have caught this. (Also includes one using an OpenType font, in case we ever try to break that codepath.)

Comment 11

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b623e256e1f4eb394f0f1d666e1c4a517e1f3e53
Bug 1342315 - Don't inadvertently ignore font feature settings when creating a new CTFont. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb71fecdc3a42bfb94f74c0df54f42ae26d821d
Bug 1342315 - Add reftests to check that we disable ligatures when letter-spacing is in effect. r=jrmuizel

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> (In reply to David Baron :dbaron: ⌚️UTC-8 from comment #2)
> > On Linux, both Firefox and Chrome seem fine.  On Windows, Firefox, Chrome,
> > and Edge seem fine.  Is this Mac-only?
> 
> Did you check that the fonts involved actually use a ligature in the absence
> of letter-spacing? The default fonts on Linux and Windows may not include a
> ligature there... Times New Roman on Windows doesn't, certainly.

I definitely did check this on Linux, but I'm not sure whether I did on Windows.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8841023 [details] [diff] [review]
Don't inadvertently ignore font feature settings when creating a new CTFont

Approval Request Comment
[Feature/Bug causing the regression]: 1321031

[User impact if declined]: ugly rendering of letter-spaced text on Mac when font includes ligatures (such as "fi")

[Is this code covered by automated tests?]: yes

[Has the fix been verified in Nightly?]: tested in local build

[Needs manual test from QE? If yes, steps to reproduce]: no

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: no

[Why is the change risky/not risky?]: trivial fix, passing the correct parameter instead of null

[String changes made/needed]: none

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b623e256e1f4
https://hg.mozilla.org/mozilla-central/rev/ebb71fecdc3a

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8841023 [details] [diff] [review]
Don't inadvertently ignore font feature settings when creating a new CTFont

Minor font rendering fix, let's take this for aurora 53.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9426697a2965
https://hg.mozilla.org/releases/mozilla-aurora/rev/a50fdc5eebdf