Skip to content
Permalink
Browse files
Delay system font shorthand resolution until after parsing
https://bugs.webkit.org/show_bug.cgi?id=241454

Reviewed by Antti Koivisto.

This is the fifth piece of https://bugs.webkit.org/show_bug.cgi?id=237817, and is the main crux
of the fix to that bug. When content says something like "font: caption" or "font: -apple-system-body"
we have to map that to CSS properties like font-size and font-weight, so inheritance works properly
across different elements. On iOS, system settings can affect this mapping. Before this patch, we
were performing this mapping inside the parser, which is wrong because we'll never re-parse things
in response to a change in the environment. So, if the page is live, and then the user changes a setting
in system preferences, we won't re-parse, which means the page won't update to accomodate the new setting
the user changed.

This patch changes the parser to not do this mapping, but instead just to emit CSSValues which directly
and simply represent the value that was present in the CSS source itself. So, if the content says
"font: caption" we'll create CSSPrimitiveValues which just hold "caption" and use that for all the longhands
of the font property. Then, we do the mapping when the values are applied, inside StyleBuilder. StyleBuilder
is re-run in response to environment changes, so this piece is necessary for system settings to immediately
take effect without a reload of the page.

This change is web-exposed, because the contents of CSSValues are exposed to webpages via inspecting the
CSSStyleSheet in JavaScript. So, the change is a little scary, but I think it's the only way to have the
right thing happen with system settings.

This patch also deletes the now-unused system font shorthand cache, which was reimplemented in
10cdfcb.

This patch isn't sufficient to make system settings fully work - there are some follow-up patches which
are still necessary on top of this:
1. Have font creation code actually interrogate system settings, and react accordingly, to create fonts
       with the appropriate size/weight
2. Make sure the right things get invalidated, so we don't get erroneous cache hits when system settings
       change
3. Make sure the right events are being delivered to the right places, and triggering the right invalidation,
       in response to system settings being changed.

* LayoutTests/fast/text/font-shorthand-resolution-expected.txt: Added.
* LayoutTests/fast/text/font-shorthand-resolution.html: Added.
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/parser/CSSPropertyParser.cpp:
(WebCore::CSSPropertyParser::consumeSystemFont):
(WebCore::CSSPropertyParser::parseShorthand):
* Source/WebCore/css/parser/CSSPropertyParserHelpers.h:
(WebCore::CSSPropertyParserHelpers::isSystemFontShorthand):
(WebCore::CSSPropertyParserHelpers::lowerFontShorthand):
* Source/WebCore/platform/graphics/SystemFontDatabase.h:
* Source/WebCore/rendering/RenderTheme.h:
* Source/WebCore/rendering/RenderThemeCocoa.h:
* Source/WebCore/rendering/RenderThemeCocoa.mm:
(WebCore::RenderThemeCocoa::systemFont const): Deleted.
* Source/WebCore/rendering/RenderThemeGtk.cpp:
(WebCore::RenderThemeGtk::systemFont const): Deleted.
* Source/WebCore/rendering/RenderThemePlayStation.cpp:
(WebCore::RenderThemePlayStation::systemFont const): Deleted.
* Source/WebCore/rendering/RenderThemeWin.cpp:
(WebCore::RenderThemeWin::systemFont const): Deleted.
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertFontWeight):
(WebCore::Style::BuilderConverter::convertFontVariantCaps):
(WebCore::Style::BuilderConverter::convertLineHeight):
* Source/WebCore/style/StyleBuilderCustom.h:
(WebCore::Style::BuilderCustom::applyValueFontFamily):
(WebCore::Style::BuilderCustom::applyValueFontStyle):
(WebCore::Style::BuilderCustom::applyValueFontSize):
* Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::contentSizeCategoryDidChange):
* Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:
(WebKit::WebProcess::accessibilityPreferencesDidChange):

Canonical link: https://commits.webkit.org/251481@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295476 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
litherum committed Jun 11, 2022
1 parent d8ee674 commit f4853ba72a0b56dcf84b9ceb0435eecc99d59a72
Show file tree
Hide file tree
Showing 24 changed files with 238 additions and 302 deletions.
@@ -0,0 +1,10 @@
PASS style.getPropertyValue('font-family') is not 'UICTFontTextStyleBody'
PASS isNaN(parseInt(style.getPropertyValue('font-size'))) is true
PASS style.getPropertyValue('font-style') is not 'normal'
PASS isNaN(parseInt(style.getPropertyValue('font-weight'))) is true
PASS style.getPropertyValue('font-variant-caps') is not 'normal'
PASS style.getPropertyValue('line-height') is not 'normal'
PASS successfullyParsed is true

