Bug 226708 – Fix CSS serialization issues affecting css-counter-styles tests

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

[Darin Adler]

Fixe CSS serialization issues affecting css-counter-styles tests

[Darin Adler]

Created attachment 430716 [details]
Patch

[Darin Adler]

The image URL issue is a little more complicated than I thought.

The cssText from the computed style must include the full resolved URL! But the cssText from the rule in the style sheet should reflect the not-yet-resolved URL. It seems, given WPT expected results for others tests and for the css-counter-styles test.

Now I am a little confused: Is the css-counter-styles test expected result correct? Is there somewhere this behavior is documented that I am missing? I’m not really in the mood to download the other browsers to check their behavior, but maybe I should.

I’m sure I can implement this correctly and efficiently once I figure out what rule I am trying to implement.

[Chris Dumez]

Comment on attachment 430716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430716&action=review

> LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-additive-symbols-syntax-expected.txt:2
> +PASS @counter-style 'additive-symbols: 0 "X"' is valid

Chrome and Firefox are passing all checks in this test.

> LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-symbols-syntax-expected.txt:5
> +PASS @counter-style 'symbols: ident "X" url("foo.jpg")' is valid

Chrome and Firefox are both failing this check so this change is a compatibility risk and the test is likely wrong.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 430716 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430716&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-additive-symbols-syntax-expected.txt:2
> > +PASS @counter-style 'additive-symbols: 0 "X"' is valid
> 
> Chrome and Firefox are passing all checks in this test.
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-symbols-syntax-expected.txt:5
> > +PASS @counter-style 'symbols: ident "X" url("foo.jpg")' is valid
> 
> Chrome and Firefox are both failing this check so this change is a
> compatibility risk and the test is likely wrong.

In Firefox:
assert_equals: expected (undefined) undefined but got (string) "ident \"X\" url(\"foo.jpg\")"

In Chrome:
assert_not_equals: got disallowed value -1

[Darin Adler]

Comment on attachment 430716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430716&action=review

>> LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-symbols-syntax-expected.txt:5
>> +PASS @counter-style 'symbols: ident "X" url("foo.jpg")' is valid
> 
> Chrome and Firefox are both failing this check so this change is a compatibility risk and the test is likely wrong.

Thank you for checking.

This feature, images, is not all browser has agreed we should be implementing. It’s switched with a separate feature flag, CSSCounterStyleAtRuleImageSymbolsEnabled. Not the main feature flag, CSSCounterStyleAtRulesEnabled.

But the real question is: in other similar uses, outside of this feature, such as background-image URLs, are such image URLs serializing as a full completed URLs when examining the style sheet, or as partial URLs. In computed style, they are serializing as full completed URLs. There may be a mistake in the way this test checks the cssText, aside from the fact that it tests the likely-not-to-be implemented image symbols. That’s something I want to figure out.

The Firefox result you cited here is peculiar:

    assert_equals: expected (undefined) undefined but got (string) "ident \"X\" url(\"foo.jpg\")"

Why would it expect undefined? Is the copy of WPT running in Firefox testing that the feature is not implemented?

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 430716 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430716&action=review
> 
> >> LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-symbols-syntax-expected.txt:5
> >> +PASS @counter-style 'symbols: ident "X" url("foo.jpg")' is valid
> > 
> > Chrome and Firefox are both failing this check so this change is a compatibility risk and the test is likely wrong.
> 
> Thank you for checking.
> 
> This feature, images, is not all browser has agreed we should be
> implementing. It’s switched with a separate feature flag,
> CSSCounterStyleAtRuleImageSymbolsEnabled. Not the main feature flag,
> CSSCounterStyleAtRulesEnabled.
> 
> But the real question is: in other similar uses, outside of this feature,
> such as background-image URLs, are such image URLs serializing as a full
> completed URLs when examining the style sheet, or as partial URLs. In
> computed style, they are serializing as full completed URLs. There may be a
> mistake in the way this test checks the cssText, aside from the fact that it
> tests the likely-not-to-be implemented image symbols. That’s something I
> want to figure out.
> 
> The Firefox result you cited here is peculiar:
> 
>     assert_equals: expected (undefined) undefined but got (string) "ident
> \"X\" url(\"foo.jpg\")"
> 
> Why would it expect undefined? Is the copy of WPT running in Firefox testing
> that the feature is not implemented?

