Skia lighter wght variation looks bolder than regular
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Dec 2016 04:44:01 +0000 (04:44 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Dec 2016 04:44:01 +0000 (04:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165948

Reviewed by Antti Koivisto.

Source/WebCore:

Test: fast/text/variations/default-value.html

This patch inspects the font's information regarding variations. It uses this information
to work around a bug in CoreText where default variation values were not getting applied.
This workaround is placed behind a version check and the macro name
"WORKAROUND_CORETEXT_VARIATIONS_DEFAULT_VALUE_BUG" so we know to delete it whenever
possible. It also uses the minimum and maximum supported values for the axis to clamp our
variation values to the closest supported point, which is in line with a recent edit to
the fonts spec:
https://github.com/w3c/csswg-drafts/commit/52b802ac38619286a30662dceb71b8a29fa72f42
This clamping behavior also revealed another bug in CoreText, which was worked around
behind another version check and macro name WORKAROUND_CORETEXT_VARIATIONS_EXTENTS_BUG so
we know to delete it whenever possible.

* platform/graphics/cocoa/FontCacheCoreText.cpp:
(WebCore::defaultVariationValues):
(WebCore::preparePlatformFont):

LayoutTests:

* fast/text/variations/default-value-expected.html: Added.
* fast/text/variations/default-value.html: Added.
* fast/text/variations/outofbounds-expected-mismatch.html: Renamed from LayoutTests/fast/text/variations/outofbounds-expected.html.
* fast/text/variations/outofbounds.html:
* platform/ios-simulator/TestExpectations:

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

LayoutTests/ChangeLog
LayoutTests/fast/text/variations/default-value-expected.html [new file with mode: 0644]
LayoutTests/fast/text/variations/default-value.html [new file with mode: 0644]
LayoutTests/fast/text/variations/outofbounds-expected-mismatch.html [moved from LayoutTests/fast/text/variations/outofbounds-expected.html with 64% similarity]
LayoutTests/fast/text/variations/outofbounds.html
LayoutTests/platform/ios-simulator/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp

index 2ed1322..2cd422f 100644 (file)
@@ -1,3 +1,16 @@
+2016-12-20  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Skia lighter wght variation looks bolder than regular
+        https://bugs.webkit.org/show_bug.cgi?id=165948
+
+        Reviewed by Antti Koivisto.
+
+        * fast/text/variations/default-value-expected.html: Added.
+        * fast/text/variations/default-value.html: Added.
+        * fast/text/variations/outofbounds-expected-mismatch.html: Renamed from LayoutTests/fast/text/variations/outofbounds-expected.html.
+        * fast/text/variations/outofbounds.html:
+        * platform/ios-simulator/TestExpectations:
+
 2016-12-20  Keith Miller  <keith_miller@apple.com>
 
         Add support for global
diff --git a/LayoutTests/fast/text/variations/default-value-expected.html b/LayoutTests/fast/text/variations/default-value-expected.html
new file mode 100644 (file)
index 0000000..8f77c6f
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.internals)
+   window.internals.settings.setVariationFontsEnabled(true);
+</script>
+</head>
+<body>
+This test makes sure that default values of variations get applied correctly. The test fails if the text below isn't thin.
+<div style="font: 70px 'Skia'; font-variation-settings: 'wght' 1.000001;">hamburgefonstiv</div>
+</body>
+<html>
diff --git a/LayoutTests/fast/text/variations/default-value.html b/LayoutTests/fast/text/variations/default-value.html
new file mode 100644 (file)
index 0000000..e81cb61
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.internals)
+   window.internals.settings.setVariationFontsEnabled(true);
+</script>
+</head>
+<body>
+This test makes sure that default values of variations get applied correctly. The test fails if the text below isn't thin.
+<div style="font: 70px 'Skia'; font-variation-settings: 'wght' 1.0;">hamburgefonstiv</div>
+</body>
+<html>
@@ -7,6 +7,6 @@ if (window.internals)
 </script>
 </head>
 <body>
-<div style="font-family: 'Skia';">Test passes if this text is not bold.</div>
+<div style="font-family: 'Skia';">Test passes if this text is bold.</div>
 </body>
 </html>
\ No newline at end of file
index c92b67d..ad926e7 100644 (file)
@@ -7,6 +7,6 @@ if (window.internals)
 </script>
 </head>
 <body>
