-
Notifications
You must be signed in to change notification settings - Fork 728
[css-values-4] [css-cascade-4] [css-color-5] [css-fonts-4] [css-images-4] [css-shapes-2] Clean up fetching #12261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…s-4] [css-shapes-2] Clean up fetching This fixes a few cases around fetching external URLs for style resources: - The base URL is the sheet's base URL if exists, otherwise the sheet's location, otherwise the document base URL. - The algorithm can take either a rule or declaration block, and derive the correct stylesheet and base URL from that. Updated the calling sites to take that onto account. - Removed redundant URL parsing in "@import". Closes w3c#12065 Closes w3c#12068 Closes w3c#12086 Closes w3c#12147
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but others should review in detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks pretty solid to me.
One thing that I've been alerted to this morning, (see LadybirdBrowser/ladybird#4972) that's related to this, is that the fetch algorithms operate on relative URLs (and expect them to not be resolved yet) but the values spec requires that the computed value of an URL is absolute:
When a appears in the computed value of a property, it is resolved to an absolute URL, as described in the preceding paragraph. The computed value of a URL that the UA cannot resolve to an absolute URL is the specified value.
https://drafts.csswg.org/css-values-4/#relative-urls
That suggests that either we're absolutizing URLs in two different places with potentially different rules, or that the absolutization in fetch is redundant as the URL will already be absolute. (Or I'm missing something.) Sorry for not catching this earlier!
Good spotting, thanks to both you and to the Ladybird project for keeping the specs honest! I've extracted out the "resolve to absolute URL" bit, and referred to it from https://drafts.csswg.org/css-values-4/#relative-urls. IO think it should make a lot more sense now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One typo nit, otherwise I think this is good! Thank you. :^)
Arg, I missed a thing until I now tried to implement this! 😅 I think this:
...is referring to the wrong definition of "CSS declaration". style resource base URL checks for |
Ah yes it needs to be a |
|
||
<div algorithm="resolve a style resource URL"> | ||
To <dfn>resolve a style resource URL</dfn> from a [=/url=] or <<url>> |urlValue|, | ||
and a a [=CSS rule=] or a [=CSS declaration=] |cssRuleOrDeclaration|: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"a a"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed
This fixes a few cases around fetching external URLs for style resources:
Closes #12065
Closes #12068
Closes #12086
Closes #12147