WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(89.42 KB, patch)
2021-08-31 18:22 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(89.44 KB, patch)
2021-08-31 18:26 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(89.23 KB, patch)
2021-09-01 00:12 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(92.64 KB, patch)
2021-09-02 01:03 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(89.14 KB, patch)
2021-09-02 14:06 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(91.15 KB, patch)
2021-09-02 15:52 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(93.38 KB, patch)
2021-09-02 19:50 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(93.44 KB, patch)
2021-09-02 21:41 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(94.57 KB, patch)
2021-09-03 02:52 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(94.72 KB, patch)
2021-09-03 16:08 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(133.03 KB, patch)
2021-09-13 14:48 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(105.96 KB, patch)
2021-09-15 18:08 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(130.36 KB, patch)
2021-09-15 23:31 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(130.36 KB, patch)
2021-09-15 23:45 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(130.36 KB, patch)
2021-09-15 23:49 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(132.01 KB, patch)
2021-09-16 18:36 PDT
,
Qiaosong Zhou
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(131.26 KB, patch)
2021-09-16 18:50 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(132.11 KB, patch)
2021-09-17 15:05 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(130.94 KB, patch)
2021-09-20 09:42 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(19)
View All
Add attachment
proposed patch, testcase, etc.
Qiaosong Zhou
Comment 1
2021-08-31 02:14:10 PDT
Created
attachment 436858
[details]
Patch
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
Created
attachment 436978
[details]
Patch
Qiaosong Zhou
Comment 4
2021-08-31 18:26:40 PDT
Created
attachment 436979
[details]
Patch
Qiaosong Zhou
Comment 5
2021-09-01 00:12:26 PDT
Created
attachment 436997
[details]
Patch
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
Created
attachment 437123
[details]
Patch
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
Created
attachment 437193
[details]
Patch
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
Created
attachment 437207
[details]
Patch
Qiaosong Zhou
Comment 12
2021-09-02 19:50:25 PDT
Created
attachment 437236
[details]
Patch
Qiaosong Zhou
Comment 13
2021-09-02 21:41:48 PDT
Created
attachment 437243
[details]
Patch
Qiaosong Zhou
Comment 14
2021-09-03 02:52:05 PDT
Created
attachment 437255
[details]
Patch
Qiaosong Zhou
Comment 15
2021-09-03 16:08:39 PDT
Created
attachment 437322
[details]
Patch
Radar WebKit Bug Importer
Comment 16
2021-09-07 02:14:16 PDT
<
rdar://problem/82812803
>
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
Created
attachment 438081
[details]
Patch
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
Created
attachment 438313
[details]
Patch
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
Created
attachment 438319
[details]
Patch
Qiaosong Zhou
Comment 23
2021-09-15 23:45:34 PDT
Created
attachment 438320
[details]
Patch
Qiaosong Zhou
Comment 24
2021-09-15 23:49:14 PDT
Created
attachment 438321
[details]
Patch
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
Created
attachment 438427
[details]
Patch
Qiaosong Zhou
Comment 28
2021-09-16 18:50:00 PDT
Created
attachment 438428
[details]
Patch
Qiaosong Zhou
Comment 29
2021-09-17 15:05:09 PDT
Created
attachment 438520
[details]
Patch
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
Created
attachment 438688
[details]
Patch
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
r282758
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug