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.

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.

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

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

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

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.

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