[0.83] fix(text): resolve app-bundled font families by passing a DirectWrite font collection - #16344
Open
FaithfulAudio wants to merge 3 commits into
Open
Conversation
… font collection WindowsTextLayoutManager::GetTextLayout passed nullptr as the font collection to CreateTextFormat, restricting resolution to the system collection. Every app-bundled font family therefore failed to resolve and every codepoint fell back to Segoe UI glyph 0 (.notdef) - bundled icon fonts render blank or as tofu. Measured with a standalone DirectWrite probe replaying this exact call sequence against the eight stock react-native-vector-icons TTFs: with a collection that contains the font each draws a real glyph index (40, 1, 1, 13, 4, 4, 4, 2); with nullptr every one resolves to Segoe UI glyph 0. Adds DWriteAppFontCollection() to DWriteHelpers - the system font set merged with every *.ttf/*.otf under the app's Assets\ and Assets\Fonts\, built once via a magic static, failing closed to nullptr so behaviour is unchanged for apps that bundle no fonts - and passes it at the CreateTextFormat call site. 0.83-stable twin of microsoft#16339. Fixes microsoft#16306 and microsoft#16308 on this line (both were misdiagnosed in their original reports; the probe refuted the space-in-name and stale-checksum theories - see the comments on those issues).
Contributor
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
acoates-ms
changed the title
… hot path Twin of the same change on main (microsoft#16339), kept byte-identical so the two branches cannot drift. The directory enumeration already ran exactly once: s_appFontCollection is a function-local static with a dynamic initializer, so it is initialized a single time and concurrent first callers wait for that initialization rather than racing or repeating it ([stmt.dcl]/4). That guarantee is now stated explicitly instead of merely implied. What the old signature did cost on every call: GetTextLayout() invokes this once per text measure, and returning winrt::com_ptr by value put an AddRef/Release pair on that path for a pointer whose lifetime is already static and process-long. The accessor now returns a non-owning raw pointer; the call site drops its .get().
Contributor
Author
|
Updated alongside the main-branch PR (#16339) to address @acoates-ms's review there: the accessor now returns a non-owning raw pointer so the per-text-measure path carries no AddRef/Release, and the once-only, thread-safe initialization of the font-file enumeration is now stated explicitly rather than implied. The three touched files are byte-identical to #16339 — verified by hash, not by eye — so the twins cannot drift. |
Contributor
There was a problem hiding this comment.
Pull request overview
Backports app-bundled font resolution to RNW 0.83 Fabric text layout.
Changes:
- Builds a cached DirectWrite collection from bundled and system fonts.
- Uses the collection when creating text formats.
- Adds the stable-branch change record.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
WindowsTextLayoutManager.cpp |
Uses the merged collection and modifies max-width handling. |
DWriteHelpers.h |
Declares the collection accessor. |
DWriteHelpers.cpp |
Builds and caches the merged collection. |
react-native-windows-fix-app-bundled-fonts-083.json |
Records the patch release change. |
| if (metrics.width > size.width) { | ||
| spTextLayout->SetMaxWidth(size.width); | ||
| } | ||
| spTextLayout->SetMaxWidth(size.width); |
Comment on lines
+85
to
+100
| // Include the system font set so that system families keep resolving when this | ||
| // collection is used in place of the system collection. | ||
| winrt::com_ptr<::IDWriteFontSet> systemFontSet; | ||
| winrt::check_hresult(factory5->GetSystemFontSet(systemFontSet.put())); | ||
| winrt::check_hresult(builder->AddFontSet(systemFontSet.get())); | ||
|
|
||
| uint32_t fontFileCount = 0; | ||
| for (const auto *subdirectory : {L"Assets\\", L"Assets\\Fonts\\"}) { | ||
| for (const auto *pattern : {L"*.ttf", L"*.otf"}) { | ||
| fontFileCount += AddFontFiles(factory5.get(), builder.get(), appDirectory + subdirectory, pattern); | ||
| } | ||
| } | ||
| if (fontFileCount == 0) { | ||
| // Nothing bundled: report "no app collection" so callers pass nullptr to DirectWrite | ||
| // and keep using DirectWrite's own (cached, updatable) system font collection. | ||
| return nullptr; |
…ad race
Two changes, one per reading of the review comment.
1. The list of bundled font files is now its own cached static
(AppFontFilePaths): the directory searches run exactly once per process and
every consumer - including any future path that rebuilds a collection -
reads the cached list and never touches the file system again.
CreateAppFontCollection() contains no enumeration by construction.
2. The genuine first-use thread race was in DWriteFactory() itself: the
existing `if (!s_dwriteFactory) { assign }` lazy-init is a data race when
two threads make first use concurrently - and DWriteAppFontCollection() is
reachable from more than one thread on first use, which makes that race
live rather than theoretical. Converted to a function-local static with a
dynamic initializer (thread-safe by [stmt.dcl]/4; /Zc:threadSafeInit is on
by default and nothing in the RNW build disables it), so concurrent first
callers wait for one initialization instead of racing it.
Also removes the stray `#pragma once` this .cpp carried.
Contributor
Author
|
Updated in lockstep with #16339's review response: the font-file list is now a separately cached static (directory searches structurally cannot repeat), and the pre-existing first-use data race in |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
vnext/Microsoft.ReactNative/Fabric/platform/react/renderer/textlayoutmanager/WindowsTextLayoutManager.cpp:201
- This is an unrelated text-layout change not described or validated by this font-resolution PR.
CreateTextLayoutalready receivessize.widthabove, so this setter is redundant, while deleting the metrics branch changes code outside the stated fix. Restore the original block (or move this independent cleanup to a separately justified change).
spTextLayout->SetMaxWidth(size.width);
vnext/Microsoft.ReactNative/Fabric/DWriteHelpers.cpp:93
- The outer
trydoes not fully provide the documented fail-closed behavior:AppDirectory,AppendFontFiles, andAppFontFilePathsarenoexcepteven though they allocate strings/vectors. If an allocation throws,std::terminateruns before this catch can returnnullptr. Removenoexceptfrom those internal helpers so exceptions reach this catch, and make the find handle RAII-safe while allowing propagation.
try {
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
0.83-stable twin of #16339. Verified this branch carries the identical defect at the same line (
WindowsTextLayoutManager.cpp:119passesnullptras the font collection), so the patch applies unchanged.Opening it against the stable line because that is the line our shipping app consumes (RNW 0.83.2), where this is the root cause of both #16306 and #16308.
Resolve app-bundled fonts in Fabric text layout
Fixes #16306. Fixes #16308. (Both issues turn out to have this single root cause — see below; both of
their filed diagnoses were wrong, and I am the person who filed them.)
Problem
On the new architecture, no app-bundled font family ever resolves.
fontFamily: 'Ionicons','Material Icons','FontAwesome'and friends all fall back to Segoe UI and render glyph 0(
.notdef), whether the app ships the TTF inAssets\Fonts\or registers it through thewindows.sharedFontspackage-manifest extension. Only fonts installed system-wide work.Root cause
WindowsTextLayoutManager::GetTextLayoutpassesnullptras the font collection toCreateTextFormat(WindowsTextLayoutManager.cppL119):nullptrmeans "resolve against the system font collection only". An app's own font files are not init, so the family is never found, DirectWrite falls back, and every icon codepoint maps to glyph 0.
That is the whole bug. It also explains why the two issues I filed looked like two different bugs and
were both misdiagnosed:
the font,
"Material Icons"resolves and draws glyph 782, and so do"material icons"and"MATERIAL ICONS". DirectWrite's family lookup is case-insensitive and perfectly happy withinterior spaces.
react-native-vector-icons@10.3.0TTFs, byte-unmodified, passIDWriteFontFile::Analyzewithsupported=1, are accepted byIDWriteFontSetBuilder1::AddFontFilewithhr=0, and draw realglyph indices.
Segoe UIglyph 0. Whether.notdefpaints a hollow box or nothing at all is a property of thefallback face, not evidence of two different failures.
Fix
Add
Microsoft::ReactNative::DWriteAppFontCollection()toDWriteHelpers: on first use, build aDirectWrite collection from the system font set merged with every
*.ttf/*.otfunder the appdirectory's
Assets\andAssets\Fonts\, and pass it at theCreateTextFormatcall site instead ofnullptr.Properties that matter for review:
nullptron any failure and when the app bundles no fonts, andnullptris exactly the current behaviour — so an app that ships no fonts, or a machine where thecollection cannot be built, behaves precisely as it does today.
static const(thread-safe magic static). Bundled assetscannot change during the process lifetime.
CreateTextFormatcall needed changing. The per-fragmentSetFontFamilyNameat L274 resolves against the collection the layout already inherited from thetext format, so it picks the app collection up for free.
Validation
What was actually measured. I wrote a standalone DirectWrite probe that replays this exact call
sequence —
CreateTextFormat(including the emptylocaleNamethe real code passes) →CreateTextLayout→Drawwith a recordingIDWriteTextRendererthat reports the resolved face andglyph indices — against the real stock TTFs. Built with VS 2022 (x64) and run on Windows 11
10.0.26200. Probe sources and raw output are attachable; here is the substance.
Every stock
react-native-vector-icons@10.3.0font, unmodified:AnalyzesupportedAddFontFilenullptr(today)hr=0acceptedMaterial Iconshr=0acceptedMaterial Design Iconshr=0acceptedIoniconshr=0acceptedFontAwesomehr=0acceptedFeatherhr=0acceptedOcticonshr=0acceptedEvilIconshr=0acceptedanticonThe codepoint in each row is taken from the face's own
IDWriteFontFace1::GetUnicodeRanges, so it isguaranteed to be one the font claims to map.
Family-string variants, holding the collection and the font constant and changing only the requested
string (MaterialIcons.ttf, drawing U+E7FD):
fontFamilyFindFamilyNameMaterial Iconsmaterial iconsMATERIAL ICONSMaterialIcons" Material Icons"(leading space)"Material Icons "(trailing space)Material Icons(doubled interior space)Interior spaces are fine; case does not matter; leading/trailing/duplicated whitespace is not
normalised by DirectWrite and RNW does not trim the string either. Worth knowing, but not fixed
here — a separate, arguable question.
The API sequence this patch uses (
IDWriteFactory5::CreateFontSetBuilder→GetSystemFontSet→AddFontSet→AddFontFile→CreateFontSet→CreateFontCollectionFromFontSet→ query forIDWriteFontCollection) is the same sequence the probe compiled and ran successfully, so the APIusage is exercised rather than merely plausible.
Honest limits — nothing in RNW was compiled or run:
yarn lint,yarn format:verify,clang-format, typecheck and the build are all unrun. The probe is separate standalone code, not
this patch.
raw.githubusercontent.combytes atc69cf55f67f9b03f467502dac1007ac2d9ebe209;git apply --checkagainst a pristine scratch copy withcore.autocrlf false→ exit 0; realgit apply→ exit 0 with 0 CRLF sequences in all threefiles;
git apply --checkagainst a CRLF-converted working copy withcore.autocrlf true(what anormal checkout produces, since
.gitattributesdeclares*.cpp text eol=crlf) → exit 0.Longest added line is 107 columns, within the 120-column
ColumnLimit— hand-counted, notclang-format-verified.
windows.sharedFontsstate. The probe machine has none ofthese fonts installed, so its
nullptrrows fail for the simple reason that the font is absentfrom the system collection. I therefore cannot claim to have measured the exact configuration in
DirectWrite text layout cannot resolve font families whose names contain spaces — 'Material Icons' renders tofu, same font renamed 'MaterialIcons' renders #16306/Registered icon-font TTFs render blank glyphs until table checksums/table directory are recomputed — silent failure in the DWrite font path #16308, only to have shown that neither spaces nor checksums are the mechanism and that
nullptrcannot resolve a font that is not installed.Assets\/Assets\Fonts\convention is a judgement call, not a measurement. It matcheswhere RNW app templates put font assets and where our production app puts them, but if maintainers
would rather this be an explicit API (an app-settable collection, or something aligned with
Implement IProvideFontInfo to unify font loading #15750's
IProvideFontInfodirection) than a directory convention, that is a reasonable objectionand I will rework it.
GetSystemFontSetplus building a merged collection happens once,on the first text layout. I have not profiled it.
AppDirectory()uses aMAX_PATHbuffer and returns empty (→nullptr→ today's behaviour) if themodule path is longer.
TextInputgoes through RichEdit, not DirectWrite textlayout, so app-bundled fonts there are unaffected by this change. The placeholder is fixed,
because
CreatePlaceholderLayoutroutes throughWindowsTextLayoutManager::GetTextLayout. ThenullptrinScrollViewComponentView.cppL474 is fine as-is — it requests the system fontSegoe Fluent Icons.Prior art
Our production app (Facilitron FIT, RNW 0.83.2) ships a near-identical patch as a
yarnpatch and itresolves app-bundled font families without the
windows.sharedFontsmanifest extension — whichmatters because that extension can be rejected during Store package acceptance. This PR is that patch
generalised and cleaned up for upstream. Related: #15316, #15750, #3463.
Change file
change/react-native-windows-fix-app-bundled-fonts.json,"type": "prerelease"(correct formain,whose
vnext/package.jsonversion is0.0.0-canary.1057; the repo's beachball transform downgradesprereleasetopatchautomatically on released branches).Microsoft Reviewers: Open in CodeFlow