Skip to content
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

[css-syntax-3][css-values-3][css-cascade-4][css-fonts-4] Formalize fetching and resource timing reporting #6715

Merged
merged 14 commits into from Nov 3, 2021

Conversation

noamr
Copy link
Collaborator

@noamr noamr commented Oct 5, 2021

Add a generic method to fetch style resources, and make use of it in
fonts & imports.

[css-syntax-3]: Make sure we have a location when parsing a stylesheet, as the location is necessary for parsing import rules.
[css-values-3]: Add a reusable method to fetch a style-originated
resource.
[css-cascade-4]: Formalize the import algorithm to use the new fetch
method.
[css-fonts-4]: Formalize font loading to use the new fetch method. Removed some of the font loading "guidelines" as they are now baked in to the algorithm.
[css-shapes-2]: Load css-shapes external images/SVG using fetch style resoure

See #562

Fixes #6076

Tests: web-platform-tests/wpt#31344

@noamr
Copy link
Collaborator Author

noamr commented Oct 5, 2021

Open-ish issues:

  • The reusable method has two modes: "cors" and "no-cors". "cors" is used currently only for fonts. When "cors" is used, the referrer is set to the stylesheet's url rather than to the document's origin, and credentials are included only when the request is same-origin. I believe that this should be the general rule for anonymous cors style resources, if there are ever additional ones. But we should look at this carefully, maybe when there is a second use-case for cors style-originated resources.
  • The PR adds a new ref and uses it, does it need to be in separate PRs to work?
  • Requires some new tests, especially for the referrer stuff, which currently has different behaviors across browsers.
  • Initial loading of the stylesheet (from link or CSSOM) is not handled in this PR, only style-originated resources.
  • Not yet integrated with [css-images] and with SVG for [css-shapes] (any resource that I've missed?).

Copy link
Member

@tabatkins tabatkins left a comment

Choose a reason for hiding this comment

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

In addition to the individual comments nits, most of which can be fixed by accepting the provided suggestions, there's some style nits where your additions don't match the surrounding spec and would need to be reformatted at some point by us.

CSSWG style is to give the algo steps their actual numbers when using Markdown numbered lists, and most importantly, we use semantic linebreaks, breaking at sentence/phrase/clause boundaries (usually after commas or before conjunctions) as feels appropriate for meaningful units of text that aren't overly long.

Otherwise looks great, thanks so much for filling in details that I was gradually loading into my head on my own. Would you mind also doing the CSSOM "load a stylesheet" algo? (In this PR or separate, doesn't matter.)

css-cascade-4/Overview.bs Outdated Show resolved Hide resolved
css-syntax-3/Overview.bs Outdated Show resolved Hide resolved
css-syntax-3/Overview.bs Outdated Show resolved Hide resolved
css-values-4/Overview.bs Outdated Show resolved Hide resolved
css-cascade-4/Overview.bs Outdated Show resolved Hide resolved
css-fonts-4/Overview.bs Outdated Show resolved Hide resolved
@tabatkins
Copy link
Member

Not yet integrated with [css-images] and with SVG for [css-shapes] (any resource that I've missed?).

I think Colors 4, for @color-profile, is the only other place we load resources currently.

@noamr
Copy link
Collaborator Author

noamr commented Oct 6, 2021

In addition to the individual comments nits, most of which can be fixed by accepting the provided suggestions, there's some style nits where your additions don't match the surrounding spec and would need to be reformatted at some point by us.

CSSWG style is to give the algo steps their actual numbers when using Markdown numbered lists, and most importantly, we use semantic linebreaks, breaking at sentence/phrase/clause boundaries (usually after commas or before conjunctions) as feels appropriate for meaningful units of text that aren't overly long.

OK I fixed that as well. Let me know if anything else is missing.

Would you mind also doing the CSSOM "load a stylesheet" algo? (In this PR or separate, doesn't matter.)

I will try to find the time to look at this separately, and it probably wouldn't use the "fetch a style resource" algorithm as stylesheets loaded like that are not exactly style resources. Seems like that algorithm is only used internally in CSSOM, for things like XML stylesheets and link headers. I wouldn't use it for HTML, as it would require too much information about link element specifics.

@svgeesus
Copy link
Contributor

svgeesus commented Oct 6, 2021

@noamr could you please link your W3C account to your GitHub account? Our IPR bot doesn't know who you are, otherwise. Thanks!

@svgeesus
Copy link
Contributor

svgeesus commented Oct 6, 2021

I think Colors 4, for @color-profile, is the only other place we load resources currently.

Yes, 10.3. Specifying a color profile: the @color-profile at-rule which should point to the new, centrally-defined "how CSS does fetch" stuff.

Thanks for doing all this, @noamr much appreciated.

css-fonts-4/Overview.bs Show resolved Hide resolved
css-fonts-4/Overview.bs Outdated Show resolved Hide resolved
@noamr
Copy link
Collaborator Author

noamr commented Oct 6, 2021

@noamr could you please link your W3C account to your GitHub account? Our IPR bot doesn't know who you are, otherwise. Thanks!

I just logged in to my w3c account (noamr) and it's connected. I think I'm not in the repository's group though, there was some similar issue when I contributed to css-shapes a few months ago.

@noamr
Copy link
Collaborator Author

noamr commented Oct 11, 2021

@noamr could you please link your W3C account to your GitHub account? Our IPR bot doesn't know who you are, otherwise. Thanks!

I just logged in to my w3c account (noamr) and it's connected. I think I'm not in the repository's group though, there was some similar issue when I contributed to css-shapes a few months ago.

@svgeesus anything we can do about this? Not sure what's missing IPR-wise

noamr and others added 9 commits October 11, 2021 10:09
Add a generic method to fetch style resources, and make use of it in
fonts & imports.

[css-syntax-3]: Make sure we have a location when parsing a stylesheet.
[css-values-3]: Add a reusable method to fetch a style-originated
resource.
[css-cascade-4]: Formalize the import algorithm to use the new fetch
method.
[css=fonts-4]: Formalize font loading to use the new fetch method.
Co-authored-by: Tab Atkins Jr. <jackalmage@gmail.com>
Co-authored-by: Tab Atkins Jr. <jackalmage@gmail.com>
Co-authored-by: Tab Atkins Jr. <jackalmage@gmail.com>
Co-authored-by: Tab Atkins Jr. <jackalmage@gmail.com>
@noamr
Copy link
Collaborator Author

noamr commented Oct 11, 2021

Added support for CSS shapes, still missing css-images and color profiles.

@svgeesus
Copy link
Contributor

@svgeesus anything we can do about this? Not sure what's missing IPR-wise

Yes, I just sent you an email with a non-participant commitment. Actually I will send two, one now for the spec with the largest change and then (once you are done patching all our fetch holes) another listing all the other specs that were changed, that says "and the same for these".

@noamr
Copy link
Collaborator Author

noamr commented Oct 12, 2021

@svgeesus anything we can do about this? Not sure what's missing IPR-wise

Yes, I just sent you an email with a non-participant commitment. Actually I will send two, one now for the spec with the largest change and then (once you are done patching all our fetch holes) another listing all the other specs that were changed, that says "and the same for these".

So, you suggest that I finish the remaining FETCH holes as part of this PR and do an IPR thing for all these specs together?
Only css-color and css-images are now missing (CSSOM stylesheet loading will be separate).

@svgeesus
Copy link
Contributor

Yes, that would be perfect.

is |environmentSettings|'s [=environment settings object/origin=],
[=request/credentials mode=] is "same-origin", [=request/use-url-credentials flag=] is
set, and whose [=request/header list=] is a [=/header list=] containing a [=header=]
with its [=header/name=] set to "referrer" and its [=header/value=] set to |referrer|.
Copy link
Member

Choose a reason for hiding this comment

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

This is somewhat wrong. You want to set the referrer field on the request. That will set the Referer (note, one r less) header. But good that it sets it explicitly though as CSS cannot take it from the global.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, fixed.

@noamr
Copy link
Collaborator Author

noamr commented Oct 12, 2021

Yes, that would be perfect.

That's all done now. No additional specs planned for this PR :)

@svgeesus
Copy link
Contributor

@tabatkins have your requested changes been made?

@noamr
Copy link
Collaborator Author

noamr commented Oct 12, 2021

@tabatkins have your requested changes been made?

I think I made lots of changes since. It would be good to re-review.
Also, I put color-profile fetches as no-cors, which is an assumption as it was not specified so far.

@domenic
Copy link
Collaborator

domenic commented Oct 12, 2021

Also, I put color-profile fetches as no-cors, which is an assumption as it was not specified so far.

As mentioned in whatwg/fetch#1324, it'd be best if they used CORS, like all other modern web platform features (w3ctag/design-principles#171 (comment))

@noamr
Copy link
Collaborator Author

noamr commented Oct 18, 2021

@tabatkins have your requested changes been made?

Reminder to take another look when possible, @tabatkins :)

@noamr
Copy link
Collaborator Author

noamr commented Nov 3, 2021

Ping?

@svgeesus svgeesus dismissed tabatkins’s stale review November 3, 2021 18:06

Timed out, better to merge now and fix any nits later

@svgeesus svgeesus merged commit 53ead22 into w3c:main Nov 3, 2021
@fantasai
Copy link
Collaborator

fantasai commented Nov 9, 2021

@svgeesus Can you rebase PRs before you merge, please? Unless it's a complicated conflicty thing that really requires a merge commit, history is a lot cleaner with a rebase. (There's a drop-down button next to the Merge button in GH that let's you choose whether to rebase or not. I'm pretty sure I've mentioned this before...)

@fantasai
Copy link
Collaborator

fantasai commented Nov 9, 2021

@noamr If it's not too much trouble, would you mind adding yourself to the acknowledgements section of at least some of these specs? Your call on which ones, and I'm happy to merge that PR. But I think you deserve some credit for sorting this out. :)

@noamr
Copy link
Collaborator Author

noamr commented Nov 9, 2021

@svgeesus Can you rebase PRs before you merge, please? Unless it's a complicated conflicty thing that really requires a merge commit, history is a lot cleaner with a rebase. (There's a drop-down button next to the Merge button in GH that let's you choose whether to rebase or not. I'm pretty sure I've mentioned this before...)

I think the Github settings allow to change the default to "Squash and merge", wouldn't that be preferable?

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed URLs vs Fetch.

The full IRC log of that discussion <fantasai> Topic: URLs vs Fetch
<fantasai> github: https://github.com//pull/6715/files
<dbaron> github: github: https://github.com//pull/6715
<drott> fantasai: areas where we need to better integarte with the fetch spec
<dbaron> github: https://github.com//pull/6715
<drott> fantasai: chris recently merged PR contributed by contributor
<drott> fantasai: we need those to be reviewed - and ask to publish these changes
<chris> rrsagent, here
<RRSAgent> See https://www.w3.org/2021/11/10-css-irc#T17-04-45
<drott> ikilpatrick: did these get reviewed by anna or nicole?
<drott> tab: I did review it
<drott> tab: anna and domenic coordinating on reviewing...
<chris> Yeah I reviewed it too
<fantasai> s/anna/anne/
<drott> fantasai: if it sounds like it's been reviewed, then I'd suggest to accept the changes
<dbaron> s/nicole/Dominic Denicola/
<drott> > s/nicole/Dominic Denicola/ - sorry about, that, of course
<fantasai> i/github: github:/scribenick: drott/
<fantasai> scribenick: fantasai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[css-fonts-4] “Font fetching requirements” doesn’t fully specify necessary requirements
7 participants