Skip to content

Commit

Permalink
Fix font-variant shorthand serialization
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=247919
rdar://102339716

Reviewed by Darin Adler.

- Fixed bug where font-variant-ligatures: none + another non-default longhand would serialize even if it not possible
- Clean up fontValue()/fontVariantValue() to not consider implicit/explicit anymore used to represent default/system values
- Refactor to remove appendFontLonghandValueIfExplicit()

* LayoutTests/fast/text/font-variant-shorthand-expected.txt:
* LayoutTests/fast/text/font-variant-shorthand.html:
* LayoutTests/fast/css/font-property-priority-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/font-shorthand-serialization-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/font-variant-shorthand-serialization-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/font-variant-shorthand-serialization.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/font-shorthand-serialization-expected.txt:
* Source/WebCore/css/CSSProperties.json:
Updated ordering to match order from the spec (which matters for serialization).

* Source/WebCore/css/StyleProperties.cpp:
(WebCore::StyleProperties::fontValue const):
Inlined parts of appendFontLonghandValueIfExplicit, simplified font-stretch handling.

(WebCore::StyleProperties::fontVariantValue const):
Rewrite to stop using appendFontLonghandValueIfExplicit, fix various serialization issues when collapsing longhands.

(WebCore::canUseShorthandForLonghand):
Add notes on why collapsing is not possible.

(WebCore::StyleProperties::appendFontLonghandValueIfExplicit const): Deleted.

* Source/WebCore/css/StyleProperties.h:
* Source/WebCore/css/parser/CSSPropertyParser.cpp:
(WebCore::CSSPropertyParser::consumeFontVariantShorthand):

Canonical link: https://commits.webkit.org/256681@main
  • Loading branch information
nt1m committed Nov 15, 2022
1 parent c36eb0b commit cee0d60
Show file tree
Hide file tree
Showing 9 changed files with 226 additions and 86 deletions.
4 changes: 2 additions & 2 deletions LayoutTests/fast/css/font-property-priority-expected.txt
Expand Up @@ -2,10 +2,10 @@ Test for rdar://problem/6065547 REGRESSION (r34879): "Subject" in unread emails

Property 'font-weight' has priority 'important'.
Property 'font-variant-ligatures' has priority 'important'.
Property 'font-variant-numeric' has priority 'important'.
Property 'font-variant-caps' has priority 'important'.
Property 'font-variant-alternates' has priority 'important'.
Property 'font-variant-position' has priority 'important'.
Property 'font-variant-numeric' has priority 'important'.
Property 'font-variant-east-asian' has priority 'important'.
Property 'font-variant-position' has priority 'important'.
Property 'font-style' has priority 'important'.

