Resolve logical viewport units properly in font-size

https://bugs.webkit.org/show_bug.cgi?id=238982

Reviewed by Antti Koivisto.

Logical viewport units depend on the current element's writing mode, and they can be used in font-size, so that means we need to:

- Make sure writing-mode is always applied first and introduce a new top-priority property type
- Stop passing in unconditionally the parentStyle into CSSToLengthConversionData, since viewport units use the current element's writing mode, not the parent element's

* LayoutTests/TestExpectations:
* Source/WebCore/css/CSSPrimitiveValue.cpp:
(WebCore::CSSPrimitiveValue::computeNonCalcLengthDouble):
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/CSSToLengthConversionData.h:
(WebCore::CSSToLengthConversionData::fontCascadeForFontUnits const):
(WebCore::CSSToLengthConversionData::computedLineHeightForFontUnits const):
(WebCore::CSSToLengthConversionData::copyForFontSize const):
(WebCore::CSSToLengthConversionData::copyForFontSizeWithParentStyle const): Deleted.
* Source/WebCore/css/makeprop.pl:
(addProperty):
(sortByDescendingPriorityAndName):
* Source/WebCore/style/StyleBuilder.cpp:
(WebCore::Style::Builder::applyAllProperties):
(WebCore::Style::Builder::applyTopPriorityProperties):
(WebCore::Style::Builder::applyHighPriorityProperties):
* Source/WebCore/style/StyleBuilderCustom.h:
(WebCore::Style::BuilderCustom::applyValueFontSize):
* Source/WebCore/style/StyleResolver.cpp:
(WebCore::Style::Resolver::applyMatchedProperties):
* Tools/Scripts/webkitpy/style/checkers/jsonchecker.py:

Canonical link: https://commits.webkit.org/253087@main

LayoutTests/TestExpectations

@@ -3460,7 +3460,6 @@ webkit.org/b/242465 imported/w3c/web-platform-tests/css/css-values/calc-width-ta
 webkit.org/b/242465 imported/w3c/web-platform-tests/css/css-values/calc-width-table-fixed-1.html [ Skip ]
 webkit.org/b/242466 imported/w3c/web-platform-tests/css/css-values/cap-unit-001.html [ ImageOnlyFailure ]
 webkit.org/b/242467 imported/w3c/web-platform-tests/css/css-values/calc-text-indent-intrinsic-1.html [ ImageOnlyFailure ]
-webkit.org/b/238982 imported/w3c/web-platform-tests/css/css-values/viewport-units-writing-mode-font-size.html [ ImageOnlyFailure ]
 
 # wpt css-sizing failures
 webkit.org/b/203509 imported/w3c/web-platform-tests/css/css-sizing/auto-scrollbar-inside-stf-abspos.html [ ImageOnlyFailure ]

Source/WebCore/css/CSSPrimitiveValue.cpp

