Bug 199653 - New York font erroneously gets synthetic bold
Summary: New York font erroneously gets synthetic bold
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 199816
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-09 19:53 PDT by Myles C. Maxfield
Modified: 2019-07-16 09:18 PDT (History)
12 users (show)

See Also:


Attachments
WIP (20.27 KB, patch)
2019-07-09 19:53 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (19.39 KB, patch)
2019-07-10 13:34 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Needs tests (52.21 KB, patch)
2019-07-10 18:04 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (86.61 KB, patch)
2019-07-11 13:08 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (87.64 KB, patch)
2019-07-11 15:29 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (87.71 KB, patch)
2019-07-11 15:32 PDT, Myles C. Maxfield
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-07-09 19:53:09 PDT
New York erroneously gets synthetic bold
Comment 1 Myles C. Maxfield 2019-07-09 19:53:28 PDT
Created attachment 373813 [details]
WIP
Comment 2 Myles C. Maxfield 2019-07-10 13:34:43 PDT
Created attachment 373863 [details]
WIP
Comment 3 Myles C. Maxfield 2019-07-10 18:04:43 PDT
Created attachment 373883 [details]
Needs tests
Comment 4 Myles C. Maxfield 2019-07-10 18:05:22 PDT
One thing I shouldn't forget to do is to add code to catch the dot-prefixed name to make sure it explicitly doesn't work.
Comment 5 Build Bot 2019-07-10 18:05:47 PDT
Attachment 373883 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Myles C. Maxfield 2019-07-10 18:19:33 PDT
<rdar://problem/51692592>
Comment 7 Jiang Jiang 2019-07-10 23:12:13 PDT
Comment on attachment 373883 [details]
Needs tests

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

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:60
> +    auto result = adoptCF(CTFontCreateUIFontForLanguage(kCTFontUIFontSystem, parameters.size, locale));

Shouldn't need to start with this.

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:85
> +    // FIXME: Use applyWeightItalicsAndFallbackBehavior() once <rdar://problem/33046041> is fixed.

Isn't this fixed long long time ago?

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:141
> +    CFTypeRef traitsValues[] = { weightNumber.get(), italicsNumber.get(), design ? static_cast<CFTypeRef>(design) : static_cast<CFTypeRef>(kCFBooleanTrue) };

We shouldn't use kCFBooleanTrue as the value in any case, either left design out, or use kCTFontUIFontDesignDefault.
Comment 8 Myles C. Maxfield 2019-07-11 13:02:35 PDT
Comment on attachment 373883 [details]
Needs tests

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

>> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:85
>> +    // FIXME: Use applyWeightItalicsAndFallbackBehavior() once <rdar://problem/33046041> is fixed.
> 
> Isn't this fixed long long time ago?

I'll remove this in a subsequent patch.
Comment 9 Myles C. Maxfield 2019-07-11 13:08:14 PDT
Created attachment 373941 [details]
Patch
Comment 10 Myles C. Maxfield 2019-07-11 14:06:18 PDT
Running fast/text/design-system-ui-{11,14}.html causes #14 to fail
Comment 11 Myles C. Maxfield 2019-07-11 14:15:41 PDT
file:///Users/mmaxfield/src/WebKit/OpenSource/LayoutTests/fast/text/design-system-ui-11.html
file:///Users/mmaxfield/src/WebKit/OpenSource/LayoutTests/fast/text/design-system-ui-14.html
Comment 12 Myles C. Maxfield 2019-07-11 14:20:31 PDT
window.internals.invalidateFontCache(); in the test appears to fix this.
Comment 13 Myles C. Maxfield 2019-07-11 15:29:56 PDT
Created attachment 373964 [details]
Patch
Comment 14 Myles C. Maxfield 2019-07-11 15:32:09 PDT
Created attachment 373965 [details]
Patch
Comment 15 Simon Fraser (smfr) 2019-07-11 16:18:21 PDT
Comment on attachment 373965 [details]
Patch

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

> Source/WTF/ChangeLog:3
> +        New York erroneously gets synthetic bold

The state, or the city? Oh, you mean the font? Please say so.

> Source/WebCore/ChangeLog:15
> +        WKPreferencesSetShouldAllowDesignSystemUIFonts(true)

Nope

> Source/WebCore/ChangeLog:16
> +        or -[WKPreferences _shouldAllowDesignSystemUIFonts] = YES.

OK

> Source/WTF/wtf/Platform.h:1598
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || (PLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 60000) || (PLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 130000)

