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] Empty quoted font family name should not parse as valid #4510

Closed
drott opened this issue Nov 14, 2019 · 7 comments
Closed

[css-fonts] Empty quoted font family name should not parse as valid #4510

drott opened this issue Nov 14, 2019 · 7 comments
Labels
Closed as Question Answered Used when the issue is more of a question than a problem, and it's been answered. css-fonts-4 Current Work

Comments

@drott
Copy link
Collaborator

drott commented Nov 14, 2019

In a recent Chrome issue we discovered that a style definition such as:
font: 1px "";
would parse as valid according to the spec prose.

Similarly, a JS font loading call such as:
document.fonts.load("1px \"\"")
leads to a promise with status resolved / fulfilled in Safari / Firefox, even though to me that does not make sense, as a font with an empty family name cannot be reasonably matched.

I suggest to add normative text in section 3.1 below the following paragraph:

Font family names other than generic families must either be given quoted as strings, or unquoted as a sequence of one or more identifiers. This means most punctuation characters and digits at the start of each token must be escaped in unquoted font family names.

similar to

The empty quoted string: "" is an invalid font family name.

Then we can add to the examples:

font-family: "", sans-serif;
font-family: "";

Perhaps we need to add something as well to describe that:
font-family: ,,,;
also should not parse as valid, i.e. a sequence of null identifiers separated by commas.

@drott
Copy link
Collaborator Author

drott commented Nov 14, 2019

CC @tabatkins @kojiishi @lilles

@lilles
Copy link
Member

lilles commented Nov 14, 2019

Why can't we just handle the empty string the same way we handle a non-empty string that does not map to a known font?

I think "font-family: ,,,;" is already covered by the current syntax in the spec? There is no such thing as a null identifier?

@drott
Copy link
Collaborator Author

drott commented Nov 14, 2019

I find it inefficient to let an empty string "" travel up to font matching if it can be rejected at the parsing stage. Also, fulfilling a promise document.fonts.load("1px \"\"") is hiding a potential bug in a page's JS code if for example a variable for font family wasn't set. In my opinion, resolving this promise successfully is semantically misleading, as no font will have been matched or loaded for the empty family name.

@drott
Copy link
Collaborator Author

drott commented Nov 14, 2019

CC @jfkthame @litherum

@jfkthame
Copy link
Contributor

I don't see that

 document.fonts.load("1px \"\"")

is any more "misleading" than

 document.fonts.load("1px 'Some random font name that does not actually exist'")

which also resolves to a fulfilled promise. In either case, no actual font loading takes place because the given name doesn't match any available font.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 14, 2019
When document.fonts.load() is called with font specification such as
'1px ""' (containing two quotes) as the argument, CSS font property
parsing parses this successfully as the empty string. FontFaceCache
was not prepared to handle that correctly.

Fix FontFaceCache to guard against that. Add test to actually
ensure that this font specification's load promise resolves
successfully.

In parallel, raised
w3c/csswg-drafts#4510
in CSS WG to discuss whether it would make sense to reject such
a font specification at the parsing stage already.

Bug: 1023206
Change-Id: If46aef0c7fcd2e06bc657480b9189857438d8cc1
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 14, 2019
When document.fonts.load() is called with font specification such as
'1px ""' (containing two quotes) as the argument, CSS font property
parsing parses this successfully as the empty string. FontFaceCache
was not prepared to handle that correctly.

Fix FontFaceCache to guard against that. Add test to actually
ensure that this font specification's load promise resolves
successfully.

In parallel, raised
w3c/csswg-drafts#4510
in CSS WG to discuss whether it would make sense to reject such
a font specification at the parsing stage already.

Bug: 1023206
Change-Id: If46aef0c7fcd2e06bc657480b9189857438d8cc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1914392
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715261}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 14, 2019
When document.fonts.load() is called with font specification such as
'1px ""' (containing two quotes) as the argument, CSS font property
parsing parses this successfully as the empty string. FontFaceCache
was not prepared to handle that correctly.

Fix FontFaceCache to guard against that. Add test to actually
ensure that this font specification's load promise resolves
successfully.

In parallel, raised
w3c/csswg-drafts#4510
in CSS WG to discuss whether it would make sense to reject such
a font specification at the parsing stage already.

Bug: 1023206
Change-Id: If46aef0c7fcd2e06bc657480b9189857438d8cc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1914392
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715261}
@svgeesus svgeesus added the css-fonts-4 Current Work label Nov 15, 2019
@litherum
Copy link
Contributor

@jfkthame is right.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 29, 2019
…FaceCache, a=testonly

Automatic update from web-platform-tests
Avoid looking up empty font name in FontFaceCache

When document.fonts.load() is called with font specification such as
'1px ""' (containing two quotes) as the argument, CSS font property
parsing parses this successfully as the empty string. FontFaceCache
was not prepared to handle that correctly.

Fix FontFaceCache to guard against that. Add test to actually
ensure that this font specification's load promise resolves
successfully.

In parallel, raised
w3c/csswg-drafts#4510
in CSS WG to discuss whether it would make sense to reject such
a font specification at the parsing stage already.

Bug: 1023206
Change-Id: If46aef0c7fcd2e06bc657480b9189857438d8cc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1914392
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715261}

--

wpt-commits: 67da4dd8159574619c10b26f527f46f9172e805b
wpt-pr: 20237
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Nov 29, 2019
…FaceCache, a=testonly

Automatic update from web-platform-tests
Avoid looking up empty font name in FontFaceCache