-<div style="font-family: 'Skia'; font-variation-settings: 'wght' 27;">Test passes if this text is not bold.</div>
+<div style="font-family: 'Skia'; font-variation-settings: 'wght' 27;">Test passes if this text is bold.</div>
 </body>
 </html>
\ No newline at end of file
index 6f36e72..0a00e3c 100644 (file)
@@ -2713,6 +2713,8 @@ webkit.org/b/162739 fast/images/gif-loop-count.html [ Pass ImageOnlyFailure ]
 
 # This variation font test requires Skia which isn't available on iOS.
 webkit.org/b/163093 fast/text/variations/advances.html [ Failure ]
+webkit.org/b/163093 fast/text/variations/outofbounds.html [ ImageOnlyFailure ]
+webkit.org/b/163093 fast/text/variations/default-value.html [ ImageOnlyFailure ]
 
 webkit.org/b/162647 http/tests/xmlhttprequest/onabort-response-getters.html [ Pass Failure ]
 
index bb66744..7f5a72b 100644 (file)
@@ -1,3 +1,28 @@
+2016-12-20  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Skia lighter wght variation looks bolder than regular
+        https://bugs.webkit.org/show_bug.cgi?id=165948
+
+        Reviewed by Antti Koivisto.
+
+        Test: fast/text/variations/default-value.html
+
+        This patch inspects the font's information regarding variations. It uses this information
+        to work around a bug in CoreText where default variation values were not getting applied.
+        This workaround is placed behind a version check and the macro name
+        "WORKAROUND_CORETEXT_VARIATIONS_DEFAULT_VALUE_BUG" so we know to delete it whenever
+        possible. It also uses the minimum and maximum supported values for the axis to clamp our
+        variation values to the closest supported point, which is in line with a recent edit to
+        the fonts spec:
+        https://github.com/w3c/csswg-drafts/commit/52b802ac38619286a30662dceb71b8a29fa72f42
+        This clamping behavior also revealed another bug in CoreText, which was worked around
+        behind another version check and macro name WORKAROUND_CORETEXT_VARIATIONS_EXTENTS_BUG so
+        we know to delete it whenever possible.
+
+        * platform/graphics/cocoa/FontCacheCoreText.cpp:
+        (WebCore::defaultVariationValues):
+        (WebCore::preparePlatformFont):
+
 2016-12-20  Tim Horton  <timothy_horton@apple.com>
 
         Remove a duplicate reference to ScrollingMomentumCalculatorMac.h in the Xcode project
index 284958e..b0d6408 100644 (file)
@@ -363,6 +363,66 @@ static FeaturesMap computeFeatureSettingsFromVariants(const FontVariantSettings&
     return result;
 }
 
