Bug 188386 – [css-logical] Flow relative margin, padding, border and sizing properties

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Oriol Brufau]

Spec: https://drafts.csswg.org/css-logical-1/#box

Implement
 * margin-{block,inline}-{start,end}
 * padding-{block,inline}-{start,end}
 * border-{block,inline}-{start,end}
 * border-{block,inline}-{start,end}-{width,style,color}
 * {block,inline}-size
 * {min,max}-{block,inline}-size
using the standard names (currently they're already supported using -webkit prefix
and slightly different names).

Only longhand properties and border shorthands for specific sides would be implemented
as part of this, the other shorthands are not supported yet prefixed so they're left
for a different issue.

The full list of properties would be: margin-block-start, margin-block-end,
margin-inline-start, margin-inline-end, padding-block-start, padding-block-end,
padding-inline-start, padding-inline-end, border-block-start-width, border-block-end-width,
border-inline-start-width, border-inline-end-width, border-block-start-style,
border-block-end-style, border-inline-start-style, border-inline-end-style,
border-block-start-color, border-block-end-color, border-inline-start-color,
border-inline-end-color, border-block-start, border-block-end, border-inline-start,
border-inline-end, block-size, inline-size, min-block-size. min-inline-size,
max-block-size, max-inline-size.

All of them have already been shipped in Firefox and Blink.

[Oriol Brufau]

Created attachment 346764 [details]
Patch

[EWS Watchlist]

Comment on attachment 346764 [details]
Patch

Attachment 346764 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8797759

New failing tests:
fast/css/style-enumerate-properties.html

[EWS Watchlist]

Created attachment 346765 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346764 [details]
Patch

Attachment 346764 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8797779

New failing tests:
fast/css/style-enumerate-properties.html

[EWS Watchlist]

Created attachment 346766 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346764 [details]
Patch

Attachment 346764 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8797822

New failing tests:
fast/css/style-enumerate-properties.html

[EWS Watchlist]

Created attachment 346767 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346764 [details]
Patch

Attachment 346764 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8797841

New failing tests:
fast/css/style-enumerate-properties.html

[EWS Watchlist]

Created attachment 346768 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Manuel Rego Casasnovas]

Comment on attachment 346764 [details]
Patch

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

It looks good but I think it still needs some minor changes.
Apart from that you should fix the test afilures reported by the EWSs.

> Source/WebCore/ChangeLog:35
> +        (WebCore::isLayoutDependent):

Please document this change here, as this is something special.

> Source/WebCore/css/CSSProperties.json:3069
> -                "category": "css-22",
> -                "url": "https://www.w3.org/TR/CSS22/box.html#propdef-padding"
> +                "category": "css-22",
> +                "url": "https://www.w3.org/TR/CSS22/box.html#propdef-padding"

What's the change in these lines?

> LayoutTests/imported/w3c/ChangeLog:14
> +        These properties provide the author with the ability to control margins
> +        through logical, rather than physical, direction and dimension mappings.
> +
> +        Only longhand properties and border shorthands for specific sides are
> +        implemented as part of this patch.
> +
> +        The existing prefixed properties become aliases of the new ones.

Probably this specific ChangeLog (the one in "LayoutTests/imported/w3c/") should mention that you're importing the WPT test suite for CSS Logical Properties and Values spec.

> LayoutTests/imported/w3c/web-platform-tests/css/css-logical/animation-001-expected.txt:2
> +FAIL Logical properties can be animated using object notation assert_equals: expected "50px" but got "0px"

It'd be nice to report a bug about these failures.

> LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color-expected.txt:15
> +FAIL Test that logical border-*-color properties share computed values with their physical associates, with 'writing-mode: sideways-rl; direction: rtl; '. assert_equals: logical properties on one declaration, writing mode properties on another, 'writing-mode: sideways-rl; direction: rtl; ', border-bottom-color expected "rgb(1, 1, 1)" but got "rgb(4, 4, 4)"

Add a comment on ChangeLog about all sideways tests failing as they're not supported.

> LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logical-box-margin-expected.txt:4
> +FAIL Test that margin-* shorthands set the computed value of both logical and physical longhands, with 'writing-mode: horizontal-tb; direction: ltr; '. assert_equals: shorthand properties on one declaration, writing mode properties on another, 'writing-mode: horizontal-tb; direction: ltr; ', margin-inline-start expected "1px" but got "0px"

Why this stuff is failing?

[Oriol Brufau]

(In reply to Manuel Rego Casasnovas from comment #10)
> > Source/WebCore/css/CSSProperties.json:3069
> > -                "category": "css-22",
> > -                "url": "https://www.w3.org/TR/CSS22/box.html#propdef-padding"
> > +                "category": "css-22",
> > +                "url": "https://www.w3.org/TR/CSS22/box.html#propdef-padding"
> 
> What's the change in these lines?

No change. I don't understand why it's marked as such.

> > LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logical-box-margin-expected.txt:4
> > +FAIL Test that margin-* shorthands set the computed value of both logical and physical longhands, with 'writing-mode: horizontal-tb; direction: ltr; '. assert_equals: shorthand properties on one declaration, writing mode properties on another, 'writing-mode: horizontal-tb; direction: ltr; ', margin-inline-start expected "1px" but got "0px"
> 
> Why this stuff is failing?

Because margin-block and margin-inline have not been implemented yet. I checked that the test results are the same as in Chromium without experimental web features.

[Oriol Brufau]

Created attachment 346773 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 346773 [details]
Patch

Thanks, this LGTM but please wait until Monday for landing (as you said on the mail to webkit-dev).

[WebKit Commit Bot]

Comment on attachment 346773 [details]
Patch

Clearing flags on attachment: 346773

Committed r234798: <https://trac.webkit.org/changeset/234798>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43230305>