Fix styling of th elements when explicitly specifiying text-align:inh…

…erit https://bugs.webkit.org/show_bug.cgi?id=138577 Patch by Ryan Reno <rreno@apple.com> on 2022-06-16 Reviewed by Tim Nguyen. <th> elements were being incorrectly centered when specifying text-align: inherit. This fixes that bug by adding a new internal CSS value for use in the UA stylesheet. This also removes a non-inherited flag that was meant to be used for detecting this special case but ultimately didn't work due to conflicts with the `all` property. * Source/WebCore/css/CSSValueKeywords.in: * Source/WebCore/css/html.css: (th): * Source/WebCore/css/parser/CSSParserFastPaths.cpp: (WebCore::CSSParserFastPaths::isValidKeywordPropertyAndValue): * Source/WebCore/css/parser/CSSParserIdioms.cpp: (WebCore::isValueAllowedInMode): * Source/WebCore/rendering/style/RenderStyle.cpp: (WebCore::RenderStyle::RenderStyle): * Source/WebCore/rendering/style/RenderStyle.h: (WebCore::RenderStyle::NonInheritedFlags::operator== const): (WebCore::RenderStyle::hasExplicitlySetTextAlign const): Deleted. (WebCore::RenderStyle::setHasExplicitlySetTextAlign): Deleted. * Source/WebCore/style/StyleAdjuster.cpp: (WebCore::Style::Adjuster::adjust const): * Source/WebCore/style/StyleBuilderConverter.h: (WebCore::Style::BuilderConverter::convertTextAlign): * Source/WebCore/css/CSSProperties.json: * Source/WebCore/style/StyleBuilderCustom.h: (WebCore::Style::BuilderCustom::applyInitialTextAlign): Deleted. (WebCore::Style::BuilderCustom::applyValueTextAlign): Deleted. * LayoutTests/fast/css/internal-th-center-ua-only-expected.txt: Added. * LayoutTests/fast/css/internal-th-center-ua-only.html: Added. * LayoutTests/fast/table/center-th-when-parent-has-initial-text-align-expected.html: * LayoutTests/fast/table/center-th-when-parent-has-initial-text-align.html: Canonical link: https://commits.webkit.org/251630@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295625 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jun 17, 2022 · 6975dc56ad0e73dcc5f9ae547d08b1ef3d847b96 · 6975dc5
1 parent 71960be
commit 6975dc56ad0e73dcc5f9ae547d08b1ef3d847b96
Show file tree

Hide file tree

Showing 14 changed files with 67 additions and 46 deletions.
diff --git a/LayoutTests/fast/css/internal-th-center-ua-only-expected.txt b/LayoutTests/fast/css/internal-th-center-ua-only-expected.txt
@@ -0,0 +1,4 @@
+
+PASS "text-align" property does not support value "-internal-th-center".
+PASS "text-align" property cannot be set to "-internal-th-center" by the author stylesheet.
+
diff --git a/LayoutTests/fast/css/internal-th-center-ua-only.html b/LayoutTests/fast/css/internal-th-center-ua-only.html
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+
+<style>
+th {
+    text-align: right;
+    text-align: -internal-th-center;
+}
+</style>
+<table>
+  <tr>
+    <th id="header"></th>
+  </tr>
+</table>
+
+<script>
+  test(function() {
+    assert_false(CSS.supports('text-align', '-internal-th-center'));
+  }, '"text-align" property does not support value "-internal-th-center".');
+
+  test(function() {
+    assert_equals(getComputedStyle(header).textAlign, 'right');
+  }, '"text-align" property cannot be set to "-internal-th-center" by the author stylesheet.');
+</script>
diff --git a/LayoutTests/fast/table/center-th-when-parent-has-initial-text-align-expected.html b/LayoutTests/fast/table/center-th-when-parent-has-initial-text-align-expected.html
@@ -4,19 +4,21 @@
 <title>This tests that th is text-align centered when parent has initial value.</title>
 <style>
     table { 
-    width: 500px; border: 1px solid green; 
+    width: 500px; border: 1px solid green;
 }
 </style>
 <table><tbody>
     <tr><th style="text-align: center">no explicit style</th></tr>
     <tr><th style="text-align: left">th initial</th></tr>
     <tr><th style="text-align: left">th start</th></tr>
     <tr><th style="text-align: center">th center</th></tr>
