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

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jeremychen, Assigned: jfkthame)

Tracking

({regression})

53 Branch
mozilla54
Unspecified
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 unaffected, firefox52 unaffected, firefox53 fixed, firefox54 fixed)

Details

()

Attachments

(2 attachments)

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.
Looks like Chrome also has this issue....
Test case in comment 0 looks even worse on Safari since "fic" are concatenated to one ligature.
On Linux, both Firefox and Chrome seem fine.  On Windows, Firefox, Chrome, and Edge seem fine.  Is this Mac-only?
Assignee

Comment 3

2 years ago
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...
Assignee

Comment 4

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

Comment 5

2 years ago
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.
Assignee

Comment 6

2 years ago
(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.
Assignee

Comment 7

2 years ago
(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.
Assignee

Comment 8

2 years ago
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.
OS: Unspecified → Mac OS X
Summary: CSS letter-spacing is not working properly on "fi" ligature → CSS letter-spacing is not working properly on "fi" ligature when using an AAT font on macOS
Assignee

Comment 9

2 years ago
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.
Attachment #8841023 - Flags: review?(jmuizelaar)
Assignee

Updated

2 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee

Comment 10

2 years ago
Adding a reftest that would have caught this. (Also includes one using an OpenType font, in case we ever try to break that codepath.)
Attachment #8841024 - Flags: review?(jmuizelaar)
Attachment #8841023 - Flags: review?(jmuizelaar) → review+
Attachment #8841024 - Flags: review?(jmuizelaar) → review+
Assignee

Comment 11

2 years ago
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
(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.
Assignee

Comment 13

2 years ago
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
Attachment #8841023 - Flags: approval-mozilla-aurora?

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b623e256e1f4
https://hg.mozilla.org/mozilla-central/rev/ebb71fecdc3a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Version: unspecified → 53 Branch
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.
Attachment #8841023 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.