Skip to content

[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

Merged
merged 4 commits into from
Jun 3, 2025

Conversation

noamr
Copy link
Collaborator

@noamr noamr commented Jun 2, 2025

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 #12065
Closes #12068
Closes #12086
Closes #12147

…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
@noamr noamr requested a review from tabatkins June 2, 2025 19:39
@noamr
Copy link
Collaborator Author

noamr commented Jun 2, 2025

/cc @AtkinsSJ @weinig, please check that this change is sane :)

@noamr noamr changed the title [css-vlaues-4] [css-cascade-4] [css-color-5] [css-fonts-4] [css-images-4] [css-shapes-2] Clean up fetching Jun 2, 2025
@noamr noamr added the css-values-4 Current Work label Jun 2, 2025
Copy link
Contributor

@svgeesus svgeesus left a 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

Copy link
Contributor

@AtkinsSJ AtkinsSJ left a 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!

@noamr
Copy link
Collaborator Author

noamr commented Jun 3, 2025

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.

Copy link
Contributor

@AtkinsSJ AtkinsSJ left a 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. :^)

@noamr noamr merged commit 034a433 into w3c:main Jun 3, 2025
1 check passed
@noamr noamr deleted the fix-fetching branch June 3, 2025 13:03
@AtkinsSJ
Copy link
Contributor

AtkinsSJ commented Jun 5, 2025

Arg, I missed a thing until I now tried to implement this! 😅

I think this:

given an CSS rule or a css declaration cssRuleOrDeclaration

...is referring to the wrong definition of "CSS declaration". style resource base URL checks for CSSStyleDeclaration and that seems to be the one we want. The naming of these is unhelpful.

@noamr
Copy link
Collaborator Author

noamr commented Jun 5, 2025

Arg, I missed a thing until I now tried to implement this! 😅

I think this:

given an CSS rule or a css declaration cssRuleOrDeclaration

...is referring to the wrong definition of "CSS declaration". style resource base URL checks for CSSStyleDeclaration and that seems to be the one we want. The naming of these is unhelpful.

Ah yes it needs to be a CSS declaration block.


<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|:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"a a"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed

AtkinsSJ added a commit to AtkinsSJ/ladybird that referenced this pull request Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-values-4 Current Work
3 participants