Bug 162546 - Fonts lie about being monospaced
Summary: Fonts lie about being monospaced
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 215727 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-09-26 01:01 PDT by Hunseop Jeong
Modified: 2020-08-25 09:03 PDT (History)
20 users (show)

See Also:


Attachments
index.html (652 bytes, text/html)
2016-09-26 01:01 PDT, Hunseop Jeong
no flags Details
Acme-Regular.ttf (22.91 KB, application/octet-stream)
2016-09-26 01:02 PDT, Hunseop Jeong
no flags Details
tick.ttf (30.51 KB, application/octet-stream)
2016-09-26 01:03 PDT, Hunseop Jeong
no flags Details
Actual.png (2.84 KB, image/png)
2016-09-26 01:03 PDT, Hunseop Jeong
no flags Details
Expected.png (2.84 KB, image/png)
2016-09-26 01:04 PDT, Hunseop Jeong
no flags Details
Needs a test font (2.73 KB, patch)
2016-10-04 00:06 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (1013.48 KB, application/zip)
2016-10-04 01:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (986.24 KB, application/zip)
2016-10-04 01:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2 (9.00 MB, application/zip)
2016-10-04 01:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.76 MB, application/zip)
2016-10-04 01:17 PDT, Build Bot
no flags Details
Patch (11.71 KB, patch)
2016-10-05 15:33 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (15.05 KB, patch)
2016-10-05 16:25 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2 (9.29 MB, application/zip)
2016-10-05 17:21 PDT, Build Bot
no flags Details
Patch for committing (21.32 KB, patch)
2020-08-25 02:08 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 Hunseop Jeong 2016-09-26 01:01:54 PDT
Created attachment 289804 [details]
index.html

Test: index.html with attached font.


Expected: Expected.png (Chrome, firefox)

Actual: Actual.png (Safari)


Some characters is displayed over the right edge of the block if using the 'tick'.
Comment 1 Hunseop Jeong 2016-09-26 01:02:55 PDT
Created attachment 289805 [details]
Acme-Regular.ttf
Comment 2 Hunseop Jeong 2016-09-26 01:03:10 PDT
Created attachment 289806 [details]
tick.ttf
Comment 3 Hunseop Jeong 2016-09-26 01:03:48 PDT
Created attachment 289807 [details]
Actual.png
Comment 4 Hunseop Jeong 2016-09-26 01:04:03 PDT
Created attachment 289808 [details]
Expected.png
Comment 5 Hunseop Jeong 2016-09-26 18:11:50 PDT
mmaxifield, Do you know what is wrong?
Comment 6 Radar WebKit Bug Importer 2016-09-27 07:10:01 PDT
<rdar://problem/28494654>
Comment 7 Myles C. Maxfield 2016-10-04 00:01:37 PDT
The font says that it is monospaced, but it is incorrect. The space character is not as wide as the rest of the characters.
Comment 8 Myles C. Maxfield 2016-10-04 00:06:34 PDT
Created attachment 290573 [details]
Needs a test font
Comment 9 Build Bot 2016-10-04 01:01:15 PDT
Comment on attachment 290573 [details]
Needs a test font

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

New failing tests:
fast/text/line-break-after-question-mark.html
fast/regions/box-decorations-over-region-padding-fragmented.html
Comment 10 Build Bot 2016-10-04 01:01:18 PDT
Created attachment 290575 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Build Bot 2016-10-04 01:05:29 PDT
Comment on attachment 290573 [details]
Needs a test font

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

New failing tests:
fast/text/line-break-after-question-mark.html
fast/regions/box-decorations-over-region-padding-fragmented.html
Comment 12 Build Bot 2016-10-04 01:05:33 PDT
Created attachment 290578 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 13 Build Bot 2016-10-04 01:17:06 PDT
Comment on attachment 290573 [details]
Needs a test font

Attachment 290573 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2215850

New failing tests:
fast/text/line-break-after-question-mark.html
Comment 14 Build Bot 2016-10-04 01:17:10 PDT
Created attachment 290581 [details]
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 15 Build Bot 2016-10-04 01:17:33 PDT
Comment on attachment 290573 [details]
Needs a test font

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

New failing tests:
fast/text/line-break-after-question-mark.html
fast/regions/box-decorations-over-region-padding-fragmented.html
Comment 16 Build Bot 2016-10-04 01:17:36 PDT
Created attachment 290582 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 17 Myles C. Maxfield 2016-10-05 15:33:35 PDT
Created attachment 290753 [details]
Patch
Comment 18 Myles C. Maxfield 2016-10-05 16:25:18 PDT
Created attachment 290759 [details]
Patch
Comment 19 Simon Fraser (smfr) 2016-10-05 17:18:44 PDT
Comment on attachment 290759 [details]
Patch

Does this have performance implications?
Comment 20 Build Bot 2016-10-05 17:21:13 PDT
Comment on attachment 290759 [details]
Patch

Attachment 290759 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2227744

New failing tests:
fast/text/font-erroneous-monospace.html
Comment 21 Build Bot 2016-10-05 17:21:18 PDT
Created attachment 290764 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 22 Darin Adler 2016-10-06 12:48:11 PDT
Comment on attachment 290759 [details]
Patch

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

Test is failing on iOS Simulator. Please don’t land without fixing that.

> Source/WebCore/ChangeLog:16
> +        When a font reports itself to be monospace, we use this as a
> +        signal that we can perform width computations by assuming all
> +        characters have the same width as the space character. However,
> +        some fonts erroneously claim to be monospaced. Firefox and
> +        Chrome both do not use this signal, so therefore they both
> +        correctly render these fonts. We should ignore this bit in the
> +        font as well. Also, CJK fonts generally do not have this bit
> +        set (because they usually have at least one character which is
> +        not fullwidth) so this isn't a concern there.

I understand that you are saying that this optimization was implemented incorrectly. You seem to be saying either you determined that we don’t need the optimization, or that even though the optimization would be nice there is no practical way to do it correctly. Which one of those is the rationale? How did you determine that?

> LayoutTests/fast/text/script-tests/line-break-after-question-mark.js:11
> -    return div.offsetHeight > 25;
> +    return div.offsetHeight > 34;

Is the best change to alter this number, or is the best test removing the character that triggers fallback?
Comment 23 Myles C. Maxfield 2020-08-24 18:00:12 PDT
*** Bug 215727 has been marked as a duplicate of this bug. ***
Comment 24 Darin Adler 2020-08-24 19:01:36 PDT
I wasn’t expecting that we’d wait 4 years after that review.
Comment 25 Myles C. Maxfield 2020-08-24 23:03:32 PDT
PLT says this patch isn't a regression.

$ compare-results -a plt-results-vanilla -b plt-results-patch 
a mean = 699.26720
b mean = 698.09886
pValue = 0.9183320159
(Smaller means are better.)
1.002 times better
Results ARE NOT significant
Comment 26 Myles C. Maxfield 2020-08-25 00:03:59 PDT
Comment on attachment 290759 [details]
Patch

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

>> LayoutTests/fast/text/script-tests/line-break-after-question-mark.js:11
>> +    return div.offsetHeight > 34;
> 
> Is the best change to alter this number, or is the best test removing the character that triggers fallback?

We should remove the characters that triggers fallback.

Ahem doesn't support U+0027 and U+007F.
Comment 27 Myles C. Maxfield 2020-08-25 02:08:12 PDT
Created attachment 407181 [details]
Patch for committing
Comment 28 EWS 2020-08-25 09:03:04 PDT
Committed r266118: <https://trac.webkit.org/changeset/266118>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407181 [details].