When document.fonts.load() is called with font specification such as
'1px ""' (containing two quotes) as the argument, CSS font property
parsing parses this successfully as the empty string. FontFaceCache
was not prepared to handle that correctly.

Fix FontFaceCache to guard against that. Add test to actually
ensure that this font specification's load promise resolves
successfully.

In parallel, raised
w3c/csswg-drafts#4510
in CSS WG to discuss whether it would make sense to reject such
a font specification at the parsing stage already.

Bug: 1023206
Change-Id: If46aef0c7fcd2e06bc657480b9189857438d8cc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1914392
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715261}

--

wpt-commits: 67da4dd8159574619c10b26f527f46f9172e805b
wpt-pr: 20237
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Nov 30, 2019
…FaceCache, a=testonly

Automatic update from web-platform-tests
Avoid looking up empty font name in FontFaceCache

When document.fonts.load() is called with font specification such as
'1px ""' (containing two quotes) as the argument, CSS font property
parsing parses this successfully as the empty string. FontFaceCache
was not prepared to handle that correctly.

Fix FontFaceCache to guard against that. Add test to actually
ensure that this font specification's load promise resolves
successfully.

In parallel, raised
w3c/csswg-drafts#4510
in CSS WG to discuss whether it would make sense to reject such
a font specification at the parsing stage already.

Bug: 1023206
Change-Id: If46aef0c7fcd2e06bc657480b9189857438d8cc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1914392
Reviewed-by: Rune Lillesveen <futharkchromium.org>
Reviewed-by: Koji Ishii <kojiichromium.org>
Commit-Queue: Dominik Röttsches <drottchromium.org>
Cr-Commit-Position: refs/heads/master{#715261}

--

wpt-commits: 67da4dd8159574619c10b26f527f46f9172e805b
wpt-pr: 20237

UltraBlame original commit: 1abac1b710f443dc4c8f8e009288629e34a3db91
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Nov 30, 2019
…FaceCache, a=testonly

Automatic update from web-platform-tests
Avoid looking up empty font name in FontFaceCache

When document.fonts.load() is called with font specification such as
'1px ""' (containing two quotes) as the argument, CSS font property
parsing parses this successfully as the empty string. FontFaceCache
was not prepared to handle that correctly.

Fix FontFaceCache to guard against that. Add test to actually
ensure that this font specification's load promise resolves
successfully.

In parallel, raised
w3c/csswg-drafts#4510
in CSS WG to discuss whether it would make sense to reject such
a font specification at the parsing stage already.

Bug: 1023206
Change-Id: If46aef0c7fcd2e06bc657480b9189857438d8cc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1914392
Reviewed-by: Rune Lillesveen <futharkchromium.org>
Reviewed-by: Koji Ishii <kojiichromium.org>
Commit-Queue: Dominik Röttsches <drottchromium.org>
Cr-Commit-Position: refs/heads/master{#715261}

--

wpt-commits: 67da4dd8159574619c10b26f527f46f9172e805b
wpt-pr: 20237

UltraBlame original commit: 1abac1b710f443dc4c8f8e009288629e34a3db91
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Nov 30, 2019
…FaceCache, a=testonly

Automatic update from web-platform-tests
Avoid looking up empty font name in FontFaceCache

When document.fonts.load() is called with font specification such as
'1px ""' (containing two quotes) as the argument, CSS font property
parsing parses this successfully as the empty string. FontFaceCache
was not prepared to handle that correctly.

Fix FontFaceCache to guard against that. Add test to actually
ensure that this font specification's load promise resolves
successfully.

In parallel, raised
w3c/csswg-drafts#4510
in CSS WG to discuss whether it would make sense to reject such
a font specification at the parsing stage already.

Bug: 1023206
Change-Id: If46aef0c7fcd2e06bc657480b9189857438d8cc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1914392
Reviewed-by: Rune Lillesveen <futharkchromium.org>
Reviewed-by: Koji Ishii <kojiichromium.org>
Commit-Queue: Dominik Röttsches <drottchromium.org>
Cr-Commit-Position: refs/heads/master{#715261}

--

wpt-commits: 67da4dd8159574619c10b26f527f46f9172e805b
wpt-pr: 20237

UltraBlame original commit: 1abac1b710f443dc4c8f8e009288629e34a3db91
@svgeesus
Copy link
Contributor

So, discussion on this issue seemed to conclude that a promise returning does not mean the font was successfully loaded; and thus, that no changes are needed. Is that the consensus?

@svgeesus svgeesus added the Closed as Question Answered Used when the issue is more of a question than a problem, and it's been answered. label Nov 3, 2020
@svgeesus svgeesus closed this as completed Nov 3, 2020
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 15, 2023
This makes interop 2023 listed
css/css-fonts/font-palette-empty-font-family.html
pass because we no longer ignore the @font-face with the empty
font family name.

The previous test
css/css-font-loading/empty-family-load.html
from issue 1023206 where we blocked accessing
empty-named @font-face families still passes. Which means that
using the font loading API with empty family names does not cause
issues after this change. The latter is also inline with the discussion in [1] where there was consensus that empty family names should be acceptable for defining and matching @font-faces.

[1] w3c/csswg-drafts#4510

Fixed: 1408640
Change-Id: I5b98309b2bfbee508b1db8eb90d84d3a47b7dc59
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4334276
Reviewed-by: Frédéric Wang <fwang@igalia.com>
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1117408}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed as Question Answered Used when the issue is more of a question than a problem, and it's been answered. css-fonts-4 Current Work
Projects
None yet
Development

No branches or pull requests

5 participants