Bug 201214 - [Win][Uniscribe] Material icons containing underscore or numbers aren't shown because ScriptItemize splits them apart
Summary: [Win][Uniscribe] Material icons containing underscore or numbers aren't shown...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on: 201213
Blocks:
  Show dependency treegraph
 
Reported: 2019-08-28 02:25 PDT by Fujii Hironori
Modified: 2020-07-22 13:05 PDT (History)
8 users (show)

See Also:


Attachments
screenshot (67.74 KB, image/png)
2019-08-28 02:26 PDT, Fujii Hironori
no flags Details
WIP patch (2.29 KB, patch)
2020-07-16 00:54 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (7.57 KB, patch)
2020-07-16 14:25 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (7.51 KB, patch)
2020-07-17 00:52 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (7.46 KB, patch)
2020-07-19 21:30 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (5.64 KB, patch)
2020-07-21 19:49 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2019-08-28 02:25:17 PDT
[Win][UniscribeController] Material icons containing underscore or numbers aren't shown because ScriptItemize splits them apart

For example, 'accessible_forward' are split as three items (accessible _ forward). Then, 'accessible' icon is shown incorrectly.
'3d_rotation' is split as four items (3 d _ rotation).

https://material.io/resources/icons/
Comment 1 Fujii Hironori 2019-08-28 02:26:21 PDT
Created attachment 377437 [details]
screenshot
Comment 2 Fujii Hironori 2019-08-28 02:32:24 PDT
Google News is also using Material icons.

https://news.google.com/

'star_border' shows 'star' icons.
'directions_bike' shows 'directions' icons.
Comment 3 Fujii Hironori 2019-08-28 02:47:57 PDT
Screenshot of Google News: https://ibb.co/CWqJw7L
Comment 4 Myles C. Maxfield 2019-08-28 10:40:37 PDT
Yikes. Are we calling ScriptItemize incorrectly?
Comment 5 Fujii Hironori 2020-06-18 12:48:12 PDT
[BUG] Webkit font rendering (spacing / icon fonts) · Issue #2626 · microsoft/playwright
https://github.com/microsoft/playwright/issues/2626
Comment 6 Fujii Hironori 2020-07-16 00:54:29 PDT
Playwright's downstream patch: https://github.com/pavelfeldman/webkit/commit/ee5fd605f6d9ea3391317e269ebb5d948e896464
Comment 7 Fujii Hironori 2020-07-16 00:54:55 PDT
Created attachment 404428 [details]
WIP patch
Comment 8 Fujii Hironori 2020-07-16 14:25:59 PDT
Created attachment 404481 [details]
Patch
Comment 9 Fujii Hironori 2020-07-16 18:06:12 PDT
Interestingly, mac and ios testing EWS failed.
I created a ligature font to test the patch.
I don't know this is a Mac bug or my font bug.
I'm going to check it.
Comment 10 Fujii Hironori 2020-07-17 00:52:44 PDT
Created attachment 404547 [details]
Patch
Comment 11 Fujii Hironori 2020-07-19 21:30:25 PDT
Created attachment 404697 [details]
Patch
Comment 12 Don Olmstead 2020-07-21 13:34:59 PDT
Comment on attachment 404697 [details]
Patch

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

r=me with nits

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:187
> +    if (font->platformData().isSystemFont() || font->platformData().hasVariations())

Will variations never hit this same issue? If not then remove it.

> LayoutTests/ChangeLog:10
> +        * fonts/ligature.woff: Added.

Do you maybe want to add where it came from in case of licensing questions in the future.
Comment 13 Fujii Hironori 2020-07-21 13:57:00 PDT
Comment on attachment 404697 [details]
Patch

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

>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:187
>> +    if (font->platformData().isSystemFont() || font->platformData().hasVariations())
> 
> Will variations never hit this same issue? If not then remove it.

Will check.

>> LayoutTests/ChangeLog:10
>> +        * fonts/ligature.woff: Added.
> 
> Do you maybe want to add where it came from in case of licensing questions in the future.

Agreed. It was created by me. It contains only ▲ and ■ glyphs.
Comment 14 Myles C. Maxfield 2020-07-21 14:33:44 PDT
Comment on attachment 404697 [details]
Patch

How is this supposed to work? What do non-browser Windows apps do when they want to use material icons?
Comment 15 Fujii Hironori 2020-07-21 19:24:27 PDT
Myles, you are right. ScriptItemize has a option (fMergeNeutralItems) to control it.

https://docs.microsoft.com/en-us/windows/win32/api/usp10/ns-usp10-script_control
Comment 16 Fujii Hironori 2020-07-21 19:49:05 PDT
Created attachment 404893 [details]
Patch
Comment 17 Fujii Hironori 2020-07-22 13:04:23 PDT
Comment on attachment 404893 [details]
Patch

Clearing flags on attachment: 404893

Committed r264722: <https://trac.webkit.org/changeset/264722>
Comment 18 Fujii Hironori 2020-07-22 13:04:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2020-07-22 13:05:16 PDT
<rdar://problem/65951341>