Bug 176215 – Letter-spacing should disable ligatures

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Naresh Kapse]

Created attachment 319586 [details]
letter-spacing in AudioWide Font between f & l letter for Chrome, Safari and Firefox

Chrome Version       : 60.0.3112.101
OS Version: OS X 10.12.5
URLs (if applicable) :
Other browsers tested:
  Add OK or FAIL after other browsers where you have tested this issue:
     Safari 5: FAIL
  Firefox 4.x: OK
     IE 7/8/9: OK

What steps will reproduce the problem?
1. Use audiowide font on the web
2. Ensure you have text like flash in h1 tag
3. Include letter-spacing for h1 tag.

What is the expected result?
letter-spacing should have created a gap between letters f & l.

What happens instead of that?
There is no letter-spacing between letters f & l. 

Please provide any additional information below. Attach a screenshot if
possible.
Attached screenshots for chrome, firefox

UserAgentString: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.101 Safari/537.36

[Myles C. Maxfield]

This is because we erroneously leave ligatures on, even when letter-spacing is enabled.

[Myles C. Maxfield]

<rdar://problem/17044265>

[Myles C. Maxfield]

Created attachment 319643 [details]
Patch

[Tim Horton]

Comment on attachment 319643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319643&action=review

> Source/WebCore/ChangeLog:3
> +        No letter spacing between f & l for audiowide font (not happening in firefox)

Can this adopt the title from the bug, which is much better?

> Source/WebCore/platform/graphics/FontDescription.h:142
> +        setVariantDiscretionaryLigatures(FontVariantLigatures::No);
> +        setVariantHistoricalLigatures(FontVariantLigatures::No);

Technically no test for these two?

[Build Bot]

Comment on attachment 319643 [details]
Patch

Attachment 319643 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4428781

New failing tests:
fast/css/word-space-extra.html
editing/mac/attributed-string/letter-spacing.html

[Build Bot]

Created attachment 319648 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319643 [details]
Patch

Attachment 319643 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4428787

New failing tests:
fast/css/word-space-extra.html

[Build Bot]

Created attachment 319650 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319643 [details]
Patch

Attachment 319643 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4428809

New failing tests:
fast/css/word-space-extra.html

[Build Bot]

Created attachment 319652 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Naresh Kapse]

I have got rid of the issue by using css style font-variant-ligatures:none;

I will leave this to the individual browser teams to decide if they should enable ligatures or not.

I do not see any advantage of enabling ligatures by default unless the font is using cursive writing style.

https://bugs.chromium.org/p/chromium/issues/detail?id=761281&can=2&start=0&num=100&q=&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified&groupby=&sort=

[Myles C. Maxfield]

Created attachment 320588 [details]
Patch for committing

[Build Bot]

Comment on attachment 320588 [details]
Patch for committing

Attachment 320588 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4529159

New failing tests:
editing/mac/attributed-string/letter-spacing.html

[Build Bot]

Created attachment 320596 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Jon Lee]

Thinking out loud here, but is there a case where ligatures may still be desirable with negative letter-spacing? I guess at extreme values it wouldn't.

[Myles C. Maxfield]

I don't think this patch is right, because other browsers don't let letter-spacing change the web-exposed computed value of font-variant-ligatures.

[Myles C. Maxfield]

The CSS spec defines exactly how this should work:

https://drafts.csswg.org/css-fonts-4/#feature-variation-precedence

11. Feature settings determined by properties other than font-variant or font-feature-settings are applied. For example, setting a non-default value for the letter-spacing property disables optional ligatures.

[Myles C. Maxfield]

justification should disable ligatures, too.

[Myles C. Maxfield]

"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"

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

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #18)
> justification should disable ligatures, too.

But only if justification adds space in between characters. text-justify:inter-character can do that[1], but WebKit doesn't implement that (yet).

https://drafts.csswg.org/css-text-3/#text-justify-property

[Myles C. Maxfield]

Created attachment 407949 [details]
Patch

[Myles C. Maxfield]

Created attachment 407950 [details]
Patch

[Myles C. Maxfield]

Created attachment 407951 [details]
Patch

[Antti Koivisto]

Comment on attachment 407951 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407951&action=review

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:536
> +// FIXME: This function is 180 lines long. We should factor it into smaller pieces.

This seems bit unnecessary. It will also easily become a lie when someone adds more lines and forgets to update the number.

[Myles C. Maxfield]

Created attachment 408068 [details]
Patch for committing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 408068 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Myles C. Maxfield]

Created attachment 408088 [details]
Patch for committing

[Myles C. Maxfield]

Committed r266683: <https://trac.webkit.org/changeset/266683>