Fail the CSS compile on @font-face rules the runtime can't honor - #5506
Fail the CSS compile on @font-face rules the runtime can't honor#5506shai-almog wants to merge 2 commits into
Conversation
An .otf sailed through compilation and only broke on the device. The compiler loads fonts with java.awt.Font.createFont(TRUETYPE_FONT, ...), which also parses OpenType/CFF, so the font resolved, rendered in the simulator and got written into theme.res -- while Font.createTrueTypeFont rejects any file name not ending in .ttf and IPhoneBuilder never registered it in UIAppFonts. The reported symptom is a font that just doesn't change, with no error anywhere. CSSTheme.updateResources now validates every declared @font-face first and throws with the offending rules listed, which CN1CSSCLI turns into a non-zero exit and CompileCSSMojo into a failed build. Rejected: - anything not ending in .ttf, with the advice split by case: convert an .otf, rename an upper-case .TTF (the runtime's endsWith is case-sensitive) - a local font that doesn't exist, which used to surface as a bare FileNotFoundException naming no rule - a local font outside the directory holding the CSS file; merge mode syncs only that directory, so a ../ reference resolves for the author and breaks in a real build - two rules resolving to different files that share a file name, since fonts are deployed next to theme.res by file name alone and one would overwrite the other. Two families pointing at the same file stay legal -- that copy is idempotent, and rejecting it would break a legitimate alias. Every declared rule is checked, not only the referenced ones, so a typo fails the build that introduced it instead of the later build that first uses the family. Remote fonts are judged by URL alone, so validation never downloads. Containment is measured against baseURL rather than cssFile: cssFile is a "test.css" placeholder unless a caller assigns it, so using it rejected every font in the existing tests. Updates the guide and the initializr CSS reference, which described the old compile-clean-fail-at-runtime behaviour.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2dbae3510
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!"file".equals(url.getProtocol())) { | ||
| // Remote fonts are downloaded into the CSS directory, so only the name matters here. | ||
| continue; |
There was a problem hiding this comment.
When src uses ftp://... or another non-file scheme ending in .ttf, this branch treats it as a downloadable remote font and validation succeeds. However, FontFace.getFontFile() only downloads protocols whose name starts with http; for these accepted schemes it returns null, so a referenced family silently falls back instead of being deployed. Restrict this branch to the remote protocols supported by getFontFile(), or extend the loader accordingly.
Useful? React with 👍 / 👎.
| String path = url.getPath(); | ||
| try { | ||
| path = java.net.URLDecoder.decode(path, "UTF-8"); |
There was a problem hiding this comment.
For a local URL, URLDecoder applies form-encoding semantics and converts a literal + in the path to a space. Thus valid rules for distinct files such as url(A+B.ttf) and url(A%20B.ttf) both produce the collision key A B.ttf and the new validator rejects the stylesheet, even though new File(url.toURI()).getName() deploys them as A+B.ttf and A B.ttf. Decode percent escapes without translating literal plus signs.
Useful? React with 👍 / 👎.
| String source = canonicalSource(url); | ||
| String previousSource = sourceByFileName.put(fileName, source); | ||
| String previousFamily = familyByFileName.put(fileName, family); | ||
| if (previousSource != null && !previousSource.equals(source)) { |
There was a problem hiding this comment.
When compilation runs on a case-insensitive filesystem, rules for different files named, for example, Regular.ttf and regular.ttf occupy separate map keys here even though their deployment targets are the same file. FontFace.getFontFile() then skips copying the second target because it already exists, while theme.res still records each source's original casing; on a case-sensitive device one of those recorded filenames is absent. Reject case-only collisions whenever the deployment filesystem treats the names as identical.
Useful? React with 👍 / 👎.
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Two more ways a font passed the compile and then failed on a device, which is
the case the name check alone doesn't catch.
A file the font parser rejects left EditorTTFFont.actualFont null, because
refresh() swallows the Throwable, and then died as a bare NPE inside
EditableResources.save at getNativeFont()).getPSName() naming no rule. This
matters more now that the compile insists on a .ttf name: renaming an .otf is
the obvious workaround, and anything that isn't really loadable has to be
caught here rather than on the device.
A font with no PostScript name renders in the simulator and on Android, which
both look fonts up by file name, while iOS resolves purely by the PostScript
name written into the resource and falls back to the system font. That is
invisible until someone runs the app on an iPhone.
validateFontFaces now parses each local font and checks for a usable
PostScript name, reporting which rule and file is at fault.
Also corrects the extension message, which overclaimed. The constraint is the
file NAME -- Font.createTrueTypeFont tests endsWith(".ttf") and IPhoneBuilder
filters UIAppFonts the same way -- not the outline format. iOS registers
through UIAppFonts and resolves with [UIFont fontWithName:], Android uses
Typeface.createFromAsset, and both read CFF/OpenType content, so no check is
made against the sfnt flavour: an OpenType font renamed to .ttf is not proven
broken and isn't rejected on a guess.
|
Follow-up commit ( Two gaps closed:
Correction to the original message in this PR. It said "OpenType fonts aren't supported — convert to TrueType", which overclaims. The constraint is the file name: I deliberately did not add a check against the sfnt flavour. I can't prove that path broken without a device run, and rejecting it on a guess could break a shipping setup. If you'd rather be strict — reject Verified through the real forked CLI: 17 css-compiler + 8 css-cli tests green; guide gates (snippet validator, asciidoctor, Vale, paragraph-cap, LanguageTool) clean; copyright headers pass; new Java is ASCII-only. |
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a8151d907
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| + "move the font beside the CSS file or into a subdirectory of it"); | ||
| continue; | ||
| } | ||
| String contentError = fontContentError(fontFile); |
There was a problem hiding this comment.
When a Maven build first fails this check and the developer fixes only the font file, subsequent builds keep validating the stale copy under target/css/cn1-merged-files. In the inspected stateless flow, CN1CSSCLI.updateMergeFileStateless() decides whether to call syncDirectories() using only the input theme.css timestamps, even though CompileCSSMojo correctly reruns for changes anywhere under the CSS directory. Consequently, replacing a corrupt or missing-PostScript-name font without touching theme.css continues to produce the old error until the user cleans target or touches the stylesheet; the merge cache must account for asset changes before this validation runs.
Useful? React with 👍 / 👎.
| return url.toString(); | ||
| } | ||
| } | ||
| return url.toString(); |
There was a problem hiding this comment.
When two families alias the same remote font using URLs such as https://example.com/Font.ttf#body and https://example.com/Font.ttf#caption, this return value differs even though an HTTP fragment is not sent to the server and getFontFile() downloads both into the same Font.ttf cache file. The collision check therefore rejects a legitimate shared-file alias solely because its fragment differs; compare remote identities without the fragment before deciding that their contents can overwrite one another.
Useful? React with 👍 / 👎.
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
|
Compared 181 screenshots: 181 matched. |
|
Compared 217 screenshots: 217 matched. |
|
Compared 144 screenshots: 144 matched. |
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
|
Folded into #5508 - one PR for the whole change. |
1 participant
Stacked on #5502 — review/merge that first; this PR's diff is only the validation change.
Why
An
.otfsailed through compilation and only broke on the device. The compiler loads fonts withjava.awt.Font.createFont(TRUETYPE_FONT, ...), which also parses OpenType/CFF, so the font resolved, rendered in the simulator and got written intotheme.res— whileFont.createTrueTypeFontrejects any file name not ending in.ttf(Font.java:342) andIPhoneBuildernever registered it inUIAppFonts(IPhoneBuilder.java:4989). The reported symptom is a font that just doesn't change, with no error anywhere.What it rejects
CSSTheme.updateResourcesnow validates every declared@font-facefirst and throws with the offending rules listed, whichCN1CSSCLIturns into a non-zero exit andCompileCSSMojointo a failed build..ttfFont.createTrueTypeFontthrows; iOS never registers it. Advice splits by case: convert an.otf, rename an upper-case.TTF(the runtime'sendsWithis case-sensitive)FileNotFoundExceptionnaming no rule../reference resolves for the author and breaks in a real buildtheme.resby file name alone; one would overwrite the otherTwo families pointing at the same file stay legal — that copy is idempotent, and rejecting it would break a legitimate alias. There's a test for it.
Every declared rule is checked, not only the referenced ones, so a typo fails the build that introduced it rather than the later build that first uses the family. Remote fonts are judged by URL alone, so validation never downloads a font.
Note for reviewers
Containment is measured against
baseURL, notcssFile.cssFileis anew File("test.css")placeholder unless a caller assigns it (onlyCN1CSSCLIdoes), so the first version of this check rejected every font in the existing tests.Behaviour change
A project that today ships a
.otfand renders the system font on device will now fail its build. That's the point — but it is a build-breaking change for anyone in that state, and the error message tells them to convert the font.Verification
css-compiler+css-cli: 24 tests green, 9 of them new inCSSFontFaceValidationTest(each rejection path, plus the shared-file alias and a valid sheet that must still compile)core-unittestsCSS suites (CSSThemeCompilerTest,CSSThemeBorderRadiusTest,CSSThemeGradientTest): 28 tests greentheme.res.otf→ exit 1 and:@font-facein the repo already uses.ttf, so nothing in-tree starts failing🤖 Generated with Claude Code