Summary: | REGRESSION(r266674) [css-logical] CSSOM "set a CSS declaration" is broken when mixing logical and physical properties | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | collimarco91 | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Oriol Brufau <obrufau> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Critical | CC: | bfulgham, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, macpherson, menard, obrufau, sam, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | Safari 14 | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=216178 https://bugs.webkit.org/show_bug.cgi?id=238874 |
||||||||||||||||
Bug Depends on: | 226878 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
collimarco91
2021-05-31 01:38:41 PDT
Interestingly the layout recovers when forced through WebInspector (toggle an unrelated property on the element like background-color) Regression from flow-relative properties: http://trac.webkit.org/changeset/266674/webkit This seems like https://bugs.chromium.org/p/chromium/issues/detail?id=1154570, i.e. caused by the fact that CSSOM is broken for logical 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! Well, for me there are 2 reasons that it doesn't show up. The first one is that _.clientPushAPI is somehow null, and then it aborts. Not sure where this clientPushAPI comes from, but seems unrelated to logical properties. Maybe it's just a thing of my WebKitGTK and it's not null in Safari. And the 2nd reason is indeed caused by logical properties. Just run (function() { var prompt = document.createElement('div'); prompt.style.all = "initial"; prompt.style.position = "fixed"; prompt.style.bottom = 0; prompt.style.left = 0; prompt.textContent = "foobar"; document.body.appendChild(prompt); })(); First, 'all' sets all longhands in lexicographic order, so 'bottom' comes before 'inset-block-end'. Then it sets 'bottom: 0', but this just updates the value in-place. So the property is still overridden by 'inset-block-end: initial'. Solution: when setting a property value, if afterwards there is another property that belongs to the same logical property group but has a different mapping logic, remove the original property and set the new value at the end. https://drafts.csswg.org/cssom/#set-a-css-declaration I don't think WebKit has the concept of logical property groups and mapping logic. That will need to be implemented first. Created attachment 430308 [details]
Patch
Comment on attachment 430308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430308&action=review > Source/WebCore/css/CSSProperty.cpp:207 > +void CSSProperty::logicalPropertyInfo(CSSPropertyID propertyID, LogicalPropertyGroup& group, MappingLogic& logic) This should return a std::pair<LogicalPropertyGroup, MappingLogic> (or perhaps an std::optional< std::pair<LogicalPropertyGroup, MappingLogic>> if you wanted to get rid of the None values in the enumeration) rather than using out parameters. Perhaps we should generate this logic from CSSProperties.json (with some additional annotations)? Created attachment 430347 [details]
Patch
(In reply to Sam Weinig from comment #9) > This should return a std::pair<LogicalPropertyGroup, MappingLogic> (or > perhaps an std::optional< std::pair<LogicalPropertyGroup, MappingLogic>> if > you wanted to get rid of the None values in the enumeration) rather than > using out parameters. Done. > Perhaps we should generate this logic from CSSProperties.json (with some > additional annotations)? Sure, but for fixing this bug it seemed simpler to just do it like this. Generating this logic automatically from CSSProperties.json annotations seems more suited for a follow-up. By the way, iterating the vector in setProperty() caused a regressed a perf test in Chromium (https://crbug.com/1156321). So I avoided it by adding a flag that tells whether the MutableStyleProperties may have some logical property, and if it doesn't it skips the loop. But that caused a memory regression (https://crbug.com/1169971), and the perf didn't improve if there are logical properties, so not sure if it was worth it. Tell me it you want something like that. Created attachment 430349 [details]
Patch
Created attachment 430352 [details]
Patch
Comment on attachment 430352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430352&action=review > Source/WebCore/css/CSSProperty.cpp:222 > + case CSSPropertyBorderBottom: > + case CSSPropertyBorderLeft: > + case CSSPropertyBorderRight: > + case CSSPropertyBorderTop: > + return std::make_pair(LogicalPropertyGroup::Border, MappingLogic::Physical); > + case CSSPropertyBorderBlockEndColor: > + case CSSPropertyBorderBlockStartColor: > + case CSSPropertyBorderInlineEndColor: Can we autogenerate all this goop? > Source/WebCore/css/CSSProperty.h:64 > +enum class LogicalPropertyGroup : uint8_t { There's ambiguity here. Is this enum defining a group, or a set of properties that form groups? It looks to me like these are shorthands that create logical property groups? If so, it needs a better name. (In reply to Simon Fraser (smfr) from comment #14) > Comment on attachment 430352 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430352&action=review > > > Source/WebCore/css/CSSProperty.cpp:222 > > + case CSSPropertyBorderBottom: > > + case CSSPropertyBorderLeft: > > + case CSSPropertyBorderRight: > > + case CSSPropertyBorderTop: > > + return std::make_pair(LogicalPropertyGroup::Border, MappingLogic::Physical); > > + case CSSPropertyBorderBlockEndColor: > > + case CSSPropertyBorderBlockStartColor: > > + case CSSPropertyBorderInlineEndColor: > > Can we autogenerate all this goop? Yes, but it seemed to me that it would be better to keep it manual for now and just apply the fix. But if there is no hurry for the fix I can try to autogenerate, yes. > > > Source/WebCore/css/CSSProperty.h:64 > > +enum class LogicalPropertyGroup : uint8_t { > > There's ambiguity here. Is this enum defining a group, or a set of > properties that form groups? > It looks to me like these are shorthands that create logical property > groups? If so, it needs a better name. It's the former. It's true that typically there is a shorthand with that name, but not necessarily. For example, there isn't (yet) a 'size' shorthand of 'width' and 'height'. Also, both the related logical and physical properties belong to a logical property group, but shorthands only have physical longhands, e.g. the 'margin' shorthand only has 'margin-{top,right,bottom,left}', but 'margin-{inline,block}-{start,end}' also belong to the margin group. Comment on attachment 430352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430352&action=review > Source/WebCore/css/CSSProperty.cpp:207 > +std::optional<std::pair<LogicalPropertyGroup, MappingLogic>> CSSProperty::logicalPropertyInfo(CSSPropertyID propertyID) The code would read better if this returned an optional struct instead of a pair. > Source/WebCore/css/CSSProperty.cpp:214 > + return std::make_pair(LogicalPropertyGroup::Border, MappingLogic::Logical); These can be written as return std::pair { LogicalPropertyGroup::Border, MappingLogic::Logical }; or probably just return { { LogicalPropertyGroup::Border, MappingLogic::Logical } }; > Source/WebCore/css/CSSProperty.cpp:335 > + if (auto pair = logicalPropertyInfo(propertyID)) 'pair' is not a great variable name. 'propertyInfo'? > Source/WebCore/css/CSSProperty.cpp:336 > + return pair.value().second == MappingLogic::Logical; pair->second > Source/WebCore/css/StyleProperties.cpp:937 > + // If the property is in a logical property group, we can't just update the value in-place, > + // because afterwards there might be another property of the same group but different mapping logic. > + // In that case the latter might override the former, so setProperty would have no effect. > + // Instead, remove the original property, and append it to the end with the new value. This things is quite a tower. How about factoring it into a function/lambda? > Source/WebCore/css/StyleProperties.cpp:938 > + if (auto pair = CSSProperty::logicalPropertyInfo(property.id())) { 'pair' is not a great variable name. > Source/WebCore/css/StyleProperties.cpp:941 > + LogicalPropertyGroup group; > + MappingLogic logic; > + std::tie(group, logic) = pair.value(); auto [group, logic] = *pair; > Source/WebCore/css/StyleProperties.cpp:948 > + LogicalPropertyGroup currentGroup; > + MappingLogic currentLogic; > + std::tie(currentGroup, currentLogic) = currentPair.value(); auto [currentGroup, currentLogic] = *currentPair; Created attachment 430959 [details]
Patch
Seems this will need to be autogenerated after all, since bug 226768 landed first. Created attachment 431658 [details]
Patch
Comment on attachment 431658 [details] Patch The api-mac EWS keeps failing TestWebKitAPI.WebKit.AudioBufferSize but that seems unrelated to my patch. In https://ews-build.webkit.org/#/builders/3 there are several other patches with the same failure. cq+ Committed r279044 (238964@main): <https://commits.webkit.org/238964@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431658 [details]. I use v14.1.2 and the bug is still present What Safari version will include the fix? *** Bug 218090 has been marked as a duplicate of this bug. *** |