Bug 176215 - Letter-spacing should disable ligatures
Summary: Letter-spacing should disable ligatures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: Safari 10
Hardware: Mac All
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 216211
  Show dependency treegraph
 
Reported: 2017-09-01 01:51 PDT by Naresh Kapse
Modified: 2020-09-06 15:21 PDT (History)
20 users (show)

See Also:


Attachments
letter-spacing in AudioWide Font between f & l letter for Chrome, Safari and Firefox (94.47 KB, image/png)
2017-09-01 01:51 PDT, Naresh Kapse
no flags Details
Patch (6.67 KB, patch)
2017-09-01 13:49 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.82 MB, application/zip)
2017-09-01 14:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (2.13 MB, application/zip)
2017-09-01 15:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (2.36 MB, application/zip)
2017-09-01 15:18 PDT, Build Bot
no flags Details
Patch for committing (234.32 KB, patch)
2017-09-12 17:33 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.34 MB, application/zip)
2017-09-12 18:42 PDT, Build Bot
no flags Details
Patch (66.30 KB, patch)
2020-09-04 01:37 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (66.61 KB, patch)
2020-09-04 01:39 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (67.29 KB, patch)
2020-09-04 01:42 PDT, Myles C. Maxfield
koivisto: review+
Details | Formatted Diff | Diff
Patch for committing (103.14 KB, patch)
2020-09-05 00:37 PDT, Myles C. Maxfield
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for committing (35.41 KB, patch)
2020-09-05 10:56 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Naresh Kapse 2017-09-01 01:51:35 PDT
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
Comment 1 Myles C. Maxfield 2017-09-01 11:41:55 PDT
This is because we erroneously leave ligatures on, even when letter-spacing is enabled.
Comment 2 Myles C. Maxfield 2017-09-01 11:43:55 PDT
<rdar://problem/17044265>
Comment 3 Myles C. Maxfield 2017-09-01 13:49:37 PDT
Created attachment 319643 [details]
Patch
Comment 4 Tim Horton 2017-09-01 13:55:14 PDT
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?
Comment 5 Build Bot 2017-09-01 14:56:12 PDT
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
Comment 6 Build Bot 2017-09-01 14:56:14 PDT
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
Comment 7 Build Bot 2017-09-01 15:01:08 PDT
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
Comment 8 Build Bot 2017-09-01 15:01:09 PDT
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
Comment 9 Build Bot 2017-09-01 15:18:09 PDT
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
Comment 10 Build Bot 2017-09-01 15:18:11 PDT
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
Comment 11 Naresh Kapse 2017-09-02 23:30:03 PDT
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=
Comment 12 Myles C. Maxfield 2017-09-12 17:33:36 PDT
Created attachment 320588 [details]
Patch for committing
Comment 13 Build Bot 2017-09-12 18:42:29 PDT
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
Comment 14 Build Bot 2017-09-12 18:42:31 PDT
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
Comment 15 Jon Lee 2017-09-12 22:57:30 PDT
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.
Comment 16 Myles C. Maxfield 2020-09-03 15:09:55 PDT
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.
Comment 17 Myles C. Maxfield 2020-09-03 18:59:01 PDT
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.
Comment 18 Myles C. Maxfield 2020-09-03 22:28:43 PDT
justification should disable ligatures, too.
Comment 19 Myles C. Maxfield 2020-09-03 22:29:16 PDT
"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
Comment 20 Myles C. Maxfield 2020-09-03 23:36:28 PDT
(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
Comment 21 Myles C. Maxfield 2020-09-04 01:37:14 PDT
Created attachment 407949 [details]
Patch
Comment 22 Myles C. Maxfield 2020-09-04 01:39:25 PDT
Created attachment 407950 [details]
Patch
Comment 23 Myles C. Maxfield 2020-09-04 01:42:31 PDT
Created attachment 407951 [details]
Patch
Comment 24 Antti Koivisto 2020-09-04 02:50:58 PDT
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.
Comment 25 Myles C. Maxfield 2020-09-05 00:37:48 PDT
Created attachment 408068 [details]
Patch for committing
Comment 26 EWS 2020-09-05 10:39:24 PDT
Tools/Scripts/svn-apply failed to apply attachment 408068 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 27 Myles C. Maxfield 2020-09-05 10:56:06 PDT
Created attachment 408088 [details]
Patch for committing
Comment 28 Myles C. Maxfield 2020-09-06 15:21:04 PDT
Committed r266683: <https://trac.webkit.org/changeset/266683>