Repository navigation
Conversation
|
Size Change: +716 B (+0.01%) Total Size: 7.73 MB 📦 View Changed
|
|
Flaky tests detected in 45adc19. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/29041733920
|
83fa554 to
6613fb8
Compare
c5ea4cb to
fa23641
Compare
sirreal
added
[Type] Bug
4674759 to
3498d76
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @Copilot. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This reverts commit 82f1bb1.
3b73910 to
45adc19
Compare
|
Testing the full round trip through core surfaced two problems with 1. The output is not stable through core's sanitization.
Escapes shorter than six digits need a whitespace terminator when the following character is a hex digit or a space, and that terminator interacts badly with whitespace collapsing. Example: The e2e test doesn't catch this because its assertions match 2. Core prints the value unquoted.
Constraints these impose on the escaper:
The fixed-point property of the reworked escaper was verified against the actual sanitizer with a PHP round trip, and unit round-trip tests plus exact-name e2e assertions pin down encode → sanitize → quote-strip → parse → same name. The rework is on |
Resolve conflicts in packages/global-styles-ui after the move to vitest. Use the trunk package.json and package-lock.json, and drop @jest/globals. Keep the gutenberg-env types in tsconfig.json for globalThis.SCRIPT_DEBUG. Import the createCssString test helpers from vitest. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
🤖 PR meta 🤖📦 Bundle sizeSize Change: +593 B (+0.01%) Total Size: 8.29 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
🏁 Flaky testsSome tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information. should cut and paste individual blocks with collapsed selection in
|
A browser does not throw on an invalid font-family descriptor in `insertRule()`. It removes the descriptor and keeps the rule. Older data, theme.json files, and font collections on older WordPress versions can hold a plain name that is not valid CSS, such as `Exo 2`. For such a name, `getCssFontFaceRule()` returned a rule with no family. `unloadFontFaceInBrowser()` then compared only the style and the weight, and deleted every managed font face with the same values. Now `getCssFontFaceRule()` rejects a rule without a font family. It then tries again with the first name of the value as a CSS string. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Remove the `FontFileMetadata` type. `getFontFaceMetadata()` keeps the plain name in `fontFamily`, as in trunk. `makeFamiliesFromFaces()` and the upload preview convert the name to a CSS string with `createCssString()`. The behavior does not change: the upload still trims the name, skips a font with an empty name, and sends a CSS string to the REST API. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`getCssFontFaceRule()` now writes only the font family, style, weight, and source, as the trunk code did with the FontFace API. The other descriptors are not necessary for a preview of the font. `formatFontFaceName()` has no callers, because the loader now reads the font family as CSS. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Hi @sirreal. I tested this PR together with the core PR WordPress/wordpress-develop#13610 in Chrome. I uploaded eight fonts with special names, for example I pushed three commits to this branch. Please review them, and revert any part that you do not agree with. 1. 315167d: Reject font face rules without a font family (bug fix) Chrome does not throw in
2. ef0e98f: Keep the upload changes close to trunk I removed the 3. 346d863: Simplify the font face rule and remove
This commit reverses two of your design choices. If you want the more exact preview, please restore the descriptors. Related change in core The core PR now escapes |
The `lint:tsconfig` check requires a dev project for TypeScript test files, and `global-styles-ui` has none. The test has no TypeScript syntax, and the other tests in this package are JavaScript files. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Testing evidence: core
|
| Setup | Chromium 149 | Chrome 153 | Firefox 151 |
|---|---|---|---|
Core trunk |
29 of 57 (29 of 86 fonts) | 29 of 57 (29 of 86 fonts) | 29 of 57 (29 of 86 fonts) |
Core trunk + this PR |
40 of 57 (45 of 86 fonts) | 40 of 57 (45 of 86 fonts) | 40 of 57 (46 of 86 fonts) |
| Core PR #13610 + this PR | 57 of 57 (79 of 86 fonts) | 57 of 57 (79 of 86 fonts) | 57 of 57 (79 of 86 fonts) |
The last row comes from the videos on the core PR.
- This PR fixes 19 cases on
trunk: 1, 2, 3, 4, 6, 7, 9, 10, 30, 31, 32, 35, 37, 38, 54, 55, 67, 81, and 82. - 3 cases work on
trunkbut fail with this PR: 33, 34, and 42. This PR sends an escape with its space, for example"Tom \26 Jerry". Coretrunkchanges the two spaces to one, so it stores"Tom \26 Jerry". The escape uses the only space, and the name becomesTom &Jerry. The text renders in the font, but under a different name. Core PR Fonts: Keep font names through CSS validation, storage, and output wordpress-develop#13610 keeps the spaces, and these cases pass with both PRs. - 17 cases that must work still fail with this PR on
trunk. Coretrunkchanges or rejects the value. These cases need Fonts: Keep font names through CSS validation, storage, and output wordpress-develop#13610:- No face (cases 11, 12, 13, 20, 22, 23, 39, 40, 43):
trunkprints the name without quotes in@font-face, for examplefont-family:A=B;. The browser rejects the rule. - Changed name (cases 27, 28, 33, 34, 36, 41, 42):
trunkremoves percent sequences or a space after an escape. - Rejected (case 44):
trunkreads the name0as empty and returns 400.
- No face (cases 11, 12, 13, 20, 22, 23, 39, 40, 43):
- Browser differences:
- Case 60 (
revert-layer) with this PR:trunkprintsfont-family:revert-layer;without quotes. Chromium and Chrome reject the rule. Firefox accepts it, so the case passes only in Firefox. - Case 67 (
A[U+000A]B) ontrunkwithout this PR: Firefox sends no REST request. Chromium and Chrome store the nameA B. With this PR, the case passes in all three browsers.
- Case 60 (
Videos: core trunk + this PR
Chromium 149
edge-4-trunk-and-gutenberg-pr-chromium.mp4
Chrome 153 and Firefox 151
Chrome 153
edge-4-trunk-and-gutenberg-pr-chrome.mp4
Firefox 151
edge-4-trunk-and-gutenberg-pr-firefox.mp4
Videos: core trunk only
Chromium 149
edge-1-trunk-chromium.mp4
Chrome 153 and Firefox 151
Chrome 153
edge-1-trunk-chrome.mp4
Firefox 151
edge-1-trunk-firefox.mp4
In each video, case 1 plays at normal speed, and cases 2–87 play at 5× speed. Each video ends with a summary table.
Result of each case
When the browsers give different results, the cell shows each browser. "No face" means that the browser has no face with the name, because the @font-face rule is invalid. "Not sent" means that the editor sends no REST request.
| Case | Name | Expected | Core trunk |
Core trunk + this PR |
|---|---|---|---|---|
| 1 | O'Reilly Sans |
Works | ✘ no face | ✔ |
| 2 | O"Reilly Sans |
Works | ✘ stored as invalid CSS "O"Reilly Sans" |
✔ |
| 3 | O'Reilly "Sans" |
Works | ✘ stored as invalid CSS "O'Reilly "Sans" |
✔ |
| 4 | Suisse BP Int'l |
Works | ✘ no face | ✔ |
| 5 | ‘Curly’ “Quotes” |
Works | ✔ | ✔ |
| 6 | 'Leading apostrophe |
Works | ✘ stored name Leading apostrophe |
✔ |
| 7 | Trailing quote" |
Works | ✘ stored name Trailing quote |
✔ |
| 8 | ACME, Sans |
Limit (comma) | ✘ stored as a list: ACME, Sans |
✘ stored name ACME,Sans |
| 9 | A;B |
Works | ✘ no face | ✔ |
| 10 | A{B} |
Works | ✘ no face | ✔ |
| 11 | A=B |
Works | ✘ no face | ✘ no face |
| 12 | What? |
Works | ✘ no face | ✘ no face |
| 13 | A:B |
Works | ✘ no face | ✘ no face |
| 14 | Font (Display) |
Works | ✔ | ✔ |
| 15 | Font [Beta] |
Works | ✔ | ✔ |
| 16 | Font !important |
Works | ✔ | ✔ |
| 17 | Dr. Font |
Works | ✔ | ✔ |
| 18 | Font #1 |
Works | ✔ | ✔ |
| 19 | Font @Home |
Works | ✔ | ✔ |
| 20 | Font/Slash |
Works | ✘ no face | ✘ no face |
| 21 | A/*c*/B |
Limit (comment) | ✘ no face | ✘ no face |
| 22 | Bodoni* |
Works | ✘ no face | ✘ no face |
| 23 | Jost* |
Works | ✘ no face | ✘ no face |
| 24 | Rounded M+ 1c |
Works | ✔ | ✔ |
| 25 | C++ Mono |
Works | ✔ | ✔ |
| 26 | 50% Gray |
Works | ✔ | ✔ |
| 27 | Font 50%AB |
Works | ✘ stored name Font 50 |
✘ stored name Font 50 |
| 28 | Font%2c Sans |
Works | ✘ stored name Font Sans |
✘ stored name Font Sans |
| 29 | Font, Sans |
Limit (comma) | ✘ stored as a list: Font, Sans |
✘ stored name Font,Sans |
| 30 | A\B |
Limit (escape) | ✘ stored name A[U+000B] |
✔ |
| 31 | Trailing\ |
Works | ✘ stored as invalid CSS "Trailing\" |
✔ |
| 32 | \0030 |
Limit (escape) | ✘ stored name 0 |
✔ |
| 33 | Tom & Jerry |
Works | ✔ | ✘ stored name Tom &Jerry |
| 34 | Tom & Jerry |
Works | ✔ | ✘ stored name Tom &Jerry |
| 35 | A<B> |
Works | ✘ stored name A |
✔ |
| 36 | Test </style> Sans |
Works | ✘ stored name Test Sans |
✘ stored name Test </style>Sans |
| 37 | </style><script>alert(1)</script> |
Works | ✘ stored as an empty value | ✔ |
| 38 | <!-- x --> |
Works | ✘ stored as an empty value | ✔ |
| 39 | url(javascript:alert(1)) |
Works | ✘ no face | ✘ no face |
| 40 | expression(alert(1)) |
Works | ✘ no face | ✘ no face |
| 41 | A"; color: red; x:" |
Works | ✘ stored as invalid CSS "A"; color: red; x:" |
✘ stored name A";color: red;x:" |
| 42 | A} body { color: red |
Works | ✔ | ✘ stored name A}body {color: red |
| 43 | 12345 |
Works | ✘ no face | ✘ no face |
| 44 | 0 |
Works | ✘ 400 | ✘ 400 |
| 45 | -1 Font |
Works | ✔ | ✔ |
| 46 | 1942 report |
Works | ✔ | ✔ |
| 47 | Press Start 2P |
Works | ✔ | ✔ |
| 48 | --custom |
Works | ✔ | ✔ |
| 49 | -apple-system |
Works | ✔ | ✔ |
| 50 | serif |
Limit (keyword) | ✘ stored as generic serif |
✘ no face |
| 51 | Serif |
Limit (keyword) | ✘ stored as generic serif |
✘ no face |
| 52 | sans-serif |
Limit (keyword) | ✘ stored as generic sans-serif |
✘ no face |
| 53 | system-ui |
Limit (keyword) | ✘ stored as generic system-ui |
✘ no face |
| 54 | emoji |
Limit (keyword) | ✘ stored as generic emoji |
✔ |
| 55 | fangsong |
Limit (keyword) | ✘ stored as generic fangsong |
✔ |
| 56 | inherit |
Limit (keyword) | ✘ stored as keyword inherit |
✘ no face |
| 57 | INHERIT |
Limit (keyword) | ✘ stored as keyword inherit |
✘ no face |
| 58 | initial |
Limit (keyword) | ✘ stored as keyword initial |
✘ no face |
| 59 | unset |
Limit (keyword) | ✘ stored as keyword unset |
✘ no face |
| 60 | revert-layer |
Limit (keyword) | ✘ stored as keyword revert-layer |
Chromium: ✘ no face Chrome: ✘ no face Firefox: ✔ |
| 61 | default |
Limit (keyword) | ✘ stored as keyword default |
✘ no face |
| 62 | generic(kai) |
Limit (keyword) | ✘ stored as generic generic(kai) |
✘ no face |
| 63 | A B |
Limit (spaces) | ✘ stored name A B |
✘ stored name A B |
| 64 | Leading space |
Works (trimmed name) | ✔ | ✔ |
| 65 | Trailing space |
Works (trimmed name) | ✔ | ✔ |
| 66 | A[U+0009]B |
Limit (spaces) | ✘ stored name A B |
✘ stored name A B |
| 67 | A[U+000A]B |
Limit (spaces) | Chromium: ✘ stored name A BChrome: ✘ stored name A BFirefox: ✘ not sent |
✔ |
| 68 | A[U+00A0]B |
Works | ✔ | ✔ |
| 69 | A[U+3000]B |
Works | ✔ | ✔ |
| 70 | A[U+200B]B |
Works | ✔ | ✔ |
| 71 | 日本語 😀 |
Limit (slug) | ✘ 400 | ✘ 400 |
| 72 | 微软雅黑 |
Limit (slug) | ✘ 400 | ✘ 400 |
| 73 | ＭＳ ゴシック |
Limit (slug) | ✘ 400 | ✘ 400 |
| 74 | Ñandú |
Works | ✔ | ✔ |
| 75 | Café |
Works | ✔ | ✔ |
| 76 | Café |
Works | ✔ | ✔ |
| 77 | وزیرمتن |
Limit (slug) | ✘ 400 | ✘ 400 |
| 78 | A[U+202E]B |
Works | ✔ | ✔ |
| 79 | Dev 👩[U+200D]💻 |
Works | ✔ | ✔ |
| 80 | AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA… |
Works | ✔ | ✔ |
| 81 | A\A\A\A\A\A\A\A\A\A\A\A\A\A\A\A\A\A\… |
Works | ✘ stored as invalid CSS "A\A\A\A\A\A\A\A\A\A\A\A\A\A\A\A\A\A… |
✔ |
| 82 | A[U+0000]B |
Works | ✘ no face | ✔ |
| 83 | A[U+0001]B |
Rejected (400) | ✘ no face | ✘ no face |
| 85 | A[U+D800]B |
Rejected (400) | ✘ 400 | ✘ 400 |
| 86 | (empty) | Rejected (no name) | ✘ 400 | ✘ not sent |
| 87 | (one space) | Rejected (no name) | ✘ 400 | ✘ not sent |
Limits: I did not test WebKit or Safari. A script sets the body font in the global styles post, as an admin save does. It does not use the Styles UI.
What?
Fixes Trac #63568 / #70426
Use quoted and escaped strings for
@font-facefont-familyvalues of uploaded fonts.The problem was that the REST API request was made with a plain string for the
font-family, whereas the backend expects a string containing CSS text. Something likeWordPress' fontis invalid, it is unquoted (not a CSS string) the unescaped'would start a CSS string.With this change, arbitrary font names from uploaded fonts are likely to work well. The font family name is transformed into a CSS string with a number of CSS Unicode escape sequences to remove characters that are often problematic for other parts of the application like KSES or font normalization.
Why?
The REST API call was made incorrectly, providing a plain string instead of a string containing CSS text. This caused fonts with names containing special characters (e.g.
O'Reilly Sans) to produce invalid CSS and break the fonts or more CSS on the site.How?
createCssString()— New function that produces a properly quoted, Unicode-escaped CSS string, neutralizing characters problematic in CSS and WordPress HTML contexts.CSSStyleSheetinstead ofFontFaceAPI — Font faces are now inserted as@font-facerules into managedadoptedStyleSheets, giving full control over CSS output.loadFontFaceInBrowseris now synchronous.FontFileMetadatatype carries the rawfontDisplayName(for UI/slugs) separately from the escapedfontFamily(for CSS).The
FontFaceAPI was very problematic because of its inconsistent behavior across browsers. It also expects a "plain"font-familystring, which made it more difficult to use with the CSSfont-familythat the rest of the WordPress Font API expects to use. Thefamilyof aFontFaceinstance has inconsistent normalization and is not a CSS string, making it very difficult to compart correctly when attempting to unload a font face.An E2E test is added with a problematic font name
"Ephesis" font with <special \> {chars} & things, ya'know?to confirm it's working well.Testing Instructions
/wp-admin/site-editor.php?p=%2Fstyles§ion=%2Ftypography)test/e2e/assets/Ephesis-modified-name.ttf. It has more and different special characters.Check for any regressions:
Fira CodeandManropein twentytwentyfive.Known issues
I discovered some issues with font display name and font
-familythat are edge cases that should not impact the font's behavior. These are related to some sanitization that was introduced in #58636.The expected
font-familyis incorrectly sanitized by the REST API (these are the decoded values):This is incorrect, although it may be harmless. It appears to be the result of
sanitize_text_field()stripping "extra" whitespace. This could be addressed in this PR by Unicode escaping carriage returns, line feeds, horizontal tabs, and spaces but that seems excessive. The collapsing of multiple spaces to a single space should maintain correct Unicode escapes, but it means that the following whitespace disappears because it is consumed by the Unicode escape:\3E following textbecomes\3E following textwhich is decoded as>following textinstead of the expected> following text. Because the font-family mostly just needs to match, this issue should not be critical although it is worth addressing in follow-up work.Font display names are also sanitized in a destructive way.
Font families are stored in a
wp_font_familypost type, where the font family name is stored in the post title. JSON is stored in the post content.The family name is sanitized, so a family name is changed:
The family name could be stored without additional sanitization in the JSON instead of the post title.
Use of AI Tools
Claude Code (Claude Opus 4.6) was used to perform some development tasks.
Various AI tools were used for PR feedback.