RESOLVED FIXED 229702
Add support for CSSUnparsedValue parsing and reification of various CSSValue
https://bugs.webkit.org/show_bug.cgi?id=229702
Summary Add support for CSSUnparsedValue parsing and reification of various CSSValue
Qiaosong Zhou
Reported 2021-08-31 02:13:16 PDT
Add support for CSSUnparsedValue parsing through CSSStyleValue.parse()
Attachments
Patch (18.47 KB, patch)
2021-08-31 02:14 PDT, Qiaosong Zhou
no flags
Patch (89.42 KB, patch)
2021-08-31 18:22 PDT, Qiaosong Zhou
no flags
Patch (89.44 KB, patch)
2021-08-31 18:26 PDT, Qiaosong Zhou
no flags
Patch (89.23 KB, patch)
2021-09-01 00:12 PDT, Qiaosong Zhou
no flags
Patch (92.64 KB, patch)
2021-09-02 01:03 PDT, Qiaosong Zhou
no flags
Patch (89.14 KB, patch)
2021-09-02 14:06 PDT, Qiaosong Zhou
no flags
Patch (91.15 KB, patch)
2021-09-02 15:52 PDT, Qiaosong Zhou
no flags
Patch (93.38 KB, patch)
2021-09-02 19:50 PDT, Qiaosong Zhou
no flags
Patch (93.44 KB, patch)
2021-09-02 21:41 PDT, Qiaosong Zhou
no flags
Patch (94.57 KB, patch)
2021-09-03 02:52 PDT, Qiaosong Zhou
no flags
Patch (94.72 KB, patch)
2021-09-03 16:08 PDT, Qiaosong Zhou
no flags
Patch (133.03 KB, patch)
2021-09-13 14:48 PDT, Qiaosong Zhou
no flags
Patch (105.96 KB, patch)
2021-09-15 18:08 PDT, Qiaosong Zhou
no flags
Patch (130.36 KB, patch)
2021-09-15 23:31 PDT, Qiaosong Zhou
no flags
Patch (130.36 KB, patch)
2021-09-15 23:45 PDT, Qiaosong Zhou
no flags
Patch (130.36 KB, patch)
2021-09-15 23:49 PDT, Qiaosong Zhou
no flags
Patch (132.01 KB, patch)
2021-09-16 18:36 PDT, Qiaosong Zhou
ews-feeder: commit-queue-
Patch (131.26 KB, patch)
2021-09-16 18:50 PDT, Qiaosong Zhou
no flags
Patch (132.11 KB, patch)
2021-09-17 15:05 PDT, Qiaosong Zhou
no flags
Patch (130.94 KB, patch)
2021-09-20 09:42 PDT, Alex Christensen
no flags
Qiaosong Zhou
Comment 1 2021-08-31 02:14:10 PDT
Alex Christensen
Comment 2 2021-08-31 11:32:59 PDT
Comment on attachment 436858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436858&action=review Looks like some test expectations need updating. Looks like a progression. > Source/WebCore/css/CSSVariableReferenceValue.h:64 > + // FIXME: is friend the best solution? > + friend class CSSStyleValue; It isn't. A better solution would probably be to expose public functions to do what CSSStyleValue needs to do. > Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:75 > + m_fallback->serialize(builder); nice! > Source/WebCore/css/typedom/CSSStyleImageValue.h:57 > + CSSStyleImageValue(Ref<CSSImageValue>&&, Document* = nullptr); It looks like the default value is unneeded because only one call site has nullptr. Also, it looks like the Document is used by the CanvasRenderingContext2DBase::drawImage which takes a CSSStyleImageValue&, and having a null Document would make this unable to be drawn. I'm not sure what the consequences of that would be, and I'm not sure how finished CanvasRenderingContext2DBase::drawImage is. It might be worth a comment saying that the CSSStyleImageValue created with no document currently can't be used for drawing. > Source/WebCore/css/typedom/CSSStyleValue.cpp:51 > +ExceptionOr<Vector<Ref<CSSStyleValue>>> CSSStyleValue::parseStyleValue(const String& cssProperty, const String& cssText, bool) Ah, I see the comment below. parseMultiple will be brought back. > Source/WebCore/css/typedom/CSSStyleValue.cpp:80 > + nit: unnecessary space addition. > Source/WebCore/css/typedom/CSSStyleValue.cpp:95 > + // FIXME: Add Reification control flow. Returns nullptr if failed We can't return nullptr here. An exception? > Source/WebCore/css/typedom/CSSStyleValue.cpp:119 > +CSSParserContext CSSStyleValue::getVariableReferenceParserContext() > +{ > + auto parserContext = CSSParserContext(HTMLStandardMode); Since this is static, do we need to make a new CSSParserContext every time it is called? Could we use NeverDestroyed and return a const CSSParserContext&? It doesn't look like the CSSParserContext stores state that we would need to reset or anything like that.
Qiaosong Zhou
Comment 3 2021-08-31 18:22:35 PDT
Qiaosong Zhou
Comment 4 2021-08-31 18:26:40 PDT
Qiaosong Zhou
Comment 5 2021-09-01 00:12:26 PDT
Alex Christensen
Comment 6 2021-09-01 09:41:00 PDT
Comment on attachment 436997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436997&action=review > Source/WebCore/css/CSSVariableReferenceValue.h:49 > + CSSVariableData& data() const { return m_data.get(); } This should return a const CSSVariableData& > Source/WebCore/css/typedom/CSSStyleImageValue.h:57 > + CSSStyleImageValue(Ref<CSSImageValue>&&, Document* = nullptr); This default value of "= nullptr" should not be added. It is only called from create, which always has a value. > Source/WebCore/css/typedom/CSSStyleValue.cpp:96 > + // FIXME: Add Reification control flow. Returns default CSSStyleValue if failed. I don't think I understand why this FIXME comment is here. Is there something left to do after this change? > Source/WebCore/css/typedom/CSSStyleValue.cpp:128 > + parserContext->springTimingFunctionEnabled = true; These values are supposed to come from the Document. We shouldn't hard code them here. Is it possible to get a Document& to here using [CallWith=Document]? We would have to make this no longer a static function and not use a NeverDestroyed, but that would probably be a better design if it's possible. > Source/WebCore/css/typedom/CSSStyleValue.h:77 > + static const CSSParserContext& getVariableReferenceParserContext(); I don't think this name needs "get"
Qiaosong Zhou
Comment 7 2021-09-02 01:03:30 PDT
Alex Christensen
Comment 8 2021-09-02 12:08:13 PDT
Comment on attachment 437123 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437123&action=review > Source/WebCore/css/typedom/CSSStyleValue.cpp:126 > + parserContext->springTimingFunctionEnabled = true; This should not be hard-coded. We should either get the values from a Document, or use the default value. Can't we just use strictCSSParserContext instead of all this?
Qiaosong Zhou
Comment 9 2021-09-02 14:06:18 PDT
Chris Dumez
Comment 10 2021-09-02 14:15:29 PDT
Comment on attachment 437193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437193&action=review > Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:77 > + builder.append(")"); nit: builder.append(')'); > Source/WebCore/css/typedom/CSSStyleValue.cpp:52 > +ExceptionOr<Vector<Ref<CSSStyleValue>>> CSSStyleValue::parseStyleValue(const String& cssProperty, const String& cssText, bool) The parameter name is not obvious so I wouldn't omit it. You can use UNUSED_PARAM(parseMultiple); if necessary. > Source/WebCore/css/typedom/CSSStyleValue.cpp:102 > + if (is<CSSImageValue>(cssValue.get())) shouldn't this be `else if`? > Source/WebCore/css/typedom/CSSStyleValue.cpp:105 > + if (is<CSSVariableReferenceValue>(cssValue.get())) { shouldn't this be `else if`? > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:105 > + if (topIdentifier) { if (auto topIdentifier = identifiers.takeLast()) {
Qiaosong Zhou
Comment 11 2021-09-02 15:52:10 PDT
Qiaosong Zhou
Comment 12 2021-09-02 19:50:25 PDT
Qiaosong Zhou
Comment 13 2021-09-02 21:41:48 PDT
Qiaosong Zhou
Comment 14 2021-09-03 02:52:05 PDT
Qiaosong Zhou
Comment 15 2021-09-03 16:08:39 PDT
Radar WebKit Bug Importer
Comment 16 2021-09-07 02:14:16 PDT
Alex Christensen
Comment 17 2021-09-07 09:21:29 PDT
Comment on attachment 437322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437322&action=review Great progress. Let's get this polished up and landed. > Source/WebCore/css/typedom/CSSStyleValue.cpp:75 > + auto parserContext = strictCSSParserContext(); This isn't used in this patch. Please remove it from this patch and add it when it is used. > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:66 > + if (currentToken.value() == "var") { Would it be equivalent to check .functionId() and see if it's CSSValueVar? That would probably be better than hard-coding a css keyword here. > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:79 > + // Create a new vector of CSSUnparsedSegment > + segmentStack.append({ }); I don't think this comment adds anything. > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:88 > + ASSERT_NOT_REACHED(); I feel like this assertion can be hit with invalid input. What happens when it is hit? What is the behavior of Chrome? Please add such a test if possible. > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:96 > + ASSERT(segmentStack.size() > 0); "> 0" is unnecessary > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:102 > + ASSERT(!identifiers.isEmpty()); > + if (identifiers.isEmpty()) > + return Exception { SyntaxError, "Cannot parse string as CSSUnparsedValue, parentheses do not match."_s }; If identifiers can be empty when parentheses don't match, the assertion should be removed and we should add a test that tries to parse a value with mismatching parentheses and verify that a SyntaxError exception is thrown. ASSERT should be used for things that can't be hit no matter how invalid the input, not for things that shouldn't happen with valid input.
Qiaosong Zhou
Comment 18 2021-09-13 14:48:39 PDT
Qiaosong Zhou
Comment 19 2021-09-15 15:00:24 PDT
(In reply to Alex Christensen from comment #17) > > > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:88 > > + ASSERT_NOT_REACHED(); > > I feel like this assertion can be hit with invalid input. What happens when > it is hit? What is the behavior of Chrome? Please add such a test if > possible. > > > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:96 > > + ASSERT(segmentStack.size() > 0); > > "> 0" is unnecessary > > > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:102 > > + ASSERT(!identifiers.isEmpty()); > > + if (identifiers.isEmpty()) > > + return Exception { SyntaxError, "Cannot parse string as CSSUnparsedValue, parentheses do not match."_s }; > > If identifiers can be empty when parentheses don't match, the assertion > should be removed and we should add a test that tries to parse a value with > mismatching parentheses and verify that a SyntaxError exception is thrown. > ASSERT should be used for things that can't be hit no matter how invalid the > input, not for things that shouldn't happen with valid input. According to my testing, syntax error would be found during the tokenizing stage. Presumably if the CSSPropertyParser is bug-free, then variable references syntax would be correct, and parentheses should match.
Qiaosong Zhou
Comment 20 2021-09-15 18:08:03 PDT
EWS Watchlist
Comment 21 2021-09-15 18:09:16 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Qiaosong Zhou
Comment 22 2021-09-15 23:31:04 PDT
Qiaosong Zhou
Comment 23 2021-09-15 23:45:34 PDT
Qiaosong Zhou
Comment 24 2021-09-15 23:49:14 PDT
Alex Christensen
Comment 25 2021-09-16 09:37:05 PDT
Comment on attachment 438321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438321&action=review > Source/WebCore/css/typedom/CSSStyleValueFactory.cpp:2 > + * Copyright (C) 2019 Apple Inc. All rights reserved. 2021 > Source/WebCore/css/typedom/CSSStyleValueFactory.h:2 > + * Copyright (C) 2018 Apple Inc. All rights reserved. 2021 > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:103 > + RELEASE_ASSERT(!variableReference.hasException()); I think a lot of these could just be ASSERTs. RELEASE_ASSERT is typically used for cases where we want to crash if this condition is somehow reached because otherwise something bad will happen like writing out of bounds. I don't think that's the case here. > Source/WebCore/dom/StyledElement.cpp:122 > + auto shorthand = shorthandForProperty(propertyID); > + for (auto longhand : shorthand) { > + if (auto cssValue = element.inlineStyle()->getPropertyCSSValue(longhand)) > + return StylePropertyMapReadOnly::reifyValue(cssValue.get(), element.document(), &element); > + } I don't understand what this change does, why it's necessary outside of typedom, and what else it could change. Could this change go in a separate patch? > LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/inline/get.html:32 > + new CSSUnparsedValue([remove_leading_spaces(' auto')])); Does this still pass in Chrome? The remove_leading_spaces might be in the wrong place. To do this change you could just replace ' auto' with 'auto'. The intent of this change was to make the test more permissive of unspecified behavior where Safari and Firefox have different behavior than Chrome.
Qiaosong Zhou
Comment 26 2021-09-16 11:39:04 PDT
(In reply to Alex Christensen from comment #25) > > Source/WebCore/dom/StyledElement.cpp:122 > > + auto shorthand = shorthandForProperty(propertyID); > > + for (auto longhand : shorthand) { > > + if (auto cssValue = element.inlineStyle()->getPropertyCSSValue(longhand)) > > + return StylePropertyMapReadOnly::reifyValue(cssValue.get(), element.document(), &element); > > + } > > I don't understand what this change does, why it's necessary outside of > typedom, and what else it could change. Could this change go in a separate > patch? It adds the correct behavior that parses shorthand CSSPropertyID to reify the correct CSSValue. I think it is relevant because it is part of the usage of CSS Typed OM. > > LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/inline/get.html:32 > > + new CSSUnparsedValue([remove_leading_spaces(' auto')])); > > Does this still pass in Chrome? The remove_leading_spaces might be in the > wrong place. To do this change you could just replace ' auto' with 'auto'. > The intent of this change was to make the test more permissive of > unspecified behavior where Safari and Firefox have different behavior than > Chrome. Ops. I'm changing it now.
Qiaosong Zhou
Comment 27 2021-09-16 18:36:42 PDT
Qiaosong Zhou
Comment 28 2021-09-16 18:50:00 PDT
Qiaosong Zhou
Comment 29 2021-09-17 15:05:09 PDT
Alex Christensen
Comment 30 2021-09-17 17:35:37 PDT
Comment on attachment 438520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438520&action=review > Source/WebCore/css/typedom/CSSStyleValueFactory.cpp:65 > + extra spaces > Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:51 > + return (result.hasException() ? nullptr : RefPtr<CSSStyleValue> { result.releaseReturnValue() }); I wonder if we'll want to return ExceptionOr<RefPtr> here to pass the exact exception up the call stack. > LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/inline/get.html:35 > + // assert_style_value_equals(styleMap.get('--foo'), This should probably be removed.
EWS
Comment 31 2021-09-20 08:47:22 PDT
Tools/Scripts/svn-apply failed to apply attachment 438520 [details] to trunk. Please resolve the conflicts and upload a new patch.
Alex Christensen
Comment 32 2021-09-20 09:42:53 PDT
EWS
Comment 33 2021-09-20 09:45:52 PDT
ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.
Alex Christensen
Comment 34 2021-09-20 10:41:25 PDT
Note You need to log in before you can comment on or make changes to this bug.