From a5823cea4efae4e06809c1c1e83a465919d68bbf Mon Sep 17 00:00:00 2001 From: Sergey Fedorov Date: Tue, 21 Jul 2026 21:28:23 +0000 Subject: [PATCH 08/13] fix(vendor): verify by-content font fallback actually covers the codepoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveByCoverage() (added by 1aba69e6's CJK/emoji fallback) accepted the first locator candidate that merely opened as a font file, without checking it actually maps the requested codepoint to a real glyph. A locator's answer is only a plausible cascade match, not a guarantee -- CoreText's CTFontCreateForString is explicitly best-effort. For an isolated CJK punctuation mark (e.g. U+300A/U+300B "《"/"》", which does not merge with covered surrounding ASCII, so it is resolved alone) this let a font that "generally" seemed CJK-capable win the coverage cache permanently while actually lacking that specific glyph, silently rendering nothing instead of falling through to a font that does cover it or, failing that, the primary font's own replacement-glyph box. A dropped-but-uncopyable character is worse than a visible tofu box: it made filenames containing such punctuation impossible to address from the shell. Fixed by checking FT_Get_Char_Index on each candidate before accepting it. Regression test added mirroring the existing coverage-cache tests, using a locator that lists a non-covering font first. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LzTmVgP2ruMz987VJ78k77 (cherry picked from commit f5b10575590b8e31b94297636f6e57c43fac2a17) --- src/text_shaper/OpenShaper.cpp | 30 +++++++++++++++++++++-------- src/text_shaper/OpenShaper_test.cpp | 29 ++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/src/text_shaper/OpenShaper.cpp b/src/text_shaper/OpenShaper.cpp index 2e2ea49d..d05528ec 100644 --- a/src/text_shaper/OpenShaper.cpp +++ b/src/text_shaper/OpenShaper.cpp @@ -990,14 +990,28 @@ struct OpenShaper::PrivateOpenShaper // {{{ auto resolved = optional { nullopt }; for (auto const& source: locator->resolve(gsl::span(codepoints.data(), codepoints.size()))) { - resolved = getOrCreateKeyForFont(source, fontInfo.size, fontInfo.weight); - if (resolved.has_value()) - { - textShapingLog()("Resolved U+{:04X} by coverage to font key:{}.", - static_cast(cacheKey), - *resolved); - break; - } + auto const candidate = getOrCreateKeyForFont(source, fontInfo.size, fontInfo.weight); + if (!candidate.has_value()) + continue; + + // The locator only promises this font is a plausible cascade match for the script/ + // language in general (e.g. CoreText's CTFontCreateForString on 10.6 is a best-effort + // substitution, not a guarantee) -- it may still lack the SPECIFIC codepoint we asked + // about. Accepting it unconditionally here previously let such a font "win" the coverage + // cache permanently (see cacheKey below), silently rendering as a blank/absent glyph + // instead of falling through to the next candidate or, ultimately, the primary font's own + // replacement-glyph box -- a regression that made isolated CJK punctuation (e.g. U+300A/ + // U+300B, unmerged with surrounding covered ASCII by mergeAdjacentMissing) disappear + // entirely rather than render as tofu. + auto* const ftFace = fontKeyToHbFontInfoMapping.at(*candidate).ftFace.get(); + if (!FT_Get_Char_Index(ftFace, cacheKey)) + continue; + + resolved = candidate; + textShapingLog()("Resolved U+{:04X} by coverage to font key:{}.", + static_cast(cacheKey), + *resolved); + break; } coverageCache[cacheKey] = diff --git a/src/text_shaper/OpenShaper_test.cpp b/src/text_shaper/OpenShaper_test.cpp index 1475d05c..4bbf5e14 100644 --- a/src/text_shaper/OpenShaper_test.cpp +++ b/src/text_shaper/OpenShaper_test.cpp @@ -677,6 +677,35 @@ TEST_CASE("OpenShaper.coverage.is_spent_once_per_span", "[OpenShaper][fallback]" CHECK(result[1].glyph.index.value == replacement->glyph.index.value); } +TEST_CASE("OpenShaper.coverage.skips_a_candidate_that_opens_but_does_not_cover_the_codepoint", + "[OpenShaper][fallback]") +{ + // Regression test. The locator only promises a candidate is a plausible cascade match (e.g. + // CoreText's CTFontCreateForString is best-effort, not a guarantee) -- resolveByCoverage() used + // to accept the FIRST candidate that merely opened, without checking it actually maps the + // codepoint to a real glyph. A font that opens but lacks the specific codepoint (isolated CJK + // punctuation such as U+300A/U+300B is a real-world case: it doesn't merge with surrounding + // covered ASCII, so it is resolved alone) then "won" the coverage cache permanently, silently + // rendering nothing instead of falling through to a candidate that actually covers it. + auto uselessFont = BDFFont { "useless", Monospaced, { { U'B', 8 } } }; + auto coverageFont = BDFFont { "coverage", Monospaced, { { Snowman, 8 } } }; + + auto env = FallbackEnv { { + { .name = "primary", .monospace = Monospaced, .glyphs = { { U'A', 8 }, { Replacement, 8 } } }, + } }; + // Locator lists the non-covering font FIRST: the fix must not stop there. + MockFontLocator::configureCoverage({ uselessFont.source(), coverageFont.source() }); + + auto const primary = env.key("primary"); + auto const result = shapeCells(env.shaper(), primary, u32string { U'A', Snowman }); + REQUIRE(result.size() == 2); + + CHECK(result[0].glyph.font == primary); + // Must have come from the SECOND candidate, not settled for the primary's replacement glyph. + CHECK(result[1].glyph.font != primary); + CHECK(result[1].glyph.index.value != 0); +} + TEST_CASE("OpenShaper.resizeFont.reports_not_resized_by_returning_the_key_it_was_given", "[OpenShaper][resize]") {