[Cocoa] Google Fonts doesn't work if the user has the requested font locally-installed
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Jul 2018 00:15:20 +0000 (00:15 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Jul 2018 00:15:20 +0000 (00:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187228
<rdar://problem/40967280>

Reviewed by Brent Fulgham.

Source/WebCore:

This is due to the local() items in the src: descriptor in the @font-family blocks.

This is because of a behavior difference between CSSFontFaceSource::load() and CSSFontFaceSource::font().
load() is supposed to set the status() to Success iff the font can be used, and then CSSFontFaceSource::font()
is supposed to return the font itself to use. load() works by constructing a dummy FontDescription and
performing a system lookup (to see if the local font really exists). However, this dummy FontDescription
doesn't set the ShouldAllowUserInstalledFonts flag. Then, in CSSFontFaceSource::font(), a similar lookup is
performed, except this one has the original FontDescription (with the correct value of the
ShouldAllowUserInstalledFonts flag set. Therefore, the two functions disagree about the state of the flag.

When the CSSFontFaceSource's status gets set to Success, that means "this is the font face source that
represents the @font-face block" but when CSSFontFaceSource::font() returns nullptr, that means "The font face
source can't be used for some reason" so we then continue searching down the font-family list (and render the
text in Helvetica or whatever comes next).

The solution is simple - just set the ShouldAllowUserInstalledFonts flag correctly in the dummy
FontDescription.

Test: fast/text/user-installed-fonts/local.html

* css/CSSFontFace.cpp:
(WebCore::CSSFontFace::allowUserInstalledFonts const):
* css/CSSFontFace.h:
* css/CSSFontFaceSet.cpp:
(WebCore::CSSFontFaceSet::ensureLocalFontFacesForFamilyRegistered):
* css/CSSFontFaceSource.cpp:
(WebCore::CSSFontFaceSource::load):

Tools:

The test only fails before the patch if the lookup for Helvetica2 is allowed to occur.

* WebKitTestRunner/mac/TestControllerMac.mm:
(WTR::allowedFontFamilySet):

LayoutTests:

* fast/text/user-installed-fonts/local-expected.html: Added.
* fast/text/user-installed-fonts/local.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233447 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/text/user-installed-fonts/local-expected.html [new file with mode: 0644]
LayoutTests/fast/text/user-installed-fonts/local.html [new file with mode: 0644]
LayoutTests/platform/ios/TestExpectations
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/css/CSSFontFace.cpp
Source/WebCore/css/CSSFontFace.h
Source/WebCore/css/CSSFontFaceSet.cpp
Source/WebCore/css/CSSFontFaceSource.cpp
Tools/ChangeLog
Tools/WebKitTestRunner/mac/TestControllerMac.mm

index aff86c4..edd74ee 100644 (file)
@@ -1,3 +1,14 @@
+2018-07-02  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        [Cocoa] Google Fonts doesn't work if the user has the requested font locally-installed
+        https://bugs.webkit.org/show_bug.cgi?id=187228
+        <rdar://problem/40967280>
+
+        Reviewed by Brent Fulgham.
+
+        * fast/text/user-installed-fonts/local-expected.html: Added.
+        * fast/text/user-installed-fonts/local.html: Added.
+
 2018-07-02  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [WK1] editing/spelling/markers.html is failing on recent builds of macOS Mojave
diff --git a/LayoutTests/fast/text/user-installed-fonts/local-expected.html b/LayoutTests/fast/text/user-installed-fonts/local-expected.html
new file mode 100644 (file)
index 0000000..a65d5b6
--- /dev/null
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+@font-face {
+    font-family: "WebFont";
+    src: url("../../../resources/Ahem.otf");
+}
+</style>
+</head>
+<body>
+This test makes sure that local() doesn't find a user-installed font, and that
+local() will be skipped if it lists a user-installed font.
+The test passes if you see some black boxes below (and no text below).
+<div style="font: 48px 'WebFont';">Hello, World</div>
+</body>
+</html>
diff --git a/LayoutTests/fast/text/user-installed-fonts/local.html b/LayoutTests/fast/text/user-installed-fonts/local.html
new file mode 100644 (file)
index 0000000..2638031
--- /dev/null
@@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.testRunner)
+    testRunner.installFakeHelvetica("Helvetica2-400");
+if (window.internals)
+    internals.settings.setShouldAllowUserInstalledFonts(false);
+</script>
+<style>
+@font-face {
+    font-family: "WebFont";
+    src: local("Helvetica2"), url("../../../resources/Ahem.otf");
+}
+</style>
+</head>
+<body>
+This test makes sure that local() doesn't find a user-installed font, and that
+local() will be skipped if it lists a user-installed font.
+The test passes if you see some black boxes below (and no text below).
+<div style="font: 48px 'WebFont', 'AmericanTypewriter';">Hello, World</div>
+</body>
+</html>
index 808da13..1f6baa2 100644 (file)
@@ -3278,6 +3278,7 @@ webkit.org/b/180062 fast/text/user-installed-fonts/shadow-family.html [ ImageOnl
 webkit.org/b/180062 fast/text/user-installed-fonts/shadow-postscript.html [ ImageOnlyFailure ]
 webkit.org/b/180062 fast/text/user-installed-fonts/shadow.html [ ImageOnlyFailure ]
 webkit.org/b/180062 fast/text/user-installed-fonts/shadow-postscript-family.html [ ImageOnlyFailure ]
+webkit.org/b/187228 fast/text/user-installed-fonts/local.html [ ImageOnlyFailure ]
 
 webkit.org/b/181838 js/slow-stress/Int32Array-alloc-huge-long-lived.html [ Slow ]
 
index 10a90be..97b75cb 100644 (file)
@@ -1688,6 +1688,8 @@ webkit.org/b/176693 storage/indexeddb/modern/idbtransaction-objectstore-failures
 webkit.org/b/180062 [ ElCapitan Sierra HighSierra ] fast/text/user-installed-fonts/disable.html [ ImageOnlyFailure ]
 webkit.org/b/180062 fast/text/user-installed-fonts/shadow-postscript.html [ ImageOnlyFailure ]
 webkit.org/b/180062 fast/text/user-installed-fonts/shadow.html [ ImageOnlyFailure ]
+webkit.org/b/187228 [ ElCapitan Sierra HighSierra ] fast/text/user-installed-fonts/local.html [ ImageOnlyFailure ]
+
 
 webkit.org/b/180560 accessibility/mac/html5-input-number.html [ Pass Failure ]
 
index 956e62e..b07dd97 100644 (file)
@@ -1,3 +1,39 @@
+2018-07-02  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        [Cocoa] Google Fonts doesn't work if the user has the requested font locally-installed
+        https://bugs.webkit.org/show_bug.cgi?id=187228
+        <rdar://problem/40967280>
+
+        Reviewed by Brent Fulgham.
+
+        This is due to the local() items in the src: descriptor in the @font-family blocks.
+
+        This is because of a behavior difference between CSSFontFaceSource::load() and CSSFontFaceSource::font().
+        load() is supposed to set the status() to Success iff the font can be used, and then CSSFontFaceSource::font()
+        is supposed to return the font itself to use. load() works by constructing a dummy FontDescription and
+        performing a system lookup (to see if the local font really exists). However, this dummy FontDescription
+        doesn't set the ShouldAllowUserInstalledFonts flag. Then, in CSSFontFaceSource::font(), a similar lookup is
+        performed, except this one has the original FontDescription (with the correct value of the
+        ShouldAllowUserInstalledFonts flag set. Therefore, the two functions disagree about the state of the flag.
+
+        When the CSSFontFaceSource's status gets set to Success, that means "this is the font face source that
+        represents the @font-face block" but when CSSFontFaceSource::font() returns nullptr, that means "The font face
+        source can't be used for some reason" so we then continue searching down the font-family list (and render the
+        text in Helvetica or whatever comes next).
+
+        The solution is simple - just set the ShouldAllowUserInstalledFonts flag correctly in the dummy
+        FontDescription.
+
+        Test: fast/text/user-installed-fonts/local.html
+
+        * css/CSSFontFace.cpp:
+        (WebCore::CSSFontFace::allowUserInstalledFonts const):
+        * css/CSSFontFace.h:
+        * css/CSSFontFaceSet.cpp:
+        (WebCore::CSSFontFaceSet::ensureLocalFontFacesForFamilyRegistered):
+        * css/CSSFontFaceSource.cpp:
+        (WebCore::CSSFontFaceSource::load):
+
 2018-06-29  Ryosuke Niwa  <rniwa@webkit.org>
 
         Generate event and event target interface types directly instead of via macros
index f7d775f..cdec5a3 100644 (file)
@@ -582,6 +582,13 @@ void CSSFontFace::adoptSource(std::unique_ptr<CSSFontFaceSource>&& source)
     ASSERT(!m_sourcesPopulated);
 }
 
+AllowUserInstalledFonts CSSFontFace::allowUserInstalledFonts() const
+{
+    if (m_fontSelector && m_fontSelector->document())
+        return m_fontSelector->document()->settings().shouldAllowUserInstalledFonts() ? AllowUserInstalledFonts::Yes : AllowUserInstalledFonts::No;
+    return AllowUserInstalledFonts::Yes;
+}
+
 static Settings::FontLoadTimingOverride fontLoadTimingOverride(CSSFontSelector* fontSelector)
 {
     auto overrideValue = Settings::FontLoadTimingOverride::None;
index adefcbe..d0aa769 100644 (file)
@@ -161,6 +161,8 @@ public:
 
     bool purgeable() const;
 
+    AllowUserInstalledFonts allowUserInstalledFonts() const;
+
     void updateStyleIfNeeded();
 
 #if ENABLE(SVG_FONTS)
index b810403..66c78d5 100644 (file)
@@ -113,7 +113,7 @@ void CSSFontFaceSet::ensureLocalFontFacesForFamilyRegistered(const String& famil
 
     Vector<Ref<CSSFontFace>> faces;
     for (auto item : capabilities) {
-        Ref<CSSFontFace> face = CSSFontFace::create(nullptr, nullptr, nullptr, true);
+        Ref<CSSFontFace> face = CSSFontFace::create(m_owningFontSelector, nullptr, nullptr, true);
         
         Ref<CSSValueList> familyList = CSSValueList::createCommaSeparated();
         familyList->append(CSSValuePool::singleton().createFontFamilyValue(familyName));
index 1092213..4d603b5 100644 (file)
@@ -179,6 +179,7 @@ void CSSFontFaceSource::load(CSSFontSelector* fontSelector)
             FontCascadeDescription fontDescription;
             fontDescription.setOneFamily(m_familyNameOrURI);
             fontDescription.setComputedSize(1);
+            fontDescription.setShouldAllowUserInstalledFonts(m_face.allowUserInstalledFonts());
             success = FontCache::singleton().fontForFamily(fontDescription, m_familyNameOrURI, nullptr, nullptr, FontSelectionSpecifiedCapabilities(), true);
         }
         setStatus(success ? Status::Success : Status::Failure);
index 9fd5ae8..c1ec18c 100644 (file)
@@ -1,3 +1,16 @@
+2018-07-02  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        [Cocoa] Google Fonts doesn't work if the user has the requested font locally-installed
+        https://bugs.webkit.org/show_bug.cgi?id=187228
+        <rdar://problem/40967280>
+
+        Reviewed by Brent Fulgham.
+
+        The test only fails before the patch if the lookup for Helvetica2 is allowed to occur.
+
+        * WebKitTestRunner/mac/TestControllerMac.mm:
+        (WTR::allowedFontFamilySet):
+
 2018-07-02  Ryosuke Niwa  <rniwa@webkit.org>
 
         run-benchmark should include the version number of MotionMark's results
index e800948..f9dd39d 100644 (file)
@@ -212,6 +212,7 @@ static NSSet *allowedFontFamilySet()
         @"Helvetica CY",
         @"Helvetica Neue",
         @"Helvetica",
+        @"Helvetica2",
         @"Herculanum",
         @"Hiragino Kaku Gothic Pro",
         @"Hiragino Kaku Gothic ProN",