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.
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}
Created attachment 347814 [details] Patch
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
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
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
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
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
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
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
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
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
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
Created attachment 347859 [details] Patch
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
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
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
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
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
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
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
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
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
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
Created attachment 347970 [details] Patch
Created attachment 347973 [details] Patch
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.
Created attachment 348002 [details] Patch
Created attachment 348006 [details] Patch
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
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
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
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
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
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
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
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
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
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
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
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
Created attachment 348100 [details] Patch
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
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.
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.
Created attachment 348342 [details] Patch
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.
Created attachment 348354 [details] Patch
Created attachment 349335 [details] Patch
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
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
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?
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.
Created attachment 349604 [details] Patch
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.
Created attachment 350364 [details] Patch
Created attachment 350367 [details] Patch
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.
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?
(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.
(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)
Created attachment 350758 [details] Patch
(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.
Created attachment 352609 [details] Patch
(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?
Created attachment 352630 [details] Patch
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
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
Created attachment 352661 [details] Patch
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.
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
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
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
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
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
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
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
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
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
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
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
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
Created attachment 352715 [details] Patch
Created attachment 352720 [details] Patch
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
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
Created attachment 352749 [details] Patch
Created attachment 352784 [details] Patch
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
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
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
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
Created attachment 353532 [details] Patch
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
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
Created attachment 353540 [details] Patch
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.
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!
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
Created attachment 354934 [details] Patch
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
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
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
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
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
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
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
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
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
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
Created attachment 354961 [details] Patch
I have updated the expectations for the list of enabled CSS properties, now EWS should be green.
Comment on attachment 354961 [details] Patch Clearing flags on attachment: 354961 Committed r238244: <https://trac.webkit.org/changeset/238244>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46105624>
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.
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>
Created attachment 359698 [details] Patch
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.
Comment on attachment 359698 [details] Patch Ok, let's see re-land this and watch to bots to see if the crash appears again.
Comment on attachment 359698 [details] Patch Clearing flags on attachment: 359698 Committed r240251: <https://trac.webkit.org/changeset/240251>
Committed r240310: <https://trac.webkit.org/changeset/240310>