Changeset 291546 in webkit


Ignore:
Timestamp:
Mar 20, 2022 9:37:43 AM (4 months ago)
Author:
Oriol Brufau
Message:

Fix CSS cascade regarding logical properties
​https://bugs.webkit.org/show_bug.cgi?id=236199

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

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.

Source/WebCore:

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/CSSComputedStyleDeclaration.cpp:

(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):

  • css/CSSProperties.json:
  • css/parser/CSSParser.cpp:

(WebCore::CSSParser::parseValueWithVariableReferences):

  • style/PropertyCascade.cpp:

(WebCore::Style::shouldApplyPropertyInParseOrder):
(WebCore::Style::PropertyCascade::PropertyCascade):
(WebCore::Style::PropertyCascade::set):
(WebCore::Style::PropertyCascade::setDeferred):
(WebCore::Style::PropertyCascade::resolveDirectionAndWritingMode const): Deleted.
(WebCore::Style::PropertyCascade::direction const): Deleted.

  • style/PropertyCascade.h:

(WebCore::Style::PropertyCascade::areDeferredInOrder const):

  • style/StyleBuilder.cpp:

(WebCore::Style::Builder::Builder):
(WebCore::Style::Builder::applyProperty):
(WebCore::Style::directionFromStyle): Deleted.