This is the check that Firefox is failing:
```
    let rule = style.sheet.cssRules[0];
    // TODO: The spec is inconsistent on when the entire rule is invalid
    // (and hence absent from OM), and when only the descriptor is invalid.
    // Revise when spec issue is resolved.
    // See https://github.com/w3c/csswg-drafts/issues/5717
    if (!rule) {
      assert_equals(expected, undefined);
      return;
    }
```

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Darin Adler from comment #5)
> > Comment on attachment 430716 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=430716&action=review
> > 
> > >> LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-symbols-syntax-expected.txt:5
> > >> +PASS @counter-style 'symbols: ident "X" url("foo.jpg")' is valid
> > > 
> > > Chrome and Firefox are both failing this check so this change is a compatibility risk and the test is likely wrong.
> > 
> > Thank you for checking.
> > 
> > This feature, images, is not all browser has agreed we should be
> > implementing. It’s switched with a separate feature flag,
> > CSSCounterStyleAtRuleImageSymbolsEnabled. Not the main feature flag,
> > CSSCounterStyleAtRulesEnabled.
> > 
> > But the real question is: in other similar uses, outside of this feature,
> > such as background-image URLs, are such image URLs serializing as a full
> > completed URLs when examining the style sheet, or as partial URLs. In
> > computed style, they are serializing as full completed URLs. There may be a
> > mistake in the way this test checks the cssText, aside from the fact that it
> > tests the likely-not-to-be implemented image symbols. That’s something I
> > want to figure out.
> > 
> > The Firefox result you cited here is peculiar:
> > 
> >     assert_equals: expected (undefined) undefined but got (string) "ident
> > \"X\" url(\"foo.jpg\")"
> > 
> > Why would it expect undefined? Is the copy of WPT running in Firefox testing
> > that the feature is not implemented?
> 
> This is the check that Firefox is failing:
> ```
>     let rule = style.sheet.cssRules[0];
>     // TODO: The spec is inconsistent on when the entire rule is invalid
>     // (and hence absent from OM), and when only the descriptor is invalid.
>     // Revise when spec issue is resolved.
>     // See https://github.com/w3c/csswg-drafts/issues/5717
>     if (!rule) {
>       assert_equals(expected, undefined);
>       return;
>     }
> ```

I merely opened http://w3c-test.org/css/css-counter-styles/counter-style-symbols-syntax.html in Firefox so it should be the latest version of the WPT test.

[Darin Adler]

The real question is what the browsers do on a test file like this:

    <style>a { background-image: url("foo.jpg") }</style>
    <script> document.write(document.head.firstChild.sheet.cssRules[0].cssText) </script>

[Darin Adler]

Created attachment 430790 [details]
Simple URL serialization test

[Chris Dumez]

