Bug 226461 - REGRESSION(r266674) [css-logical] CSSOM "set a CSS declaration" is broken when mixing logical and physical properties
Summary: REGRESSION(r266674) [css-logical] CSSOM "set a CSS declaration" is broken whe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari 14
Hardware: Unspecified Unspecified
: P2 Critical
Assignee: Oriol Brufau
URL:
Keywords: InRadar
Depends on: 226878
Blocks:
  Show dependency treegraph
 
Reported: 2021-05-31 01:38 PDT by collimarco91
Modified: 2021-06-18 12:44 PDT (History)
13 users (show)

See Also:


Attachments
Patch (15.54 KB, patch)
2021-06-01 17:29 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (15.10 KB, patch)
2021-06-02 07:23 PDT, Oriol Brufau
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (15.15 KB, patch)
2021-06-02 07:51 PDT, Oriol Brufau
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (15.17 KB, patch)
2021-06-02 08:11 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (15.85 KB, patch)
2021-06-09 06:24 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (8.94 KB, patch)
2021-06-17 06:49 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description collimarco91 2021-05-31 01:38:41 PDT
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.
Comment 1 Radar WebKit Bug Importer 2021-05-31 21:40:41 PDT
<rdar://problem/78699477>
Comment 2 zalan 2021-05-31 21:42:07 PDT
Interestingly the layout recovers when forced through WebInspector (toggle an unrelated property on the element like background-color)
Comment 3 Radar WebKit Bug Importer 2021-05-31 21:42:18 PDT
<rdar://problem/78699501>
Comment 4 Simon Fraser (smfr) 2021-06-01 10:21:32 PDT
<rdar://problem/78699477>
Comment 5 Simon Fraser (smfr) 2021-06-01 10:29:21 PDT
Regression from flow-relative properties: http://trac.webkit.org/changeset/266674/webkit
Comment 6 Oriol Brufau 2021-06-01 10:49:18 PDT
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!
Comment 7 Oriol Brufau 2021-06-01 14:23:08 PDT
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.
Comment 8 Oriol Brufau 2021-06-01 17:29:56 PDT
Created attachment 430308 [details]
Patch
Comment 9 Sam Weinig 2021-06-01 18:24:00 PDT
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)?
Comment 10 Oriol Brufau 2021-06-02 07:23:01 PDT
Created attachment 430347 [details]
Patch
Comment 11 Oriol Brufau 2021-06-02 07:32:15 PDT
(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.
Comment 12 Oriol Brufau 2021-06-02 07:51:39 PDT
Created attachment 430349 [details]
Patch
Comment 13 Oriol Brufau 2021-06-02 08:11:24 PDT
Created attachment 430352 [details]
Patch
Comment 14 Simon Fraser (smfr) 2021-06-08 10:55:22 PDT
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.
Comment 15 Oriol Brufau 2021-06-08 11:15:28 PDT
(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 16 Antti Koivisto 2021-06-09 02:26:42 PDT
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;
Comment 17 Oriol Brufau 2021-06-09 06:24:06 PDT
Created attachment 430959 [details]
Patch
Comment 18 Oriol Brufau 2021-06-09 06:37:48 PDT
Seems this will need to be autogenerated after all, since bug 226768 landed first.
Comment 19 Oriol Brufau 2021-06-17 06:49:24 PDT
Created attachment 431658 [details]
Patch
Comment 20 Oriol Brufau 2021-06-18 12:09:24 PDT
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+
Comment 21 EWS 2021-06-18 12:44:40 PDT
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].