+#if ENABLE(VARIATION_FONTS)
+struct VariationDefaults {
+    float defaultValue;
+    float minimumValue;
+    float maximumValue;
+};
+
+typedef HashMap<FontTag, VariationDefaults, FourCharacterTagHash, FourCharacterTagHashTraits> VariationDefaultsMap;
+
+static VariationDefaultsMap defaultVariationValues(CTFontRef font)
+{
+    VariationDefaultsMap result;
+    auto axes = adoptCF(CTFontCopyVariationAxes(font));
+    if (!axes)
+        return result;
+    auto size = CFArrayGetCount(axes.get());
+    for (CFIndex i = 0; i < size; ++i) {
+        CFDictionaryRef axis = static_cast<CFDictionaryRef>(CFArrayGetValueAtIndex(axes.get(), i));
+        CFNumberRef axisIdentifier = static_cast<CFNumberRef>(CFDictionaryGetValue(axis, kCTFontVariationAxisIdentifierKey));
+        CFNumberRef defaultValue = static_cast<CFNumberRef>(CFDictionaryGetValue(axis, kCTFontVariationAxisDefaultValueKey));
+        CFNumberRef minimumValue = static_cast<CFNumberRef>(CFDictionaryGetValue(axis, kCTFontVariationAxisMinimumValueKey));
+        CFNumberRef maximumValue = static_cast<CFNumberRef>(CFDictionaryGetValue(axis, kCTFontVariationAxisMaximumValueKey));
+        int64_t rawAxisIdentifier = 0;
+        Boolean success = CFNumberGetValue(axisIdentifier, kCFNumberSInt64Type, &rawAxisIdentifier);
+        ASSERT_UNUSED(success, success);
+        float rawDefaultValue = 0;
+        float rawMinimumValue = 0;
+        float rawMaximumValue = 0;
+        CFNumberGetValue(defaultValue, kCFNumberFloatType, &rawDefaultValue);
+        CFNumberGetValue(minimumValue, kCFNumberFloatType, &rawMinimumValue);
+        CFNumberGetValue(maximumValue, kCFNumberFloatType, &rawMaximumValue);
+
+        // FIXME: Remove when <rdar://problem/28893836> is fixed
+#define WORKAROUND_CORETEXT_VARIATIONS_EXTENTS_BUG (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)
+#if WORKAROUND_CORETEXT_VARIATIONS_EXTENTS_BUG
+        float epsilon = 0.001;
+        rawMinimumValue += epsilon;
+        rawMaximumValue -= epsilon;
+#endif
+#undef WORKAROUND_CORETEXT_VARIATIONS_EXTENTS_BUG
+
+        if (rawMinimumValue > rawMaximumValue)
+            std::swap(rawMinimumValue, rawMaximumValue);
+
+        auto b1 = rawAxisIdentifier >> 24;
+        auto b2 = (rawAxisIdentifier & 0xFF0000) >> 16;
+        auto b3 = (rawAxisIdentifier & 0xFF00) >> 8;
+        auto b4 = rawAxisIdentifier & 0xFF;
+        ASSERT(b1 >= 0 && b1 <= std::numeric_limits<char>::max());
+        ASSERT(b2 >= 0 && b2 <= std::numeric_limits<char>::max());
+        ASSERT(b3 >= 0 && b3 <= std::numeric_limits<char>::max());
+        ASSERT(b4 >= 0 && b4 <= std::numeric_limits<char>::max());
+        FontTag resultKey = {{ static_cast<char>(b1), static_cast<char>(b2), static_cast<char>(b3), static_cast<char>(b4) }};
+        VariationDefaults resultValues = { rawDefaultValue, rawMinimumValue, rawMaximumValue };
+        result.set(resultKey, resultValues);
+    }
+    return result;
+}
+#endif
+
 RetainPtr<CTFontRef> preparePlatformFont(CTFontRef originalFont, TextRenderingMode textRenderingMode, const FontFeatureSettings* fontFaceFeatures, const FontVariantSettings* fontFaceVariantSettings, const FontFeatureSettings& features, const FontVariantSettings& variantSettings, const FontVariationSettings& variations)
 {
     if (!originalFont || (!features.size() && variations.isEmpty() && (textRenderingMode == AutoTextRendering) && variantSettings.isAllNormal()
@@ -402,9 +462,25 @@ RetainPtr<CTFontRef> preparePlatformFont(CTFontRef originalFont, TextRenderingMo
         featuresToBeApplied.set(newFeature.tag(), newFeature.value());
 
 #if ENABLE(VARIATION_FONTS)
+    auto defaultValues = defaultVariationValues(originalFont);
+
     VariationsMap variationsToBeApplied;
-    for (auto& newVariation : variations)
-        variationsToBeApplied.set(newVariation.tag(), newVariation.value());
+    for (auto& newVariation : variations) {
+        auto iterator = defaultValues.find(newVariation.tag());
+        if (iterator != defaultValues.end()) {
+            float valueToApply = std::max(std::min(newVariation.value(), iterator->value.maximumValue), iterator->value.minimumValue);
+
+            // FIXME: Remove when <rdar://problem/28707822> is fixed
+#define WORKAROUND_CORETEXT_VARIATIONS_DEFAULT_VALUE_BUG (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)
+#if WORKAROUND_CORETEXT_VARIATIONS_DEFAULT_VALUE_BUG
+            if (valueToApply == iterator->value.defaultValue)
+                valueToApply += 0.0001;
+#endif
+#undef WORKAROUND_CORETEXT_VARIATIONS_DEFAULT_VALUE_BUG
+
+            variationsToBeApplied.set(newVariation.tag(), valueToApply);
+        }
+    }
 #endif
 
     RetainPtr<CFMutableDictionaryRef> attributes = adoptCF(CFDictionaryCreateMutable(kCFAllocatorDefault, 2, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));