Apply the bold text accessibility weight to Skia font fallback - #3325
Apply the bold text accessibility weight to Skia font fallback#3325kohei.okamoto (okaponta) wants to merge 2 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
The iOS "Bold Text" accessibility setting is implemented by IOSResolveInterceptor, which raises the requested font weight by 200. That interception happens inside FontFamilyResolver, so only the resolved typeface reflects it. ComputedStyle.toSkTextStyle() stored the raw FontWeight in SkTextStyle.fontStyle while assigning the intercepted typeface, leaving the two out of sync. Latin glyphs are taken from the typeface and render bold, but CJK glyphs are not covered by .AppleSystemUIFont and go through Skia's font fallback, which matches on fontStyle. The fallback therefore picked a regular-weight face such as Hiragino Sans W3 and the setting appeared to have no effect on CJK text. Expose the resolver's intercepted weight and use it for the weight stored in the Skia text style, so fallback faces are matched at the same weight as the primary typeface. The fontWeight == null case is covered too, since the typeface is resolved from FontWeight.Normal and is intercepted as well. Platforms whose interceptor is a no-op are unaffected: both SkFontStyle.NORMAL and SkFontStyle.ITALIC already carry weight 400, so withWeight(400) is a no-op there.
316857a to
c95207b
Compare
|
The Desktop failure looks unrelated to this change — `DialogWindowV2StateTest and doesn't touch text or fonts. Could you re-run that job when you get a chance? |
|
Thanks for the re-run! It failed on a different set of tests this time — Would it be possible to merge this? |
|
Hi kohei.okamoto (@okaponta)! If you want this change to be accepted, please make sure to do the changes (which touch the commonCode) in the AOSP project first or rewrite these without changing the common code here. |
|
Thanks for the review and the pointer! |
Contributions that touch the common code of this fork have to go through the AndroidX repository first, so the previous commit's accessor on FontFamilyResolverImpl is not acceptable here. Read the platform interceptor directly from nonAndroidMain instead. This is equivalent: createPlatformResolveInterceptor() is what createPlatformFontFamilyResolver() builds the resolver with, and on iOS it returns the same object every time. The interceptor can no longer be substituted per resolver, so the unit test that injected a fake one is replaced by a desktop test asserting that a platform without an interceptor leaves the weight untouched, which is the property that keeps desktop and web unaffected.
|
Rewrote it without touching common code — the seam is now in |
2 participants
Fixes https://youtrack.jetbrains.com/issue/CMP-10193
On iOS,
IOSResolveInterceptorraises the requested font weight for the "Bold Text" accessibility setting, but only insideFontFamilyResolver.ComputedStyle.toSkTextStyle()assigned the intercepted typeface while storing the raw weight inSkTextStyle.fontStyle, leaving the two out of sync.Latin glyphs are taken from the typeface and render bold. CJK glyphs are not covered by
.AppleSystemUIFont, so they go through Skia's font fallback, which matches onfontStyle— and therefore picked a regular-weight face such as Hiragino Sans W3.This applies the same interception to the weight stored in
fontStyle, so fallback faces are matched at the same weight as the typeface. Platforms whose interceptor is a no-op are unaffected.Testing
Added
PlatformInterceptedFontWeightTestfor the new seam.:compose:ui:ui-text:desktopTestand:compose:ui:ui-skiko:desktopTestpass locally.The rendered result is not covered by any test.
IOSResolveInterceptorreadsUIAccessibilityIsBoldTextEnabled()once at initialisation, so a test cannot toggle the setting, and CI simulators run with it off — making it testable would need the interceptor to become injectable, which felt out of scope here. I also don't have an iOS environment to check it by hand. This should be tested by QA.Release Notes
Fixes - iOS
FontFamily.DefaultGoogle CLA
Signed