Closed Bug 1784058 Opened 2 years ago Closed 2 years ago

The format() function in the @font-face src descriptor should not accept a list of strings

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
105 Branch
Tracking Flags:
Tracking Status
firefox105 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1784058 - Add WPT test for @font-face format() parsing issues. r=#firefox-style-system-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1784058 - Do not allow a list of strings in the @font-face src descriptor's format() function, only a single format string. r=#firefox-style-system-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1784058 - Simplify @font-face format hint handling in gfx/thebes, now that it is explicitly only a single hint, not a set. r=#gfx-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1784058 - Fix up legacy @font-face tests that depended on previous behavior, no longer allowed by the spec. r=#firefox-style-system-reviewers
48 bytes, text/x-phabricator-request
Details | Review

In CSS Fonts 3, the syntax of the src descriptor allowed the format() function to take a comma-separated list of format hint strings:

Name:	src
Value:	[ <url> [format(<string> #)]? | <font-face-name> ] #

However, this was never actually useful, as any given font resource cannot in fact be simultaneously multiple formats (except for "truetype" and "opentype", which are synonymous). I think the intention at one time was to support additional hints such as "truetype-variations", but these are not defined in the spec, and although Firefox accepts some such strings, they have no real function.

In CSS Fonts 4, the src descriptor syntax has been updated such that a list of format() hints is no longer valid, and the new tech() function is introduced to address the things that were perhaps envisaged for a format() list:

<url> [ format(<font-format>)]? [ tech( <font-tech>#)]? | local(<font-face-name>)

Currently, Gecko accepts a comma-separated list of strings in format(), although this offers authors no benefit. AFAICT neither Webkit nor Blink allow this; if multiple strings are specified, they drop the src list item altogether as invalid.

As a step towards CSS Fonts 4 conformity, and to align with the other browsers, we should stop accepting a list in the format() function.

This will also simplify the extension of format() to accept keywords, as required by the spec and requested in bug 650372.

I notice there's a lack of test coverage here, so before making changes I'd like to
add a simple set of tests so that we can see the effect of the upcoming fix.

Assignee: nobody → jfkthame
Status: NEW → ASSIGNED

This aligns with CSS Fonts 4 (rather than Fonts 3) and with behavior in other browsers;
I don't expect any significant breakage, given that specifying multiple format strings
was never supported in other engines AFAIK, and never served any useful purpose.

Depends on D154234

Turns out we do have some mochitests/reftests that assume the old behavior of allowing a list of strings, so we'll need to fix those to conform to the newer spec behavior.

Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35634f66cb37
Add WPT test for @font-face format() parsing issues. r=emilio
https://hg.mozilla.org/integration/autoland/rev/5d21f6e4f8e0
Do not allow a list of strings in the @font-face src descriptor's format() function, only a single format string. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6d71bcfcc1a1
Simplify @font-face format hint handling in gfx/thebes, now that it is explicitly only a single hint, not a set. r=gfx-reviewers,aosmond,lsalzman
https://hg.mozilla.org/integration/autoland/rev/597837e9753f
Fix up legacy @font-face tests that depended on previous behavior, no longer allowed by the spec. r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/35434 for changes under testing/web-platform/tests

Backed out for causing reftest failures on variation-format-hint-1a.html

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/font-face/variation-format-hint-1a.html == layout/reftests/font-face/variation-format-hint-1A-ref.html | image comparison, max difference: 255, number of differing pixels: 2851
Flags: needinfo?(jfkthame)
Upstream PR was closed without merging
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5b24489d365
Add WPT test for @font-face format() parsing issues. r=emilio
https://hg.mozilla.org/integration/autoland/rev/714d84a2680a
Do not allow a list of strings in the @font-face src descriptor's format() function, only a single format string. r=emilio
https://hg.mozilla.org/integration/autoland/rev/40b00a214019
Simplify @font-face format hint handling in gfx/thebes, now that it is explicitly only a single hint, not a set. r=gfx-reviewers,aosmond,lsalzman
https://hg.mozilla.org/integration/autoland/rev/cbe59d9bbcbb
Fix up legacy @font-face tests that depended on previous behavior, no longer allowed by the spec. r=emilio

Backed out for causing wpt failures on format-specifiers-variations.html.

Push with failures

Failure log

Also please take a look at this Tvw failure.

Backout link

[task 2022-08-11T19:40:28.010Z] 19:40:28     INFO - TEST-START | /css/css-fonts/format-specifiers-variations.html
[task 2022-08-11T19:40:28.023Z] 19:40:28     INFO - Closing window 0103ec62-616c-493b-954a-33164fc6586c
[task 2022-08-11T19:40:28.378Z] 19:40:28     INFO - 
[task 2022-08-11T19:40:28.378Z] 19:40:28     INFO - TEST-PASS | /css/css-fonts/font-variant-alternates-parsing.html | CSS Test:  font-variant-alternates: historical-forms; parses case-insensitively 
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - TEST-FAIL | /css/css-fonts/font-variation-settings-serialization-001.html | font-feature-settings should be serialized to not include duplicates - assert_in_array: value "\"bldA\" 1, \"bldA\" 2" not in array ["\"bldA\" 2", "'bldA' 2"]
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - @http://web-platform.test:8000/css/css-fonts/font-variation-settings-serialization-001.html:19:32
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2590:25
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - test@http://web-platform.test:8000/resources/testharness.js:628:30
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - @http://web-platform.test:8000/css/css-fonts/font-variation-settings-serialization-001.html:18:17
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - TEST-PASS | /css/css-fonts/format-specifiers-variations.html | Load Ahem with format woff 
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - TEST-PASS | /css/css-fonts/format-specifiers-variations.html | Load Ahem with format truetype 
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - TEST-PASS | /css/css-fonts/format-specifiers-variations.html | Load Ahem with format opentype 
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - TEST-PASS | /css/css-fonts/format-specifiers-variations.html | Load Ahem with format woff2 
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - TEST-PASS | /css/css-fonts/format-specifiers-variations.html | Load Ahem with format woff-variations 
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - TEST-PASS | /css/css-fonts/format-specifiers-variations.html | Load Ahem with format truetype-variations 
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - TEST-PASS | /css/css-fonts/format-specifiers-variations.html | Load Ahem with format opentype-variations 
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - TEST-PASS | /css/css-fonts/format-specifiers-variations.html | Load Ahem with format woff2-variations 
[task 2022-08-11T19:40:28.379Z] 19:40:28     INFO - TEST-UNEXPECTED-FAIL | /css/css-fonts/format-specifiers-variations.html | Do not load Ahem with format xyzwoff - assert_unreached: Should have rejected: undefined Reached unreachable code
[task 2022-08-11T19:40:28.380Z] 19:40:28     INFO - 
[task 2022-08-11T19:40:28.380Z] 19:40:28     INFO - TEST-UNEXPECTED-FAIL | /css/css-fonts/format-specifiers-variations.html | Do not load Ahem with format xyztruetype - assert_unreached: Should have rejected: undefined Reached unreachable code
[task 2022-08-11T19:40:28.380Z] 19:40:28     INFO - 
[task 2022-08-11T19:40:28.380Z] 19:40:28     INFO - TEST-UNEXPECTED-FAIL | /css/css-fonts/format-specifiers-variations.html | Do not load Ahem with format xyzopentype - assert_unreached: Should have rejected: undefined Reached unreachable code
[task 2022-08-11T19:40:28.381Z] 19:40:28     INFO - 
[task 2022-08-11T19:40:28.381Z] 19:40:28     INFO - TEST-UNEXPECTED-FAIL | /css/css-fonts/format-specifiers-variations.html | Do not load Ahem with format xyzwoff2 - assert_unreached: Should have rejected: undefined Reached unreachable code
[task 2022-08-11T19:40:28.382Z] 19:40:28     INFO - 
[task 2022-08-11T19:40:28.382Z] 19:40:28     INFO - TEST-UNEXPECTED-FAIL | /css/css-fonts/format-specifiers-variations.html | Do not load Ahem with format xyzwoff-variations - assert_unreached: Should have rejected: undefined Reached unreachable code
[task 2022-08-11T19:40:28.383Z] 19:40:28     INFO - 
[task 2022-08-11T19:40:28.383Z] 19:40:28     INFO - TEST-UNEXPECTED-FAIL | /css/css-fonts/format-specifiers-variations.html | Do not load Ahem with format xyztruetype-variations - assert_unreached: Should have rejected: undefined Reached unreachable code
[task 2022-08-11T19:40:28.383Z] 19:40:28     INFO - 
<...>
[task 2022-08-11T19:40:28.409Z] 19:40:28     INFO - TEST-UNEXPECTED-FAIL | /css/css-fonts/format-specifiers-variations.html | Do not load Ahem with format opentye-variations - assert_unreached: Should have rejected: undefined Reached unreachable code
[task 2022-08-11T19:40:28.409Z] 19:40:28     INFO - 
[task 2022-08-11T19:40:28.409Z] 19:40:28     INFO - TEST-UNEXPECTED-FAIL | /css/css-fonts/format-specifiers-variations.html | Do not load Ahem with format woff2variations - assert_unreached: Should have rejected: undefined Reached unreachable code
[task 2022-08-11T19:40:28.493Z] 19:40:28     INFO - TEST-OK | /css/css-fonts/format-specifiers-variations.html | took 482ms
[task 2022-08-11T19:40:29.216Z] 19:40:29     INFO - STDOUT: cleanup aborted: args: /builds/worker/fetches/android-sdk-linux/platform-tools/adb wait-for-device shell rm /data/data/org.mozilla.geckoview.test_runner/files/mozilla/profiles.ini; echo adb_returncode=$?, exitcode: 1, stdout: rm: /data/data/org.mozilla.geckoview.test_runner/files/mozilla/profiles.ini: No such file or directory
[task 2022-08-11T19:40:29.218Z] 19:40:29     INFO - Closing logging queue
[task 2022-08-11T19:40:29.218Z] 19:40:29     INFO - queue closed
[task 2022-08-11T19:40:29.233Z] 19:40:29     INFO - Setting up ssl
[task 2022-08-11T19:40:29.247Z] 19:40:29     INFO - certutil | b''
[task 2022-08-11T19:40:29.265Z] 19:40:29     INFO - certutil | b''
[task 2022-08-11T19:40:29.281Z] 19:40:29     INFO - certutil | b'\nCertificate Nickname                                         Trust Attributes\n                                                             SSL,S/MIME,JAR/XPI\n\nweb-platform-tests                                           CT,, \n'
[task 2022-08-11T19:40:34.663Z] 19:40:34     INFO - STDOUT: timed out waiting for profiles.ini
[task 2022-08-11T19:40:34.817Z] 19:40:34     INFO - adb Granting important runtime permissions to org.mozilla.geckoview.test_runner
[task 2022-08-11T19:40:36.399Z] 19:40:36     INFO - adb launch_application: am start -W -n org.mozilla.geckoview.test_runner/org.mozilla.geckoview.test_runner.TestRunnerActivity -a android.intent.action.MAIN --es env0 MOZ_CRASHREPORTER=1 --es env1 MOZ_CRASHREPORTER_NO_REPORT=1 --es env2 MOZ_CRASHREPORTER_SHUTDOWN=1 --es env3 MOZ_HIDE_RESULTS_TABLE=1 --es env4 MOZ_IN_AUTOMATION=1 --es env5 MOZ_LOG=signaling:3,mtransport:4,DataChannel:4,jsep:4 --es env6 R_LOG_LEVEL=6 --es env7 R_LOG_DESTINATION=stderr --es env8 R_LOG_VERBOSE=1 --es env9 MOZ_PROCESS_LOG=/tmp/tmpsnjkov21pidlog --es env10 MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 --es env11 STYLO_THREADS=1 --es arg0 -no-remote --es arg1 -profile --es arg2 /data/local/tmp/test_root/profile --es arg3 --marionette --es arg4 about:blank --ez use_multiprocess True
[task 2022-08-11T19:40:37.523Z] 19:40:37     INFO - Starting runner
[task 2022-08-11T19:40:38.521Z] 19:40:38     INFO - TEST-START | /css/css-fonts/generic-family-keywords-001.html
Flags: needinfo?(jfkthame)
Flags: needinfo?(jfkthame)
Upstream PR was closed without merging
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9398ed3c2567
Add WPT test for @font-face format() parsing issues. r=emilio
https://hg.mozilla.org/integration/autoland/rev/af88273dca8f
Do not allow a list of strings in the @font-face src descriptor's format() function, only a single format string. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f8439bf39126
Simplify @font-face format hint handling in gfx/thebes, now that it is explicitly only a single hint, not a set. r=gfx-reviewers,aosmond,lsalzman
https://hg.mozilla.org/integration/autoland/rev/0d8488e753ab
Fix up legacy @font-face tests that depended on previous behavior, no longer allowed by the spec. r=emilio
Flags: needinfo?(jfkthame)
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: