Attachment 451117 Details for Bug 236199 – Patch

[patch] Patch

bug-236199-20220207183243.patch (text/plain), 18.20 KB, created by Oriol Brufau on 2022-02-07 09:32:45 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Oriol Brufau

Created: 2022-02-07 09:32:45 PST

Size: 18.20 KB

patch

obsolete

>Subversion Revision: 289207
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 187e4429fad727cb76c391cec50fea8f6c98ef89..63d20ef5bee599cc8062595d91eb0865131e7108 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,39 @@
>+2022-02-07  Oriol Brufau  <obrufau@igalia.com>
>+
>+        Fix CSS cascade regarding logical properties
>+        https://bugs.webkit.org/show_bug.cgi?id=236199
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The CSS cascade was trying to resolve logical properties into physical
>+        ones too early. This failed if we still didn't know the direction or
>+        writing-mode, e.g. because they were set to a variable or to a CSS-wide
>+        keyword.
>+
>+        This patch keeps logical properties as-is during the cascade. They are
>+        only resolved when finally applied. Also, both logical properties and
>+        their physical equivalents are now set to apply in parse order, since
>+        'height: 0px; block-size: 1px' and 'block-size: 1px; height: 0px' can be
>+        different, the order matters.
>+
>+
>+        Tests: imported/w3c/web-platform-tests/css/css-logical/animation-004.html
>+               imported/w3c/web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode.html
>+
>+        * css/parser/CSSParser.cpp:
>+        (WebCore::CSSParser::parseValueWithVariableReferences):
>+        * style/PropertyCascade.cpp:
>+        (WebCore::Style::shouldApplyPropertyInParseOrder):
>+        (WebCore::Style::PropertyCascade::PropertyCascade):
>+        (WebCore::Style::PropertyCascade::set):
>+        (WebCore::Style::PropertyCascade::resolveDirectionAndWritingMode const): Deleted.
>+        (WebCore::Style::PropertyCascade::direction const): Deleted.
>+        * style/PropertyCascade.h:
>+        * style/StyleBuilder.cpp:
>+        (WebCore::Style::Builder::Builder):
>+        (WebCore::Style::Builder::applyProperty):
>+        (WebCore::Style::directionFromStyle): Deleted.
>+
> 2022-02-07  Nikolas Zimmermann  <nzimmermann@igalia.com>
> 
>         [LBSE] Apply position / size to SVG layers
>diff --git a/Source/WebCore/css/parser/CSSParser.cpp b/Source/WebCore/css/parser/CSSParser.cpp
>index 70e921de7cdba4b783dd21ed300482e4165118d4..9ddec1476b97b37917089900c322f3fc7b4a3f83 100644
>--- a/Source/WebCore/css/parser/CSSParser.cpp
>+++ b/Source/WebCore/css/parser/CSSParser.cpp
>@@ -213,10 +213,7 @@ RefPtr<CSSValue> CSSParser::parseValueWithVariableReferences(CSSPropertyID propI
>             return nullptr;
> 
>         for (auto& property : parsedProperties) {
>-            CSSPropertyID currentId = property.id();
>-            if (CSSProperty::isDirectionAwareProperty(currentId))
>-                currentId = CSSProperty::resolveDirectionAwareProperty(currentId, direction, writingMode);
>-            if (currentId == propID)
>+            if (property.id() == propID)
>                 return property.value();
>         }
> 
>diff --git a/Source/WebCore/style/PropertyCascade.cpp b/Source/WebCore/style/PropertyCascade.cpp
>index 9e9b29b7c454486152eea3aee8c76ded7ad7810e..d3926947fa5c3fb7dc60252665fb966bdcdf1d9b 100644
>--- a/Source/WebCore/style/PropertyCascade.cpp
>+++ b/Source/WebCore/style/PropertyCascade.cpp
>@@ -68,15 +68,14 @@ static inline bool shouldApplyPropertyInParseOrder(CSSPropertyID propertyID)
>     case CSSPropertyTextDecoration:
>         return true;
>     default:
>-        return false;
>+        return CSSProperty::isInLogicalPropertyGroup(propertyID);
>     }
> }
> 
>-PropertyCascade::PropertyCascade(const MatchResult& matchResult, CascadeLevel maximumCascadeLevel, IncludedProperties includedProperties, Direction direction)
>+PropertyCascade::PropertyCascade(const MatchResult& matchResult, CascadeLevel maximumCascadeLevel, IncludedProperties includedProperties)
>     : m_matchResult(matchResult)
>     , m_includedProperties(includedProperties)
>     , m_maximumCascadeLevel(maximumCascadeLevel)
>-    , m_direction(direction)
> {
>     buildCascade();
> }
>@@ -86,8 +85,6 @@ PropertyCascade::PropertyCascade(const PropertyCascade& parent, CascadeLevel max
>     , m_includedProperties(parent.m_includedProperties)
>     , m_maximumCascadeLevel(maximumCascadeLevel)
>     , m_maximumCascadeLayerPriorityForRollback(maximumCascadeLayerPriorityForRollback)
>-    , m_direction(parent.direction())
>-    , m_directionIsUnresolved(false)
> {
>     buildCascade();
> }
>@@ -132,11 +129,6 @@ void PropertyCascade::setPropertyInternal(Property& property, CSSPropertyID id,
> 
> void PropertyCascade::set(CSSPropertyID id, CSSValue& cssValue, const MatchedProperties& matchedProperties, CascadeLevel cascadeLevel)
> {
>-    if (CSSProperty::isDirectionAwareProperty(id)) {
>-        auto direction = this->direction();
>-        id = CSSProperty::resolveDirectionAwareProperty(id, direction.textDirection, direction.writingMode);
>-    }
>-
>     ASSERT(!shouldApplyPropertyInParseOrder(id));
> 
>     auto& property = m_properties[id];
>@@ -303,50 +295,5 @@ void PropertyCascade::addImportantMatches(CascadeLevel cascadeLevel)
>         addMatch(matchedDeclarations[match.index], cascadeLevel, true);
> }
> 
>-PropertyCascade::Direction PropertyCascade::resolveDirectionAndWritingMode(Direction inheritedDirection) const
>-{
>-    Direction result = inheritedDirection;
>-
>-    bool hadImportantWritingMode = false;
>-    bool hadImportantDirection = false;
>-
>-    for (auto cascadeLevel : { CascadeLevel::UserAgent, CascadeLevel::User, CascadeLevel::Author }) {
>-        for (const auto& matchedProperties : declarationsForCascadeLevel(m_matchResult, cascadeLevel)) {
>-            for (unsigned i = 0, count = matchedProperties.properties->propertyCount(); i < count; ++i) {
>-                auto property = matchedProperties.properties->propertyAt(i);
>-                if (!property.value()->isPrimitiveValue() || property.value()->isCSSWideKeyword())
>-                    continue;
>-                switch (property.id()) {
>-                case CSSPropertyWritingMode:
>-                    if (!hadImportantWritingMode || property.isImportant()) {
>-                        result.writingMode = downcast<CSSPrimitiveValue>(*property.value());
>-                        hadImportantWritingMode = property.isImportant();
>-                    }
>-                    break;
>-                case CSSPropertyDirection:
>-                    if (!hadImportantDirection || property.isImportant()) {
>-                        result.textDirection = downcast<CSSPrimitiveValue>(*property.value());
>-                        hadImportantDirection = property.isImportant();
>-                    }
>-                    break;
>-                default:
>-                    break;
>-                }
>-            }
>-        }
>-    }
>-
>-    return result;
>-}
>-
>-PropertyCascade::Direction PropertyCascade::direction() const
>-{
>-    if (m_directionIsUnresolved) {
>-        m_direction = resolveDirectionAndWritingMode(m_direction);
>-        m_directionIsUnresolved = false;
>-    }
>-    return m_direction;
>-}
>-
> }
> }
>diff --git a/Source/WebCore/style/PropertyCascade.h b/Source/WebCore/style/PropertyCascade.h
>index 5064f781b34e706584013025f8b95ec681a7684a..8421c5f6dd1215c60551c75794fd5ec516af1bea 100644
>--- a/Source/WebCore/style/PropertyCascade.h
>+++ b/Source/WebCore/style/PropertyCascade.h
>@@ -40,12 +40,7 @@ class PropertyCascade {
> public:
>     enum IncludedProperties { All, InheritedOnly };
> 
>-    struct Direction {
>-        TextDirection textDirection;
>-        WritingMode writingMode;
>-    };
>-
>-    PropertyCascade(const MatchResult&, CascadeLevel, IncludedProperties, Direction);
>+    PropertyCascade(const MatchResult&, CascadeLevel, IncludedProperties);
>     PropertyCascade(const PropertyCascade&, CascadeLevel, std::optional<CascadeLayerPriority> maximumCascadeLayerPriorityForRollback = { });
> 
>     ~PropertyCascade();
>@@ -68,8 +63,6 @@ public:
>     const Vector<Property, 8>& deferredProperties() const { return m_deferredProperties; }
>     const HashMap<AtomString, Property>& customProperties() const { return m_customProperties; }
> 
>-    Direction direction() const;
>-
> private:
>     void buildCascade();
>     bool addNormalMatches(CascadeLevel);
>@@ -80,14 +73,10 @@ private:
>     void setDeferred(CSSPropertyID, CSSValue&, const MatchedProperties&, CascadeLevel);
>     static void setPropertyInternal(Property&, CSSPropertyID, CSSValue&, const MatchedProperties&, CascadeLevel);
> 
>-    Direction resolveDirectionAndWritingMode(Direction inheritedDirection) const;
>-
>     const MatchResult& m_matchResult;
>     const IncludedProperties m_includedProperties;
>     const CascadeLevel m_maximumCascadeLevel;
>     const std::optional<CascadeLayerPriority> m_maximumCascadeLayerPriorityForRollback;
>-    mutable Direction m_direction;
>-    mutable bool m_directionIsUnresolved { true };
> 
>     Property m_properties[numCSSProperties + 2];
>     std::bitset<numCSSProperties + 2> m_propertyIsPresent;
>diff --git a/Source/WebCore/style/StyleBuilder.cpp b/Source/WebCore/style/StyleBuilder.cpp
>index ccbb49b467676de7cb3cf9c12c7d0930d7f8c904..ff8efa55b9650af8ad2698df354c14f5564bfdbd 100644
>--- a/Source/WebCore/style/StyleBuilder.cpp
>+++ b/Source/WebCore/style/StyleBuilder.cpp
>@@ -48,11 +48,6 @@ namespace Style {
> 
> static const CSSPropertyID firstLowPriorityProperty = static_cast<CSSPropertyID>(lastHighPriorityProperty + 1);
> 
>-inline PropertyCascade::Direction directionFromStyle(const RenderStyle& style)
>-{
>-    return { style.direction(), style.writingMode() };
>-}
>-
> inline bool isValidVisitedLinkProperty(CSSPropertyID id)
> {
>     switch (id) {
>@@ -81,7 +76,7 @@ inline bool isValidVisitedLinkProperty(CSSPropertyID id)
> }
> 
> Builder::Builder(RenderStyle& style, BuilderContext&& context, const MatchResult& matchResult, CascadeLevel cascadeLevel, PropertyCascade::IncludedProperties includedProperties)
>-    : m_cascade(matchResult, cascadeLevel, includedProperties, directionFromStyle(style))
>+    : m_cascade(matchResult, cascadeLevel, includedProperties)
>     , m_state(*this, style, WTFMove(context))
> {
> }
>@@ -273,8 +268,8 @@ void Builder::applyProperty(CSSPropertyID id, CSSValue& value, SelectorChecker::
>     auto valueToApply = resolveValue(id, value);
> 
>     if (CSSProperty::isDirectionAwareProperty(id)) {
>-        auto direction = m_cascade.direction();
>-        CSSPropertyID newId = CSSProperty::resolveDirectionAwareProperty(id, direction.textDirection, direction.writingMode);
>+        const RenderStyle& style = m_state.style();
>+        CSSPropertyID newId = CSSProperty::resolveDirectionAwareProperty(id, style.direction(), style.writingMode());
>         ASSERT(newId != id);
>         return applyProperty(newId, valueToApply.get(), linkMatchMask);
>     }
>diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
>index b11a68bd880596530730f2e949fce046774644e1..dcf98b4f7d11935e6b06540da47a11348154b7d1 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,3 +1,17 @@
>+2022-02-07  Oriol Brufau  <obrufau@igalia.com>
>+
>+        Fix CSS cascade regarding logical properties
>+        https://bugs.webkit.org/show_bug.cgi?id=236199
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Expect animation-004.html to pass.
>+        Add new test logicalprops-with-deferred-writing-mode.html
>+
>+        * web-platform-tests/css/css-logical/animation-004-expected.txt:
>+        * web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode-expected.txt: Added.
>+        * web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode.html: Added.
>+
> 2022-02-06  Sam Weinig  <weinig@apple.com>
> 
>         Update serialization of rgb() functions with none components to latest spec
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/animation-004-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/animation-004-expected.txt
>index b28fb3097efeb42720d68ffca5ee776e7bc61e6e..0c76b52f319966ba170255e813480297d64dc009 100644
>--- a/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/animation-004-expected.txt
>+++ b/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/animation-004-expected.txt
>@@ -11,7 +11,7 @@ PASS Transitions update when the writing-mode is changed
> PASS Filling transitions update when the writing-mode is changed
> PASS The number of interpolating properties can be increased when the writing-mode is changed
> PASS The number of interpolating properties can be decreased when the writing-mode is changed
>-FAIL Transitions update when the writing-mode is changed through a CSS variable assert_equals: expected "50px" but got "0px"
>+PASS Transitions update when the writing-mode is changed through a CSS variable
> PASS Transitions update when the direction is changed
> PASS Transitions from logical to physical update when the direction is changed
> PASS Transitions from physical to logical update when the direction is changed
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..88ba2794b9970d0836f7718a4cf8109f4cff825e
>--- /dev/null
>+++ b/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode-expected.txt
>@@ -0,0 +1,8 @@
>+
>+PASS Writing mode with variable
>+PASS Writing mode with nested variables
>+PASS Writing mode with 'inherit'
>+PASS Writing mode with 'initial'
>+PASS Writing mode with 'revert'
>+PASS Writing mode with 'revert-layer'
>+
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..a213d375bdf95d669b14f19f50d414cb6ca7439a
>--- /dev/null
>+++ b/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode.html
>@@ -0,0 +1,175 @@
>+<!DOCTYPE html>
>+<meta charset="utf-8">
>+<title>Logical properties with deferred <code>writing-mode</code></title>
>+<link rel="author" title="Oriol Brufau" href="mailto:obrufau@igalia.com" />
>+<link rel="help" href="https://drafts.csswg.org/css-logical-1/#box">
>+<link rel="help" href="https://drafts.csswg.org/css-variables-1/">
>+<link rel="help" href="https://drafts.csswg.org/css-values-4/#common-keywords">
>+<meta name="assert" content="Checks that logical properties are resolved correctly when the writing mode isn't known until computed-value time.">
>+<style>
>+#parent {
>+  writing-mode: vertical-lr;
>+}
>+
>+@layer {
>+  .revert-layer {
>+    writing-mode: vertical-rl;
>+  }
>+}
>+@layer {
>+  .revert-layer {
>+    writing-mode: horizontal-tb;
>+    writing-mode: revert-layer;
>+  }
>+}
>+</style>
>+<div id="parent">
>+  <div id="target"></div>
>+</div>
>+<script src="/resources/testharness.js"></script>
>+<script src="/resources/testharnessreport.js"></script>
>+<script>
>+const target = document.getElementById("target");
>+const computedStyle = getComputedStyle(target);
>+
>+function check(expected) {
>+  for (let [prop, value] of Object.entries(expected)) {
>+    assert_equals(computedStyle.getPropertyValue(prop), value, prop);
>+  }
>+}
>+
>+test(function() {
>+  target.style.cssText = `
>+    --wm: vertical-rl;
>+    writing-mode: var(--wm);
>+    margin-block-start: 1px;
>+    margin-block-end: 2px;
>+    margin-inline-start: 3px;
>+    margin-inline-end: 4px;
>+  `;
>+  check({
>+    // Logicals
>+    "margin-block-start": "1px",
>+    "margin-block-end": "2px",
>+    "margin-inline-start": "3px",
>+    "margin-inline-end": "4px",
>+    // Physicals
>+    "margin-right": "1px",
>+    "margin-left": "2px",
>+    "margin-top": "3px",
>+    "margin-bottom": "4px",
>+  });
>+}, "Writing mode with variable");
>+
>+test(function() {
>+  target.style.cssText = `
>+    --wm1: vertical-rl;
>+    --wm2: var(--wm1);
>+    writing-mode: var(--wm2);
>+    margin-block-start: 1px;
>+    margin-block-end: 2px;
>+    margin-inline-start: 3px;
>+    margin-inline-end: 4px;
>+  `;
>+  check({
>+    // Logicals
>+    "margin-block-start": "1px",
>+    "margin-block-end": "2px",
>+    "margin-inline-start": "3px",
>+    "margin-inline-end": "4px",
>+    // Physicals
>+    "margin-right": "1px",
>+    "margin-left": "2px",
>+    "margin-top": "3px",
>+    "margin-bottom": "4px",
>+  });
>+}, "Writing mode with nested variables");
>+
>+test(function() {
>+  target.style.cssText = `
>+    writing-mode: inherit;
>+    margin-block-start: 1px;
>+    margin-block-end: 2px;
>+    margin-inline-start: 3px;
>+    margin-inline-end: 4px;
>+  `;
>+  check({
>+    // Logicals
>+    "margin-block-start": "1px",
>+    "margin-block-end": "2px",
>+    "margin-inline-start": "3px",
>+    "margin-inline-end": "4px",
>+    // Physicals
>+    "margin-left": "1px",
>+    "margin-right": "2px",
>+    "margin-top": "3px",
>+    "margin-bottom": "4px",
>+  });
>+}, "Writing mode with 'inherit'");
>+
>+test(function() {
>+  target.style.cssText = `
>+    writing-mode: initial;
>+    margin-block-start: 1px;
>+    margin-block-end: 2px;
>+    margin-inline-start: 3px;
>+    margin-inline-end: 4px;
>+  `;
>+  check({
>+    // Logicals
>+    "margin-block-start": "1px",
>+    "margin-block-end": "2px",
>+    "margin-inline-start": "3px",
>+    "margin-inline-end": "4px",
>+    // Physicals
>+    "margin-top": "1px",
>+    "margin-bottom": "2px",
>+    "margin-left": "3px",
>+    "margin-right": "4px",
>+  });
>+}, "Writing mode with 'initial'");
>+
>+test(function() {
>+  target.style.cssText = `
>+    writing-mode: revert;
>+    margin-block-start: 1px;
>+    margin-block-end: 2px;
>+    margin-inline-start: 3px;
>+    margin-inline-end: 4px;
>+  `;
>+  check({
>+    // Logicals
>+    "margin-block-start": "1px",
>+    "margin-block-end": "2px",
>+    "margin-inline-start": "3px",
>+    "margin-inline-end": "4px",
>+    // Physicals
>+    "margin-left": "1px",
>+    "margin-right": "2px",
>+    "margin-top": "3px",
>+    "margin-bottom": "4px",
>+  });
>+}, "Writing mode with 'revert'");
>+
>+test(function() {
>+  target.className = "revert-layer";
>+  target.style.cssText = `
>+    margin-block-start: 1px;
>+    margin-block-end: 2px;
>+    margin-inline-start: 3px;
>+    margin-inline-end: 4px;
>+  `;
>+  check({
>+    // Logicals
>+    "margin-block-start": "1px",
>+    "margin-block-end": "2px",
>+    "margin-inline-start": "3px",
>+    "margin-inline-end": "4px",
>+    // Physicals
>+    "margin-right": "1px",
>+    "margin-left": "2px",
>+    "margin-top": "3px",
>+    "margin-bottom": "4px",
>+  });
>+}, "Writing mode with 'revert-layer'");
>+</script>

Actions: View | Formatted Diff | Diff

Attachments on bug 236199: 451039 | 451117 | 451169 | 454860 | 454861 | 454863 | 455057 | 455060 | 455061 | 455195 | 458293 | 458413 | 458444 | 458454