Closed Bug 1342315 Opened 3 years ago Closed 3 years ago

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

Categories

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

53 Branch
Unspecified
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: chenpighead, Assigned: jfkthame)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

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?
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...
On macOS, it works for me on Firefox release (51.0.1), but fails on Nightly. So a recent-ish regression.
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.
(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.
(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.
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
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: nobody → jfkthame
Status: NEW → ASSIGNED
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+
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.
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?
https://hg.mozilla.org/mozilla-central/rev/b623e256e1f4
https://hg.mozilla.org/mozilla-central/rev/ebb71fecdc3a
Status: ASSIGNED → RESOLVED
Closed: 3 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.