-
Notifications
You must be signed in to change notification settings - Fork 349
Remove "font" from initiator type value list #1837
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
base: main
Are you sure you want to change the base?
Conversation
I am revising/updating the "initiator type" value list. I may have missed something. But I cannot find anything about the "font" value except here. Chromium code base and WPT test suggest that "font" shouldn't be one of the possible values. I don't know the history. Could experts who has experience with this value provide some advice? ==== below is what I have found. ================== I think the "font" is a possible request destination, which works with a initiator type of "css". And "font" is not a "initiator type" value. In fetch standard, "font" is associated with "CSS' @font-face". See the table right below https://fetch.spec.whatwg.org/#request-initiator-type In the WPT test, it's "css" that's returned for that. There is also a test that shows "font" as a |
Can you restore the pull request template? |
Done. Additionally I edited my comments above. |
Can you also fill out the template appropriately? |
ah I see I should put "N/A, .... " if the item doesn't apply. Done. |
Thanks, getting pretty close. The tests you link to do not appear to test this issue. The first is for scripts, and the second is just a directory, not a specific test. Can you point to the specific test which shows that fetching fonts does not use initiator-type font? Second, there needs to be multiple implementers in support of a feature, not just Chromium. So, for example, multiple implementers passing the tests that you point to would suffice here. |
(This PR is to correct/update the fetch spec, not proposing a new feature. That's why I skipped the templates. The test that shows "font" shouldn't be listed is linked in that comment too.) Can you please read my first comment below the PR description? I hope someone here knows better about the "font" than I do and can advise. Thanks. |
To be clear, the template is required for all normative changes, not only for new features. If there was some documentation that gave you the impression it was for new features only, let us know so that we can fix that documentation. I read that. I'm not any more expert on the font initiator than you are. I'm not sure anyone will be. I'm just trying to help you contribute productively to the specification. To do that, it's much easier for the maintainers if you provide the proper information, in the format they expect it, instead of in a bunch of text. It seems like if you did that, by moving the test you mention into the issue template, then this would be ready for review. |
Thanks. I updated the template. Is it O.K. now? |
You missed this part:
|
The reason I feel the template is for new feature only(or, proposing add/change feature), is that I feel these items are to describe such. If a PR is just to edit the text without changing the spec, or like this PR, which is very unusual, is that I am thinking there may be a mistake in the spec. Then I don't know how to fill the template. That's why I ended up answering "N/A" in most of the cases. "Test" is the item that I actually filled in the first place. But I referred to the whole set of the test about "initiator type", not that particular one, the reason is that, I thought, since I am proposing that "font" doesn't exist, I should refer to the whole set, and say that "font" is not there. So yes I have been confused with this template. I guess I would know better next time :) |
Following your advice I added how multiple browsers do in the test. It's a paragraph describing the situation. is it O.K. now? The test result shows that none of the browsers return "font". (Firefox returns "other" which is a default value, it may not have implemented the "font face" case) |
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 LGTM.
I've checked that https://drafts.csswg.org/css-fonts/#font-fetching-requirements does not reference this. (It doesn't correctly reference "css" either, but, at least deleting this will not leave a dangling reference.)
I think this needs to be reviewed by @noamr. It also seems rather weird to me that we'd use "css" instead of "font". If we ever add fetching of fonts elsewhere that will surely become quite confusing. Also, speaking as editor: just because WebKit passes the test, doesn't mean that WebKit agrees with this change. The same goes for Gecko. We need to actually ask them. cc @smaug---- |
The wpt hints that it was written to match Chromium, not to match any spec. |
But that's the point of initiators... A css style fetching a font would use the However, the |
font
is no longer a valid "initaitor type" value from resource timing (see the next comment); therefore, it's removed.Bug: w3c/resource-timing#364
Chrome/Edge, Firefox, and Safari all implemented "initiator type" feature.
The test result for the test listed below can be found here:
https://wpt.fyi/results/resource-timing/initiator-type/style.html?label=master&label=experimental&aligned&q=style.html
Chrome/Edge and Safari all pass the test, returning "css". Firefox fails the test, returning "other".
In conclusion, none of them returns "font".
Preview | Diff
Preview | Diff