+    <tr><th style="text-align: left">th initial</th></tr>
     <tr><th style="text-align: right">th end</th></tr>
     <tr><th style="text-align: center">tr initial</th></tr>
     <tr><th style="text-align: center">tr start</th></tr>
     <tr><th style="text-align: center">tr center</th></tr>
     <tr><th style="text-align: right">tr end</th></tr>
+    <tr><th style="text-align: right">tr end</th></tr>
     <tr><th style="text-align: left">tr initial, th initial</th></tr>
     <tr><th style="text-align: left">tr initial, th start</th></tr>
     <tr><th style="text-align: left">tr start, th start</th></tr>

diff --git a/LayoutTests/fast/table/center-th-when-parent-has-initial-text-align.html b/LayoutTests/fast/table/center-th-when-parent-has-initial-text-align.html
@@ -4,19 +4,21 @@
 <title>This tests that th is text-align centered when parent has initial value.</title>
 <style>
     table { 
-    width: 500px; border: 1px solid green; 
+    width: 500px; border: 1px solid green;
 }
 </style>
 <table><tbody>
     <tr><th>no explicit style</th></tr>
     <tr><th style="text-align: initial">th initial</th></tr>
     <tr><th style="text-align: start">th start</th></tr>
     <tr><th style="text-align: center">th center</th></tr>
+    <tr><th style="text-align: inherit">th initial</th></tr>
     <tr><th style="text-align: end">th end</th></tr>
     <tr style="text-align: initial"><th>tr initial</th></tr>
     <tr style="text-align: start"><th>tr start</th></tr>
     <tr style="text-align: center"><th>tr center</th></tr>
     <tr style="text-align: end"><th>tr end</th></tr>
+    <tr style="text-align: end"><th style="text-align: inherit">tr end</th></tr>
     <tr style="text-align: initial"><th style="text-align: initial">tr initial, th initial</th></tr>
     <tr style="text-align: initial"><th style="text-align: start">tr initial, th start</th></tr>
     <tr style="text-align: start"><th style="text-align: start">tr start, th start</th></tr>