(In reply to Darin Adler from comment #8)
> The real question is what the browsers do on a test file like this:
> 
>     <style>a { background-image: url("foo.jpg") }</style>
>     <script>
> document.write(document.head.firstChild.sheet.cssRules[0].cssText) </script>

Safari:
a { background-image: url("file:///Users/cdumez/Downloads/foo.jpg"); }

Chrome:
a { background-image: url("foo.jpg"); }

Firefox:
a { background-image: url("foo.jpg"); }

[Darin Adler]

Comment on attachment 430716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430716&action=review

>>>>> LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-symbols-syntax-expected.txt:5
>>>>> +PASS @counter-style 'symbols: ident "X" url("foo.jpg")' is valid
>>>> 
>>>> Chrome and Firefox are both failing this check so this change is a compatibility risk and the test is likely wrong.
>>> 
>>> Thank you for checking.
>>> 
>>> This feature, images, is not all browser has agreed we should be implementing. It’s switched with a separate feature flag, CSSCounterStyleAtRuleImageSymbolsEnabled. Not the main feature flag, CSSCounterStyleAtRulesEnabled.
>>> 
>>> But the real question is: in other similar uses, outside of this feature, such as background-image URLs, are such image URLs serializing as a full completed URLs when examining the style sheet, or as partial URLs. In computed style, they are serializing as full completed URLs. There may be a mistake in the way this test checks the cssText, aside from the fact that it tests the likely-not-to-be implemented image symbols. That’s something I want to figure out.
>>> 
>>> The Firefox result you cited here is peculiar:
>>> 
>>>     assert_equals: expected (undefined) undefined but got (string) "ident \"X\" url(\"foo.jpg\")"
>>> 
>>> Why would it expect undefined? Is the copy of WPT running in Firefox testing that the feature is not implemented?
>> 
>> This is the check that Firefox is failing:
>> ```
>>     let rule = style.sheet.cssRules[0];
>>     // TODO: The spec is inconsistent on when the entire rule is invalid
>>     // (and hence absent from OM), and when only the descriptor is invalid.
>>     // Revise when spec issue is resolved.
>>     // See https://github.com/w3c/csswg-drafts/issues/5717
>>     if (!rule) {
>>       assert_equals(expected, undefined);
>>       return;
>>     }
>> ```
> 
> I merely opened http://w3c-test.org/css/css-counter-styles/counter-style-symbols-syntax.html in Firefox so it should be the latest version of the WPT test.

OK, makes sense. The confusing message is because the test script css/css-counter-styles/support/counter-style-testcommon.js  has an error, it should be:

      assert_equals(undefined, expected)

Because the script has an error, the message is backwards. Given that mistake, then the message simply means they haven’t implemented parsing of the image URL at all here, which makes sense given that a lot of people are unsure whether to support it.

Leaves the question about partial vs. resolved URLs still outstanding. Maybe you can try my test file in the other browsers for me?

[Darin Adler]

(In reply to Chris Dumez from comment #10)
> Safari:
> a { background-image: url("file:///Users/cdumez/Downloads/foo.jpg"); }
> 
> Chrome:
> a { background-image: url("foo.jpg"); }
> 
> Firefox:
> a { background-image: url("foo.jpg"); }

Yes, this is what I am trying to fix.

[Chris Dumez]

(In reply to Darin Adler from comment #12)
> (In reply to Chris Dumez from comment #10)
> > Safari:
> > a { background-image: url("file:///Users/cdumez/Downloads/foo.jpg"); }
> > 
> > Chrome:
> > a { background-image: url("foo.jpg"); }
> > 
> > Firefox:
> > a { background-image: url("foo.jpg"); }
> 
> Yes, this is what I am trying to fix.

Just to clarify, this is the output from the test you asked me to run. So it does appear we are not aligned with other browsers here.

[Darin Adler]

(In reply to Chris Dumez from comment #13)
> So it
> does appear we are not aligned with other browsers here.

Yes, this is what I am trying to fix. Without breaking other related things.

[Darin Adler]

Created attachment 431026 [details]
Patch

[Darin Adler]

Created attachment 431036 [details]
Patch

[Chris Dumez]

Looks like some of the EWS failures may be real (although EWS has been terrible in terms of false positives in the last few days...)

[Darin Adler]

I will look at those WK1 test results.

[Antti Koivisto]

Comment on attachment 431036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431036&action=review

> Source/WebCore/css/CSSImageValue.cpp:37
> +CSSImageValue::CSSImageValue(URL&& url, URL&& resolvedURL, LoadedFromOpaqueSource loadedFromOpaqueSource)

Looking into this patch I feel baseURL/url pair would result in more understandable code. 

Also maybe the pair should be factored into a struct, something like URLAndBase?  I think there are other places it should be used like CSSFontFaceSrcValue.

> Source/WebCore/css/CSSImageValue.cpp:79
> +URL CSSImageValue::resolvedURL(const Document& document) const
> +{
> +    return document.completeURL(m_resolvedURLString.isNull() ? m_url.string() : m_resolvedURLString);
> +}

Why are we ever calling completeURL on resolved URL?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2269
> +        std::optional<IntPoint> hotSpot;
>          if (auto x = consumeNumberRaw(range)) {
> -            hotSpot.setX(static_cast<int>(*x));
>              auto y = consumeNumberRaw(range);
>              if (!y)
>                  return nullptr;
> -            hotSpot.setY(static_cast<int>(*y));
> -            hotSpotSpecified = true;
> +            // FIXME: Should we clamp or round instead of just casting from double to int?
> +            hotSpot = IntPoint { static_cast<int>(*x), static_cast<int>(*y) };
>          }

I like factoring this sort of code into a lambda.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4703
> +        if (integer && symbol) {
> +            auto pair = CSSValueList::createSpaceSeparated();
> +            pair->append(integer.releaseNonNull());
> +            pair->append(symbol.releaseNonNull());
> +            values->append(WTFMove(pair));
> +        } else if (integer)
> +            values->append(integer.releaseNonNull());
> +        else
> +            values->append(symbol.releaseNonNull());

This too might read better as a lambda

auto symbolToAppend = [&] { ... };

value->append(symbolToAppend());

[Darin Adler]

Comment on attachment 431036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431036&action=review

Thanks for your comments Antti!

>> Source/WebCore/css/CSSImageValue.cpp:37
>> +CSSImageValue::CSSImageValue(URL&& url, URL&& resolvedURL, LoadedFromOpaqueSource loadedFromOpaqueSource)
> 
> Looking into this patch I feel baseURL/url pair would result in more understandable code. 
> 
> Also maybe the pair should be factored into a struct, something like URLAndBase?  I think there are other places it should be used like CSSFontFaceSrcValue.

I did the baseURL/URL pair at first, implemented it that way. I liked it better than what you had suggested.

But that didn’t work, because to replicate the CSSParserContext::completeURL algorithm, besides baseURL, we also need charset and CSSParserMode. And saying this, I now realize that my current patch screws that up, because it breaks the blocking of non-data URLs in VTT parsing since it does not correctly interpret a null resolved URL as "block this URL entirely". I will fix that, but I don’t think I want to pass all that context in. I think I need to stick with resolvedURL.

I am totally willing to factor it into a struct, for use elsewhere, yes absolutely. But didn’t seem important to do that in this patch. It’s already pretty confusing!

>> Source/WebCore/css/CSSImageValue.cpp:79
>> +}
> 
> Why are we ever calling completeURL on resolved URL?

Because sometimes the CSS parser context does not come with a base URL, like styles in a document that doesn’t have a URL. And in those cases the so called "resolved" URL is not fully resolved. If I don’t call completeURL here on already-resolved URLs then we have tests that will fail. I know because I tried it!

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:2269
>>          }
> 
> I like factoring this sort of code into a lambda.

OK, not to my taste (yet), but I will try it your way.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:4703
>> +            values->append(symbol.releaseNonNull());
> 
> This too might read better as a lambda
> 
> auto symbolToAppend = [&] { ... };
> 
> value->append(symbolToAppend());

Will do.

[Darin Adler]

Comment on attachment 431036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431036&action=review

>>> Source/WebCore/css/parser/CSSPropertyParser.cpp:2269
>>>          }
>> 
>> I like factoring this sort of code into a lambda.
> 
> OK, not to my taste (yet), but I will try it your way.

I decided to do everything you suggested (including this).

I tried to turn this code into a lambda in an elegant way, and the hard part was handling the failure case (x but not y). How do I reflect that? Can’t use std::nullopt for that, because that means "neither X nor Y". What would you do in a case like that?

[Darin Adler]

Comment on attachment 431036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431036&action=review

>>> Source/WebCore/css/parser/CSSPropertyParser.cpp:2269
>>>          }
>> 
>> I like factoring this sort of code into a lambda.
> 
> OK, not to my taste (yet), but I will try it your way.

I decided to do everything you suggested (including this).

I tried to turn this code into a lambda in an elegant way, and the hard part was handling the failure case (x but not y). How do I reflect that? Can’t use std::nullopt for that, because that means "neither X nor Y". What would you do in a case like that?

[Radar WebKit Bug Importer]

<rdar://problem/79274857>

[Darin Adler]

Here’s why I am not done yet:

Turns out we have two ways that we depend on CSS values not being copied when we do style resolution:

1) When building a web archive, we collect a list of subresource URLs to figure out what to include in the archive. StyledElement::addSubresourceAttributeURLs does this by calling traverseSubresources on the inline style object, which calls traverseSubresources on all the CSS values. That only worked before because the actual cached resources were pointed-to by the CSS values from the inline style. That’s no longer true with my patch because style resolution creates new CSS values. Not 100% clear how to work around this. Would be nice to be able to get the URLs from the resolved style rather than from the specified style.

