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-fonts] should supported tech() be validated during parsing or loading? #8793

Open
vitorroriz opened this issue May 4, 2023 · 10 comments
Labels
css-fonts-4 Current Work

Comments

@vitorroriz
Copy link

The way I interpreted @font-face src tech() from WPT tests, we should parse all valid keywords for tech() and then, at loading, check if the parsed tech is supported or not. That’s what WPT expects, for example, here, otherwise a browser not supporting color-colrv1, for example, would fail some subtests.

However, spec says:

If a component value is parsed correctly and is of a font format or font tech that the UA supports, add it to the list of supported sources. If parsing a component value results in a parsing error or its format or tech are unsupported, do not add it to the list of supported sources.
If there are no supported entries at the end of this process, the value for the src descriptor is a parse error.

For unsupported tech to be a parsing error, we would need to validate for supported tech already during parsing, but then a browser not supporting, for example, color-colrv1 would fail the mentioned WPT subtest.

@svgeesus svgeesus added the css-fonts-4 Current Work label May 13, 2023
@cdoublev
Copy link
Collaborator

cdoublev commented May 13, 2023

I think I am asking a similar question in #8795. A browser not supporting a property will fail a WPT subtest that uses this property in a container or feature query, if <declaration> does not match any syntactically valid declaration.

edit: actually the WPT will succeed with a match against <general-enclosed>

@jfkthame
Copy link
Contributor

I think it's wrong for unsupported font formats or technologies in the src descriptor to potentially result in a parse error (if there's no alternative, supported source in the list). In Selecting items in the src, we're told to

skip downloading a font resource if the format hint indicates an unsupported or unknown font format, or if any of the font technologies are unsupported by the user agent

which implies that resources with unsupported formats/technologies are nevertheless parsed successfully and become part of the list; they just won't get selected for download and use.

See also #9054 (comment).

@vitorroriz
Copy link
Author

vitorroriz commented Jul 11, 2023

Yeah, that does make sense, but it seems to be conflicting with the other part of the spec at https://www.w3.org/TR/css-fonts-4/#font-face-src-parsing that says:

If a component value is parsed correctly and is of a font format or font tech that the UA supports, add it to the list of supported sources. If parsing a component value results in a parsing error or its format or tech are unsupported, do not add it to the list of supported sources.

If there are no supported entries at the end of this process, the value for the src descriptor is a parse error.

This should be probably discussed by the CSSWG.

@jfkthame
Copy link
Contributor

I agree it conflicts with the text about parsing. Tagged this for the CSSWG agenda in the hope we can get it clarified.

@svgeesus
Copy link
Contributor

Does it though? It seems like a queue of operations:

  1. does it parse
  2. is it supported
  3. can you fetch it
  4. use it

So it is easily possible to have something that does parse and is not supported. Which means you don't try to fetch it.

I must be missing something here, this seems straightforward.

@jfkthame
Copy link
Contributor

It seems like a queue of operations:

  1. does it parse
  2. is it supported
  3. can you fetch it
  4. use it

That seems straightforward enough, yes.

But the issue is that the current spec text:

If parsing a component value results in a parsing error or its format or tech are unsupported, do not add it to the list of supported sources.

If there are no supported entries at the end of this process, the value for the src descriptor is a parse error.

says (as I read it) that if the entries in the src descriptor are parsed successfully, but not supported by the UA, this results in a parse error. It shouldn't; the descriptor was parsed fine, it just resulted in a value that is not supported and therefore should not be fetched.

@jfkthame
Copy link
Contributor

jfkthame commented Jul 11, 2023

So ISTM that the quoted spec fragment should be more like

If parsing a component value results in a parsing error, do not add it to the list of sources.

If there are no valid entries at the end of this process, the value for the src descriptor is a parse error.

(with no reference to whether the format or tech are supported, which is irrelevant to parsing; these are considered during the selecting items in the src process.)

@svgeesus
Copy link
Contributor

OK so we are going from building a list of supported sources, to building a list of parsed sources (some of which may be unsupported).

Current spec text:

If a component value is parsed correctly and is of a font format or font tech that the UA supports, add it to the list of supported sources. If parsing a component value results in a parsing error or its format or tech are unsupported, do not add it to the list of supported sources.

If there are no supported entries at the end of this process, the value for the src descriptor is a parse error.

and we want

If a component value is parsed correctly, add it to the list of sources. If parsing a component value results in a parsing error , do not add it to the list of sources.

If there are no entries at the end of this process, the value for the src descriptor is a parse error.

and the supported part is already covered

External references consist of a URL, followed by an optional hint describing the format of the font resource referenced by that URL. Conformant user agents must skip downloading a font resource if the format hint indicates an unsupported or unknown font format, or if any of the font technologies are unsupported by the user agent. If no format hint is supplied, the user agent should download the font resource.

@svgeesus
Copy link
Contributor

although we could usefully linkify formats and technologies

webkit-commit-queue pushed a commit to vitorroriz/WebKit that referenced this issue Jul 12, 2023
https://bugs.webkit.org/show_bug.cgi?id=259145
rdar://112135896

Reviewed by Myles C. Maxfield.

A valid tech() keyword argument should be parsed even if the represented
techonology is not supported by the engine. The engine should reject loading
the font for unsupported techonologies but this should happen only at
loading time and not at parsing time.

The reason why we were rejecting it before at parsing time it is for some
inconsistency in different parts of the spec which suggested that. That
will be fixed soon by the CWWG, see: w3c/csswg-drafts#8793

* LayoutTests/imported/w3c/web-platform-tests/css/css-fonts/parsing/font-face-src-tech-expected.txt:
* Source/WebCore/css/CSSFontFaceSrcValue.cpp:
(WebCore::CSSFontFaceSrcResourceValue::fontLoadRequest):
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeFontTech):
* Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:
(WebCore::CSSPropertyParserHelpersWorkerSafe::consumeFontFaceSrcURI):
* Source/WebCore/css/parser/CSSSupportsParser.cpp:
(WebCore::CSSSupportsParser::consumeSupportsFontTechFunction):

Canonical link: https://commits.webkit.org/265999@main
webkit-commit-queue pushed a commit to vitorroriz/WebKit that referenced this issue Jul 13, 2023
https://bugs.webkit.org/show_bug.cgi?id=259144
rdar://112135869

Reviewed by Tim Nguyen.

A valid format() keyword (ident) argument should be parsed even if the represented
format is not supported by the engine. The engine should reject loading
the font for unsupported formats but this should happen only at
loading time and not at parsing time.

The reason why we were rejecting it before, at parsing time, is for some
inconsistency in different parts of the spec. That
will be fixed soon by the CSSWG, see: w3c/csswg-drafts#8793

* LayoutTests/imported/w3c/web-platform-tests/css/css-fonts/parsing/font-face-src-format-expected.txt:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::isFontFormatKeywordValid):
(WebCore::CSSPropertyParserHelpers::consumeFontFormat):
* Source/WebCore/css/parser/CSSSupportsParser.cpp:
(WebCore::CSSSupportsParser::consumeSupportsFontFormatFunction):

Canonical link: https://commits.webkit.org/266043@main
mnutt pushed a commit to movableink/webkit that referenced this issue Jul 18, 2023
https://bugs.webkit.org/show_bug.cgi?id=259145
rdar://112135896

Reviewed by Myles C. Maxfield.

A valid tech() keyword argument should be parsed even if the represented
techonology is not supported by the engine. The engine should reject loading
the font for unsupported techonologies but this should happen only at
loading time and not at parsing time.

The reason why we were rejecting it before at parsing time it is for some
inconsistency in different parts of the spec which suggested that. That
will be fixed soon by the CWWG, see: w3c/csswg-drafts#8793

* LayoutTests/imported/w3c/web-platform-tests/css/css-fonts/parsing/font-face-src-tech-expected.txt:
* Source/WebCore/css/CSSFontFaceSrcValue.cpp:
(WebCore::CSSFontFaceSrcResourceValue::fontLoadRequest):
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeFontTech):
* Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:
(WebCore::CSSPropertyParserHelpersWorkerSafe::consumeFontFaceSrcURI):
* Source/WebCore/css/parser/CSSSupportsParser.cpp:
(WebCore::CSSSupportsParser::consumeSupportsFontTechFunction):

Canonical link: https://commits.webkit.org/265999@main
mnutt pushed a commit to movableink/webkit that referenced this issue Jul 18, 2023
https://bugs.webkit.org/show_bug.cgi?id=259144
rdar://112135869

Reviewed by Tim Nguyen.

A valid format() keyword (ident) argument should be parsed even if the represented
format is not supported by the engine. The engine should reject loading
the font for unsupported formats but this should happen only at
loading time and not at parsing time.

The reason why we were rejecting it before, at parsing time, is for some
inconsistency in different parts of the spec. That
will be fixed soon by the CSSWG, see: w3c/csswg-drafts#8793

* LayoutTests/imported/w3c/web-platform-tests/css/css-fonts/parsing/font-face-src-format-expected.txt:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::isFontFormatKeywordValid):
(WebCore::CSSPropertyParserHelpers::consumeFontFormat):
* Source/WebCore/css/parser/CSSSupportsParser.cpp:
(WebCore::CSSSupportsParser::consumeSupportsFontFormatFunction):

Canonical link: https://commits.webkit.org/266043@main
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed tech() validation, and agreed to the following:

  • RESOLVED: Make edits outlined by Chris
The full IRC log of that discussion <fantasai> Topic: tech() validation
<fantasai> github: https://github.com//issues/8793
<fantasai> jfkthame: Current spec text is a bit confusing
<fantasai> jfkthame: question is, if the font technology or font format that is declared for a font resource is not supported, how does this get processed
<fantasai> jfkthame: one part says if not supported, then that source is dropped from list of available sources
<fantasai> jfkthame: if no sources left, that's a parse error
<fantasai> jfkthame: another part of spec says that the format and tech are evaluated at the time of deciding what resources to load
<fantasai> jfkthame: so skip if don't support them
<fantasai> jfkthame: but it's not a CSS parse error
<fantasai> jfkthame: general agreement among those in discussion
<fantasai> jfkthame: is that unsupported tech or formats shouldn't be a parse error, it's still valid, you just don't fetch that resource
<fantasai> jfkthame: currently that's not what a strict reading of the spec says
<fantasai> jfkthame: if we agree it's not a parse error, then we need to fix the spec
<TabAtkins> +1, spec is simply written a little wrong, easy fix
<fantasai> +1
<vitorroriz> (I'm not on the call due to a meeting but +1)
<fantasai> astearns: proposed resolution is to remove bit about it being a parse error?
<fantasai> jfkthame: yes, looking at Chris's comment from July 11th
<fantasai> https://github.com//issues/8793#issuecomment-1631228711
<fantasai> jfkthame: Proposed edits in that comment makes sense to me
<fantasai> RESOLVED: Make edits outlined by Chris

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-fonts-4 Current Work
Projects
None yet
Development

No branches or pull requests

5 participants