diff --git a/Source/WebCore/css/CSSProperties.json b/Source/WebCore/css/CSSProperties.json
@@ -4587,8 +4587,7 @@
                 }
             ],
             "codegen-properties": {
-                "converter": "TextAlign",
-                "custom": "Initial|Value"
+                "converter": "TextAlign"
             },
             "specification": {
                 "category": "css-22",

diff --git a/Source/WebCore/css/CSSValueKeywords.in b/Source/WebCore/css/CSSValueKeywords.in
@@ -345,6 +345,7 @@ justify
 -webkit-center
 match-parent
 -webkit-match-parent
+-internal-th-center
 //start
 //end
 //

diff --git a/Source/WebCore/css/html.css b/Source/WebCore/css/html.css
@@ -269,6 +269,7 @@ td, th {
 
 th {
     font-weight: bold;
+    text-align: -internal-th-center;
 }
 
 caption {

diff --git a/Source/WebCore/css/parser/CSSParserFastPaths.cpp b/Source/WebCore/css/parser/CSSParserFastPaths.cpp
@@ -715,7 +715,7 @@ bool CSSParserFastPaths::isValidKeywordPropertyAndValue(CSSPropertyID propertyId
     case CSSPropertyTableLayout: // auto | fixed
         return valueID == CSSValueAuto || valueID == CSSValueFixed;
     case CSSPropertyTextAlign:
-        return (valueID >= CSSValueWebkitAuto && valueID <= CSSValueWebkitMatchParent) || valueID == CSSValueStart || valueID == CSSValueEnd;
+        return (valueID >= CSSValueWebkitAuto && valueID <= CSSValueInternalThCenter) || valueID == CSSValueStart || valueID == CSSValueEnd;
     case CSSPropertyTextAlignLast:
         // auto | start | end | left | right | center | justify | match-parent
         return (valueID >= CSSValueLeft && valueID <= CSSValueJustify) || valueID == CSSValueStart || valueID == CSSValueEnd || valueID == CSSValueAuto || valueID == CSSValueMatchParent;

diff --git a/Source/WebCore/css/parser/CSSParserIdioms.cpp b/Source/WebCore/css/parser/CSSParserIdioms.cpp
@@ -40,6 +40,8 @@ bool isValueAllowedInMode(unsigned short id, CSSParserMode mode)
         return isUASheetBehavior(mode);
     case CSSValueWebkitFocusRingColor:
         return isUASheetBehavior(mode) || isQuirksModeBehavior(mode);
+    case CSSValueInternalThCenter:
+        return isUASheetBehavior(mode);
     default:
         return true;
     }

diff --git a/Source/WebCore/rendering/style/RenderStyle.cpp b/Source/WebCore/rendering/style/RenderStyle.cpp
@@ -197,7 +197,6 @@ RenderStyle::RenderStyle(CreateDefaultStyleTag)
     m_nonInheritedFlags.hasExplicitlySetBorderTopRightRadius = false;
     m_nonInheritedFlags.hasExplicitlySetDirection = false;
     m_nonInheritedFlags.hasExplicitlySetWritingMode = false;
-    m_nonInheritedFlags.hasExplicitlySetTextAlign = false;
     m_nonInheritedFlags.usesViewportUnits = false;
     m_nonInheritedFlags.usesContainerUnits = false;
     m_nonInheritedFlags.hasExplicitlyInheritedProperties = false;

diff --git a/Source/WebCore/rendering/style/RenderStyle.h b/Source/WebCore/rendering/style/RenderStyle.h
@@ -1570,9 +1570,6 @@ class RenderStyle {
     bool hasExplicitlySetWritingMode() const { return m_nonInheritedFlags.hasExplicitlySetWritingMode; }
     void setHasExplicitlySetWritingMode(bool v) { m_nonInheritedFlags.hasExplicitlySetWritingMode = v; }
 
-    bool hasExplicitlySetTextAlign() const { return m_nonInheritedFlags.hasExplicitlySetTextAlign; }
-    void setHasExplicitlySetTextAlign(bool v) { m_nonInheritedFlags.hasExplicitlySetTextAlign = v; }
-
     // A unique style is one that has matches something that makes it impossible to share.
     bool unique() const { return m_nonInheritedFlags.isUnique; }
     void setUnique() { m_nonInheritedFlags.isUnique = true; }
@@ -1990,7 +1987,6 @@ class RenderStyle {
         unsigned hasExplicitlySetBorderTopRightRadius : 1;
         unsigned hasExplicitlySetDirection : 1;
         unsigned hasExplicitlySetWritingMode : 1;
-        unsigned hasExplicitlySetTextAlign : 1;
 #if ENABLE(DARK_MODE_CSS)
         unsigned hasExplicitlySetColorScheme : 1;
 #endif
@@ -2132,7 +2128,6 @@ inline bool RenderStyle::NonInheritedFlags::operator==(const NonInheritedFlags&
         && hasExplicitlySetBorderTopRightRadius == other.hasExplicitlySetBorderTopRightRadius
         && hasExplicitlySetDirection == other.hasExplicitlySetDirection
         && hasExplicitlySetWritingMode == other.hasExplicitlySetWritingMode
-        && hasExplicitlySetTextAlign == other.hasExplicitlySetTextAlign
 #if ENABLE(DARK_MODE_CSS)
         && hasExplicitlySetColorScheme == other.hasExplicitlySetColorScheme
 #endif

diff --git a/Source/WebCore/style/StyleAdjuster.cpp b/Source/WebCore/style/StyleAdjuster.cpp
@@ -315,13 +315,6 @@ void Adjuster::adjust(RenderStyle& style, const RenderStyle* userAgentAppearance
                 style.setFloating(Float::None);
             }
 
-            // User agents are expected to have a rule in their user agent stylesheet that matches th elements that have a parent
-            // node whose computed value for the 'text-align' property is its initial value, whose declaration block consists of
-            // just a single declaration that sets the 'text-align' property to the value 'center'.
-            // https://html.spec.whatwg.org/multipage/rendering.html#rendering
-            if (m_element->hasTagName(thTag) && !style.hasExplicitlySetTextAlign() && m_parentStyle.textAlign() == RenderStyle::initialTextAlign())
-                style.setTextAlign(TextAlignMode::Center);
-
             if (m_element->hasTagName(legendTag))
                 style.setEffectiveDisplay(DisplayType::Block);
         }

diff --git a/Source/WebCore/style/StyleBuilderConverter.h b/Source/WebCore/style/StyleBuilderConverter.h
@@ -610,23 +610,37 @@ inline OptionSet<TextEmphasisPosition> BuilderConverter::convertTextEmphasisPosi
 
 inline TextAlignMode BuilderConverter::convertTextAlign(BuilderState& builderState, const CSSValue& value)
 {
-    auto& primitiveValue = downcast<CSSPrimitiveValue>(value);
+    const auto& primitiveValue = downcast<CSSPrimitiveValue>(value);
     ASSERT(primitiveValue.isValueID());
 
-    if (primitiveValue.valueID() != CSSValueWebkitMatchParent && primitiveValue.valueID() != CSSValueMatchParent)
-        return primitiveValue;
+    const auto& parentStyle = builderState.parentStyle();
 
-    auto* element = builderState.element();
-    if (element && element == builderState.document().documentElement())
-        return TextAlignMode::Start;
+    // User agents are expected to have a rule in their user agent stylesheet that matches th elements that have a parent
+    // node whose computed value for the 'text-align' property is its initial value, whose declaration block consists of
+    // just a single declaration that sets the 'text-align' property to the value 'center'.
+    // https://html.spec.whatwg.org/multipage/rendering.html#rendering
+    if (primitiveValue.valueID() == CSSValueInternalThCenter) {
+        if (parentStyle.textAlign() == RenderStyle::initialTextAlign())
+            return TextAlignMode::Center;
+        return parentStyle.textAlign();
+    }
 
-    auto& parentStyle = builderState.parentStyle();
-    if (parentStyle.textAlign() == TextAlignMode::Start)
-        return parentStyle.isLeftToRightDirection() ? TextAlignMode::Left : TextAlignMode::Right;
-    if (parentStyle.textAlign() == TextAlignMode::End)
-        return parentStyle.isLeftToRightDirection() ? TextAlignMode::Right : TextAlignMode::Left;
-    return parentStyle.textAlign();
+    if (primitiveValue.valueID() == CSSValueWebkitMatchParent || primitiveValue.valueID() == CSSValueMatchParent) {
+        const auto* element = builderState.element();
+
+        if (element && element == builderState.document().documentElement())
+            return TextAlignMode::Start;
+        if (parentStyle.textAlign() == TextAlignMode::Start)
+            return parentStyle.isLeftToRightDirection() ? TextAlignMode::Left : TextAlignMode::Right;
+        if (parentStyle.textAlign() == TextAlignMode::End)
+            return parentStyle.isLeftToRightDirection() ? TextAlignMode::Right : TextAlignMode::Left;
+
+        return parentStyle.textAlign();
+    }
+
+    return primitiveValue;
 }
+
 inline TextAlignLast BuilderConverter::convertTextAlignLast(BuilderState& builderState, const CSSValue& value)
 {
     auto& primitiveValue = downcast<CSSPrimitiveValue>(value);

diff --git a/Source/WebCore/style/StyleBuilderCustom.h b/Source/WebCore/style/StyleBuilderCustom.h
@@ -136,10 +136,6 @@ class BuilderCustom {
     static void applyInheritVerticalAlign(BuilderState&);
     static void applyValueVerticalAlign(BuilderState&, CSSValue&);
 
-    // Custom handling of initial + value only.
-    static void applyInitialTextAlign(BuilderState&);
-    static void applyValueTextAlign(BuilderState&, CSSValue&);
-
     // Custom handling of value setting only.
     static void applyValueBaselineShift(BuilderState&, CSSValue&);
     static void applyValueDirection(BuilderState&, CSSValue&);
@@ -192,18 +188,6 @@ inline void BuilderCustom::applyValueDirection(BuilderState& builderState, CSSVa
     builderState.style().setHasExplicitlySetDirection(true);
 }
 
-inline void BuilderCustom::applyInitialTextAlign(BuilderState& builderState)
-{
-    builderState.style().setTextAlign(RenderStyle::initialTextAlign());
-    builderState.style().setHasExplicitlySetTextAlign(true);
-}
-
-inline void BuilderCustom::applyValueTextAlign(BuilderState& builderState, CSSValue& value)
-{
-    builderState.style().setTextAlign(BuilderConverter::convertTextAlign(builderState, value));
-    builderState.style().setHasExplicitlySetTextAlign(true);
-}
-
 inline void BuilderCustom::resetEffectiveZoom(BuilderState& builderState)
 {
     // Reset the zoom in effect. This allows the setZoom method to accurately compute a new zoom in effect.