Bug 229702 – Add support for CSSUnparsedValue parsing and reification of various CSSValue

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

[Qiaosong Zhou]

Add support for CSSUnparsedValue parsing through CSSStyleValue.parse()

[Qiaosong Zhou]

Created attachment 436858 [details]
Patch

[Alex Christensen]

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]

Created attachment 436978 [details]
Patch

[Qiaosong Zhou]

Created attachment 436979 [details]
Patch

[Qiaosong Zhou]

Created attachment 436997 [details]
Patch

[Alex Christensen]

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]

Created attachment 437123 [details]
Patch

[Alex Christensen]

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]

Created attachment 437193 [details]
Patch

[Chris Dumez]

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]

Created attachment 437207 [details]
Patch

[Qiaosong Zhou]

Created attachment 437236 [details]
Patch

[Qiaosong Zhou]

Created attachment 437243 [details]
Patch

[Qiaosong Zhou]

Created attachment 437255 [details]
Patch

[Qiaosong Zhou]

Created attachment 437322 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/82812803>

[Alex Christensen]

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]

Created attachment 438081 [details]
Patch

[Qiaosong Zhou]

(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]

Created attachment 438313 [details]
Patch

[EWS Watchlist]

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]

Created attachment 438319 [details]
Patch

[Qiaosong Zhou]

Created attachment 438320 [details]
Patch

[Qiaosong Zhou]

Created attachment 438321 [details]
Patch

[Alex Christensen]

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]

(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]

Created attachment 438427 [details]
Patch

[Qiaosong Zhou]

Created attachment 438428 [details]
Patch

[Qiaosong Zhou]

Created attachment 438520 [details]
Patch

[Alex Christensen]

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]

Tools/Scripts/svn-apply failed to apply attachment 438520 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Alex Christensen]

Created attachment 438688 [details]
Patch

[EWS]

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

[Alex Christensen]

r282758