Bug 190039

Summary: Registered custom properties should support syntax parameter for <length> and *
Product: WebKit Reporter: Justin Michaud <justin_michaud>
Component: Layout and RenderingAssignee: Justin Michaud <justin_michaud>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, dino, don.olmstead, ews-watchlist, koivisto, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 184542, 189692    
Attachments:
Description Flags
patch
none
Archive of layout-test-results from ews206 for win-future
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-sierra
none
Patch
none
Style refactor (most contentious subset of full patch)
none
Patch
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Archive of layout-test-results from ews206 for win-future
none
Patch
none
Patch
none
Patch
none
Patch none

Justin Michaud
Reported 2018-09-27 08:42:36 PDT
Registered custom properties should support syntax parameter for <length> and *. In addition, these values should be correctly substituted for inherited properties, and cycles caused by relative units should be correctly identified.
Attachments
patch (74.71 KB, patch)
2018-10-09 20:24 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews206 for win-future (12.74 MB, application/zip)
2018-10-10 13:17 PDT, EWS Watchlist 		no flags
Details
Patch (135.86 KB, patch)
2018-10-12 20:27 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (135.13 KB, patch)
2018-10-15 17:10 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (135.39 KB, patch)
2018-10-15 18:30 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (120.26 KB, patch)
2018-10-15 19:10 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (120.71 KB, patch)
2018-10-15 19:39 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews101 for mac-sierra (2.38 MB, application/zip)
2018-10-15 20:52 PDT, EWS Watchlist 		no flags
Details
Patch (120.78 KB, patch)
2018-10-15 20:55 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Style refactor (most contentious subset of full patch) (69.08 KB, patch)
2018-10-15 21:20 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (128.46 KB, patch)
2018-10-16 10:51 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews100 for mac-sierra (2.56 MB, application/zip)
2018-10-16 11:50 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.31 MB, application/zip)
2018-10-16 12:52 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews206 for win-future (12.70 MB, application/zip)
2018-10-16 13:21 PDT, EWS Watchlist 		no flags
Details
Patch (124.61 KB, patch)
2018-10-16 13:33 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (128.67 KB, patch)
2018-10-16 15:01 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (127.77 KB, patch)
2018-10-19 19:38 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (127.52 KB, patch)
2018-10-22 15:29 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.
Justin Michaud
Comment 1 2018-10-09 20:24:01 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 2 2018-10-10 10:40:45 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 3 2018-10-10 13:17:07 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 4 2018-10-10 13:17:18 PDT Comment hidden (obsolete)
Justin Michaud
Comment 5 2018-10-12 20:27:48 PDT
Created attachment 352251 [details] Patch
Justin Michaud
Comment 6 2018-10-12 20:41:41 PDT
Comment on attachment 352251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352251&action=review This is a massive patch, sorry about that Antti. This patch fixes a bunch of bugs caused by the refactoring, as well as a few from the initial implementation. I am sure it is going to require at least a few more iterations, but hopefully it can pave the way for future refactoring as well as other custom property types. If you see anything that can be spun off into a separate patch, let me know. I couldn't find anything that would split nicely. > Source/WebCore/css/CSSStyleDeclaration.h:63 > + virtual ExceptionOr<void> setPropertyWebkit1(const String& propertyName, const String& value, const String& priority) = 0; Not sure what to do about this. I couldn't get a document from the webkit 1 code. It is script wrappable, but everything is null if you try to do [frameElement setProperty:...] from mini browser. > Source/WebCore/css/CSSVariableData.h:64 > // FIXME-NEWPARSER: We want to cache StyleProperties once we support @apply. Not sure what this means > Source/WebCore/css/StyleResolver.cpp:432 > + ApplyCascadedPropertyState applyState { this, &cascade, &result, { }, { }, { }, { } }; These changes seem likely to break something or cause a perf regression. I am not sure how to test this. > Source/WebCore/css/parser/CSSParser.cpp:247 > + // FIXME handle REM cycles. I will make a separate bug for this if it gets merged. > Source/WebCore/css/parser/CSSPropertyParser.cpp:4282 > +bool CSSPropertyParser::canParseTypedCustomPropertyValue(const String& syntax) This behavior matches chrome, but it is not in the spec. Inline styles seem to reject obviously bad values, but accept anything with var in it. See the inline.html test case, but I think this deserves a spec bug. > Source/WebCore/css/parser/CSSPropertyParser.h:54 > + CSSPropertyParser(const CSSParserTokenRange&, const CSSParserContext&, Vector<CSSProperty, 256>*, bool consumeWhitespace = true); This was needed to fix a regression caused by the extra parsing for unresolved custom properties. I added a test, so it shouldn't cause any regressions. > LayoutTests/css-custom-properties-api/crash.html:1 > +<!DOCTYPE html><!-- webkit-test-runner [ experimental:CSSCustomPropertiesAndValuesEnabled=true ] --> See https://bugs.webkit.org/show_bug.cgi?id=184542 > LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/register-property-syntax-parsing-expected.txt:88 > +FAIL syntax:'*', initialValue:'initial' is invalid assert_throws: function "() => CSS.registerProperty({name: name, syntax: syntax, initialValue: initialValue, inherits: false})" did not throw I think this can go in a separate bug
Antti Koivisto
Comment 7 2018-10-14 00:18:34 PDT
Comment on attachment 352251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352251&action=review >> Source/WebCore/css/CSSStyleDeclaration.h:63 >> + virtual ExceptionOr<void> setPropertyWebkit1(const String& propertyName, const String& value, const String& priority) = 0; > > Not sure what to do about this. I couldn't get a document from the webkit 1 code. It is script wrappable, but everything is null if you try to do [frameElement setProperty:...] from mini browser. Any style declaration that actually participates in style resolution is connected to a document. You should be able to reach the associated document via either parentElement() or parentStyleSheet() (depending if it is an inline style declaration or part of a style rule on a stylesheet).
Justin Michaud
Comment 8 2018-10-15 17:10:35 PDT
Created attachment 352407 [details] Patch
Justin Michaud
Comment 9 2018-10-15 18:30:08 PDT
Created attachment 352418 [details] Patch
Justin Michaud
Comment 10 2018-10-15 19:10:45 PDT
Created attachment 352422 [details] Patch
Justin Michaud
Comment 11 2018-10-15 19:39:52 PDT
Created attachment 352425 [details] Patch
EWS Watchlist
Comment 12 2018-10-15 20:52:50 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 13 2018-10-15 20:52:52 PDT Comment hidden (obsolete)
Justin Michaud
Comment 14 2018-10-15 20:55:39 PDT
Created attachment 352433 [details] Patch
Justin Michaud
Comment 15 2018-10-15 21:20:19 PDT
Created attachment 352436 [details] Style refactor (most contentious subset of full patch) This is a subset of the full patch that contains the bulk of the refactoring. Hopefully, this should be easier to review. Once this part looks good, then the rest of the review should be fairly straightforward.
Antti Koivisto
Comment 16 2018-10-16 01:50:28 PDT
Comment on attachment 352433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352433&action=review > Source/WebCore/css/CSSStyleDeclaration.h:61 > - virtual ExceptionOr<void> setProperty(const String& propertyName, const String& value, const String& priority) = 0; > + virtual ExceptionOr<void> setProperty(const Document&, const String& propertyName, const String& value, const String& priority) = 0; This and all the related changes seem unnecessary. Document is available to concrete implementations of CSSStyleDeclaration when they are in-document.
Antti Koivisto
Comment 17 2018-10-16 01:54:33 PDT
> Style refactor (most contentious subset of full patch) What is contentious about it? In general it would be good to find subsets that can be landed separately. A non-compiling subset patch is not helpful.
Justin Michaud
Comment 18 2018-10-16 10:51:54 PDT
Created attachment 352473 [details] Patch
EWS Watchlist
Comment 19 2018-10-16 11:50:07 PDT
Comment on attachment 352473 [details] Patch Attachment 352473 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9624274 New failing tests: fast/css/getMatchedCSSProperties-rule-mutation.html
EWS Watchlist
Comment 20 2018-10-16 11:50:09 PDT
Created attachment 352482 [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
EWS Watchlist
Comment 21 2018-10-16 12:52:53 PDT
Comment on attachment 352473 [details] Patch Attachment 352473 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9625328 New failing tests: fast/css/getMatchedCSSProperties-rule-mutation.html
EWS Watchlist
Comment 22 2018-10-16 12:52:55 PDT
Created attachment 352490 [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 23 2018-10-16 13:21:09 PDT
Comment on attachment 352473 [details] Patch Attachment 352473 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9625420 New failing tests: fast/css/getMatchedCSSProperties-rule-mutation.html
EWS Watchlist
Comment 24 2018-10-16 13:21:20 PDT
Created attachment 352492 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Justin Michaud
Comment 25 2018-10-16 13:33:14 PDT
Created attachment 352496 [details] Patch
Justin Michaud
Comment 26 2018-10-16 15:01:57 PDT
Created attachment 352508 [details] Patch
Justin Michaud
Comment 27 2018-10-16 15:09:13 PDT
(In reply to Antti Koivisto from comment #17) > > Style refactor (most contentious subset of full patch) > > What is contentious about it? > > In general it would be good to find subsets that can be landed separately. A > non-compiling subset patch is not helpful. I figured the changes to applyCascadedProperty would be the riskiest, and also the most likely to introduce performance regressions. I am not really sure how to split this up into parts that can land separately, since most of the changes that are not related to the refactor fix regressions caused by it. If you see anything, please let me know. I removed the document changes, and that helped to bring down the size of the patch. Thanks for the tip. The reason it wasn't working before was because it caused setProperty to crash when used on a style from DOMWindow::getMatchedCSSRules. Moving the willMutate() check up fixed that though.
Dean Jackson
Comment 28 2018-10-16 15:33:26 PDT
Comment on attachment 352508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352508&action=review > Source/WebCore/css/CSSVariableReferenceValue.cpp:68 > + // Apply this variable first, in case it is still unresolved Nit: End with . > Source/WebCore/css/CSSVariableReferenceValue.cpp:71 > + // Apply fallback to detect cycles Ditto. > Source/WebCore/css/CSSVariableReferenceValue.cpp:100 > + bool success = true; > + while (!range.atEnd()) { > + if (range.peek().functionId() == CSSValueVar || range.peek().functionId() == CSSValueEnv) > + success &= resolveVariableReference(range.consumeBlock(), result, state); > + else > + result.append(range.consume()); > + } > + return success; If the resolve fails, could you just return false instantly? > Source/WebCore/css/DOMCSSRegisterCustomProperty.cpp:49 > + // We don't actually need the values to be accurate, since they will be rejected later anyway Nit: .
Justin Michaud
Comment 29 2018-10-16 15:43:17 PDT
(In reply to Dean Jackson from comment #28) > > Source/WebCore/css/CSSVariableReferenceValue.cpp:100 > > + bool success = true; > > + while (!range.atEnd()) { > > + if (range.peek().functionId() == CSSValueVar || range.peek().functionId() == CSSValueEnv) > > + success &= resolveVariableReference(range.consumeBlock(), result, state); > > + else > > + result.append(range.consume()); > > + } > > + return success; > > If the resolve fails, could you just return false instantly? I don't think so. The original code probably could, but now if I would return early, it could miss some cycles. The spec says that any cycles should cause all custom properties in the cycle to be reset, even if we would have been able to resolve the value.
Antti Koivisto
Comment 30 2018-10-19 05:39:12 PDT
Comment on attachment 352508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352508&action=review Looks good, r=me with the issue in PropertySetCSSStyleDeclaration::setProperty fixed. It would also be nice to add a test case for that. > Source/WebCore/css/CSSCustomPropertyValue.cpp:45 > + auto visitor = WTF::makeVisitor([&](const Ref<CSSVariableReferenceValue>& value) { > + return value.get() == WTF::get<Ref<CSSVariableReferenceValue>>(other.m_value).get(); > + }, [&](const CSSValueID& value) { > + return value == WTF::get<CSSValueID>(other.m_value); > + }, [&](const Ref<CSSVariableData>& value) { > + return value.get() == WTF::get<Ref<CSSVariableData>>(other.m_value).get(); > + }, [&](const Length& value) { > + return value == WTF::get<Length>(other.m_value); > + }); > + return WTF::visit(visitor, m_value); Variant has operator==. Why can't you use that? > Source/WebCore/css/CSSCustomPropertyValue.h:44 > +using CSSCustomPropertyValueVariant = Variant<Ref<CSSVariableReferenceValue>, CSSValueID, Ref<CSSVariableData>, Length>; > + > class CSSCustomPropertyValue final : public CSSValue { You might consider scoping the Variant type into the CSSCustomPropertyValue namespace? CSSCustomPropertyValue::VariantValue or something. > Source/WebCore/css/CSSStyleSheet.h:84 > - Document* ownerDocument() const; > + WEBCORE_EXPORT Document* ownerDocument() const; Does this really need to be exported? > Source/WebCore/css/DOMCSSRegisterCustomProperty.cpp:70 > + document.styleScope().didChangeStyleSheetEnvironment(); This triggers full re-evaluation of stylesheets. Is this something that is expected to be rare in practice? Are there ways to optimize this in the future? > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:233 > + ASSERT(parentElement() || parentStyleSheet()); This is not correct. You can have completely detached CSSStyleDeclaration, for example: let styleDecl = element.style; element.removeAttribute("style"); // styleDecl is now detached (didn't try, just by reading code) Maybe willMutate() should return false in this case, but it currently doesn't. > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:241 > + ASSERT(document); As a result document may be null too. > Source/WebCore/css/StyleResolver.cpp:432 > + ApplyCascadedPropertyState applyState { this, &cascade, &result, { }, { }, { }, { } }; Please get rid of { }, { }, { }, { } repeated everywhere. I think you can just remove them, but if not, just add a constructor. > Source/WebCore/css/StyleResolver.cpp:2342 > + if (state.inProgress.contains(propertyID)) { 'inProgressProperties' and 'inProgressCustomProperties' would be more consistent and informative names. > Source/WebCore/css/StyleResolver.h:539 > +// State to use when applying properties, to keep track of which custom and high-priority > +// properties have been resolved. > +struct ApplyCascadedPropertyState { > + StyleResolver* styleResolver; > + StyleResolver::CascadedProperties* cascade; > + const StyleResolver::MatchResult* matchResult; > + HashSet<CSSPropertyID> appliedProperties; > + HashSet<String> appliedCustomProperties; > + HashSet<CSSPropertyID> inProgress; > + HashSet<String> inProgressCustom; > +}; We should split out some of this stuff from StyleResolver at some point. > Source/WebCore/css/parser/CSSParser.cpp:234 > + auto visitor = WTF::makeVisitor([&](const Ref<CSSVariableReferenceValue>& value) { > + valueWithReferences = value.ptr(); > + }, [&](const CSSValueID&) { > + ASSERT_NOT_REACHED(); > + }, [&](const Ref<CSSVariableData>&) { > + ASSERT_NOT_REACHED(); > + }, [&](const Length&) { > + ASSERT_NOT_REACHED(); > + }); > + WTF::visit(visitor, customPropValue.value()); Please just use WTF::get<> instead of a visitor. > Source/WebCore/css/parser/CSSPropertyParser.h:54 > + CSSPropertyParser(const CSSParserTokenRange&, const CSSParserContext&, Vector<CSSProperty, 256>*, bool consumeWhitespace = true); Please use enum class for consumeWhitespace, booleans in call sites are confusing.
Justin Michaud
Comment 31 2018-10-19 15:31:35 PDT
Comment on attachment 352508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352508&action=review >> Source/WebCore/css/CSSCustomPropertyValue.cpp:45 >> + return WTF::visit(visitor, m_value); > > Variant has operator==. Why can't you use that? Ref<> doesn't seem to have operator==, or at least if it does, I couldn't get it to work. Should I just add it? >> Source/WebCore/css/DOMCSSRegisterCustomProperty.cpp:70 >> + document.styleScope().didChangeStyleSheetEnvironment(); > > This triggers full re-evaluation of stylesheets. Is this something that is expected to be rare in practice? Are there ways to optimize this in the future? I am not sure how common this will be in practice. That depends on the number of registered custom properties that web authors use. I suppose we could only invalidate styles that contain a reference to this variable in the future.
Justin Michaud
Comment 32 2018-10-19 17:01:22 PDT
Comment on attachment 352508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352508&action=review >> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:233 >> + ASSERT(parentElement() || parentStyleSheet()); > > This is not correct. You can have completely detached CSSStyleDeclaration, for example: > > let styleDecl = element.style; > element.removeAttribute("style"); > // styleDecl is now detached > > (didn't try, just by reading code) > > Maybe willMutate() should return false in this case, but it currently doesn't. I removed the assertion, and treat the case without a document as if there are no registered properties. I tried to write a test for it, but removeAttribute doesn't actually unset the parentElement (same on Chrome). I can't think of any way to trigger this case, please let me know if you have any ideas.
Justin Michaud
Comment 33 2018-10-19 19:38:14 PDT
Created attachment 352848 [details] Patch
Antti Koivisto
Comment 34 2018-10-20 01:12:48 PDT
> Ref<> doesn't seem to have operator==, or at least if it does, I couldn't > get it to work. Should I just add it? It doesn't need to be done for this patch. Maybe the reason it hasn't been added is the question whether it should be a deep comparison or a pointer comparison (it probably should be deep, for consistency with &).
Antti Koivisto
Comment 35 2018-10-20 01:14:21 PDT
(In reply to Justin Michaud from comment #32) > Comment on attachment 352508 [details] > Patch > I removed the assertion, and treat the case without a document as if there > are no registered properties. I tried to write a test for it, but > removeAttribute doesn't actually unset the parentElement (same on Chrome). I > can't think of any way to trigger this case, please let me know if you have > any ideas. StyledElement::styleAttributeChanged has a path that calls clearParentElement so this appears to be possible
Antti Koivisto
Comment 36 2018-10-20 01:25:12 PDT
Comment on attachment 352848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352848&action=review > Source/WebCore/css/StyleProperties.cpp:794 > + auto* registered = document? document->getCSSRegisteredCustomPropertySet().get(propertyName) : nullptr; Space before '?' > Source/WebCore/css/StyleResolver.h:545 > + HashSet<CSSPropertyID> appliedProperties; > + HashSet<String> appliedCustomProperties; > + HashSet<CSSPropertyID> inProgressProperties; > + HashSet<String> inProgressPropertiesCustom; > + > + ApplyCascadedPropertyState(StyleResolver* styleResolver, StyleResolver::CascadedProperties* cascade, const StyleResolver::MatchResult* matchResult) > + : styleResolver(styleResolver) > + , cascade(cascade) > + , matchResult(matchResult) > + { > + } Wonder if just having HashSet<CSSPropertyID> appliedProperties = { }; ... would work instead of constructor? This stuff has been changing in C++11/14/17 > Source/WebCore/css/parser/CSSParser.cpp:222 > + const CSSCustomPropertyValue& customPropValue = downcast<CSSCustomPropertyValue>(value); could just use auto to avoid repeating CSSCustomPropertyValue > Source/WebCore/css/parser/CSSParser.cpp:223 > + const auto& valueWithReferences = WTF::get<Ref<CSSVariableReferenceValue>>(customPropValue.value()).get(); Should there be an assert before get? Not sure if get asserts itself.
Justin Michaud
Comment 37 2018-10-22 15:29:14 PDT
Created attachment 352916 [details] Patch
Justin Michaud
Comment 38 2018-10-22 15:30:00 PDT
(In reply to Antti Koivisto from comment #36) > Should there be an assert before get? Not sure if get asserts itself. I checked. It seems like it does assert that the type index is correct.
WebKit Commit Bot
Comment 39 2018-10-22 23:11:06 PDT
Comment on attachment 352916 [details] Patch Clearing flags on attachment: 352916 Committed r237347: <https://trac.webkit.org/changeset/237347>
WebKit Commit Bot
Comment 40 2018-10-22 23:11:08 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 41 2018-10-22 23:12:35 PDT
<rdar://problem/45477680>
Note You need to log in before you can comment on or make changes to this bug.