Changeset 279044 in webkit


Ignore:
Timestamp:
Jun 18, 2021 12:44:30 PM (13 months ago)
Author:
Oriol Brufau
Message:

[css-logical] Fix cssom "set a CSS declaration" for logical properties
​https://bugs.webkit.org/show_bug.cgi?id=226461

Reviewed by Antti Koivisto.

LayoutTests/imported/w3c:

This test is now passing.

  • web-platform-tests/css/cssom/cssstyledeclaration-setter-logical-expected.txt:

Source/WebCore:

Test: imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-logical.html

Before this patch, setting a value to a property already in the list of
declarations, would just update its value, without reordering.

The problem was that the order is important when there is a mix of
logical and physical properties:

el.style.paddingTop = "1px";
el.style.paddingBlockStart = "2px";
el.style.cssText; "padding-top: 1px; padding-block-start: 2px"
el.style.paddingTop = "3px";
el.style.cssText; "padding-top: 3px; padding-block-start: 2px"
getComputedStyle(el).paddingTop; "2px" -- no effect!

Therefore, this patch implements this part of the spec:

If there are CSS declarations in declarations whose property name is
in the same logical property group as property, but has a different
mapping logic, target declaration must be at an index after all of
those CSS declarations.

This change is based on this Chromium CL:
​https://chromium-review.googlesource.com/c/chromium/src/+/2575081/

  • css/CSSProperty.h:
  • css/StyleProperties.cpp:

