Bug 194895 - Incorrect use of String::foldCase for font family names
Summary: Incorrect use of String::foldCase for font family names
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-20 21:30 PST by Darin Adler
Modified: 2019-02-25 11:52 PST (History)
2 users (show)

See Also:


Attachments
Patch (2.71 KB, patch)
2019-02-20 21:33 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (6.52 KB, patch)
2019-02-20 21:52 PST, Darin Adler
mmaxfield: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2019-02-20 21:30:00 PST
Incorrect use of String::foldCase for font family names
Comment 1 Darin Adler 2019-02-20 21:33:26 PST Comment hidden (obsolete)
Comment 2 Darin Adler 2019-02-20 21:34:11 PST
Myles, could you review this?
Comment 3 Darin Adler 2019-02-20 21:52:13 PST
Created attachment 362591 [details]
Patch
Comment 4 Myles C. Maxfield 2019-02-21 19:35:50 PST
Comment on attachment 362591 [details]
Patch

Looks good. https://drafts.csswg.org/css-fonts-4/#localized-name-matching says font names can be non-ascii but it looks like this code handles that case correctly.
Comment 5 Darin Adler 2019-02-21 19:43:25 PST
(In reply to Myles C. Maxfield from comment #4)
> Looks good. https://drafts.csswg.org/css-fonts-4/#localized-name-matching
> says font names can be non-ascii but it looks like this code handles that
> case correctly.

Actually, that draft says that we need to do full Unicode case folding. If we wanted to conform to that, then we’d have to change our hash and equality computation to use full Unicode case folding, too. We don’t have an efficient implementation of that but we could still do it.

My patch goes the other direction and does only ASCII case insensitive matching, which won’t fold non-ASCII characters. In most cases this is what we want, but the CSS specification specifically calls for the full case folding, sort of the opposite of what my patch does!

Now I don’t know what to do.
Comment 6 Darin Adler 2019-02-21 19:56:44 PST
Comment on attachment 362591 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCascadeDescription.cpp:127
>      return ASCIICaseInsensitiveHash::equal(family1, family2);

Writing a version of this for the Unicode case insensitivity algorithm would probably require using u_strCaseCompare. We don’t have any helpers for this because it’s not done anywhere else in WebKit, intentionally.

> Source/WebCore/platform/graphics/FontCascadeDescription.cpp:136
>      return ASCIICaseInsensitiveHash::hash(family);

Writing a version of this for the Unicode case insensitivity algorithm would be quite difficult. Might just need to call foldCase and then call hash, which would allocate/deallocate memory and be pretty slow!
Comment 7 Darin Adler 2019-02-21 19:59:57 PST
I wonder if we could get the standard changed. Requiring full case folding doesn’t really match the rest of the web platform. Things tend to be either case sensitive or ASCII case insensitive. Full Unicode case insensitivity is almost unheard of.
Comment 8 Darin Adler 2019-02-25 06:39:14 PST
Committed r242038: <https://trac.webkit.org/changeset/242038>
Comment 9 Darin Adler 2019-02-25 06:41:01 PST
Despite these issues I decided to land as-is. A correct implementation of ASCII case insensitivity seems better than an inconsistent implementation for now.

Outstanding issues remain: The specification says it should be full Unicode case folding. It's likely important that this logic exactly match what the underlying CoreText font name matching algorithm is on macOS and iOS. The CSS specification also talks about support for multiple names for the same font.
Comment 10 Radar WebKit Bug Importer 2019-02-25 06:41:58 PST
<rdar://problem/48361667>
Comment 11 Myles C. Maxfield 2019-02-25 11:43:32 PST
(In reply to Darin Adler from comment #9)
> Despite these issues I decided to land as-is. A correct implementation of
> ASCII case insensitivity seems better than an inconsistent implementation
> for now.
> 
> Outstanding issues remain: The specification says it should be full Unicode
> case folding. It's likely important that this logic exactly match what the
> underlying CoreText font name matching algorithm is on macOS and iOS. The
> CSS specification also talks about support for multiple names for the same
> font.

There’s a CSS F2F meeting this week; I’ll bring this up there. I agree with you about the desired behavior.
Comment 12 Myles C. Maxfield 2019-02-25 11:52:07 PST
https://github.com/w3c/csswg-drafts/issues/3675