Bug 188697 – [css-logical] Flow relative margin, padding and border shorthands

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

[Oriol Brufau]

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

Implement
 * margin-{block,inline}
 * padding-{block,inline}
 * border-{block,inline}
 * border-{block,inline}-{width,style,color}
 * inset-{block,inline}-{start,end}
 * inset-{block,inline}
 * inset

They will be implemented behind a flag, like has happened in Blink.
Firefox has shipped the inset longhands.

[Oriol Brufau]

Bug 188711 is causing problems for logical inset properties, so I will add them later in another bug.

This one will add:
 * margin-{block,inline}
 * padding-{block,inline}
 * border-{block,inline}
 * border-{block,inline}-{width,style,color}

[Oriol Brufau]

Created attachment 347814 [details]
Patch

[EWS Watchlist]

Comment on attachment 347814 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 347814 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 347829 [details]
Archive of layout-test-results from ews117 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 347814 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 347830 [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 347814 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 347814 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

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

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

[Oriol Brufau]

Created attachment 347859 [details]
Patch

[EWS Watchlist]

Comment on attachment 347859 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 347877 [details]
Archive of layout-test-results from ews101 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 347859 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 347883 [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 347859 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 347884 [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 347859 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 347859 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

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

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

[Oriol Brufau]

Created attachment 347970 [details]
Patch

[Oriol Brufau]

Created attachment 347973 [details]
Patch

[Michael Catanzaro]

Comment on attachment 347973 [details]
Patch

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

> Source/cmake/WebKitFeatures.cmake:103
> +    WEBKIT_OPTION_DEFINE(ENABLE_CSS_LOGICAL "Toggle CSS Logical Properties and Values support" PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES})

Problem is that ENABLE_EXPERIMENTAL_FEATURES isn't defined until the bottom of this file, so you can't use it here.

I don't think there's any particular reason for that -- we *could* choose to change it -- but until now we have not enabled any experimental build flags across all ports, only for particular ports. E.g. if you want it enabled for GTK, you would do that in OptionsGTK.cmake. If you don't like this, you could try moving the definition of ENABLE_EXPERIMENTAL_FEATURES up to the top of this file and see if that works.

Mostly we  use runtime flags for experimental features, which is why this hasn't been an issue until now.

[Oriol Brufau]

Created attachment 348002 [details]
Patch

[Oriol Brufau]

Created attachment 348006 [details]
Patch

[EWS Watchlist]

Comment on attachment 348006 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 348012 [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 348006 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 348006 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 348016 [details]
Archive of layout-test-results from ews116 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 348006 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 348017 [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

[EWS Watchlist]

Comment on attachment 348006 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 348006 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 348029 [details]
Archive of layout-test-results from ews112 for mac-sierra

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

[Oriol Brufau]

Created attachment 348100 [details]
Patch

[Javier Fernandez]

Comment on attachment 348100 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4232
> +    bool showEnd = !compareCSSValuePtr(startValue, endValue);

Do we really need this variable ?

> Source/WebCore/css/StyleProperties.cpp:423
> +    if (start.isImportant() != end.isImportant())

Shouldn't we move this check before the one we do for 'initial' ? Isn't having the same value of 'importance' a requirement for a valid parsing ?

> Source/WebCore/css/StyleProperties.cpp:700
> +String StyleProperties::borderPropertyValue(

I think this function has an incorrect indentation; it should be declared in a single line.

> Source/WebCore/css/StyleProperties.h:167
> +    String borderPropertyValue(

Please, declare this function in a single line.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5685
> +        return m_range.atEnd();

Shouldn't consumeBorder already do the check fr m_range.atEnd ? In any case, we could move it above, before adding the properties' value and just finishing the logic with return true. 

It may be a good idea to have ASSERTS here for ensuring we got non-null pointers from consumeBorder if such function returns true. Although not mandatory, since releaseNonNull should avoid referencing null pointers already, it'd would hep us to catch mistakes in the consumeBorder funcition's  parsing logic.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5710
> +        return m_range.atEnd();

Ditto

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5781
> +        return m_range.atEnd();

Ditto

[Oriol Brufau]

Comment on attachment 348100 [details]
Patch

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

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4232
>> +    bool showEnd = !compareCSSValuePtr(startValue, endValue);
> 
> Do we really need this variable ?

I can remove it, but this way is more consistent with getCSSPropertyValuesFor4SidesShorthand below and with Blink
(https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/style_property_serializer.cc?rcl=da886a9725b3b5f44903354f3cf01a1c7ccb1a0c&l=813)

>> Source/WebCore/css/StyleProperties.cpp:423
>> +    if (start.isImportant() != end.isImportant())
> 
> Shouldn't we move this check before the one we do for 'initial' ? Isn't having the same value of 'importance' a requirement for a valid parsing ?

You are right, my code is based on get4Values which has this bug.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5685
>> +        return m_range.atEnd();
> 
> Shouldn't consumeBorder already do the check fr m_range.atEnd ? In any case, we could move it above, before adding the properties' value and just finishing the logic with return true. 
> 
> It may be a good idea to have ASSERTS here for ensuring we got non-null pointers from consumeBorder if such function returns true. Although not mandatory, since releaseNonNull should avoid referencing null pointers already, it'd would hep us to catch mistakes in the consumeBorder funcition's  parsing logic.

Not sure. It seems that currently WebKit doesn't check m_range.atEnd() in consumeBorder. Blink checks it after AddExpandedPropertyForValue.
If I understand correctly, this would mean that border longhands are set before checking that there is no extra value at the end, but I can't reproduce this.
WebKit also checks m_range.atEnd() after addProperty in consume4Values.
So not sure if I'm misunderstanding something or if there is no difference in behavior.

[Oriol Brufau]

Comment on attachment 348100 [details]
Patch

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

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5685
>> +        return m_range.atEnd();
> 
> It may be a good idea to have ASSERTS here for ensuring we got non-null pointers from consumeBorder if such function returns true. Although not mandatory, since releaseNonNull should avoid referencing null pointers already, it'd would hep us to catch mistakes in the consumeBorder funcition's  parsing logic.

consumeBorder already returns false if some pointer is null, so adding ASSERTS after every call doesn't seem useful to me.

[Oriol Brufau]

Created attachment 348342 [details]
Patch

[Javier Fernandez]

Comment on attachment 348100 [details]
Patch

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

>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4232
>>> +    bool showEnd = !compareCSSValuePtr(startValue, endValue);
>> 
>> Do we really need this variable ?
> 
> I can remove it, but this way is more consistent with getCSSPropertyValuesFor4SidesShorthand below and with Blink
> (https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/style_property_serializer.cc?rcl=da886a9725b3b5f44903354f3cf01a1c7ccb1a0c&l=813)

Ok, it's not a big deal, so lets keep it.

>>>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5685
>>>> +        return m_range.atEnd();
>>> 
>>> Shouldn't consumeBorder already do the check fr m_range.atEnd ? In any case, we could move it above, before adding the properties' value and just finishing the logic with return true. 
>>> 
>>> It may be a good idea to have ASSERTS here for ensuring we got non-null pointers from consumeBorder if such function returns true. Although not mandatory, since releaseNonNull should avoid referencing null pointers already, it'd would hep us to catch mistakes in the consumeBorder funcition's  parsing logic.
>> 
>> Not sure. It seems that currently WebKit doesn't check m_range.atEnd() in consumeBorder. Blink checks it after AddExpandedPropertyForValue.
>> If I understand correctly, this would mean that border longhands are set before checking that there is no extra value at the end, but I can't reproduce this.
>> WebKit also checks m_range.atEnd() after addProperty in consume4Values.
>> So not sure if I'm misunderstanding something or if there is no difference in behavior.
> 
> consumeBorder already returns false if some pointer is null, so adding ASSERTS after every call doesn't seem useful to me.

Yes, I agree the asserts are not necessary. 

Additionally, since the parseShorthand will return false in case of !m_range.AtEnd(), the added values will be discarded, so doing the check before doesn't change the behavior. It seems there is not much consistent on this kind of logic; CSSProperyOverflow, CSSPropertyMarker or CSSPropertyBackgroundPosition are some examples of shorthand doing the check before adding the parsed values. 

Anyway, since there is no behavior change I think it's not a problem to keep the patch as it is. My comment was just a suggestion, looking at what other shorthand do. However, keeping consistence with the Blink implementation may be a good idea as well.

[Oriol Brufau]

Created attachment 348354 [details]
Patch

[Oriol Brufau]

Created attachment 349335 [details]
Patch

[EWS Watchlist]

Comment on attachment 349335 [details]
Patch

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

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

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

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

[Manuel Rego Casasnovas]

Comment on attachment 349335 [details]
Patch

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

It'd be nice to be more verbose on the ChangeLog files documenting the changes on the different files to ease the review process.
I don't have knowledge to review the changes related to the compilation flag, specially on the Mac side, I hope someone else can take a look to that.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3884
> +            if (!compareCSSValuePtr<CSSValue>(value, propertyValue(CSSPropertyBorderBlockEnd, DoNotUpdateLayout)))
> +                return nullptr;

Does this ever happens? Wouldn't be enough with an ASSERT() here?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:-4936
> -    addExpandedPropertyForValue(CSSPropertyBorderImage, CSSValuePool::singleton().createImplicitInitialValue(), important);

Why are you removing this line? Is it right?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4942
> +    if (!end)

Nit: I'd use "if (endImplicit)" here.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5683
> +        addExpandedPropertyForValue(CSSPropertyBorderInlineColor, color.releaseNonNull(), important);

What happens wiht CSSPropertyBorderImage?

> Source/WebCore/css/parser/CSSPropertyParser.h:65
> +    bool consumeBorder(RefPtr<CSSValue>&, RefPtr<CSSValue>&, RefPtr<CSSValue>&);

Here the names of the arguments are important.

> Source/cmake/WebKitFeatures.cmake:178
> +macro(_WEBKIT_OPTION_ENFORCE_DEPENDS _name)
> +    foreach (_dependency ${_WEBKIT_AVAILABLE_OPTIONS_${_name}_DEPENDENCIES})
> +        if (NOT ${_dependency})
> +            message(STATUS "Disabling ${_name} since ${_dependency} is disabled.")
> +            set(${_name} OFF)
> +            set(_OPTION_CHANGED TRUE)
> +            break ()
> +        endif ()
> +    endforeach ()
> +endmacro()
> +
> +macro(_WEBKIT_OPTION_ENFORCE_ALL_DEPENDS)
> +    set(_OPTION_CHANGED TRUE)
> +    while (${_OPTION_CHANGED})
> +        set(_OPTION_CHANGED FALSE)
> +        foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +            if (${_name})
> +                _WEBKIT_OPTION_ENFORCE_DEPENDS(${_name})
> +            endif ()
> +        endforeach ()
> +    endwhile ()
> +endmacro()
> +
> +macro(_WEBKIT_OPTION_ENFORCE_CONFLICTS _name)
> +    foreach (_conflict ${_WEBKIT_AVAILABLE_OPTIONS_${_name}_CONFLICTS})
> +        if (${_conflict})
> +            message(FATAL_ERROR "${_name} conflicts with ${_conflict}. You must disable one or the other.")
> +        endif ()
> +    endforeach ()
> +endmacro()
> +
> +macro(_WEBKIT_OPTION_ENFORCE_ALL_CONFLICTS)
> +    foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +        if (${_name})
> +            _WEBKIT_OPTION_ENFORCE_CONFLICTS(${_name})
> +        endif ()
> +    endforeach ()
> +endmacro()
> +
> +macro(WEBKIT_OPTION_END)
> +    set(_SETTING_WEBKIT_OPTIONS FALSE)
> +
> +    list(SORT _WEBKIT_AVAILABLE_OPTIONS)
> +    set(_MAX_FEATURE_LENGTH 0)
> +    foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +        string(LENGTH ${_name} _name_length)
> +        if (_name_length GREATER _MAX_FEATURE_LENGTH)
> +            set(_MAX_FEATURE_LENGTH ${_name_length})
> +        endif ()
> +
> +        option(${_name} "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}" ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
> +        if (NOT ${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
> +            mark_as_advanced(FORCE ${_name})
> +        endif ()
> +    endforeach ()
> +
> +    # Run through every possible depends to make sure we have disabled anything
> +    # that could cause an unnecessary conflict before processing conflicts.
> +    _WEBKIT_OPTION_ENFORCE_ALL_DEPENDS()
> +    _WEBKIT_OPTION_ENFORCE_ALL_CONFLICTS()
> +
> +    foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +        if (${_name})
> +            list(APPEND FEATURE_DEFINES ${_name})
> +            set(FEATURE_DEFINES_WITH_SPACE_SEPARATOR "${FEATURE_DEFINES_WITH_SPACE_SEPARATOR} ${_name}")
> +        endif ()
> +    endforeach ()
> +endmacro()
> +
> +macro(PRINT_WEBKIT_OPTIONS)
> +    message(STATUS "Enabled features:")
> +
> +    set(_should_print_dots ON)
> +    foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +        if (${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
> +            string(LENGTH ${_name} _name_length)
> +            set(_message " ${_name} ")
> +
> +            # Print dots on every other row, for readability.
> +            foreach (IGNORE RANGE ${_name_length} ${_MAX_FEATURE_LENGTH})
> +                if (${_should_print_dots})
> +                    set(_message "${_message}.")
> +                else ()
> +                    set(_message "${_message} ")
> +                endif ()
> +            endforeach ()
> +
> +            set(_should_print_dots (NOT ${_should_print_dots}))
> +
> +            set(_message "${_message} ${${_name}}")
> +            message(STATUS "${_message}")
> +        endif ()
> +    endforeach ()
> +endmacro()
> +
> +set(_WEBKIT_CONFIG_FILE_VARIABLES "")
> +
> +macro(EXPOSE_VARIABLE_TO_BUILD _variable_name)
> +    list(APPEND _WEBKIT_CONFIG_FILE_VARIABLES ${_variable_name})
> +endmacro()
> +
> +macro(SET_AND_EXPOSE_TO_BUILD _variable_name)
> +    # It's important to handle the case where the value isn't passed, because often
> +    # during configuration an empty variable is the result of a failed package search.
> +    if (${ARGC} GREATER 1)
> +        set(_variable_value ${ARGV1})
> +    else ()
> +        set(_variable_value OFF)
> +    endif ()
> +
> +    set(${_variable_name} ${_variable_value})
> +    EXPOSE_VARIABLE_TO_BUILD(${_variable_name})
> +endmacro()
> +
> +option(ENABLE_EXPERIMENTAL_FEATURES "Enable experimental features" OFF)
> +SET_AND_EXPOSE_TO_BUILD(ENABLE_EXPERIMENTAL_FEATURES ${ENABLE_EXPERIMENTAL_FEATURES})
> +

Why are you moving all these lines, is this needed for this change?

> LayoutTests/imported/w3c/ChangeLog:14
> +        The tests still have some failures because sideways writing modes have
> +        not been implemented yet.

Is there any bug about this? Otherwise it'd be nice to report it and link it from here for future reference.

> LayoutTests/platform/mac/TestExpectations:1773
> +# These tests need the ENABLE_CSS_LOGICAL compile flag.
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html  [ Skip ]
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html  [ Skip ]
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html  [ Skip ]
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html  [ Skip ]
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html  [ Skip ]
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html  [ Skip ]

So the tests are skip in Mac? Why? What happens in other platforms?

[Oriol Brufau]

Comment on attachment 349335 [details]
Patch

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

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3884
>> +                return nullptr;
> 
> Does this ever happens? Wouldn't be enough with an ASSERT() here?

Yes, it happens e.g. if you have `style.cssText = "border-block-start: 1px solid blue; border-block-end: 2px dotted green"`.
Then `style.borderBlock` can't be serialized.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:-4936
>> -    addExpandedPropertyForValue(CSSPropertyBorderImage, CSSValuePool::singleton().createImplicitInitialValue(), important);
> 
> Why are you removing this line? Is it right?

I'm moving it into the `case CSSPropertyBorder`.
Yes, it's right because border-image should not be reset by border-block nor border-inline.
See https://github.com/w3c/csswg-drafts/issues/2874
I did the same for Blink.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:4942
>> +    if (!end)
> 
> Nit: I'd use "if (endImplicit)" here.

OK

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5683
>> +        addExpandedPropertyForValue(CSSPropertyBorderInlineColor, color.releaseNonNull(), important);
> 
> What happens wiht CSSPropertyBorderImage?

Shouldn't be reset by border-inline, https://github.com/w3c/csswg-drafts/issues/2874

>> Source/cmake/WebKitFeatures.cmake:178
>> +
> 
> Why are you moving all these lines, is this needed for this change?

Because ENABLE_EXPERIMENTAL_FEATURES was set at the bottom.
In order to set ENABLE_CSS_LOGICAL to ENABLE_EXPERIMENTAL_FEATURES, ENABLE_EXPERIMENTAL_FEATURES needs to appear earlier.
But ENABLE_EXPERIMENTAL_FEATURES depends on various things, which also need to be moved.

>> LayoutTests/imported/w3c/ChangeLog:14
>> +        not been implemented yet.
> 
> Is there any bug about this? Otherwise it'd be nice to report it and link it from here for future reference.

https://bugs.webkit.org/show_bug.cgi?id=166941

>> LayoutTests/platform/mac/TestExpectations:1773
>> +imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html  [ Skip ]
> 
> So the tests are skip in Mac? Why? What happens in other platforms?

Otherwise EWS for Mac fail, probably because ENABLE_EXPERIMENTAL_FEATURES is false.
In other platforms EWS seem OK.

[Oriol Brufau]

Created attachment 349604 [details]
Patch

[Oriol Brufau]

I didn't handle shorthand serialization in cssText. I need more calls to borderPropertyValue, whose signature I plan to simplify in bug 189597, so marking as a dependence.

[Oriol Brufau]

Created attachment 350364 [details]
Patch

[Oriol Brufau]

Created attachment 350367 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 350367 [details]
Patch

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

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:91
> +ENABLE_CSS_LOGICAL = ENABLE_EXPERIMENTAL_FEATURES;

I would prefer this be called ENABLE_CSS_LOGICAL_PROPERTIES. Otherwise, the reader asks "enable CSS logical what?".

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4235
> +    if (showEnd)

You could just do if (!compareCSSValuePtr(startValue, endValue)) here.

> Source/WebCore/css/StyleProperties.cpp:414
> +            // All components are "initial" and start is not implicit.

This comment duplicates the previous line of code.

> Source/WebCore/css/StyleProperties.cpp:424
> +    if (showEnd) {

if (!start.value()->equals(*end.value()))

> Source/WebCore/css/StyleProperties.cpp:1067
> +                if (!borderBlockFallbackShorthandProperty)
> +                    borderBlockFallbackShorthandProperty = CSSPropertyBorderBlockColor;
> +                // FIXME: Deal with cases where only some of border-block-(start|end) are specified.
> +                ASSERT(CSSPropertyBorderBlock - firstCSSProperty < shorthandPropertyAppeared.size());
> +                if (!shorthandPropertyAppeared[CSSPropertyBorderBlock - firstCSSProperty]) {
> +                    value = borderPropertyValue(borderBlockWidthShorthand(), borderBlockStyleShorthand(), borderBlockColorShorthand());
> +                    if (value.isNull())
> +                        shorthandPropertyAppeared.set(CSSPropertyBorderBlock - firstCSSProperty);
> +                    else
> +                        shorthandPropertyID = CSSPropertyBorderBlock;
> +                } else if (shorthandPropertyUsed[CSSPropertyBorderBlock - firstCSSProperty])
> +                    shorthandPropertyID = CSSPropertyBorderBlock;
> +                if (!shorthandPropertyID)
> +                    shorthandPropertyID = borderBlockFallbackShorthandProperty;

This is complicated enough that maybe it can move into a separate function.

> Source/WebCore/css/StyleProperties.cpp:1094
> +                if (!borderInlineFallbackShorthandProperty)
> +                    borderInlineFallbackShorthandProperty = CSSPropertyBorderInlineColor;
> +                // FIXME: Deal with cases where only some of border-inline-(start|end) are specified.
> +                ASSERT(CSSPropertyBorderInline - firstCSSProperty < shorthandPropertyAppeared.size());
> +                if (!shorthandPropertyAppeared[CSSPropertyBorderInline - firstCSSProperty]) {
> +                    value = borderPropertyValue(borderInlineWidthShorthand(), borderInlineStyleShorthand(), borderInlineColorShorthand());
> +                    if (value.isNull())
> +                        shorthandPropertyAppeared.set(CSSPropertyBorderInline - firstCSSProperty);
> +                    else
> +                        shorthandPropertyID = CSSPropertyBorderInline;
> +                } else if (shorthandPropertyUsed[CSSPropertyBorderInline - firstCSSProperty])
> +                    shorthandPropertyID = CSSPropertyBorderInline;
> +                if (!shorthandPropertyID)
> +                    shorthandPropertyID = borderInlineFallbackShorthandProperty;

Ditto.

> Source/WebCore/css/parser/CSSPropertyParser.h:69
> +    bool consume2Values(const StylePropertyShorthand&, bool important);

I would call this consume2ValueShorthand()

> Source/WebCore/css/parser/CSSPropertyParser.h:70
>      bool consume4Values(const StylePropertyShorthand&, bool important);

And rename this accordingly.

[Oriol Brufau]

Comment on attachment 350367 [details]
Patch

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

>> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:91
>> +ENABLE_CSS_LOGICAL = ENABLE_EXPERIMENTAL_FEATURES;
> 
> I would prefer this be called ENABLE_CSS_LOGICAL_PROPERTIES. Otherwise, the reader asks "enable CSS logical what?".

My idea was that this flag could be reused for CSS logical values. Do you prefer a separate flag for that?

[Simon Fraser (smfr)]

(In reply to Oriol Brufau from comment #58)
> Comment on attachment 350367 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350367&action=review
> 
> >> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:91
> >> +ENABLE_CSS_LOGICAL = ENABLE_EXPERIMENTAL_FEATURES;
> > 
> > I would prefer this be called ENABLE_CSS_LOGICAL_PROPERTIES. Otherwise, the reader asks "enable CSS logical what?".
> 
> My idea was that this flag could be reused for CSS logical values. Do you
> prefer a separate flag for that?

I see. I guess it does match the spec short name so maybe it's OK.

[Simon Fraser (smfr)]

(In reply to Oriol Brufau from comment #52)
> Otherwise EWS for Mac fail, probably because ENABLE_EXPERIMENTAL_FEATURES is
> false.
> In other platforms EWS seem OK.

macOS is our primary testing platform, so it's bad to disable the tests there. You probably need to have the tests enable the experimental feature explicitly (see webkit-dev discussion)

[Oriol Brufau]

Created attachment 350758 [details]
Patch

[Oriol Brufau]

(In reply to Simon Fraser (smfr) from comment #57)
> This is complicated enough that maybe it can move into a separate function.
It needs to access various local variables, so instead of creating a separate function with lots of parameters I created a lambda function.

(In reply to Simon Fraser (smfr) from comment #59)
> I see. I guess it does match the spec short name so maybe it's OK.
OK, I have left this as-is and addressed your other suggestions from comment #57.

(In reply to Simon Fraser (smfr) from comment #60)
> macOS is our primary testing platform, so it's bad to disable the tests
> there. You probably need to have the tests enable the experimental feature
> explicitly (see webkit-dev discussion)

Can you link that discussion? In https://lists.webkit.org/pipermail/webkit-dev/2018-August/030094.html you said "Right, sorry. I don't think we have a way to do that."

Then Michael Catanzaro said I could try setting the flags to ENABLE_EXPERIMENTAL_FEATURES which is what I did, but it seems that's false for Mac EWS.

More recently there has been this thread about Internal feature flags: https://lists.webkit.org/pipermail/webkit-dev/2018-September/030123.html
But as far as I understand these are runtime flags.

[Oriol Brufau]

Created attachment 352609 [details]
Patch

[Oriol Brufau]

(In reply to Simon Fraser (smfr) from comment #60)
> macOS is our primary testing platform, so it's bad to disable the tests
> there.
Instead of a compile flag, I have switched to a runtime flag so that the tests don't need to be skipped. I have added code to properly hide CSS properties behind runtime flags (except in cssText, which will need bug 190496), and I have added tests about this.

Do the changes look good?

[Oriol Brufau]

Created attachment 352630 [details]
Patch

[EWS Watchlist]

Comment on attachment 352630 [details]
Patch

Attachment 352630 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9643378

New failing tests:
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

Created attachment 352658 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Oriol Brufau]

Created attachment 352661 [details]
Patch

[Oriol Brufau]

Comment on attachment 352658 [details]
Archive of layout-test-results from ews202 for win-future

I added a test with lists all exposed CSS properties in order to check that disabled ones don't appear there. But it seems different platforms can have different properties so I may need to rethink this.

The clang compile error should be fixed now, let's see what EWS say about other platforms.

[EWS Watchlist]

Comment on attachment 352661 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

Created attachment 352668 [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 352661 [details]
Patch

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

New failing tests:
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 352661 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

Created attachment 352673 [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 352661 [details]
Patch

Attachment 352661 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9645981

New failing tests:
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

Created attachment 352675 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 352661 [details]
Patch

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

New failing tests:
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 352661 [details]
Patch

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

New failing tests:
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

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

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

[Oriol Brufau]

Created attachment 352715 [details]
Patch

[Oriol Brufau]

Created attachment 352720 [details]
Patch

[EWS Watchlist]

Comment on attachment 352720 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 352729 [details]
Archive of layout-test-results from ews101 for mac-sierra

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

[Oriol Brufau]

Created attachment 352749 [details]
Patch

[Oriol Brufau]

Created attachment 352784 [details]
Patch

[EWS Watchlist]

Comment on attachment 352784 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 352785 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 352784 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html
imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html

[EWS Watchlist]

Created attachment 352797 [details]
Archive of layout-test-results from ews115 for mac-sierra

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

[Oriol Brufau]

Created attachment 353532 [details]
Patch

[EWS Watchlist]

Comment on attachment 353532 [details]
Patch

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

New failing tests:
webexposed/css-properties-behind-flags.html

[EWS Watchlist]

Created attachment 353539 [details]
Archive of layout-test-results from ews100 for mac-sierra

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

[Oriol Brufau]

Created attachment 353540 [details]
Patch

[Oriol Brufau]

Comment on attachment 353540 [details]
Patch

Finally EWS is green. Requesting review.
Simon Fraser already approved the code for the logical shorthands, so only the code related with runtime flags needs to be reviewed.

[Manuel Rego Casasnovas]

r=me

This is really nice, we never had support to disable CSS properties from runtime flags so this is great.

Simon already approved the CSS changes in the previous version using compile flags, so that part should be fine already.
The rest of the patch LGTM but mabye someone wants to review the changes in CSSProperties.json and makepprop.pl, as I'm not an expert on those.

Adding some people on CC just in case they want to take a look. Thanks!

[Antti Koivisto]

Comment on attachment 353540 [details]
Patch

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

r=me, looks good

> Source/WebCore/css/CSSStyleDeclaration.cpp:244
> +        CSSPropertyID id = static_cast<CSSPropertyID>(propertyID);

Could be auto

> Source/WebCore/css/StyleProperties.cpp:390
> +        return String();

return { };

(some other places too)

> Source/WebCore/css/StyleProperties.cpp:393
> +    PropertyReference start = propertyAt(startValueIndex);
> +    PropertyReference end = propertyAt(endValueIndex);

auto

> Source/WebCore/css/parser/CSSPropertyParser.cpp:150
> +        CSSPropertyID propertyID = static_cast<CSSPropertyID>(hashTableEntry->id);

auto

[Oriol Brufau]

Created attachment 354934 [details]
Patch

[EWS Watchlist]

Comment on attachment 354934 [details]
Patch

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

New failing tests:
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

Created attachment 354940 [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 354934 [details]
Patch

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

New failing tests:
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 354934 [details]
Patch

Attachment 354934 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10004710

New failing tests:
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

Created attachment 354949 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 354934 [details]
Patch

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

New failing tests:
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

Created attachment 354950 [details]
Archive of layout-test-results from ews117 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 354934 [details]
Patch

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

New failing tests:
webexposed/css-properties-as-js-properties.html
webexposed/css-property-listing.html

[EWS Watchlist]

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

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

[Oriol Brufau]

Created attachment 354961 [details]
Patch

[Oriol Brufau]

I have updated the expectations for the list of enabled CSS properties, now EWS should be green.

[WebKit Commit Bot]

Comment on attachment 354961 [details]
Patch

Clearing flags on attachment: 354961

Committed r238244: <https://trac.webkit.org/changeset/238244>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46105624>

[Truitt Savell]

Looks like the changes in https://trac.webkit.org/changeset/238244/webkit 

has caused all high Sierra test runs to fail early with 50 crashes and caused 25 api failures. This is isolated to only High Sierra which is why EWS did not catch it. 

Build:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/9301

log from a Crash:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/r238245%20(9301)/accessibility/aria-hidden-hides-all-elements-crash-log.txt

The traceback contains CSS related files which point towards this revisions changes.

[Truitt Savell]

Reverted r238244 for reason:

Caused High Sierra test runs to fail early with 50 crashes and casued 25 API failures.

Committed r238262: <https://trac.webkit.org/changeset/238262>

[Oriol Brufau]

Created attachment 359698 [details]
Patch

[Oriol Brufau]

I have removed the webexposed tests that listed all CSS properties since each platform has different expectations and it's a mess.

The crash seemed unrelated, let's try to land again.

[Manuel Rego Casasnovas]

Comment on attachment 359698 [details]
Patch

Ok, let's see re-land this and watch to bots to see if the crash appears again.

[WebKit Commit Bot]

Comment on attachment 359698 [details]
Patch

Clearing flags on attachment: 359698

Committed r240251: <https://trac.webkit.org/changeset/240251>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Committed r240310: <https://trac.webkit.org/changeset/240310>