Bug 236199 – [css-logical] changing the writing-mode via a CSS variable does not update the physical properties

• Component: CSS
• Status: RESOLVED FIXED

[Antoine Quint]

Created attachment 451039 [details]
Test

The attached test checks that changing the writing-mode via a CSS variable correctly updates the physical properties when using a logical property to set their value. Alas, we fail to do that correctly. This costs us a unique failure in the WPT test css/css-logical/animation-004.html since we fail to update CSS transitions as values in the before and after computed styles are the same (once the fix for bug 236197 is applied).

Not using a CSS variable makes everything work as expected.

[Antoine Quint]

When PropertyCascade::set() is called we're still resolving CSSPropertyBlockSize to CSSPropertyHeight, not accounting for the change made to the CSS variable changing the value of writing-mode.

[Oriol Brufau]

So, this is not related to transitions at all.

    Test:
    <div style="--wm: vertical-lr; writing-mode: var(--wm); block-size: 200px; inline-size: 100px; background: cyan"></div>
    <br>
    Reference:
    <div style="height: 100px; width: 200px; background: cyan"></div>

The test is 200px tall and 100px wide, it should be the opposite.

The current behavior isn't only problematic with variables, e.g.

    <div style="writing-mode: vertical-lr">
      <div style="writing-mode: initial; block-size: 100px; inline-size: 200px; background: cyan"></div>
    </div>

I think I already have a patch.

[Antoine Quint]

(In reply to Oriol Brufau from comment #2)
> So, this is not related to transitions at all.

That's right, I only found out about it while looking at a transition issue. If you find a fix, please make sure it also fixes the remaining failure in css/css-logical/animation-004.html.

[Oriol Brufau]

Created attachment 451117 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Oriol Brufau]

Created attachment 451169 [details]
Patch

[Oriol Brufau]

This patch avoids failing logical-border-props-with-variables.html and some asserts.

But tests related to `revert` still fail, that's because of bug 236272.
I don't know enough of how the cascade is implemented in WebKit to fix that one.

[Radar WebKit Bug Importer]

<rdar://problem/88871725>

[Oriol Brufau]

Created attachment 454860 [details]
Patch

[Oriol Brufau]

Created attachment 454861 [details]
Patch

[Oriol Brufau]

Created attachment 454863 [details]
Patch for EWS

[Oriol Brufau]

Created attachment 455057 [details]
Patch for EWS

[Oriol Brufau]

Created attachment 455060 [details]
Patch

[Oriol Brufau]

Created attachment 455061 [details]
Patch for EWS

[Antoine Quint]

There are two ChangeLog entries in your latest patch.

[Oriol Brufau]