@@ -838,16 +838,14 @@ double CSSPrimitiveValue::computeNonCalcLengthDouble(const CSSToLengthConversion
     switch (primitiveType) {
     case CSSUnitType::CSS_EMS:
     case CSSUnitType::CSS_QUIRKY_EMS:
-        ASSERT(conversionData.style());
-        value = computeUnzoomedNonCalcLengthDouble(primitiveType, value, conversionData.propertyToCompute(), nullptr, &conversionData.style()->fontDescription());
+        value = computeUnzoomedNonCalcLengthDouble(primitiveType, value, conversionData.propertyToCompute(), nullptr, &conversionData.fontCascadeForFontUnits().fontDescription());
         break;
 
     case CSSUnitType::CSS_EXS:
         // FIXME: We have a bug right now where the zoom will be applied twice to EX units.
         // We really need to compute EX using fontMetrics for the original specifiedSize and not use
         // our actual constructed rendering font.
-        ASSERT(conversionData.style());
-        value = computeUnzoomedNonCalcLengthDouble(primitiveType, value, conversionData.propertyToCompute(), &conversionData.style()->metricsOfPrimaryFont(), &conversionData.style()->fontDescription());
+        value = computeUnzoomedNonCalcLengthDouble(primitiveType, value, conversionData.propertyToCompute(), &conversionData.fontCascadeForFontUnits().metricsOfPrimaryFont(), &conversionData.fontCascadeForFontUnits().fontDescription());
         break;
 
     case CSSUnitType::CSS_REMS:
@@ -856,8 +854,7 @@ double CSSPrimitiveValue::computeNonCalcLengthDouble(const CSSToLengthConversion
 
     case CSSUnitType::CSS_CHS:
     case CSSUnitType::CSS_IC:
-        ASSERT(conversionData.style());
-        value = computeUnzoomedNonCalcLengthDouble(primitiveType, value, conversionData.propertyToCompute(), &conversionData.style()->metricsOfPrimaryFont(), &conversionData.style()->fontDescription());
+        value = computeUnzoomedNonCalcLengthDouble(primitiveType, value, conversionData.propertyToCompute(), &conversionData.fontCascadeForFontUnits().metricsOfPrimaryFont(), &conversionData.fontCascadeForFontUnits().fontDescription());
         break;
 
     case CSSUnitType::CSS_PX:
@@ -945,12 +942,11 @@ double CSSPrimitiveValue::computeNonCalcLengthDouble(const CSSToLengthConversion
         return value * lengthOfViewportPhysicalAxisForLogicalAxis(LogicalBoxAxis::Inline, conversionData.dynamicViewportFactor(), conversionData.style());
 
     case CSSUnitType::CSS_LHS:
-        ASSERT(conversionData.style());
         if (conversionData.computingLineHeight() || conversionData.computingFontSize()) {
             // Try to get the parent's computed line-height, or fall back to the initial line-height of this element's font spacing.
-            value *= conversionData.parentStyle() ? conversionData.parentStyle()->computedLineHeight() : conversionData.style()->metricsOfPrimaryFont().lineSpacing();
+            value *= conversionData.parentStyle() ? conversionData.parentStyle()->computedLineHeight() : conversionData.fontCascadeForFontUnits().metricsOfPrimaryFont().lineSpacing();
         } else
-            value *= conversionData.style()->computedLineHeight();
+            value *= conversionData.computedLineHeightForFontUnits();
         break;
 
     case CSSUnitType::CSS_CQW: {

Source/WebCore/css/CSSProperties.json

@@ -136,6 +136,10 @@
         "corresponding \"StylePropertyShorthand propertyIdShorthand()\" function will be",
         "generated in StylePropertyShorthandFunctions.h header.",
         "",
+        "* top-priority:",
+        "Whether the property needs to be applied before high-priority properties",
+        "in CSS cascading order. Please justify usage with a comment field.",
+        "",
         "* high-priority:",
         "Whether the property needs to be applied before non-high-priority properties",
         "in CSS cascading order. High priority properties must not accept <length>",
@@ -795,7 +799,8 @@
                     "-epub-writing-mode"
                 ],
                 "custom": "Value",
-                "high-priority": true
+                "comment": "This is top priority because vi/vb units (depending on writing-mode) may be used by other properties",
+                "top-priority": true
             },
             "specification": {
                 "category": "css-writing-modes",
@@ -850,7 +855,8 @@
                 }
             ],
             "codegen-properties": {
-                "comment": "This is the highest priority property and 'is resolved before all other properties, to ensure that its value can be checked when determining a smart default font size', (<https://trac.webkit.org/browser/trunk/Source/WebCore/ChangeLog?rev=172861>)."
+                "top-priority": true,
+                "comment": "This is a top priority property to ensure that its value can be checked when determining a smart default font size', (<https://trac.webkit.org/browser/trunk/Source/WebCore/ChangeLog?rev=172861>)."
             },
             "specification": {
                 "category": "css-ruby",
@@ -6692,10 +6698,10 @@
                     "supported-color-schemes"
                 ],
                 "converter": "ColorScheme",
-                "comment": "This is the second highest priority property, to ensure that its value can be checked when resolving colors.",
+                "comment": "This is a top priority property, to ensure that its value can be checked when resolving colors.",
                 "custom": "Value",
                 "enable-if": "ENABLE_DARK_MODE_CSS",
-                "high-priority": true
+                "top-priority": true
             },
             "specification": {
                 "category": "css-color-adjust",

Source/WebCore/css/CSSToLengthConversionData.cpp

@@ -59,6 +59,26 @@ CSSToLengthConversionData::CSSToLengthConversionData(const RenderStyle& style, c
 {
 }
 
+const FontCascade& CSSToLengthConversionData::fontCascadeForFontUnits() const
+{
+    if (computingFontSize()) {
+        ASSERT(parentStyle());
+        return parentStyle()->fontCascade();
+    }
+    ASSERT(style());
+    return style()->fontCascade();
+}
+
+int CSSToLengthConversionData::computedLineHeightForFontUnits() const
+{
+    if (computingFontSize()) {
+        ASSERT(parentStyle());
+        return parentStyle()->computedLineHeight();
+    }
+    ASSERT(style());
+    return style()->computedLineHeight();
+}
+
 float CSSToLengthConversionData::zoom() const
 {
     return m_zoom.value_or(m_style ? m_style->effectiveZoom() : 1.f);

Source/WebCore/css/CSSToLengthConversionData.h

@@ -39,6 +39,7 @@ namespace WebCore {
 
 class Element;
 class FloatSize;
+class FontCascade;
 class RenderStyle;
 class RenderView;
 
@@ -65,15 +66,17 @@ class CSSToLengthConversionData {
     const RenderView* renderView() const { return m_renderView; }
     const Element* elementForContainerUnitResolution() const { return m_elementForContainerUnitResolution.get(); }
 
+    const FontCascade& fontCascadeForFontUnits() const;
+    int computedLineHeightForFontUnits() const;
+
     FloatSize defaultViewportFactor() const;
     FloatSize smallViewportFactor() const;
     FloatSize largeViewportFactor() const;
     FloatSize dynamicViewportFactor() const;
 
-    CSSToLengthConversionData copyForFontSizeWithParentStyle() const
+    CSSToLengthConversionData copyForFontSize() const
     {
         CSSToLengthConversionData copy(*this);
-        copy.m_style = parentStyle();
         copy.m_zoom = 1.f;
         copy.m_propertyToCompute = CSSPropertyFontSize;
         return copy;

Source/WebCore/css/makeprop.pl

@@ -67,6 +67,7 @@
 my $numPredefinedProperties = 2;
 my %nameIsColorProperty;
 my %nameIsDescriptorOnly;
+my %nameIsTopPriority;
 my %nameIsHighPriority;
 my %nameIsDeferred;
 my %nameIsInherited;
@@ -294,7 +295,12 @@ ($$)
                     next;
                 } elsif ($codegenOptionName eq "comment") {
                     next;
+                } elsif ($codegenOptionName eq "top-priority") {
+                    die "$name has top priority, but no comment to justify" if not (exists $codegenProperties->{"comment"});
+                    die "$name is a shorthand, but has top-priority" if exists $codegenProperties->{"longhands"};
+                    $nameIsTopPriority{$name} = 1;
                 } elsif ($codegenOptionName eq "high-priority") {
+                    die "$name can't have conflicting top/high priority" if !!$nameIsTopPriority{$name};
                     die "$name is a shorthand, but has high-priority" if exists $codegenProperties->{"longhands"};
                     $nameIsHighPriority{$name} = 1;
                 } elsif ($codegenOptionName eq "sink-priority") {
@@ -375,6 +381,13 @@ sub sortByDescendingPriorityAndName
     if (exists $propertiesWithStyleBuilderOptions{$a}{"longhands"} > exists $propertiesWithStyleBuilderOptions{$b}{"longhands"}) {
         return 1;
     }
+    # Sort longhands with top priority to the front
+    if (!!$nameIsTopPriority{$a} < !!$nameIsTopPriority{$b}) {
+        return 1;
+    }
+    if (!!$nameIsTopPriority{$a} > !!$nameIsTopPriority{$b}) {
+        return -1;
+    }
     # Sort longhands with high priority to the front
     if (!!$nameIsHighPriority{$a} < !!$nameIsHighPriority{$b}) {
         return 1;
@@ -948,6 +961,8 @@ sub sortByDescendingPriorityAndName
 my $first = $numPredefinedProperties;
 my $i = $numPredefinedProperties;
 my $maxLen = 0;
+my $firstTopPriorityPropertyName;
+my $lastTopPriorityPropertyName;
 my $firstHighPriorityPropertyName;
 my $lastHighPriorityPropertyName;
 my $firstLowPriorityPropertyName;
@@ -961,6 +976,9 @@ sub sortByDescendingPriorityAndName
   if (exists $propertiesWithStyleBuilderOptions{$name}{"longhands"}) {
     $firstShorthandPropertyName = $name if !$firstShorthandPropertyName;
     $lastShorthandPropertyName = $name;
+  } elsif ($nameIsTopPriority{$name}) {
+    $firstTopPriorityPropertyName = $name if !$firstTopPriorityPropertyName;
+    $lastTopPriorityPropertyName = $name;
   } elsif ($nameIsHighPriority{$name}) {
     $firstHighPriorityPropertyName = $name if !$firstHighPriorityPropertyName;
     $lastHighPriorityPropertyName = $name;
@@ -985,6 +1003,8 @@ sub sortByDescendingPriorityAndName
 print HEADER "const int numCSSProperties = $num;\n";
 print HEADER "const int lastCSSProperty = $last;\n";
 print HEADER "const size_t maxCSSPropertyNameLength = $maxLen;\n";
+print HEADER "const CSSPropertyID firstTopPriorityProperty = CSSProperty" . $nameToId{$firstTopPriorityPropertyName} . ";\n";
+print HEADER "const CSSPropertyID lastTopPriorityProperty = CSSProperty" . $nameToId{$lastTopPriorityPropertyName} . ";\n";
 print HEADER "const CSSPropertyID firstHighPriorityProperty = CSSProperty" . $nameToId{$firstHighPriorityPropertyName} . ";\n";
 print HEADER "const CSSPropertyID lastHighPriorityProperty = CSSProperty" . $nameToId{$lastHighPriorityPropertyName} . ";\n";
 print HEADER "const CSSPropertyID firstLowPriorityProperty = CSSProperty" . $nameToId{$firstLowPriorityPropertyName} . ";\n";

Source/WebCore/style/StyleBuilder.cpp

@@ -85,23 +85,22 @@ Builder::~Builder() = default;
 
 void Builder::applyAllProperties()
 {
+    applyTopPriorityProperties();
     applyHighPriorityProperties();
     applyNonHighPriorityProperties();
 }
 
-// High priority properties may affect resolution of other properties (they are mostly font related).
-void Builder::applyHighPriorityProperties()
+// Top priority properties affect resolution of high priority properties.
+void Builder::applyTopPriorityProperties()
 {
-    applyProperties(CSSPropertyWebkitRubyPosition, CSSPropertyWebkitRubyPosition);
+    applyProperties(firstTopPriorityProperty, lastTopPriorityProperty);
     m_state.adjustStyleForInterCharacterRuby();
+}
 
-#if ENABLE(DARK_MODE_CSS)
-    // Supported color schemes can affect resolved colors, so we need to apply that property before any color properties.
-    applyProperties(CSSPropertyColorScheme, CSSPropertyColorScheme);
-#endif
-
+// High priority properties may affect resolution of other properties (they are mostly font related).
+void Builder::applyHighPriorityProperties()
+{
     applyProperties(firstHighPriorityProperty, lastHighPriorityProperty);
-
     m_state.updateFont();
 }
 

Source/WebCore/style/StyleBuilder.h

@@ -39,6 +39,7 @@ class Builder {
     ~Builder();
 
     void applyAllProperties();
+    void applyTopPriorityProperties();
     void applyHighPriorityProperties();
     void applyNonHighPriorityProperties();
 

Source/WebCore/style/StyleBuilderCustom.h

@@ -1864,13 +1864,12 @@ inline void BuilderCustom::applyValueFontSize(BuilderState& builderState, CSSVal
     } else {
         fontDescription.setIsAbsoluteSize(parentIsAbsoluteSize || !(primitiveValue.isPercentage() || primitiveValue.isFontRelativeLength()));
         if (primitiveValue.isLength()) {
-            // font-size is resolved against the parent style instead of the current style.
-            auto conversionData = builderState.cssToLengthConversionData().copyForFontSizeWithParentStyle();
+            auto conversionData = builderState.cssToLengthConversionData().copyForFontSize();
             size = primitiveValue.computeLength<float>(conversionData);
         } else if (primitiveValue.isPercentage())
             size = (primitiveValue.floatValue() * parentSize) / 100.0f;
         else if (primitiveValue.isCalculatedPercentageWithLength()) {
-            auto conversionData = builderState.cssToLengthConversionData().copyForFontSizeWithParentStyle();
+            auto conversionData = builderState.cssToLengthConversionData().copyForFontSize();
             size = primitiveValue.cssCalcValue()->createCalculationValue(conversionData)->evaluate(parentSize);
         } else
             return;

Source/WebCore/style/StyleResolver.cpp

@@ -622,6 +622,9 @@ void Resolver::applyMatchedProperties(State& state, const MatchResult& matchResu
 
     Builder builder(*state.style(), builderContext(state), matchResult, CascadeLevel::Author, includedProperties);
 
+    // Top priority properties may affect resolution of high priority ones.
+    builder.applyTopPriorityProperties();
+
     // High priority properties may affect resolution of other properties (they are mostly font related).
     builder.applyHighPriorityProperties();
 

Tools/Scripts/webkitpy/style/checkers/jsonchecker.py

@@ -295,6 +295,7 @@ def check_codegen_properties(self, property_name, codegen_properties):
             'fill-layer-property': self.validate_boolean,
             'font-property': self.validate_boolean,
             'getter': self.validate_string,
+            'top-priority': self.validate_boolean,
             'high-priority': self.validate_boolean,
             'initial': self.validate_string,
             'internal-only': self.validate_boolean,