Summary: | CSSFontFaceSrcValue.cssText should be quoted consistently with other browsers | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | clopez, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://github.com/web-platform-tests/wpt/pull/30784 | ||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2021-09-14 12:43:04 PDT
Created attachment 438157 [details]
Patch
Created attachment 438158 [details]
Patch
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 Created attachment 438165 [details]
Patch
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. 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. 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. Committed r282415 (241674@main): <https://commits.webkit.org/241674@main> 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. (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!!! 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? (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. Committed r282442 (241695@main): <https://commits.webkit.org/241695@main> |