Bug 188386 - [css-logical] Flow relative margin, padding, border and sizing properties
Summary: [css-logical] Flow relative margin, padding, border and sizing properties
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oriol Brufau
URL:
Keywords: InRadar
Depends on:
Blocks: 185977
  Show dependency treegraph
 
Reported: 2018-08-07 12:02 PDT by Oriol Brufau
Modified: 2018-08-13 06:19 PDT (History)
8 users (show)

See Also:


Attachments
Patch (244.78 KB, patch)
2018-08-08 04:58 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.28 MB, application/zip)
2018-08-08 06:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.86 MB, application/zip)
2018-08-08 06:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.06 MB, application/zip)
2018-08-08 06:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.39 MB, application/zip)
2018-08-08 06:59 PDT, Build Bot
no flags Details
Patch (245.88 KB, patch)
2018-08-08 09:58 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 Oriol Brufau 2018-08-07 12:02:03 PDT
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.
Comment 1 Oriol Brufau 2018-08-08 04:58:24 PDT
Created attachment 346764 [details]
Patch
Comment 2 Build Bot 2018-08-08 06:12:35 PDT
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
Comment 3 Build Bot 2018-08-08 06:12:37 PDT
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
Comment 4 Build Bot 2018-08-08 06:23:31 PDT
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
Comment 5 Build Bot 2018-08-08 06:23:32 PDT
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
Comment 6 Build Bot 2018-08-08 06:45:09 PDT
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
Comment 7 Build Bot 2018-08-08 06:45:10 PDT
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
Comment 8 Build Bot 2018-08-08 06:59:30 PDT
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
Comment 9 Build Bot 2018-08-08 06:59:31 PDT
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
Comment 10 Manuel Rego Casasnovas 2018-08-08 07:17:05 PDT
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?
Comment 11 Oriol Brufau 2018-08-08 08:08:46 PDT
(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.
Comment 12 Oriol Brufau 2018-08-08 09:58:40 PDT
Created attachment 346773 [details]
Patch
Comment 13 Manuel Rego Casasnovas 2018-08-10 04:56:32 PDT
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).
Comment 14 WebKit Commit Bot 2018-08-13 06:18:28 PDT
Comment on attachment 346773 [details]
Patch

Clearing flags on attachment: 346773

Committed r234798: <https://trac.webkit.org/changeset/234798>
Comment 15 WebKit Commit Bot 2018-08-13 06:18:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-08-13 06:19:25 PDT
<rdar://problem/43230305>