2) When deciding whether a stylesheet is "still loading" in StyleSheetContents::subresourcesAllowReuse we use CSS image values to get to cached resources. Same problem as above.

[Darin Adler]

Created attachment 431462 [details]
Patch

[Darin Adler]

Comment on attachment 431462 [details]
Patch

Oops, two more things I need to fix!

[Darin Adler]

Created attachment 431500 [details]
Patch

[Darin Adler]

Finally passing tests and ready for a real review.

[Antti Koivisto]

Comment on attachment 431500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431500&action=review

> Source/WebCore/css/CSSImageValue.h:45
> +// FIXME: Move to another header? Better name? Should we call this DualURL, CSSDualURL, CSSParsedURL, or something else?
> +struct DualLocation {
> +    String specifiedURLString;
> +    String resolvedURLString;
> +};

"DualLocation" is a strange name for this. The type represents a single location after all, in resolved and unresolved form.

The name should have 'URL' in it. CompletedURL would be a good candidate, with this being the return type of completeURL(). Other alternatives: ResolvedURL, StyleURL, ...

Why does this use String instead of URL for the members?

[Darin Adler]

Comment on attachment 431500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431500&action=review

>> Source/WebCore/css/CSSImageValue.h:45
>> +};
> 
> "DualLocation" is a strange name for this. The type represents a single location after all, in resolved and unresolved form.
> 
> The name should have 'URL' in it. CompletedURL would be a good candidate, with this being the return type of completeURL(). Other alternatives: ResolvedURL, StyleURL, ...
> 
> Why does this use String instead of URL for the members?

I’ll go with ResolvedURL for now. I guess I will also change resolvedURLString to be a URL.

This used String because the resolved URL string is mostly used by re-parsing again to resolve against a different base URL, required in cases where the CSS itself was parsed in a context without a base URL. That requires a String and ignores the fact that it’s an already-parsed URL. As I mentioned earlier, font-face src values were storing String and image values were storing URL, and I had to choose one or the other. Storing the various offsets and the “is this valid” flag didn’t seem to offer much value, so I chose the smaller object.

It seems more logical to store a URL object for the resolved URL and just use a string for the specifiedURLString, and there is no big downside to that, but no big upside either. I am a bit torn, but I will change it to what seems logical.

[Antti Koivisto]

> I’ll go with ResolvedURL for now. I guess I will also change
> resolvedURLString to be a URL.

Also the field can be called just "url", "resolved" is in the type name after all.

[Darin Adler]

Committed r279050 (238970@main): <https://commits.webkit.org/238970@main>