(In reply to Antoine Quint from comment #15)
> There are two ChangeLog entries in your latest patch.

The attachment "Patch" is the actual patch, but it fails fast/css/appearance-apple-pay-button-border-radius.html because of bug 238062.

"Patch for EWS" contains the patches for both bugs to ensure that then it works.

[Darin Adler]

Comment on attachment 455060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455060&action=review

> Source/WebCore/style/StyleBuilder.cpp:271
> +        const RenderStyle& style = m_state.style();

auto&

> Source/WebCore/style/StyleBuilder.cpp:320
> +                const RenderStyle& style = m_state.style();

auto&

> Source/WebCore/style/StyleBuilder.cpp:321
> +                CSSPropertyID logicalId = CSSProperty::unresolvePhysicalProperty(id, style.direction(), style.writingMode());

auto

[Oriol Brufau]

Created attachment 455195 [details]
Patch

[EWS]

Committed r291546 (248652@main): <https://commits.webkit.org/248652@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455195 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 238218

[Darin Adler]

We’ll have to figure out why this was a significant regression in the Speedometer2/jQuery-TodoMVC test, and find a way to fix this without slowing that down so much.

[Oriol Brufau]

My guess is that the perf regression may be because if you have something like

   <div style="height: 1px; height: 2px; height: 3px; height: 4px; height: 5px"></div>

Previously only 'height: 5px' was applied, but now all declarations are applied one by one.

This was already happening for deferred properties, though. It's just that they weren't used much frequently. But my patch turned some frequently used properties into deferred.

Possible solution: in PropertyCascade::setDeferred, if m_deferredPropertiesIndices already had the CSSPropertyID, I can get the old index, invalidate the Property entry, and then skip it when applying.

Just I guess, though. Let's see if I can reproduce.

[Oriol Brufau]

My guess was on the right track (but the declarations need to be in different rules):

<!DOCTYPE html>
<style>
</style>
<div></div>
<script>
const sheet = document.styleSheets[0];
for (let i=0; i < 3e6; ++i) {
  sheet.insertRule(`.styled { width: ${i}px }`, i);
}
const target = document.querySelector("div");
getComputedStyle(target).height;
const t = performance.now();
target.className = "styled";
getComputedStyle(target).height;
document.body.prepend(performance.now() - t);
</script>

With my patch, this worsened from ~1600 to ~2000.

Invalidating the property if it's already there, and then not applying it, seems to improve things slightly, but not that much. I guess the time is spent populating the huge vector.

Something like this does the trick here:

@@ -155,7 +155,27 @@ void PropertyCascade::setDeferred(CSSPropertyID id, CSSValue& cssValue, const Ma
     Property property;
     memset(property.cssValue, 0, sizeof(property.cssValue));
     setPropertyInternal(property, id, cssValue, matchedProperties, cascadeLevel);
-    m_deferredPropertiesIndices.set(id, m_deferredProperties.size());
+    unsigned size = m_deferredProperties.size();
+    auto iterator = m_deferredPropertiesIndices.find(id);
+    if (iterator != m_deferredPropertiesIndices.end()) {
+        unsigned index = iterator.get()->value;
+        ASSERT(index < size);
+        if (index == size - 1) {
+            m_deferredProperties[index] = property;
+            return;
+        }
+        m_deferredProperties[index].id = CSSPropertyInvalid;
+    }
+    m_deferredPropertiesIndices.set(id, size);
     m_deferredProperties.append(property);
 }

but only addresses a corner case. We could then get a bad case like this:

div {width: 0px}
div {inline-size: 1px}
div {width: 2px}
div {inline-size: 3px}
div {width: 4px}
/* ... */

So I think I should actually entirely change the data structure into something like:

HashMap<CSSPropertyID, Property> m_deferredProperties;
HashMap<CSSPropertyID, unsigned> m_deferredPropertiesIndices;
unsigned m_indexForDeferred;

m_indexForDeferred is initially set to 0. Then, PropertyCascade::setDeferred should do something like

    m_deferredProperties.set(id, property);
    m_deferredPropertiesIndices.set(id, m_indexForDeferred++);

Finally, to apply the properties,

    auto deferredPropertiesVector = copyToVector(m_deferredProperties.values());
    std::sort(deferredPropertiesVector.begin(), deferredPropertiesVector.end(), comp);

with a comparator that uses m_deferredPropertiesIndices.

May be improved further, e.g. storing in m_properties instead of m_deferredProperties.

[Antti Koivisto]

PropertyCascade is a performance sensitive class. All code changes here need to be carefully evaluated for performance impact. The deferred properties path is clearly not very well optimized. It does heap allocations with a vector and a (recently added) HashMap. Use of this path can't be expanded before it has been made to perform similarly to the main path.

It can/should probably use numCSSProperties arrays/bitsets similar to the main path.

[Antti Koivisto]

As far as I see that hashmap is not even used except in the very niche rollback case.

[Oriol Brufau]

Created attachment 458293 [details]
Patch for EWS

[Oriol Brufau]

Created attachment 458413 [details]
Patch

[Oriol Brufau]

Comment on attachment 458413 [details]
Patch

Darin, I guess you should take a look again, since after the refactorings in other bugs, the patch has had some non-trivial changes.

Just the part in Source, LayoutTests is the same as last time.

[Antti Koivisto]

Comment on attachment 458413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458413&action=review

Nice!

> Source/WebCore/style/PropertyCascade.cpp:157
> +    CSSPropertyID relatedID;
> +    if (!CSSProperty::isInLogicalPropertyGroup(propertyID))
> +        relatedID = getRelatedPropertyId(propertyID);
> +    else if (CSSProperty::isDirectionAwareProperty(propertyID))
> +        relatedID = CSSProperty::resolveDirectionAwareProperty(propertyID, direction, writingMode);
> +    else
> +        relatedID = CSSProperty::unresolvePhysicalProperty(propertyID, direction, writingMode);

Would read better as a lambda.

> Source/WebCore/style/StyleBuilder.cpp:272
> +        auto& style = m_state.style();
> +        CSSPropertyID newId = CSSProperty::resolveDirectionAwareProperty(id, style.direction(), style.writingMode());

Maybe either use m_state.style() or cache style to a local on top level of the function (and use it everywhere).

> Source/WebCore/style/StyleBuilder.cpp:322
> -            } else if (auto* property = rollbackCascade->lastDeferredPropertyResolvingRelated(id)) {
> -                applyRollbackCascadeProperty(*property, linkMatchMask);
> -                return;
> +            } else {
> +                auto& style = m_state.style();
> +                if (auto* property = rollbackCascade->lastDeferredPropertyResolvingRelated(id, style.direction(), style.writingMode())) {

Maybe either use m_state.style() or cache style to a local on top level of the function (and use it everywhere).

[Oriol Brufau]

Created attachment 458444 [details]
Patch

[Antti Koivisto]

Comment on attachment 458444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458444&action=review

> Source/WebCore/style/PropertyCascade.cpp:151
> +    CSSPropertyID relatedID = [&]() {

could be auto
() is not needed

> Source/WebCore/style/StyleBuilder.cpp:269
> +    auto& style = m_state.style();

There are some other uses of m_state.style() that can also be replaced.

[Oriol Brufau]

Created attachment 458454 [details]
Patch

[EWS]

Committed r293543 (250063@main): <https://commits.webkit.org/250063@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458454 [details].