Location:
trunk
Files:
2 added
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r291537 r291546  
     12022-03-20  Oriol Brufau  <obrufau@igalia.com>
     2
     3        Fix CSS cascade regarding logical properties
     4        https://bugs.webkit.org/show_bug.cgi?id=236199
     5
     6        Reviewed by Darin Adler.
     7
     8        Expect animation-004.html to pass.
     9        Add new test logicalprops-with-deferred-writing-mode.html
     10
     11        * web-platform-tests/css/css-logical/animation-004-expected.txt:
     12        * web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode-expected.txt: Added.
     13        * web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode.html: Added.
     14
    1152022-03-19  Oriol Brufau  <obrufau@igalia.com>
    216

  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/animation-004-expected.txt

    r289167 r291546  
    1212PASS The number of interpolating properties can be increased when the writing-mode is changed
    1313PASS The number of interpolating properties can be decreased when the writing-mode is changed
    14 FAIL Transitions update when the writing-mode is changed through a CSS variable assert_equals: expected "50px" but got "0px"
     14PASS Transitions update when the writing-mode is changed through a CSS variable
    1515PASS Transitions update when the direction is changed
    1616PASS Transitions from logical to physical update when the direction is changed

  • trunk/Source/WebCore/ChangeLog

    r291545 r291546  
     12022-03-20  Oriol Brufau  <obrufau@igalia.com>
     2
     3        Fix CSS cascade regarding logical properties
     4        https://bugs.webkit.org/show_bug.cgi?id=236199
     5
     6        Reviewed by Darin Adler.
     7
     8        The CSS cascade was trying to resolve logical properties into physical
     9        ones too early. This failed if we still didn't know the direction or
     10        writing-mode, e.g. because they were set to a variable or to a CSS-wide
     11        keyword.
     12
     13        This patch keeps logical properties as-is during the cascade. They are
     14        only resolved when finally applied. Also, both logical properties and
     15        their physical equivalents are now set to apply in parse order, since
     16        'height: 0px; block-size: 1px' and 'block-size: 1px; height: 0px' can be
     17        different, the order matters.
     18
     19        Tests: imported/w3c/web-platform-tests/css/css-logical/animation-004.html
     20               imported/w3c/web-platform-tests/css/css-logical/logicalprops-with-deferred-writing-mode.html
     21
     22        * css/CSSComputedStyleDeclaration.cpp:
     23        (WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
     24        * css/CSSProperties.json:
     25        * css/parser/CSSParser.cpp:
     26        (WebCore::CSSParser::parseValueWithVariableReferences):
     27        * style/PropertyCascade.cpp:
     28        (WebCore::Style::shouldApplyPropertyInParseOrder):
     29        (WebCore::Style::PropertyCascade::PropertyCascade):
     30        (WebCore::Style::PropertyCascade::set):
     31        (WebCore::Style::PropertyCascade::setDeferred):
     32        (WebCore::Style::PropertyCascade::resolveDirectionAndWritingMode const): Deleted.
     33        (WebCore::Style::PropertyCascade::direction const): Deleted.
     34        * style/PropertyCascade.h:
     35        (WebCore::Style::PropertyCascade::areDeferredInOrder const):
     36        * style/StyleBuilder.cpp:
     37        (WebCore::Style::Builder::Builder):
     38        (WebCore::Style::Builder::applyProperty):
     39        (WebCore::Style::directionFromStyle): Deleted.
     40
    1412022-03-20  Alan Bujtas  <zalan@apple.com>
    242

  • trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp

    r291420 r291546  
    40354035        case CSSPropertyBorderBlockColor:
    40364036            return getCSSPropertyValuesFor2SidesShorthand(borderBlockColorShorthand());
     4037        case CSSPropertyBorderBlockEnd:
     4038            return getCSSPropertyValuesForShorthandProperties(borderBlockEndShorthand());
     4039        case CSSPropertyBorderBlockStart:
     4040            return getCSSPropertyValuesForShorthandProperties(borderBlockStartShorthand());
    40374041        case CSSPropertyBorderBlockStyle:
    40384042            return getCSSPropertyValuesFor2SidesShorthand(borderBlockStyleShorthand());
    … …  
    40554059        case CSSPropertyBorderInlineColor:
    40564060            return getCSSPropertyValuesFor2SidesShorthand(borderInlineColorShorthand());
     4061        case CSSPropertyBorderInlineEnd:
     4062            return getCSSPropertyValuesForShorthandProperties(borderInlineEndShorthand());
     4063        case CSSPropertyBorderInlineStart:
     4064            return getCSSPropertyValuesForShorthandProperties(borderInlineStartShorthand());
    40574065        case CSSPropertyBorderInlineStyle:
    40584066            return getCSSPropertyValuesFor2SidesShorthand(borderInlineStyleShorthand());
    … …  
    42064214
    42074215        /* Directional properties are resolved by resolveDirectionAwareProperty() before the switch. */
    4208         case CSSPropertyBorderBlockEnd:
    42094216        case CSSPropertyBorderBlockEndColor:
    42104217        case CSSPropertyBorderBlockEndStyle:
    42114218        case CSSPropertyBorderBlockEndWidth:
    4212         case CSSPropertyBorderBlockStart:
    42134219        case CSSPropertyBorderBlockStartColor:
    42144220        case CSSPropertyBorderBlockStartStyle:
    42154221        case CSSPropertyBorderBlockStartWidth:
    4216         case CSSPropertyBorderEndEndRadius:       
    4217         case CSSPropertyBorderEndStartRadius:       
    4218         case CSSPropertyBorderInlineEnd:
     4222        case CSSPropertyBorderEndEndRadius:
     4223        case CSSPropertyBorderEndStartRadius:
    42194224        case CSSPropertyBorderInlineEndColor:
    42204225        case CSSPropertyBorderInlineEndStyle:
    42214226        case CSSPropertyBorderInlineEndWidth:
    4222         case CSSPropertyBorderInlineStart:
    42234227        case CSSPropertyBorderInlineStartColor:
    42244228        case CSSPropertyBorderInlineStartStyle:

  • trunk/Source/WebCore/css/CSSProperties.json

    r291536 r291546  
    13431343                    "border-block-end-style",
    13441344                    "border-block-end-color"
    1345                 ],
    1346                 "logical-property-group": {
    1347                     "name": "border",
    1348                     "resolver": "block-end"
    1349                 }
     1345                ]
    13501346            },
    13511347            "specification": {
    … …  
    14241420                    "border-block-start-style",
    14251421                    "border-block-start-color"
    1426                 ],
    1427                 "logical-property-group": {
    1428                     "name": "border",
    1429                     "resolver": "block-start"
    1430                 }
     1422                ]
    14311423            },
    14321424            "specification": {
    … …  
    15261518                    "border-bottom-style",
    15271519                    "border-bottom-color"
    1528                 ],
    1529                 "logical-property-group": {
    1530                     "name": "border",
    1531                     "resolver": "bottom"
    1532                 }
     1520                ]
    15331521            },
    15341522            "specification": {
    … …  
    17711759                    "border-inline-end-style",
    17721760                    "border-inline-end-color"
    1773                 ],
    1774                 "logical-property-group": {
    1775                     "name": "border",
    1776                     "resolver": "inline-end"
    1777                 }
     1761                ]
    17781762            },
    17791763            "specification": {
    … …  
    18521836                    "border-inline-start-style",
    18531837                    "border-inline-start-color"
    1854                 ],
    1855                 "logical-property-group": {
    1856                     "name": "border",
    1857                     "resolver": "inline-start"
    1858                 }
     1838                ]
    18591839            },
    18601840            "specification": {
    … …  
    19541934                    "border-left-style",
    19551935                    "border-left-color"
    1956                 ],
    1957                 "logical-property-group": {
    1958                     "name": "border",
    1959                     "resolver": "left"
    1960                 }
     1936                ]
    19611937            },
    19621938            "specification": {
    … …  
    20392015                    "border-right-style",
    20402016                    "border-right-color"
    2041                 ],
    2042                 "logical-property-group": {
    2043                     "name": "border",
    2044                     "resolver": "right"
    2045                 }
     2017                ]
    20462018            },
    20472019            "specification": {
    … …  
    21632135                    "border-top-style",
    21642136                    "border-top-color"
    2165                 ],
    2166                 "logical-property-group": {
    2167                     "name": "border",
    2168                     "resolver": "top"
    2169                 }
     2137                ]
    21702138            },
    21712139            "specification": {

  • trunk/Source/WebCore/css/parser/CSSParser.cpp

    r288134 r291546  
    214214
    215215        for (auto& property : parsedProperties) {
    216             CSSPropertyID currentId = property.id();
    217             if (CSSProperty::isDirectionAwareProperty(currentId))
    218                 currentId = CSSProperty::resolveDirectionAwareProperty(currentId, direction, writingMode);
    219             if (currentId == propID)
     216            if (property.id() == propID)
    220217                return property.value();
    221218        }

  • trunk/Source/WebCore/style/PropertyCascade.cpp

    r291260 r291546  
    6969        return true;
    7070    default:
    71         return false;
    72     }
    73 }
    74 
    75 PropertyCascade::PropertyCascade(const MatchResult& matchResult, CascadeLevel maximumCascadeLevel, IncludedProperties includedProperties, Direction direction)
     71        return CSSProperty::isInLogicalPropertyGroup(propertyID);
     72    }
     73}
     74
     75PropertyCascade::PropertyCascade(const MatchResult& matchResult, CascadeLevel maximumCascadeLevel, IncludedProperties includedProperties)
    7676    : m_matchResult(matchResult)
    7777    , m_includedProperties(includedProperties)
    7878    , m_maximumCascadeLevel(maximumCascadeLevel)
    79     , m_direction(direction)
    8079{
    8180    buildCascade();
    … …  
    8786    , m_maximumCascadeLevel(maximumCascadeLevel)
    8887    , m_maximumCascadeLayerPriorityForRollback(maximumCascadeLayerPriorityForRollback)
    89     , m_direction(parent.direction())
    90     , m_directionIsUnresolved(false)
    9188{
    9289    buildCascade();
    … …  
    133130void PropertyCascade::set(CSSPropertyID id, CSSValue& cssValue, const MatchedProperties& matchedProperties, CascadeLevel cascadeLevel)
    134131{
    135     if (CSSProperty::isDirectionAwareProperty(id)) {
    136         auto direction = this->direction();
    137         id = CSSProperty::resolveDirectionAwareProperty(id, direction.textDirection, direction.writingMode);
    138     }
    139 
     132    ASSERT(!CSSProperty::isDirectionAwareProperty(id));
    140133    ASSERT(!shouldApplyPropertyInParseOrder(id));
    141134
    … …  
    168161void PropertyCascade::setDeferred(CSSPropertyID id, CSSValue& cssValue, const MatchedProperties& matchedProperties, CascadeLevel cascadeLevel)
    169162{
    170     ASSERT(!CSSProperty::isDirectionAwareProperty(id));
    171163    ASSERT(shouldApplyPropertyInParseOrder(id));
    172164
    … …  
    305297}
    306298
    307 PropertyCascade::Direction PropertyCascade::resolveDirectionAndWritingMode(Direction inheritedDirection) const
    308 {
    309     Direction result = inheritedDirection;
    310 
    311     bool hadImportantWritingMode = false;
    312     bool hadImportantDirection = false;
    313 
    314     for (auto cascadeLevel : { CascadeLevel::UserAgent, CascadeLevel::User, CascadeLevel::Author }) {
    315         for (const auto& matchedProperties : declarationsForCascadeLevel(m_matchResult, cascadeLevel)) {
    316             for (unsigned i = 0, count = matchedProperties.properties->propertyCount(); i < count; ++i) {
    317                 auto property = matchedProperties.properties->propertyAt(i);
    318                 if (!property.value()->isPrimitiveValue() || property.value()->isCSSWideKeyword())
    319                     continue;
    320                 switch (property.id()) {
    321                 case CSSPropertyWritingMode:
    322                     if (!hadImportantWritingMode || property.isImportant()) {
    323                         result.writingMode = downcast<CSSPrimitiveValue>(*property.value());
    324                         hadImportantWritingMode = property.isImportant();
    325                     }
    326                     break;
    327                 case CSSPropertyDirection:
    328                     if (!hadImportantDirection || property.isImportant()) {
    329                         result.textDirection = downcast<CSSPrimitiveValue>(*property.value());
    330                         hadImportantDirection = property.isImportant();
    331                     }
    332                     break;
    333                 default:
    334                     break;
    335                 }
    336             }
    337         }
    338     }
    339 
    340     return result;
    341 }
    342 
    343 PropertyCascade::Direction PropertyCascade::direction() const
    344 {
    345     if (m_directionIsUnresolved) {
    346         m_direction = resolveDirectionAndWritingMode(m_direction);
    347         m_directionIsUnresolved = false;
    348     }
    349     return m_direction;
    350 }
    351 
    352 }
    353 }
     299}
     300}

  • trunk/Source/WebCore/style/PropertyCascade.h

    r291260 r291546  
    4141    enum IncludedProperties { All, InheritedOnly };
    4242
    43     struct Direction {
    44         TextDirection textDirection;
    45         WritingMode writingMode;
    46     };
    47 
    48     PropertyCascade(const MatchResult&, CascadeLevel, IncludedProperties, Direction);
     43    PropertyCascade(const MatchResult&, CascadeLevel, IncludedProperties);
    4944    PropertyCascade(const PropertyCascade&, CascadeLevel, std::optional<CascadeLayerPriority> maximumCascadeLayerPriorityForRollback = { });
    5045
    … …  
    6459
    6560    bool hasDeferredProperty(CSSPropertyID) const;
     61    bool areDeferredInOrder(CSSPropertyID, CSSPropertyID) const;
    6662    const Property& deferredProperty(CSSPropertyID) const;
    6763
    … …  
    7167    const Vector<Property, 8>& deferredProperties() const { return m_deferredProperties; }
    7268    const HashMap<AtomString, Property>& customProperties() const { return m_customProperties; }
    73 
    74     Direction direction() const;
    7569
    7670private:
    … …  
    8478    static void setPropertyInternal(Property&, CSSPropertyID, CSSValue&, const MatchedProperties&, CascadeLevel);
    8579
    86     Direction resolveDirectionAndWritingMode(Direction inheritedDirection) const;
    87 
    8880    const MatchResult& m_matchResult;
    8981    const IncludedProperties m_includedProperties;
    9082    const CascadeLevel m_maximumCascadeLevel;
    9183    const std::optional<CascadeLayerPriority> m_maximumCascadeLayerPriorityForRollback;
    92     mutable Direction m_direction;
    93     mutable bool m_directionIsUnresolved { true };
    9484
    9585    Property m_properties[numCSSProperties + 2];
    … …  
    118108}
    119109
     110inline bool PropertyCascade::areDeferredInOrder(CSSPropertyID id1, CSSPropertyID id2) const
     111{
     112    ASSERT(hasDeferredProperty(id1));
     113    ASSERT(hasDeferredProperty(id2));
     114    return m_deferredPropertiesIndices.get(id1) < m_deferredPropertiesIndices.get(id2);
     115}
     116
    120117inline const PropertyCascade::Property& PropertyCascade::deferredProperty(CSSPropertyID id) const
    121118{

  • trunk/Source/WebCore/style/StyleBuilder.cpp

    r291260 r291546  
    4949static const CSSPropertyID firstLowPriorityProperty = static_cast<CSSPropertyID>(lastHighPriorityProperty + 1);
    5050
    51 inline PropertyCascade::Direction directionFromStyle(const RenderStyle& style)
    52 {
    53     return { style.direction(), style.writingMode() };
    54 }
    55 
    5651inline bool isValidVisitedLinkProperty(CSSPropertyID id)
    5752{
    … …  
    8277
    8378Builder::Builder(RenderStyle& style, BuilderContext&& context, const MatchResult& matchResult, CascadeLevel cascadeLevel, PropertyCascade::IncludedProperties includedProperties)
    84     : m_cascade(matchResult, cascadeLevel, includedProperties, directionFromStyle(style))
     79    : m_cascade(matchResult, cascadeLevel, includedProperties)
    8580    , m_state(*this, style, WTFMove(context))
    8681{
    … …  
    274269
    275270    if (CSSProperty::isDirectionAwareProperty(id)) {
    276         auto direction = m_cascade.direction();
    277         CSSPropertyID newId = CSSProperty::resolveDirectionAwareProperty(id, direction.textDirection, direction.writingMode);
     271        auto& style = m_state.style();
     272        CSSPropertyID newId = CSSProperty::resolveDirectionAwareProperty(id, style.direction(), style.writingMode());
    278273        ASSERT(newId != id);
    279274        return applyProperty(newId, valueToApply.get(), linkMatchMask);
    … …  
    321316                applyRollbackCascadeProperty(property, linkMatchMask);
    322317                return;
     318            } else if (CSSProperty::isInLogicalPropertyGroup(id)) {
     319                ASSERT(!CSSProperty::isDirectionAwareProperty(id));
     320                auto& style = m_state.style();
     321                auto logicalId = CSSProperty::unresolvePhysicalProperty(id, style.direction(), style.writingMode());
     322                bool hasPhysical = rollbackCascade->hasDeferredProperty(id);
     323                bool hasLogical = rollbackCascade->hasDeferredProperty(logicalId);
     324                if (hasPhysical || hasLogical) {
     325                    if (hasLogical && (!hasPhysical || rollbackCascade->areDeferredInOrder(id, logicalId)))
     326                        id = logicalId;
     327                    auto& property = rollbackCascade->deferredProperty(id);
     328                    applyRollbackCascadeProperty(property, linkMatchMask);
     329                    return;
     330                }
    323331            } else if (rollbackCascade->hasDeferredProperty(id)) {
    324332                auto& property = rollbackCascade->deferredProperty(id);
Note: See TracChangeset for help on using the changeset viewer.