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.
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.
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.
(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.
Created attachment 451117 [details] Patch
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
Created attachment 451169 [details] Patch
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.
<rdar://problem/88871725>
Created attachment 454860 [details] Patch
Created attachment 454861 [details] Patch
Created attachment 454863 [details] Patch for EWS
Created attachment 455057 [details] Patch for EWS
Created attachment 455060 [details] Patch
Created attachment 455061 [details] Patch for EWS
There are two ChangeLog entries in your latest patch.
(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.
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
Created attachment 455195 [details] Patch
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].
Re-opened since this is blocked by bug 238218
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.
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.
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.
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.
As far as I see that hashmap is not even used except in the very niche rollback case.
Created attachment 458293 [details] Patch for EWS
Created attachment 458413 [details] Patch
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.
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).
Created attachment 458444 [details] Patch
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.
Created attachment 458454 [details] Patch
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].