I have no idea.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:57
> +#if HAVE(DESIGN_SYSTEM_UI_FONTS)

Blank line above please.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:112
> +        if (auto use = systemFontUse(cssFamily, shouldAllowDesignSystemUIFonts()))

So hard to distinguish between 'use' noun, and 'use' verb.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:131
> +        if (auto use = systemFontUse(cssFamily, shouldAllowDesignSystemUIFonts())) {

Ditto.

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:50
> +RetainPtr<CTFontRef> SystemFontDatabaseCoreText::createSystemUI(const CascadeListParameters& parameters, CFStringRef locale)

createSystemUIFont?

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:58
> +RetainPtr<CTFontRef> SystemFontDatabaseCoreText::createDesignSystemUI(ClientUse clientUse, const CascadeListParameters& parameters)

createSystemUIFont?

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:78
> +RetainPtr<CTFontRef> SystemFontDatabaseCoreText::createTextStyle(const CascadeListParameters& parameters)

It makes a font, not a text style.

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:132
> +RetainPtr<CTFontRef> SystemFontDatabaseCoreText::applyWeightItalicsAndFallbackBehavior(CTFontRef font, CGFloat weight, bool italic, float size, AllowUserInstalledFonts allowUserInstalledFonts, CFStringRef design)

This should be called createFontByApplying...

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:214
> +        static NeverDestroyed<AtomString> systemUI = AtomString("system-ui-serif", AtomString::ConstructFromLiteral);

systemUI -> systemUISerif

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:219
> +        static NeverDestroyed<AtomString> systemUI = AtomString("system-ui-monospaced", AtomString::ConstructFromLiteral);

systemUI -> systemUIMonospaced

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:224
> +        static NeverDestroyed<AtomString> systemUI = AtomString("system-ui-rounded", AtomString::ConstructFromLiteral);

systemUI -> systemUIRounded

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.h:99
> +    enum class ClientUse {

: uint8_t if anyone stores it.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:111
> +#if HAVE(DESIGN_SYSTEM_UI_FONTS)
> +    if (fontDescription.shouldAllowDesignSystemUIFonts()) {
> +        Optional<SystemFontDatabaseCoreText::ClientUse> designSystemUI;
> +        if (equalLettersIgnoringASCIICase(family, "-apple-system-ui-serif"))
> +            designSystemUI = SystemFontDatabaseCoreText::ClientUse::ForSystemUISerif;
> +        else if (equalLettersIgnoringASCIICase(family, "-apple-system-ui-monospaced"))
> +            designSystemUI = SystemFontDatabaseCoreText::ClientUse::ForSystemUIMonospaced;
> +        else if (equalLettersIgnoringASCIICase(family, "-apple-system-ui-rounded"))
> +            designSystemUI = SystemFontDatabaseCoreText::ClientUse::ForSystemUIRounded;
> +
> +        if (designSystemUI) {
> +            auto cascadeList = SystemFontDatabaseCoreText::singleton().cascadeList(fontDescription, family, *designSystemUI, allowUserInstalledFonts);
> +            if (!cascadeList.isEmpty())
> +                return createFontForInstalledFonts(cascadeList[0].get(), size, allowUserInstalledFonts);
> +        }
> +    }
> +#endif

Need to share more of this code between macOS and iOS.

> Source/WebCore/svg/graphics/SVGImage.cpp:474
> +        m_page->settings().setShouldAllowDesignSystemUIFonts(false);

It's false by default so no need to do anything.

> Source/WebCore/testing/InternalSettings.cpp:88
> +    , m_shouldAllowDesignSystemUIFonts(settings.shouldAllowDesignSystemUIFonts())

Remove

> Source/WebCore/testing/InternalSettings.cpp:188
> +    settings.setShouldAllowDesignSystemUIFonts(m_shouldAllowDesignSystemUIFonts);

Remove

> Source/WebCore/testing/InternalSettings.cpp:704
> +ExceptionOr<void> InternalSettings::setShouldAllowDesignSystemUIFonts(bool allow)
> +{
> +    if (!m_page)
> +        return Exception { InvalidAccessError };
> +    settings().setShouldAllowDesignSystemUIFonts(allow);
> +    return { };
> +}

Remove

> Source/WebCore/testing/InternalSettings.h:89
> +    ExceptionOr<void> setShouldAllowDesignSystemUIFonts(bool);

Remove

> Source/WebCore/testing/InternalSettings.h:189
> +        bool m_shouldAllowDesignSystemUIFonts;

Remove

> Source/WebCore/testing/InternalSettings.idl:49
> +    [MayThrowException] void setShouldAllowDesignSystemUIFonts(boolean ignore);

This shouldn't be necessary. All Settings can be changed from tests already.

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:576
> +WK_EXPORT void WKPreferencesSetShouldAllowDesignSystemUIFonts(WKPreferencesRef, bool flag);
> +WK_EXPORT bool WKPreferencesGetShouldAllowDesignSystemUIFonts(WKPreferencesRef);

I don't know why we're still adding to the C SPI
Comment 16 Myles C. Maxfield 2019-07-11 18:08:35 PDT
Committed r247377: <https://trac.webkit.org/changeset/247377>
Comment 17 Radar WebKit Bug Importer 2019-07-11 18:09:21 PDT
<rdar://problem/52982969>
Comment 19 Aakash Jain 2019-07-11 19:01:21 PDT
FontCacheCoreText.cpp:1250:73: error: unused parameter 'family' [-Werror,-Wunused-parameter]
FontCacheCoreText.cpp:1250:104: error: unused parameter 'fontDescription' [-Werror,-Wunused-parameter]
FontCacheCoreText.cpp:1250:127: error: unused parameter 'size' [-Werror,-Wunused-parameter]
FontCacheCoreText.cpp:1250:157: error: unused parameter 'allowUserInstalledFonts' [-Werror,-Wunused-parameter]


In file included from /Volumes/Data/slave/ios-12-release/build/WebKitBuild/Release-iphoneos/DerivedSources/WebCore/unified-sources/UnifiedSource360.cpp:8:
./platform/graphics/cocoa/FontCacheCoreText.cpp:1250:73: error: unused parameter 'family' [-Werror,-Wunused-parameter]
static RetainPtr<CTFontRef> fontWithFamilySpecialCase(const AtomString& family, const FontDescription& fontDescription, float size, AllowUserInstalledFonts allowUserInstalledFonts)
                                                                        ^
./platform/graphics/cocoa/FontCacheCoreText.cpp:1250:104: error: unused parameter 'fontDescription' [-Werror,-Wunused-parameter]
static RetainPtr<CTFontRef> fontWithFamilySpecialCase(const AtomString& family, const FontDescription& fontDescription, float size, AllowUserInstalledFonts allowUserInstalledFonts)
                                                                                                       ^
./platform/graphics/cocoa/FontCacheCoreText.cpp:1250:127: error: unused parameter 'size' [-Werror,-Wunused-parameter]
static RetainPtr<CTFontRef> fontWithFamilySpecialCase(const AtomString& family, const FontDescription& fontDescription, float size, AllowUserInstalledFonts allowUserInstalledFonts)
                                                                                                                              ^
./platform/graphics/cocoa/FontCacheCoreText.cpp:1250:157: error: unused parameter 'allowUserInstalledFonts' [-Werror,-Wunused-parameter]
static RetainPtr<CTFontRef> fontWithFamilySpecialCase(const AtomString& family, const FontDescription& fontDescription, float size, AllowUserInstalledFonts allowUserInstalledFonts)
                                                                                                                                                            ^
4 errors generated.
Comment 20 Aakash Jain 2019-07-11 19:06:07 PDT Comment hidden (obsolete)
Comment 22 Simon Fraser (smfr) 2019-07-11 19:10:24 PDT
Why was EWS green?

Fixing.
Comment 23 Aakash Jain 2019-07-11 19:11:40 PDT
The commit in  r247377 seems substantially different from the last uploaded patch (373965). Might be good idea to atleast run the patch through EWS before landing (if not using commit-queue).
Comment 24 Simon Fraser (smfr) 2019-07-11 19:32:32 PDT
Build fix in https://trac.webkit.org/changeset/247379/webkit
Comment 25 Myles C. Maxfield 2019-07-15 17:18:04 PDT
Committed r247462: <https://trac.webkit.org/changeset/247462>
Comment 26 WebKit Commit Bot 2019-07-15 17:21:47 PDT
Re-opened since this is blocked by bug 199816
Comment 27 Myles C. Maxfield 2019-07-15 17:54:03 PDT
Committed r247465: <https://trac.webkit.org/changeset/247465>
Comment 28 Myles C. Maxfield 2019-07-16 09:18:11 PDT
Committed r247482: <https://trac.webkit.org/changeset/247482>