2 changes: 1 addition & 1 deletion LayoutTests/fast/text/font-variant-shorthand-expected.txt
Expand Up @@ -33,7 +33,7 @@ PASS window.getComputedStyle(document.getElementById('t12')).getPropertyValue('f
PASS window.getComputedStyle(document.getElementById('t13')).getPropertyValue('font-variant') is "normal"
PASS window.getComputedStyle(document.getElementById('t14')).getPropertyValue('font-variant') is "normal"
PASS window.getComputedStyle(document.getElementById('t15')).getPropertyValue('font-variant') is "normal"
PASS window.getComputedStyle(document.getElementById('t16')).getPropertyValue('font-variant') is "common-ligatures super small-caps lining-nums historical-forms simplified"
PASS window.getComputedStyle(document.getElementById('t16')).getPropertyValue('font-variant') is "common-ligatures small-caps historical-forms lining-nums simplified super"
PASS window.getComputedStyle(document.getElementById('t17')).getPropertyValue('font-variant') is "normal"
PASS window.getComputedStyle(document.getElementById('t18')).getPropertyValue('font-variant') is "none"
PASS window.getComputedStyle(document.getElementById('t19')).getPropertyValue('font-variant') is "normal"
Expand Down
2 changes: 1 addition & 1 deletion LayoutTests/fast/text/font-variant-shorthand.html
Expand Up @@ -171,7 +171,7 @@
shouldBeEqualToString("window.getComputedStyle(document.getElementById('t14')).getPropertyValue('font-variant')", "normal");
shouldBeEqualToString("window.getComputedStyle(document.getElementById('t15')).getPropertyValue('font-variant')", "normal");

shouldBeEqualToString("window.getComputedStyle(document.getElementById('t16')).getPropertyValue('font-variant')", "common-ligatures super small-caps lining-nums historical-forms simplified");
shouldBeEqualToString("window.getComputedStyle(document.getElementById('t16')).getPropertyValue('font-variant')", "common-ligatures small-caps historical-forms lining-nums simplified super");
shouldBeEqualToString("window.getComputedStyle(document.getElementById('t17')).getPropertyValue('font-variant')", "normal");
shouldBeEqualToString("window.getComputedStyle(document.getElementById('t18')).getPropertyValue('font-variant')", "none");
shouldBeEqualToString("window.getComputedStyle(document.getElementById('t19')).getPropertyValue('font-variant')", "normal");
Expand Down
@@ -0,0 +1,9 @@

PASS font-variant: normal serialization
PASS font-variant: none serialization
PASS font-variant-ligatures: none serialization with non-default value for another longhand
PASS font-variant: normal with non-default longhands
PASS CSS-wide keyword in one longhand
PASS CSS-wide keyword in shorthand
PASS font: menu serialization

@@ -0,0 +1,130 @@
<!doctype html>
<html>
<meta charset="utf-8">
<title>Serialization of font-variant shorthand</title>
<link rel="help" href="https://drafts.csswg.org/cssom-1/#serialize-a-css-declaration-block">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<link rel="stylesheet" type="text/css" href="/fonts/ahem.css" />
<div id="target"></div>
<script>
const cssWideKeywords = ["initial", "inherit", "unset", "revert", "revert-layer"];
function test_serialization_set(expected) {
for (const [property, value] of Object.entries(expected)) {
if (!CSS.supports(`${property}: initial`))
continue;
assert_equals(target.style[property], value, `${property} was set`);
}
}
function setWithValue(value) {
return {
"font-variant-ligatures": value,
"font-variant-caps": value,
"font-variant-alternates": value,
"font-variant-numeric": value,
"font-variant-east-asian": value,
"font-variant-position": value,
"font-variant-emoji": value,
"font-variant": value
};
}
const emptySet = setWithValue("");
const normalSet = setWithValue("normal");
const nonDefaultValues = {
"font-variant-ligatures": "common-ligatures discretionary-ligatures",
"font-variant-caps": "small-caps",
"font-variant-alternates": "historical-forms",
"font-variant-numeric": "oldstyle-nums stacked-fractions",
"font-variant-east-asian": "ruby",
"font-variant-position": "sub",
"font-variant-emoji": "emoji",
};
test(function(t) {
target.style.fontVariant = "normal";
t.add_cleanup(() => target.removeAttribute("style"));

test_serialization_set(normalSet);
}, "font-variant: normal serialization");

test(function(t) {
target.style.fontVariant = "normal";
target.style.fontVariantLigatures = "none";
t.add_cleanup(() => target.removeAttribute("style"));

const expected = Object.assign({}, normalSet);
expected["font-variant-ligatures"] = "none";
expected["font-variant"] = "none";

test_serialization_set(expected);
}, "font-variant: none serialization");

test(function(t) {
t.add_cleanup(() => target.removeAttribute("style"));
for (const [property, value] of Object.entries(nonDefaultValues)) {
if (property == "font-variant-ligatures" || !CSS.supports(`${property}: initial`))
continue;
target.style.fontVariant = "normal";
target.style.fontVariantLigatures = "none";
target.style[property] = value;

const expected = Object.assign({}, normalSet);
expected["font-variant-ligatures"] = "none";
expected["font-variant"] = "";
expected[property] = value;

test_serialization_set(expected);
target.removeAttribute("style");
}
}, "font-variant-ligatures: none serialization with non-default value for another longhand");

test(function(t) {
t.add_cleanup(() => target.removeAttribute("style"));

for (const [property, value] of Object.entries(nonDefaultValues)) {
if (!CSS.supports(`${property}: initial`))
continue;
target.style.fontVariant = "normal";
target.style[property] = value;

const expected = Object.assign({}, normalSet);
expected[property] = value;
expected["font-variant"] = value;
test_serialization_set(expected);

target.removeAttribute("style");
}
}, "font-variant: normal with non-default longhands");

test(function(t) {
t.add_cleanup(() => target.removeAttribute("style"));
for (const keyword of cssWideKeywords) {
target.style.fontVariant = "normal";
target.style.fontVariantLigatures = keyword;

const expected = Object.assign({}, normalSet);
expected["font-variant-ligatures"] = keyword;
expected["font-variant"] = "";
test_serialization_set(expected);
target.removeAttribute("style");
}
}, "CSS-wide keyword in one longhand");

test(function(t) {
t.add_cleanup(() => target.removeAttribute("style"));

for (const keyword of cssWideKeywords) {
target.style.fontVariant = keyword;
test_serialization_set(setWithValue(keyword));
target.removeAttribute("style");
}
}, "CSS-wide keyword in shorthand");

test(function(t) {
target.style.fontVariant = "normal";
target.style.font = "menu";
t.add_cleanup(() => target.removeAttribute("style"));

test_serialization_set(emptySet);
}, "font: menu serialization");
</script>
</html>
14 changes: 7 additions & 7 deletions Source/WebCore/css/CSSProperties.json
Expand Up @@ -750,13 +750,13 @@
"inherited": true,
"codegen-properties": {
"longhands": [
"font-variant-ligatures",
"font-variant-position",
"font-variant-caps",
"font-variant-numeric",
"font-variant-alternates",
"font-variant-east-asian"
]
"font-variant-ligatures",
"font-variant-caps",
"font-variant-alternates",
"font-variant-numeric",
"font-variant-east-asian",
"font-variant-position"
]
},
"specification": {
"category": "css-fonts",
Expand Down
141 changes: 72 additions & 69 deletions Source/WebCore/css/StyleProperties.cpp
Expand Up @@ -460,45 +460,6 @@ String StyleProperties::borderSpacingValue(const StylePropertyShorthand& shortha
return horizontalValueCSSText + ' ' + verticalValueCSSText;
}

void StyleProperties::appendFontLonghandValueIfExplicit(CSSPropertyID propertyID, StringBuilder& result, String& commonValue) const
{
int foundPropertyIndex = findPropertyIndex(propertyID);
if (foundPropertyIndex == -1)
return; // All longhands must have at least implicit values if "font" is specified.

if (propertyAt(foundPropertyIndex).isImplicit()) {
commonValue = String();
return;
}

const char* prefix = "";
switch (propertyID) {
case CSSPropertyFontStyle:
break; // No prefix.
case CSSPropertyFontFamily:
case CSSPropertyFontVariantAlternates:
case CSSPropertyFontVariantCaps:
case CSSPropertyFontVariantLigatures:
case CSSPropertyFontVariantNumeric:
case CSSPropertyFontVariantPosition:
case CSSPropertyFontVariantEastAsian:
case CSSPropertyFontWeight:
case CSSPropertyFontStretch:
prefix = " ";
break;
case CSSPropertyLineHeight:
prefix = " / ";
break;
default:
ASSERT_NOT_REACHED();
}

String value = propertyAt(foundPropertyIndex).value()->cssText();
result.append(result.isEmpty() ? "" : prefix, value);
if (!commonValue.isNull() && commonValue != value)
commonValue = String();
}

static std::optional<CSSValueID> fontStretchKeyword(double value)
{
// If the numeric value does not fit in the fixed point FontSelectionValue, don't convert it to a keyword even if it rounds to a keyword value.
Expand Down Expand Up @@ -551,36 +512,39 @@ String StyleProperties::fontValue() const

// Font stretch values can only be serialized in the font shorthand as keywords, since percentages are also valid font sizes.
// If a font stretch percentage can be expressed as a keyword, then do that.
ASCIILiteral stretchPercentageAsKeyword;
bool stretchIsNormal = false;
std::optional<CSSValueID> stretchKeyword;
if (auto stretchBase = getPropertyCSSValue(CSSPropertyFontStretch)) {
auto stretch = downcast<CSSPrimitiveValue>(stretchBase.get());
std::optional<CSSValueID> keyword;
if (!stretch->isPercentage())
keyword = stretch->valueID();
stretchKeyword = stretch->valueID();
else {
keyword = fontStretchKeyword(stretch->doubleValue());
if (!keyword)
stretchKeyword = fontStretchKeyword(stretch->doubleValue());
if (!stretchKeyword)
return emptyString();
stretchPercentageAsKeyword = nameLiteral(*keyword);
}
stretchIsNormal = keyword == CSSValueNormal;
}

// This code no longer uses commonValue, for now we define it so we can use appendFontLonghandValueIfExplicit.
String commonValue;
StringBuilder result;
appendFontLonghandValueIfExplicit(CSSPropertyFontStyle, result, commonValue);
appendFontLonghandValueIfExplicit(CSSPropertyFontVariantCaps, result, commonValue);
appendFontLonghandValueIfExplicit(CSSPropertyFontWeight, result, commonValue);
if (!stretchIsNormal) {
if (!stretchPercentageAsKeyword.isNull())
result.append(result.isEmpty() ? "" : " ", stretchPercentageAsKeyword);
else
appendFontLonghandValueIfExplicit(CSSPropertyFontStretch, result, commonValue);
}
auto appendOptionalValue = [this, &result](CSSPropertyID property) {
int foundPropertyIndex = findPropertyIndex(property);
if (foundPropertyIndex == -1)
return; // All longhands must have at least implicit values if "font" is specified.

// Omit default normal values.
if (propertyAsValueID(property) == CSSValueNormal)
return;

auto prefix = property == CSSPropertyLineHeight ? " / " : " ";
result.append(result.isEmpty() ? "" : prefix, propertyAt(foundPropertyIndex).value()->cssText());
};

appendOptionalValue(CSSPropertyFontStyle);
appendOptionalValue(CSSPropertyFontVariantCaps);
appendOptionalValue(CSSPropertyFontWeight);
if (stretchKeyword && *stretchKeyword != CSSValueNormal)
result.append(result.isEmpty() ? "" : " ", nameString(*stretchKeyword));
result.append(result.isEmpty() ? "" : " ", size->cssText());
appendFontLonghandValueIfExplicit(CSSPropertyLineHeight, result, commonValue);
appendOptionalValue(CSSPropertyLineHeight);
result.append(result.isEmpty() ? "" : " ", family->cssText());
return result.toString();
}
Expand Down Expand Up @@ -685,16 +649,54 @@ String StyleProperties::textDecorationSkipValue() const

String StyleProperties::fontVariantValue() const
{
String commonValue;
StringBuilder result;
appendFontLonghandValueIfExplicit(CSSPropertyFontVariantLigatures, result, commonValue);
if (isCSSWideValueKeyword(result.toString()))
return result.toString();
appendFontLonghandValueIfExplicit(CSSPropertyFontVariantCaps, result, commonValue);
appendFontLonghandValueIfExplicit(CSSPropertyFontVariantAlternates, result, commonValue);
appendFontLonghandValueIfExplicit(CSSPropertyFontVariantNumeric, result, commonValue);
appendFontLonghandValueIfExplicit(CSSPropertyFontVariantEastAsian, result, commonValue);
appendFontLonghandValueIfExplicit(CSSPropertyFontVariantPosition, result, commonValue);
std::optional<CSSValueID> commonCSSWideKeyword;
bool isAllNormal = true;
auto ligaturesKeyword = propertyAsValueID(CSSPropertyFontVariantLigatures);

for (auto property : fontVariantShorthand()) {
int foundPropertyIndex = findPropertyIndex(property);
if (foundPropertyIndex == -1)
return emptyString(); // All longhands must have at least implicit values.

// Bypass getPropertyCSSValue which will return an empty string for system keywords.
auto value = propertyAt(foundPropertyIndex).value();
auto keyword = valueID(value);

// If all properties are set to the same special keyword, serialize as that.
// If some but not all properties are, the shorthand can't represent that, serialize as empty string.
if (isCSSWideKeyword(keyword)) {
if (commonCSSWideKeyword.value_or(keyword) != keyword)
return emptyString();
commonCSSWideKeyword = keyword;
continue;
}
if (commonCSSWideKeyword)
return emptyString();

// Skip normal for brevity.
if (keyword == CSSValueNormal)
continue;

isAllNormal = false;

// System keywords are not representable by font-variant.
if (CSSPropertyParserHelpers::isSystemFontShorthand(keyword))
return emptyString();

// font-variant cannot represent font-variant-ligatures: none along with other non-normal longhands.
if (ligaturesKeyword.value_or(CSSValueNormal) == CSSValueNone && property != CSSPropertyFontVariantLigatures)
return emptyString();

result.append(result.isEmpty() ? "" : " ", value->cssText());
}

if (commonCSSWideKeyword)
return nameString(*commonCSSWideKeyword);

if (result.isEmpty() && isAllNormal)
return nameString(CSSValueNormal);

return result.toString();
}

Expand Down Expand Up @@ -1683,6 +1685,8 @@ static constexpr bool canUseShorthandForLonghand(CSSPropertyID shorthandID, CSSP
case CSSPropertyMask:
return longhandID != CSSPropertyMaskComposite && longhandID != CSSPropertyMaskMode && longhandID != CSSPropertyMaskSize;

// FIXME: If font-variant-ligatures is none, this depends on the value of the longhand.
case CSSPropertyFontVariant:
// FIXME: These shorthands are avoided for unknown legacy reasons, probably shouldn't be avoided.
case CSSPropertyBackground:
case CSSPropertyBorderBlockEnd:
Expand All @@ -1697,7 +1701,6 @@ static constexpr bool canUseShorthandForLonghand(CSSPropertyID shorthandID, CSSP
case CSSPropertyColumns:
case CSSPropertyContainer:
case CSSPropertyFontSynthesis:
case CSSPropertyFontVariant:
case CSSPropertyGap:
case CSSPropertyGridArea:
case CSSPropertyGridColumn:
Expand Down
1 change: 0 additions & 1 deletion Source/WebCore/css/StyleProperties.h
Expand Up @@ -169,7 +169,6 @@ class StyleProperties : public RefCounted<StyleProperties> {
String fontSynthesisValue() const;
String textDecorationSkipValue() const;
String offsetValue() const;
void appendFontLonghandValueIfExplicit(CSSPropertyID, StringBuilder& result, String& value) const;
bool shorthandHasVariableReference(CSSPropertyID, String&) const;
StringBuilder asTextInternal() const;

Expand Down

0 comments on commit cee0d60

Please sign in to comment.