Bug 188386

Summary: [css-logical] Flow relative margin, padding, border and sizing properties
Product: WebKit Reporter: Oriol Brufau <obrufau>
Component: CSSAssignee: Oriol Brufau <obrufau>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbates, ews-watchlist, jfernandez, rego, rniwa, rwlbuis, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.chromium.org/p/chromium/issues/detail?id=850000
Bug Depends on:    
Bug Blocks: 185977    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Archive of layout-test-results from ews113 for mac-sierra
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch none

Oriol Brufau
Reported 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.
Attachments
Patch (244.78 KB, patch)
2018-08-08 04:58 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews103 for mac-sierra (2.28 MB, application/zip)
2018-08-08 06:12 PDT, EWS Watchlist 		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, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews113 for mac-sierra (3.06 MB, application/zip)
2018-08-08 06:45 PDT, EWS Watchlist 		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, EWS Watchlist 		no flags
Details
Patch (245.88 KB, patch)
2018-08-08 09:58 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Oriol Brufau
Comment 1 2018-08-08 04:58:24 PDT
Created attachment 346764 [details] Patch
EWS Watchlist
Comment 2 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
EWS Watchlist
Comment 3 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
EWS Watchlist
Comment 4 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
EWS Watchlist
Comment 5 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
EWS Watchlist
Comment 6 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
EWS Watchlist
Comment 7 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
EWS Watchlist
Comment 8 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
EWS Watchlist
Comment 9 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
Manuel Rego Casasnovas
Comment 10 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?
Oriol Brufau
Comment 11 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.
Oriol Brufau
Comment 12 2018-08-08 09:58:40 PDT
Created attachment 346773 [details] Patch
Manuel Rego Casasnovas
Comment 13 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).
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2018-08-13 06:18:29 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16 2018-08-13 06:19:25 PDT
<rdar://problem/43230305>
Note You need to log in before you can comment on or make changes to this bug.