Bug 230273 – CSSFontFaceSrcValue.cssText should be quoted consistently with other browsers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

CSSFontFaceSrcValue.cssText should be quoted consistently with other browsers

[Myles C. Maxfield]

Created attachment 438157 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/79644124>

[Myles C. Maxfield]

Created attachment 438158 [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

[Myles C. Maxfield]

Created attachment 438165 [details]
Patch

[Darin Adler]

Comment on attachment 438165 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        In a src: line like "src: local(foobar)", Firefox and Chrome will both quote
> +        "foobar" when reading out of the OM. Same thing with "src: url(foobar)".
> +        We should match them.

Should we add tests cases for format strings that have unusual characters in them, like a double quote mark?

Exotic, but we are not attempting to correctly handle such cases it seems.

[Myles C. Maxfield]

Comment on attachment 438165 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        We should match them.
> 
> Should we add tests cases for format strings that have unusual characters in them, like a double quote mark?
> 
> Exotic, but we are not attempting to correctly handle such cases it seems.

This is a good idea. I'll do this.

[Myles C. Maxfield]

Comment on attachment 438165 [details]
Patch

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

>>> Source/WebCore/ChangeLog:11
>>> +        We should match them.
>> 
>> Should we add tests cases for format strings that have unusual characters in them, like a double quote mark?
>> 
>> Exotic, but we are not attempting to correctly handle such cases it seems.
> 
> This is a good idea. I'll do this.

It's a good thing you caught this - this patch is actually wrong. It should be using serializeString() instead.

[Myles C. Maxfield]

Committed r282415 (241674@main): <https://commits.webkit.org/241674@main>

[Darin Adler]

To fix the URL serialization, we need to consider an approach like the one in CSSImageValue, which uses the ResolvedURL structure to work with both the original specified URL string and a resolved URL.

[Myles C. Maxfield]

(In reply to Darin Adler from comment #10)
> To fix the URL serialization, we need to consider an approach like the one
> in CSSImageValue, which uses the ResolvedURL structure to work with both the
> original specified URL string and a resolved URL.

Oh, cool!!!

[Darin Adler]

Comment on attachment 438165 [details]
Patch

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

> LayoutTests/fast/css/font-face-src-parsing-expected.txt:3
>  Valid rules form the stylesheet:

I thin this is a typo and should be "from".

> LayoutTests/fast/css/font-face-src-parsing-expected.txt:19
>  Expected result for valid rules:
>  
>  @font-face { src: url(font.ttf); }

Looks like these are now wrong. Worth updating or maybe just deleting?

[Darin Adler]

(In reply to Myles C. Maxfield from comment #11)
> (In reply to Darin Adler from comment #10)
> > To fix the URL serialization, we need to consider an approach like the one
> > in CSSImageValue, which uses the ResolvedURL structure to work with both the
> > original specified URL string and a resolved URL.
> 
> Oh, cool!!!

The reason I had to do that is that the same object, CSSImageValue, is used in the  CSS object model both for style rules, where it needs to be the original specified URL, and for computed style where it needs to be the complete URL. And yet the completion needs to be based on where it was specified. Currently this class is used both as part of the "truth" for the style rule, and as the interface to query both original and computed style. Some day we might separate those functions, but in the mean time we need this, and the valueWithStylesResolved function, called by Style::BuilderState::resolveImageStyles, has a key role in how this is used for computed style.

[Myles C. Maxfield]

Committed r282442 (241695@main): <https://commits.webkit.org/241695@main>