(WebCore::MutableStyleProperties::canUpdateInPlace const):
(WebCore::MutableStyleProperties::setProperty):

  • css/StyleProperties.h:
  • css/makeprop.pl:
Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r279026 r279044  
     12021-06-18  Oriol Brufau  <obrufau@igalia.com>
     2
     3        [css-logical] Fix cssom "set a CSS declaration" for logical properties
     4        https://bugs.webkit.org/show_bug.cgi?id=226461
     5
     6        Reviewed by Antti Koivisto.
     7
     8        This test is now passing.
     9
     10        * web-platform-tests/css/cssom/cssstyledeclaration-setter-logical-expected.txt:
     11
    1122021-06-17  Chris Dumez  <cdumez@apple.com>
    213

  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-logical-expected.txt

    r264522 r279044  
    11
    2 FAIL newly set declaration should be after all declarations in the same logical property group but have different logical kind assert_less_than: padding-top should be after padding-block-start after setting padding-top expected a number less than 0 but got 4
     2PASS newly set declaration should be after all declarations in the same logical property group but have different logical kind
    33

  • trunk/Source/WebCore/ChangeLog

    r279043 r279044  
     12021-06-18  Oriol Brufau  <obrufau@igalia.com>
     2
     3        [css-logical] Fix cssom "set a CSS declaration" for logical properties
     4        https://bugs.webkit.org/show_bug.cgi?id=226461
     5
     6        Reviewed by Antti Koivisto.
     7
     8        Test: imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-logical.html
     9
     10        Before this patch, setting a value to a property already in the list of
     11        declarations, would just update its value, without reordering.
     12
     13        The problem was that the order is important when there is a mix of
     14        logical and physical properties:
     15
     16          el.style.paddingTop = "1px";
     17          el.style.paddingBlockStart = "2px";
     18          el.style.cssText; // "padding-top: 1px; padding-block-start: 2px"
     19          el.style.paddingTop = "3px";
     20          el.style.cssText; // "padding-top: 3px; padding-block-start: 2px"
     21          getComputedStyle(el).paddingTop; // "2px" -- no effect!
     22
     23        Therefore, this patch implements this part of the spec:
     24        > If there are CSS declarations in declarations whose property name is
     25        > in the same logical property group as property, but has a different
     26        > mapping logic, target declaration must be at an index after all of
     27        > those CSS declarations.
     28
     29        This change is based on this Chromium CL:
     30        https://chromium-review.googlesource.com/c/chromium/src/+/2575081/
     31
     32        * css/CSSProperty.h:
     33        * css/StyleProperties.cpp:
     34        (WebCore::MutableStyleProperties::canUpdateInPlace const):
     35        (WebCore::MutableStyleProperties::setProperty):
     36        * css/StyleProperties.h:
     37        * css/makeprop.pl:
     38
    1392021-06-18  Peng Liu  <peng.liu6@apple.com>
    240

  • trunk/Source/WebCore/css/CSSProperty.h

    r259585 r279044  
    8181    static Vector<String> aliasesForProperty(CSSPropertyID);
    8282    static bool isDirectionAwareProperty(CSSPropertyID);
     83    static bool isInLogicalPropertyGroup(CSSPropertyID);
     84    static bool areInSameLogicalPropertyGroupWithDifferentMappingLogic(CSSPropertyID, CSSPropertyID);
    8385    static bool isDescriptorOnly(CSSPropertyID);
    8486    static bool isColorProperty(CSSPropertyID);

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

    r278253 r279044  
    919919}
    920920
     921bool MutableStyleProperties::canUpdateInPlace(const CSSProperty& property, CSSProperty* toReplace) const
     922{
     923    // If the property is in a logical property group, we can't just update the value in-place,
     924    // because afterwards there might be another property of the same group but different mapping logic.
     925    // In that case the latter might override the former, so setProperty would have no effect.
     926    CSSPropertyID id = property.id();
     927    if (CSSProperty::isInLogicalPropertyGroup(id)) {
     928        ASSERT(toReplace >= m_propertyVector.begin());
     929        ASSERT(toReplace < m_propertyVector.end());
     930        for (CSSProperty* it = toReplace + 1; it != m_propertyVector.end(); ++it) {
     931            if (CSSProperty::areInSameLogicalPropertyGroupWithDifferentMappingLogic(id, it->id()))
     932                return false;
     933        }
     934    }
     935    return true;
     936}
     937
    921938bool MutableStyleProperties::setProperty(const CSSProperty& property, CSSProperty* slot)
    922939{
    … …  
    932949       
    933950        if (toReplace) {
    934             if (*toReplace == property)
    935                 return false;
    936 
    937             *toReplace = property;
    938             return true;
     951            if (canUpdateInPlace(property, toReplace)) {
     952                if (*toReplace == property)
     953                    return false;
     954
     955                *toReplace = property;
     956                return true;
     957            }
     958            m_propertyVector.remove(toReplace - m_propertyVector.begin());
     959            toReplace = nullptr;
    939960        }
    940961    }

  • trunk/Source/WebCore/css/StyleProperties.h

    r278253 r279044  
    268268    CSSProperty* findCustomCSSPropertyWithName(const String&);
    269269    std::unique_ptr<PropertySetCSSStyleDeclaration> m_cssomWrapper;
     270    bool canUpdateInPlace(const CSSProperty&, CSSProperty* toReplace) const;
    270271
    271272    friend class StyleProperties;

  • trunk/Source/WebCore/css/makeprop.pl

    r278985 r279044  
    659659}
    660660
     661bool CSSProperty::isInLogicalPropertyGroup(CSSPropertyID id)
     662{
     663    switch (id) {
     664EOF
     665
     666for my $logicalPropertyGroup (values %logicalPropertyGroups) {
     667    for my $kind ("logical", "physical") {
     668        for my $name (values %{ $logicalPropertyGroup->{$kind} }) {
     669            print GPERF "    case CSSPropertyID::CSSProperty" . $nameToId{$name} . ":\n";
     670        }
     671    }
     672}
     673
     674print GPERF << "EOF";
     675        return true;
     676    default:
     677        return false;
     678    }
     679}
     680
     681bool CSSProperty::areInSameLogicalPropertyGroupWithDifferentMappingLogic(CSSPropertyID id1, CSSPropertyID id2)
     682{
     683    switch (id1) {
     684EOF
     685
     686for my $logicalPropertyGroup (values %logicalPropertyGroups) {
     687    my $logical = $logicalPropertyGroup->{"logical"};
     688    my $physical = $logicalPropertyGroup->{"physical"};
     689    for my $first ($logical, $physical) {
     690        my $second = $first eq $logical ? $physical : $logical;
     691        while (my ($resolver, $name) = each %{ $first }) {
     692            print GPERF "    case CSSPropertyID::CSSProperty" . $nameToId{$name} . ":\n";
     693        }
     694        print GPERF "        switch (id2) {\n";
     695        while (my ($resolver, $name) = each %{ $second }) {
     696            print GPERF "        case CSSPropertyID::CSSProperty" . $nameToId{$name} . ":\n";
     697        }
     698        print GPERF << "EOF";
     699            return true;
     700        default:
     701            return false;
     702        }
     703EOF
     704    }
     705}
     706
     707print GPERF << "EOF";
     708    default:
     709        return false;
     710    }
     711}
     712
    661713CSSPropertyID CSSProperty::resolveDirectionAwareProperty(CSSPropertyID propertyID, TextDirection direction, WritingMode writingMode)
    662714{
Note: See TracChangeset for help on using the changeset viewer.