ADFA-5098: Require font-scale verification for new and changed screens - #1657
ADFA-5098: Require font-scale verification for new and changed screens#1657hal-eisen-adfa wants to merge 1 commit into
Conversation
CoGo is built for developers with limited vision, but nothing in our docs asked anyone to check a screen at a large system font. REVIEW.md covered TalkBack semantics only, and CLAUDE.md had no accessibility guidance at all. Text units are already correct repo-wide (0 dp text sizes, 65 sp), so this is a layout reflow problem, not a units problem. The failure mode is text that grows into a container that cannot: an sp dimen used as a margin, a 40dp box around a label, ellipsize="none", or content with nowhere to scroll. - CLAUDE.md: new constraint bullet requiring 1.0/2.0 verification, plus the adb recipe under Build & test -> Emulator / device. - REVIEW.md: widen section 8 to cover scaling, add a checklist item and an evidence-ledger entry. Required for new/changed screens, with a one-line opt-out for surfaces with no text. - architecture-review skill: rule 12, so the section 10 deep pass checks font scale the way it checks the system bars. Manual verification rather than a test: the repo has no screenshot testing, no Compose UI-test dependency, and has never used Robolectric qualifiers. This mirrors how section 11 already handles offline verification. Every command in the new CLAUDE.md block was run against an API 36 emulator before being written down.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
📝 Walkthrough
WalkthroughAdded project-wide requirements for 1.0x and 2.0x font-scale support. Added testing, evidence, checklist, layout, scrolling, and truncation guidance. ChangesFont-scale accessibility guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/architecture-review/SKILL.md:
- Line 59: Update the font-scale guidance consistently across all sites: in
.claude/skills/architecture-review/SKILL.md lines 59-59, require verification at
both 1.0 and 2.0; in REVIEW.md lines 43-43, change the checklist to require both
scales; and in REVIEW.md lines 165-165, require evidence naming both scales or
showing screenshots at both scales.
- Line 61: Complete the applicability mapping in the architecture-review skill
so every defined rule, including Rules 3, 7, and 10, has an explicit scope.
Assign Rule 10 to UI changes and add appropriate mappings for Rules 3 and 7, or
explicitly state that unlisted rules are checked whenever the diff touches their
subject.
In `@CLAUDE.md`:
- Around line 32-39: Update the “Font-scale check” commands in CLAUDE.md to
capture the current font_scale value, install a shell trap that restores that
exact value on exit, and delete the setting when the captured value is null;
remove the hard-coded 1.0 restoration while preserving the 2.0 screenshot step.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a755a57-319d-4e8b-af57-9415a94d0557
📒 Files selected for processing (3)
.claude/skills/architecture-review/SKILL.mdCLAUDE.mdREVIEW.md
| | 9 | **Dependency substitution:** don't add a Maven coordinate for something already vendored/substituted (`build-deps*`); don't add a new dependency without checking `gradle/libs.versions.toml` first. | ADR 0003 | | ||
| | 10 | **Strings** live in the `:resources` module's `strings.xml` (not per-module, not inline literals). | REVIEW.md §7 | | ||
| | 11 | **UI never drawn over the two system bars** (top status bar, bottom navigation bar). | CLAUDE.md | | ||
| | 12 | **Text scales:** new/changed screens hold up at font scale 2.0 — `sp` for text and `dp` for spacing (no `sp` dimen used as margin/padding), no text boxed in a fixed `dp` size, a scroll container on content that can grow, and `maxLines`/`singleLine`/`ellipsize` only on genuinely disposable text. | CLAUDE.md, REVIEW.md §8 | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep the font-scale requirement consistent across all review guidance.
The project contract requires verification at 1.0 and 2.0, but these enforcement points allow a 2.0-only check.
.claude/skills/architecture-review/SKILL.md#L59-L59: require verification at font scales 1.0 and 2.0.REVIEW.md#L43-L43: change the checklist item to require 1.0 and 2.0.REVIEW.md#L165-L165: require evidence that names both scales or shows screenshots at both scales.
🧰 Tools
🪛 SkillSpector (2.5.1)
[warning] 56: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
📍 Affects 2 files
.claude/skills/architecture-review/SKILL.md#L59-L59(this comment)REVIEW.md#L43-L43REVIEW.md#L165-L165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/architecture-review/SKILL.md at line 59, Update the
font-scale guidance consistently across all sites: in
.claude/skills/architecture-review/SKILL.md lines 59-59, require verification at
both 1.0 and 2.0; in REVIEW.md lines 43-43, change the checklist to require both
scales; and in REVIEW.md lines 165-165, require evidence naming both scales or
showing screenshots at both scales.
| | 12 | **Text scales:** new/changed screens hold up at font scale 2.0 — `sp` for text and `dp` for spacing (no `sp` dimen used as margin/padding), no text boxed in a fixed `dp` size, a scroll container on content that can grow, and `maxLines`/`singleLine`/`ellipsize` only on genuinely disposable text. | CLAUDE.md, REVIEW.md §8 | | ||
|
|
||
| Rules 1, 2, 6 apply to UI changes; 4, 5 to data/model changes; 8, 9 to Gradle changes. Judge by what the diff touches — don't flag rules a file doesn't engage. | ||
| Rules 1, 2, 6, 11, 12 apply to UI changes; 4, 5 to data/model changes; 8, 9 to Gradle changes. Judge by what the diff touches — don't flag rules a file doesn't engage. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Complete the applicability mapping.
Rule 10 is defined above but is not assigned to UI changes. A UI review can therefore skip the centralized strings.xml rule. Rules 3 and 7 also have no applicability entry. Add mappings for all rules, or state that unlisted rules are checked when the diff touches their subject.
🧰 Tools
🪛 SkillSpector (2.5.1)
[warning] 56: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/architecture-review/SKILL.md at line 61, Complete the
applicability mapping in the architecture-review skill so every defined rule,
including Rules 3, 7, and 10, has an explicit scope. Assign Rule 10 to UI
changes and add appropriate mappings for Rules 3 and 7, or explicitly state that
unlisted rules are checked whenever the diff touches their subject.
| **Font-scale check.** Read the current value first so you can put it back. Each change recreates the activity (only `EditorActivityKt` declares `fontScale` in `configChanges`), so this doubles as a state-restoration test: | ||
|
|
||
| ```bash | ||
| adb shell settings get system font_scale # save it (prints "null" if never set) | ||
| adb shell settings put system font_scale 2.0 | ||
| adb exec-out screencap -p > /tmp/scale-2.0.png | ||
| adb shell settings put system font_scale 1.0 # restore | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the original font scale.
The text says to save the current value, but the commands never save it and always write 1.0. This overwrites users' existing settings. If the value is null, delete the setting instead of writing 1.0. Capture the value and restore it with a shell trap.
Proposed shell update
- adb shell settings get system font_scale # save it (prints "null" if never set)
+ original_scale="$(adb shell settings get system font_scale | tr -d '\r')"
+ restore_font_scale() {
+ if [ "$original_scale" = "null" ]; then
+ adb shell settings delete system font_scale
+ else
+ adb shell settings put system font_scale "$original_scale"
+ fi
+ }
+ trap restore_font_scale EXIT
adb shell settings put system font_scale 2.0
adb exec-out screencap -p > /tmp/scale-2.0.png
- adb shell settings put system font_scale 1.0 # restore📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Font-scale check.** Read the current value first so you can put it back. Each change recreates the activity (only `EditorActivityKt` declares `fontScale` in `configChanges`), so this doubles as a state-restoration test: | |
| ```bash | |
| adb shell settings get system font_scale # save it (prints "null" if never set) | |
| adb shell settings put system font_scale 2.0 | |
| adb exec-out screencap -p > /tmp/scale-2.0.png | |
| adb shell settings put system font_scale 1.0 # restore | |
| ``` | |
| **Font-scale check.** Read the current value first so you can put it back. Each change recreates the activity (only `EditorActivityKt` declares `fontScale` in `configChanges`), so this doubles as a state-restoration test: | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` around lines 32 - 39, Update the “Font-scale check” commands in
CLAUDE.md to capture the current font_scale value, install a shell trap that
restores that exact value on exit, and delete the setting when the captured
value is null; remove the hard-coded 1.0 restoration while preserving the 2.0
screenshot step.
2 participants
Closes ADFA-5098.
Why
CoGo is built for developers with limited vision, but nothing in our docs asked anyone to look at a screen with a large system font. REVIEW.md §8 covered TalkBack semantics only; CLAUDE.md had no accessibility guidance at all.
Text units are already right repo-wide — 0
dptext sizes, 65sp— so this isn't a units problem. It's a layout reflow problem, and the failure mode is text that grows into a container that can't. That already exists here:spdimen used as spacingapp/src/main/res/layout-land/fragment_onboarding_greeting.xml:52—@dimen/_32sp(=32sp) aslayout_marginStart/End, so the margins grow with the font scale and squeeze the text columnapp/src/main/res/layout/layout_symbol_item.xml:7— aTextButtonpinned to 40dp x 40dpapp/src/main/res/layout/fragment_build_variants.xml:50,68—ellipsize="none"app/src/main/res/layout/have a scroll containerfloating-window/.../FloatingWindowChrome.kt:203,269,293— fixed.height(44.dp)/.size(36.dp)around scaling textWhat changed
Docs only — three Markdown files, 26 insertions.
adbrecipe under Build & test -> Emulator / device..claude/skills/architecture-review/SKILL.md— rule 12, so the §10 deep pass checks font scale like it checks the system bars. Also fixes a pre-existing omission: rule 11 was UI-only but wasn't listed in the applicability line.Why manual verification rather than a test
There's no mechanized option available today: no screenshot testing (Paparazzi/Roborazzi/Shot — none in the catalog), no Compose UI-test dependency, and Robolectric
@Config(qualifiers = ...)has never been used in this repo. Adding any of those is a much larger decision than this ticket, and CLAUDE.md says to avoid new dependencies.Verification
Every command in the new CLAUDE.md block was run against
emulator-5554(API 36) before being written down, then re-run copy-pasted from the rendered doc:Both file citations in the docs were verified directly (
@dimen/_32spis32spatvalues/dimens.xml:25; it is used as a margin atlayout-land/fragment_onboarding_greeting.xml:52-53), as was the 14-of-108 scroll-container count. Spotless doesn't target Markdown (java / gradle.kts / xml / .gitignore only) and the pre-push run confirmed no reformatting.Note for reviewers
The 2.0x sweep will start finding real problems the moment anyone runs it — see the table above. Those are existing screens and deliberately not fixed here; a docs PR shouldn't become a UI PR. Worth follow-up tickets, along with the 8 of 9 activities that omit
fontScalefromconfigChanges.