TEST COMPLETE

@@ -0,0 +1,22 @@
<!DOCTYPE html>
<html>
<head>
<script src="../../resources/js-test.js"></script>
<style id="style">
foo {
font: -apple-system-body;
}
</style>
</head>
<body>
<script>
let style = document.getElementById("style").sheet.cssRules[0].style;
shouldNotBe("style.getPropertyValue('font-family')", "'UICTFontTextStyleBody'");
shouldBeTrue("isNaN(parseInt(style.getPropertyValue('font-size')))");
shouldNotBe("style.getPropertyValue('font-style')", "'normal'");
shouldBeTrue("isNaN(parseInt(style.getPropertyValue('font-weight')))");
shouldNotBe("style.getPropertyValue('font-variant-caps')", "'normal'");
shouldNotBe("style.getPropertyValue('line-height')", "'normal'");
</script>
</body>
</html>
@@ -573,7 +573,8 @@
"codegen-properties": {
"name-for-methods": "VariantCaps",
"font-property": true,
"high-priority": true
"high-priority": true,
"converter": "FontVariantCaps"
},
"specification": {
"category": "css-fonts",
@@ -465,6 +465,82 @@ void StyleProperties::appendFontLonghandValueIfExplicit(CSSPropertyID propertyID
commonValue = String();
}

std::optional<CSSValueID> StyleProperties::isSingleFontShorthand() const
{
// Intentionally don't check font-stretch here, because it isn't set by the font shorthand in CSSPropertyParser::consumeSystemFont().

auto sizePropertyIndex = findPropertyIndex(CSSPropertyFontSize);
auto familyPropertyIndex = findPropertyIndex(CSSPropertyFontFamily);
auto stylePropertyIndex = findPropertyIndex(CSSPropertyFontStyle);
auto variantCapsPropertyIndex = findPropertyIndex(CSSPropertyFontVariantCaps);
auto weightPropertyIndex = findPropertyIndex(CSSPropertyFontWeight);
auto lineHeightPropertyIndex = findPropertyIndex(CSSPropertyLineHeight);

if (sizePropertyIndex == -1
|| familyPropertyIndex == -1
|| stylePropertyIndex == -1
|| variantCapsPropertyIndex == -1
|| weightPropertyIndex == -1
|| lineHeightPropertyIndex == -1)
return std::nullopt;

auto sizeProperty = propertyAt(sizePropertyIndex);
auto familyProperty = propertyAt(familyPropertyIndex);
auto styleProperty = propertyAt(stylePropertyIndex);
auto variantCapsProperty = propertyAt(variantCapsPropertyIndex);
auto weightProperty = propertyAt(weightPropertyIndex);
auto lineHeightProperty = propertyAt(lineHeightPropertyIndex);

if (sizeProperty.isImplicit()
|| familyProperty.isImplicit()
|| styleProperty.isImplicit()
|| variantCapsProperty.isImplicit()
|| weightProperty.isImplicit()
|| lineHeightProperty.isImplicit())
return std::nullopt;

auto* sizeValue = sizeProperty.value();
auto* familyValue = familyProperty.value();
auto* styleValue = styleProperty.value();
auto* variantCapsValue = variantCapsProperty.value();
auto* weightValue = weightProperty.value();
auto* lineHeightValue = lineHeightProperty.value();

if (!is<CSSPrimitiveValue>(sizeValue)
|| !is<CSSPrimitiveValue>(familyValue)
|| !is<CSSPrimitiveValue>(styleValue)
|| !is<CSSPrimitiveValue>(variantCapsValue)
|| !is<CSSPrimitiveValue>(weightValue)
|| !is<CSSPrimitiveValue>(lineHeightValue))
return std::nullopt;

auto& sizePrimitiveValue = downcast<CSSPrimitiveValue>(*sizeValue);
auto& familyPrimitiveValue = downcast<CSSPrimitiveValue>(*familyValue);
auto& stylePrimitiveValue = downcast<CSSPrimitiveValue>(*styleValue);
auto& variantCapsPrimitiveValue = downcast<CSSPrimitiveValue>(*variantCapsValue);
auto& weightPrimitiveValue = downcast<CSSPrimitiveValue>(*weightValue);
auto& lineHeightPrimitiveValue = downcast<CSSPrimitiveValue>(*lineHeightValue);

auto sizeValueID = sizePrimitiveValue.valueID();
auto familyValueID = familyPrimitiveValue.valueID();
auto styleValueID = stylePrimitiveValue.valueID();
auto variantCapsValueID = variantCapsPrimitiveValue.valueID();
auto weightValueID = weightPrimitiveValue.valueID();
auto lineHeightValueID = lineHeightPrimitiveValue.valueID();

if (sizeValueID != familyValueID
|| sizeValueID != styleValueID
|| sizeValueID != variantCapsValueID
|| sizeValueID != weightValueID
|| sizeValueID != lineHeightValueID)
return std::nullopt;

if (sizeValueID == CSSValueInvalid)
return std::nullopt;

return sizeValueID;
}

String StyleProperties::fontValue() const
{
int fontSizePropertyIndex = findPropertyIndex(CSSPropertyFontSize);
@@ -477,6 +553,9 @@ String StyleProperties::fontValue() const
if (fontSizeProperty.isImplicit() || fontFamilyProperty.isImplicit())
return emptyString();

if (auto shorthand = isSingleFontShorthand())
return getValueNameAtomString(shorthand.value());

String commonValue = fontSizeProperty.value()->cssText();
StringBuilder result;
appendFontLonghandValueIfExplicit(CSSPropertyFontStyle, result, commonValue);
@@ -167,6 +167,7 @@ class StyleProperties : public RefCounted<StyleProperties> {
String textDecorationSkipValue() const;
String offsetValue() const;
void appendFontLonghandValueIfExplicit(CSSPropertyID, StringBuilder& result, String& value) const;
std::optional<CSSValueID> isSingleFontShorthand() const;
bool shorthandHasVariableReference(CSSPropertyID, String&) const;
StringBuilder asTextInternal() const;

@@ -5192,22 +5192,22 @@ bool CSSPropertyParser::parseFontPaletteValuesDescriptor(CSSPropertyID propId)
bool CSSPropertyParser::consumeSystemFont(bool important)
{
CSSValueID systemFontID = m_range.consumeIncludingWhitespace().id();
ASSERT(systemFontID >= CSSValueCaption && systemFontID <= CSSValueStatusBar);
ASSERT(CSSPropertyParserHelpers::isSystemFontShorthand(systemFontID));
if (!m_range.atEnd())
return false;

// It's illegal to look up properties (weight, size, etc.) of the system font here,
// because those values can change (e.g. accessibility font sizes, or accessibility bold).
// Parsing (correctly) doesn't re-run in response to updateStyleAfterChangeInEnvironment().
// Instead, we stuff sentinel values into the outputted CSSValues, which are later replaced by
// real system font values inside Style::BuilderCustom and Style::BuilderConverter.

auto fontDescription = RenderTheme::singleton().systemFont(systemFontID);
if (!fontDescription.isAbsoluteSize())
return false;

addProperty(CSSPropertyFontStyle, CSSPropertyFont, CSSFontStyleValue::create(CSSValuePool::singleton().createIdentifierValue(isItalic(fontDescription.italic()) ? CSSValueItalic : CSSValueNormal)), important);
addProperty(CSSPropertyFontWeight, CSSPropertyFont, CSSValuePool::singleton().createValue(static_cast<float>(fontDescription.weight())), important);
addProperty(CSSPropertyFontSize, CSSPropertyFont, CSSValuePool::singleton().createValue(fontDescription.specifiedSize(), CSSUnitType::CSS_PX), important);
Ref<CSSValueList> fontFamilyList = CSSValueList::createCommaSeparated();
fontFamilyList->append(CSSValuePool::singleton().createFontFamilyValue(fontDescription.familyAt(0), FromSystemFontID::Yes));
addProperty(CSSPropertyFontFamily, CSSPropertyFont, WTFMove(fontFamilyList), important);
addProperty(CSSPropertyFontVariantCaps, CSSPropertyFont, CSSValuePool::singleton().createIdentifierValue(CSSValueNormal), important);
addProperty(CSSPropertyLineHeight, CSSPropertyFont, CSSValuePool::singleton().createIdentifierValue(CSSValueNormal), important);
addProperty(CSSPropertyFontStyle, CSSPropertyFont, CSSValuePool::singleton().createIdentifierValue(systemFontID), important);
addProperty(CSSPropertyFontWeight, CSSPropertyFont, CSSValuePool::singleton().createIdentifierValue(systemFontID), important);
addProperty(CSSPropertyFontSize, CSSPropertyFont, CSSValuePool::singleton().createIdentifierValue(systemFontID), important);
addProperty(CSSPropertyFontFamily, CSSPropertyFont, CSSValuePool::singleton().createIdentifierValue(systemFontID), important);
addProperty(CSSPropertyFontVariantCaps, CSSPropertyFont, CSSValuePool::singleton().createIdentifierValue(systemFontID), important);
addProperty(CSSPropertyLineHeight, CSSPropertyFont, CSSValuePool::singleton().createIdentifierValue(systemFontID), important);

// FIXME_NEWPARSER: What about FontVariantNumeric and FontVariantLigatures?

@@ -6384,7 +6384,7 @@ bool CSSPropertyParser::parseShorthand(CSSPropertyID property, bool important)
return consumeOverscrollBehaviorShorthand(important);
case CSSPropertyFont: {
const CSSParserToken& token = m_range.peek();
if (token.id() >= CSSValueCaption && token.id() <= CSSValueStatusBar)
if (CSSPropertyParserHelpers::isSystemFontShorthand(token.id()))
return consumeSystemFont(important);
return consumeFont(important);
}
@@ -1,5 +1,5 @@
// Copyright 2016 The Chromium Authors. All rights reserved.
// Copyright (C) 2016 Apple Inc. All rights reserved.
// Copyright (C) 2016-2022 Apple Inc. All rights reserved.
//
// Redistribution and use in source and binary forms, with or without
// modification, are permitted provided that the following conditions are
@@ -37,6 +37,7 @@
#include "CSSValuePool.h"
#include "Length.h" // For ValueRange
#include "StyleColor.h"
#include "SystemFontDatabase.h"
#include <variant>
#include <wtf/OptionSet.h>
#include <wtf/Vector.h>
@@ -247,6 +248,19 @@ inline bool isFontStyleAngleInRange(double angleInDegrees)
return angleInDegrees > -90 && angleInDegrees < 90;
}

inline bool isSystemFontShorthand(CSSValueID valueID)
{
// This needs to stay in sync with SystemFontDatabase::FontShorthand.
static_assert(CSSValueStatusBar - CSSValueCaption == static_cast<SystemFontDatabase::FontShorthandUnderlyingType>(SystemFontDatabase::FontShorthand::StatusBar));
return valueID >= CSSValueCaption && valueID <= CSSValueStatusBar;
}

inline SystemFontDatabase::FontShorthand lowerFontShorthand(CSSValueID valueID)
{
// This needs to stay in sync with SystemFontDatabase::FontShorthand.
ASSERT(isSystemFontShorthand(valueID));
return static_cast<SystemFontDatabase::FontShorthand>(valueID - CSSValueCaption);
}

} // namespace CSSPropertyParserHelpers

@@ -32,6 +32,7 @@
#include "CSSFontFamily.h"
#include "CSSFontStyleValue.h"
#include "CSSParser.h"
#include "CSSPropertyParserHelpers.h"
#include "CSSRuleList.h"
#include "CSSStyleRule.h"
#include "CSSValueList.h"
@@ -269,7 +270,9 @@ static bool fontWeightIsBold(CSSValue& fontWeight)
if (primitiveValue.isCSSWideKeyword())
return false;

switch (primitiveValue.valueID()) {
auto valueID = primitiveValue.valueID();

switch (valueID) {
case CSSValueNormal:
return false;
case CSSValueBold:
@@ -278,6 +281,9 @@ static bool fontWeightIsBold(CSSValue& fontWeight)
break;
}

if (CSSPropertyParserHelpers::isSystemFontShorthand(valueID))
return false;

ASSERT(primitiveValue.isNumber());
return primitiveValue.floatValue() >= static_cast<float>(boldThreshold());
}
@@ -33,8 +33,8 @@
#include "CommonVM.h"
#include "GraphicsContext.h"
#include "Page.h"
#include "RenderTheme.h"
#include "ResourceUsageThread.h"
#include "SystemFontDatabase.h"
#include <JavaScriptCore/VM.h>
#include <wtf/text/StringConcatenateNumbers.h>

@@ -75,7 +75,11 @@ class ResourceUsageOverlayPainter final : public GraphicsLayerClient {
ResourceUsageOverlayPainter(ResourceUsageOverlay& overlay)
: m_overlay(overlay)
{
auto fontDescription = RenderTheme::singleton().systemFont(CSSValueMessageBox);
auto& systemFontDatabase = SystemFontDatabase::singleton();
auto messageBox = SystemFontDatabase::FontShorthand::MessageBox;
FontCascadeDescription fontDescription;
fontDescription.setOneFamily(systemFontDatabase.systemFontShorthandFamily(messageBox));
fontDescription.setWeight(systemFontDatabase.systemFontShorthandWeight(messageBox));
fontDescription.setComputedSize(gFontSize);
m_textFont = FontCascade(WTFMove(fontDescription), 0, 0);
m_textFont.update(nullptr);
@@ -38,7 +38,7 @@ class SystemFontDatabase {
WEBCORE_EXPORT static SystemFontDatabase& singleton();

enum class FontShorthand {
// This needs to be kept in sync with CSSValue.
// This needs to be kept in sync with CSSValue and CSSPropertyParserHelpers::lowerFontShorthand().
Caption,
Icon,
Menu,
@@ -68,6 +68,7 @@ class SystemFontDatabase {
#endif
StatusBar, // This has to be kept in sync with SystemFontShorthandCache below.
};
using FontShorthandUnderlyingType = std::underlying_type<FontShorthand>::type;

const AtomString& systemFontShorthandFamily(FontShorthand);
float systemFontShorthandSize(FontShorthand);
@@ -39,7 +39,7 @@ SystemFontDatabase& SystemFontDatabase::singleton()
return database.get();
}

auto SystemFontDatabase::platformSystemFontShorthandInfo(FontShorthand fontShorthand) -> SystemFontShorthandInfo
auto SystemFontDatabase::platformSystemFontShorthandInfo(FontShorthand) -> SystemFontShorthandInfo
{
notImplemented();
return { WebKitFontFamilyNames::standardFamily, 16, normalWeightValue() };
@@ -187,7 +187,6 @@ class RenderTheme {
virtual Seconds caretBlinkInterval() const { return 500_ms; }

// System fonts and colors for CSS.
virtual FontCascadeDescription systemFont(CSSValueID) const = 0;
virtual Color systemColor(CSSValueID, OptionSet<StyleColorOptions>) const;

virtual int minimumMenuListSize(const RenderStyle&) const { return 0; }
@@ -56,8 +56,6 @@ class RenderThemeAdwaita : public RenderTheme {
bool supportsListBoxSelectionForegroundColors(OptionSet<StyleColorOptions>) const final { return true; }
bool shouldHaveCapsLockIndicator(const HTMLInputElement&) const final;

FontCascadeDescription systemFont(CSSValueID) const override { return FontCascadeDescription(); };

Color platformActiveSelectionBackgroundColor(OptionSet<StyleColorOptions>) const final;
Color platformInactiveSelectionBackgroundColor(OptionSet<StyleColorOptions>) const final;
Color platformActiveSelectionForegroundColor(OptionSet<StyleColorOptions>) const final;
@@ -46,8 +46,6 @@ class RenderThemeCocoa : public RenderTheme {
bool paintApplePayButton(const RenderObject&, const PaintInfo&, const IntRect&) override;
#endif

FontCascadeDescription systemFont(CSSValueID systemFontID) const override;

#if ENABLE(VIDEO) && ENABLE(MODERN_MEDIA_CONTROLS)
String mediaControlsStyleSheet() override;
Vector<String, 2> mediaControlsScripts() override;

0 comments on commit f4853ba

Please sign in to comment.