Bug 199653 – New York font erroneously gets synthetic bold

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

New York erroneously gets synthetic bold

[Myles C. Maxfield]

Created attachment 373813 [details]
WIP

[Myles C. Maxfield]

Created attachment 373863 [details]
WIP

[Myles C. Maxfield]

Created attachment 373883 [details]
Needs tests

[Myles C. Maxfield]

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.

[EWS Watchlist]

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.

[Myles C. Maxfield]

<rdar://problem/51692592>

[Jiang Jiang]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 373941 [details]
Patch

[Myles C. Maxfield]

Running fast/text/design-system-ui-{11,14}.html causes #14 to fail

[Myles C. Maxfield]

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

[Myles C. Maxfield]

window.internals.invalidateFontCache(); in the test appears to fix this.

[Myles C. Maxfield]

Created attachment 373964 [details]
Patch

[Myles C. Maxfield]

Created attachment 373965 [details]
Patch

[Simon Fraser (smfr)]

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

[Myles C. Maxfield]

Committed r247377: <https://trac.webkit.org/changeset/247377>

[Radar WebKit Bug Importer]

<rdar://problem/52982969>

[Aakash Jain]

This seems to break the build. 

e.g.:
failed r247377:
https://build.webkit.org/builders/Apple%20iOS%2012%20Release%20%28Build%29/builds/7431

passed r247375: 
https://build.webkit.org/builders/Apple%20iOS%2012%20Release%20%28Build%29/builds/7430

[Aakash Jain]

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.

[Aakash Jain]

Broke mac os build as well.

[Aakash Jain]

Broke macos build as well: https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20%28Build%29/builds/15784

[Simon Fraser (smfr)]

Why was EWS green?

Fixing.

[Aakash Jain]

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).

[Simon Fraser (smfr)]

Build fix in https://trac.webkit.org/changeset/247379/webkit

[Myles C. Maxfield]

Committed r247462: <https://trac.webkit.org/changeset/247462>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 199816

[Myles C. Maxfield]

Committed r247465: <https://trac.webkit.org/changeset/247465>

[Myles C. Maxfield]

Committed r247482: <https://trac.webkit.org/changeset/247482>