Bug 200092

Summary: CSS Properties and Values API: Relative URLs in properties with "<url>" syntax resolve against the base URL of the stylesheet the property is defined in
Product: WebKit Reporter: Bram <bram>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Minor CC: 247fetishstore, ahmad.saleem792, alexandr.zawgorodniy, annevk, darin, emilio, firefoxic.dev, heycam, hi, koivisto, simon.fraser, twilco.o, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 12   
Hardware: Mac   
OS: macOS 10.14   
See Also: https://bugs.webkit.org/show_bug.cgi?id=198512
Bug Depends on: 230243    
Bug Blocks:    

Bram
Reported 2019-07-24 11:26:27 PDT
Correctly resolve relative URLs for registered CSS custom properties. Relative URLs in registered custom properties must resolve against the base URL of the originating stylesheet. Currently they resolve against the base of the document. For instance, consider a stylesheet at /style/mystyle.css: --foo: url("myimage.jpg"); And a document at /index.html: <style> background-image: var(--foo); </style> If the property --foo is registered with syntax <url>, then the background-image should be /style/myimage.jpg. This is contrary to non-registered properties, in which case the background-image would be /myimage.jpg. This used to be a bug in Chromium too, but it has been fixed: https://bugs.chromium.org/p/chromium/issues/detail?id=851490 I've borrowed the description from that bug report, as it describes the issue clearly.
Attachments
Simon Fraser (smfr)
Comment 1 2019-07-25 16:47:49 PDT
Dup of bug 198512?
Radar WebKit Bug Importer
Comment 2 2019-07-25 16:48:01 PDT
Radar WebKit Bug Importer
Comment 3 2019-07-25 16:48:02 PDT
Darin Adler
Comment 4 2021-08-08 16:18:44 PDT
(In reply to Simon Fraser (smfr) from comment #1) > Dup of bug 198512? No, not the same thing. Confusingly similar. The WPT test that covers this is: css/css-properties-values-api/url-resolution.html
Darin Adler
Comment 5 2021-08-08 16:19:17 PDT
I’ll take a look at this.
Darin Adler
Comment 6 2021-08-08 16:26:02 PDT
Looked over the Chromium fix for this and the approach doesn’t seem quite like what we’d want. The value is expanded from a relative URL to an absolute URL as part of the token expansion process. I think we can find a way to make sure we pass through the proper base when the value is parsed without doing that, but not sure.
Darin Adler
Comment 7 2021-08-08 16:31:02 PDT
One interesting question is what cssText should return for such properties? Should it have the original relative URL or should it have the resolved absolute URL?
Darin Adler
Comment 8 2021-08-08 16:41:26 PDT
I think the key is the places that complete the URL; they need to get the original context for the URL's tokens rather than using the URL of the rule currently being parsed. Those places are: CSSPropertyParserHelpers::consumeImage CSSPropertyParserHelpersWorkerSafe::consumeFontFaceSrcURI These are the only two places where this resolution is done. We need to figure out a way to include the context for these tokens when the variable is expanded. Obviously we don’t want every single CSSParserToken to carry this context, but I think we can find a way to carry it when needed. One way might one to teach CSSParserContext to understand the different context for various stretches of variables? Also unsure if we need to support RawStringAsURL or just URLFunction.
Darin Adler
Comment 9 2021-08-08 16:45:31 PDT
The RawStringAsURL case is for image sets. My guess is that they won’t work correctly in the other browsers, because the var value in that case wouldn’t even need to contain "url()". Definitely didn’t see code that would correctly handle that in the Chromium patch. Not sure: will have to see if I can construct a test case. Not all that familiar with image set syntax.
Darin Adler
Comment 10 2021-08-08 16:57:48 PDT
The behavior that Chromium implemented and that this bug asks for is a nice idea, but is not specified in the current CSS draft. See this: https://drafts.csswg.org/css-variables-1/#syntax That section contains a note that says: For example, relative URLs in CSS are resolved against the base URL of the stylesheet the value appears in. However, if a custom property like --my-image: url(foo.jpg); shows up in an "/a/style.css" stylesheet, it will not resolve into an absolute URL immediately; if that variable is later used in a different "/b/style.css" stylesheet like background: var(--my-image);, it will resolve at that point to "/b/foo.jpg". Given that comment, I am not sure whether to just "do what Chromium does" or follow the CSS specification. Before I start coding we probably need to clear this up. Would like to know if Firefox has also made this change. Maybe someone could test there?
Vadim Makeev
Comment 11 2021-08-08 17:40:55 PDT
> Would like to know if Firefox has also made this change. Maybe someone could test there? It works the same way both in Firefox and Chrome. I’m afraid it’s too late to change that, since there are two independent implementations.
Darin Adler
Comment 12 2021-08-09 07:27:02 PDT
(In reply to Vadim Makeev from comment #11) > I’m afraid it’s too late > to change that, since there are two independent implementations. Not sure that’s right. Why didn’t we change the CSS specification when we started changing the browsers? Someone needs to raise the issue and get the CSS specification fixed.
Darin Adler
Comment 13 2021-08-09 07:27:39 PDT
One of the problems with not having this defined in CSS is that it’s likely messed up for image-set and not even defined what the behavior should be for that case, which has a different syntax.
firefoxic.dev
Comment 14 2021-08-10 09:48:39 PDT
My regards! > Would like to know if Firefox has also made this change. Maybe someone could test there? I have prepared a test based on the behavior described in that note in the specification: https://firefoxic.github.io/test-custom-properties-working-with-url/ > Why didn’t we change the CSS specification when we started changing the browsers? As the test shows, Firefox and Chromium behave as described in the specification, and even expected by web developers. I hope the test will help in resolving the issue. If I missed something, I am ready to correct it promptly.
Darin Adler
Comment 15 2021-08-10 10:15:59 PDT
(In reply to firefoxic.dev from comment #14) > My regards! > > > Would like to know if Firefox has also made this change. Maybe someone could test there? > > I have prepared a test based on the behavior described in that note in the > specification: > https://firefoxic.github.io/test-custom-properties-working-with-url/ Thanks! Is that test really consistent with what the note says? Maybe I am misreading the specification, but I thought the note says it should *not* do that.
firefoxic.dev
Comment 16 2021-08-29 06:33:58 PDT
(In reply to Darin Adler from comment #15) > Is that test really consistent with what the note says? Maybe I am misreading the specification, but I thought the note says it should *not* do that. Many thanks to Ilya Streltsyn, who figured out this issue. He found the confusion in the first message. > If the property --foo is registered with syntax <url>, then the background-image should be /style/myimage.jpg. > This is contrary to non-registered properties, in which case the background-image would be /myimage.jpg. It seems that this is what confused you. After all, the opposite is true: if a custom property is not registered, then the path must be resolved at the place of application, not the property definition because such property is passed as a token; but the registered custom property is passed not as a token, but as an already processed value, in which case the path should be resolved at the place of definition immediately. Since this bug and my test are both not about registered properties, the test is built correctly. Well, a failure of such a variant as “outside”, when a custom property is simply used somewhere separated by a comma and does not contain a path — in any case, it is not correct. Especially considering that the behavior for all “shorthand” variants has already been fixed.
Darin Adler
Comment 17 2021-08-30 13:22:31 PDT
Does WPT contain tests both for the registered custom property case and the unregistered custom property case?
Darin Adler
Comment 18 2021-09-02 09:40:03 PDT
Can someone help me find the text that explains this difference between registered <url> and non-registered properties in the standards documents? That will help me understand precisely how to do this.
Darin Adler
Comment 19 2021-09-02 10:07:53 PDT
Here’s something I haven’t figured out yet that is delaying my work to fix this bug: At the moment now I can’t find any code in WebKit that parses syntax definitions. The only thing I see is a check if the syntax string is literally "*". The WPT test for this, css/css-properties-values-api/url-resolution.html registers a property with the syntax "<url> | none". I don’t know if the URL resolution behavior depends on the property being registered with syntax "<url>" or if, alternatively, any URL encountered as the value of any registered property should be resolved this way, regardless of syntax. Not sure how to construct a test that could tell the difference in this. I suppose this confusion is partly because I am so new to the CSS registered properties feature. Very difficult to understand it! So I don’t yet know if I first have to implement registered property syntax parsing before tackling this bug. Or if it’s OK for now to go on not parsing the registered property syntax, because we can possibly handle all URLs in any registered property this way regardless of syntax. Which seems peculiar but maybe it’s correct. Is it syntax independent? Is "*" a special case for this URL resolution? Pretty sure the WPT tests in the css/css-properties-values-api directory do not cover this since custom properties are all registered with syntax "<url> | none", so there’s no test that can tell how the syntax string affects this.
Darin Adler
Comment 20 2021-09-02 10:09:45 PDT
Also, since I can’t fully understand the specification, I can’t yet use the specification language to resolve this ambiguity. The specification talks about "<url>" but I can’t yet tell if that’s just referring to the type of the value or tokens or talking about the registered syntax.
Emilio Cobos Álvarez (:emilio)
Comment 21 2021-09-03 01:49:55 PDT
(In reply to firefoxic.dev from comment #14) > As the test shows, Firefox and Chromium behave as described in the > specification, and even expected by web developers. I think this is confusing. Firefox doesn't implement the properties-and-values api, so we have no concept of registered properties yet. Or is this not specific to registered properties as the title and other bits imply?
Darin Adler
Comment 22 2021-09-03 15:09:11 PDT
I’d love to work on this next, but right now I’m not clear enough on what the requirements are.
Emilio Cobos Álvarez (:emilio)
Comment 23 2021-09-03 15:15:56 PDT
I think there are two bugs here. One interop issue with regular custom properties where WebKit behaves incorrectly (comment 14), and the different registered custom property behavior which is specific to properties-and-values (comment 0). It's probably worth splitting those up (and I suspect comment 14 is more urgent since iirc WebKit is not shipping properties and values).
Darin Adler
Comment 24 2021-09-03 15:57:59 PDT
I agree. Could someone write up the thing from comment 14? I am particularly interested in where and how the specification calls for this; it seems the implementers on Firefox and Chrome didn’t have any problem understanding it, but it is proving difficult for me. I still think the specification says the opposite of what the browsers have implemented!
Emilio Cobos Álvarez (:emilio)
Comment 25 2021-09-06 02:29:42 PDT
(In reply to Darin Adler from comment #24) > I still think the specification says the > opposite of what the browsers have implemented! How so? In the example of comment 14, the background-image declaration is in ./b/style.css, so background-image: var(--my-image) resolves to ./b/foo.svg, which is the "Success!" image. That's the behavior in comment 10, for unregistered properties. It seems WebKit is using the document base URI rather than the stylesheet's base uri to resolve the image, which is clearly wrong. Now if in Chrome you run this on the console: CSS.registerProperty({ name: "--my-image", syntax: "<url>", inherits: false, initialValue: "url(about:invalid)" }) Then you get two "Wrong folder" images. That's the behavior from comment 0, but that's tangential to comment 14.
Emilio Cobos Álvarez (:emilio)
Comment 26 2021-09-06 02:31:21 PDT
For reference, the way we implement it in Firefox is keeping the stylesheet's url data in the unparsed value: https://searchfox.org/mozilla-central/rev/fdcee847713a33b5e2b4175bb3c72d4aa2215515/servo/components/style/properties/properties.mako.rs#1647-1658
firefoxic.dev
Comment 27 2021-09-07 06:41:45 PDT
You can completely ditch the differences between registered and unregistered custom properties and just take a look at the property: background-image: url("./foo.svg"), linear-gradient(var(--my-color), var(--my-color)); Obviously, the path to the image in this property has absolutely nothing to do with custom properties and must be resolved relative to the style file (i.e. '/b/foo.svg'), but it is resolved (in the latest WebKit) relative to the markup file (i.e. '/ foo.svg '). I absolutely cannot understand how the mention of a custom property (no matter which one) somewhere separated by a comma in another part of the multiple background value affects the explicitly specified path to the image. At the same time, if you replace the longhand with a shorthand: background: url("./foo.svg"), linear-gradient(var(--my-color), var(--my-color)); then everything works as expected (thanks for fixing bug #198512).
Darin Adler
Comment 28 2021-09-07 13:49:43 PDT
Thanks for the help figuring this out. So if I understand correctly, there is a something wrong in WebKit that is *not* the problem originally described on comment #0. This thing is already correct in both Firefox’s engine and Chromium, and everyone except me thinks it’s pretty clearly explained correctly already in the relevant standards. We should make a new separate bug report about that. And I plan to work to fix that bug. But this bug should remain, tracking the issue from comment #0, which, for example, apparently doesn’t even apply yet to Firefox since there is no registration of CSS custom properties in the Firefox engine yet. Does that seem accurate to everyone else?
Darin Adler
Comment 29 2021-09-13 21:37:00 PDT
Wrote a new bug 230243 for the problem I am fixing first.
Darin Adler
Comment 30 2021-09-14 09:06:55 PDT
Sorry I was so slow to understand, I get it now.
Darin Adler
Comment 31 2021-10-05 11:31:25 PDT
Back to the original question: Can someone point me to the specification where this behavior for URLs in registered custom CSS properties is defined?
Darin Adler
Comment 32 2021-10-05 11:59:33 PDT
I think I found it. Substitution of registered custom properties says it must be replaced with an "equivalent token sequence": https://www.w3.org/TR/css-properties-values-api-1/#substitution Those are based on computed values, and computed values for "<url"> are the resolved absolute URL: https://www.w3.org/TR/css-properties-values-api-1/#calculation-of-computed-values So I think the starting point is to make sure that our CSS code tries to use computed values and equivalent token sequences for registered properties.
Darin Adler
Comment 33 2021-10-06 08:18:01 PDT
Note, this bug does not yet affect the Safari web browser. It is a bug in the CSS Custom Values and Properties API, which is a Safari experimental feature, not yet complete, and currently off by default. That is part of what confused me for so long. My test pages weren’t working at all.
Darin Adler
Comment 34 2021-10-06 08:24:54 PDT
This is only a tiny part of what is currently missing from our implementation of CSS Properties and Values API. When someone works on that, they should definitely fix this along with the rest of the specification. Currently we don’t even look at the syntax of custom properties, except for checking if it is "*" or not. Fixing this bug requires checking the syntax for "<url>". I did briefly look at the Chromium patch and it does not check the syntax, so I don’t think it matches what the specification calls for, so I think there’s more than just "fix this bug". I think we need to work on some tests. Now that I know this isn’t just one specific bug, I’m not going to be working on this further right now. I will help whoever ends up working on completing our implementation of CSS Properties and Values API to get this right. If you are that person, please get in touch!
Ahmad Saleem
Comment 35 2023-08-01 05:26:49 PDT
WPT test case for URL Resolution: https://wpt.fyi/results/css/css-properties-values-api/url-resolution.html?label=experimental&label=master&aligned= ^ Safari Technical Preview 175 is passing all tests.
Anne van Kesteren
Comment 36 2023-08-23 01:00:28 PDT
Resolving as per Ahmad's comment.
Note You need to log in before you can comment on or make changes to this bug.