REGRESSION(r266674) [css-logical] CSSOM "set a CSS declaration" is broken when mixing logical and physical properties

• Component: Layout and Rendering
• Status: RESOLVED FIXED

The latest version of Safari (testing with 14.1 on MacOS 11.4) does not display an element with position fixed.

Previous versions of Safari have always worked properly and all the other browsers display the element properly.

1. Visit this page: https://pushpad.xyz
2. The element div#pushpad-prompt is not displayed (it is a sort of "toast" message in the bottom left of the page, you can see it by visiting the page with Chrome).

As you can see using the inspector **the element is visible, with display block and positioned at bottom 0, left 0**. So it must be visible in the viewport.

However Safari (only the latest versions) is not displaying it.

[Radar WebKit Bug Importer]

<rdar://problem/78699477>

[zalan]

Interestingly the layout recovers when forced through WebInspector (toggle an unrelated property on the element like background-color)

[Radar WebKit Bug Importer]

<rdar://problem/78699501>

[Simon Fraser (smfr)]

<rdar://problem/78699477>

[Simon Fraser (smfr)]

Regression from flow-relative properties: http://trac.webkit.org/changeset/266674/webkit

[Oriol Brufau]

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!

[Oriol Brufau]

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.

[Oriol Brufau]

Created attachment 430308 [details]
Patch

[Sam Weinig]

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)?

[Oriol Brufau]

Created attachment 430347 [details]
Patch

[Oriol Brufau]

(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.

[Oriol Brufau]

Created attachment 430349 [details]
Patch

[Oriol Brufau]

Created attachment 430352 [details]
Patch

[Simon Fraser (smfr)]

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.

[Oriol Brufau]

(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.

[Antti Koivisto]

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;

[Oriol Brufau]

Created attachment 430959 [details]
Patch

[Oriol Brufau]

Seems this will need to be autogenerated after all, since bug 226768 landed first.

[Oriol Brufau]

Created attachment 431658 [details]
Patch

[Oriol Brufau]

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+

[EWS]

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?

[Oriol Brufau]

*** Bug 218090 has been marked as a duplicate of this bug. ***