From acb76fe5f7a2a7b7d6a3206054485af8d8a14e10 Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Tue, 11 Aug 2026 12:02:26 -0700 Subject: [PATCH] ADFA-5098: Require font-scale verification for new and changed screens 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/skills/architecture-review/SKILL.md | 3 ++- CLAUDE.md | 12 ++++++++++++ REVIEW.md | 14 ++++++++++++-- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/.claude/skills/architecture-review/SKILL.md b/.claude/skills/architecture-review/SKILL.md index 89d7f128df..db1d2b401f 100644 --- a/.claude/skills/architecture-review/SKILL.md +++ b/.claude/skills/architecture-review/SKILL.md @@ -56,8 +56,9 @@ For each changed first-party file, check the applicable rules. Each rule cites i | 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 | -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. For a **large diff (~15+ first-party files)**, fan out: spawn a subagent per dimension (UI/state, DI, persistence, Gradle/modules), each instructed to read the relevant ADR and report only its dimension's findings; then merge. For a small diff, do it inline. diff --git a/CLAUDE.md b/CLAUDE.md index 22b9a794b6..0e238095d1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,6 +29,17 @@ Every module carries `v7` (`armeabi-v7a`) and `v8` (`arm64-v8a`) flavors, so bui At least one Android emulator or device is available. Find it with `adb devices -l | grep -v offline`, then target it with the `ANDROID_SERIAL` env var. Note the app is **arm-only** (`v7`/`v8` flavors, no x86) — an x86_64 emulator can't run it (not always even via a translation layer), so testing often needs a **physical arm device** or an arm-translation emulator. +**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 +``` + +At 2.0, look for text cut off mid-word, labels overrunning their control, actions pushed off the bottom with no way to scroll to them, and overlapping rows. + ## Architecture See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for the module map, layering/data flow, dependency rules, tech stack (DI, async, persistence, networking), state management, and testing strategy. Don't re-document those here; update ARCHITECTURE.md. @@ -38,6 +49,7 @@ See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for th - **Avoid new dependencies** — the build almost certainly already has what's needed. Check `gradle/libs.versions.toml` and `build.gradle.kts` first. - **Persistence:** prefer **Room** for relational data and the filesystem/preferences for settings; raw SQLite only for justified exceptions — see [ADR 0001](docs/adr/0001-prefer-room-for-persistence.md). - **Protect the two Android system bars** in any UI work: the top status bar (clock, notifications, status icons) and the bottom navigation bar (home, back, recents). Don't draw over or intercept them. +- **Every screen must survive 2x font scale.** Users with low vision run large system fonts, and a screen that clips or hides content at 2.0 is broken for them. Verify any new or changed screen at font scale **1.0 and 2.0** (see Build & test, Emulator / device) and say in the PR that you did. Text grows, so: use `sp` for text and `dp` for spacing — never an `sp` dimen as a margin or padding; don't box text in a fixed `dp` height or width; give content that can grow somewhere to scroll; and reserve `maxLines`/`singleLine`/`ellipsize` for text that is genuinely disposable. - **Plan and size before building.** Prefer **one PR per ticket/use case** — don't force-split a coherent change (splitting has its own overhead when later edits span the pieces). When a change is large, break it into **reviewable commits** — mechanical/refactor commits separate from behavioral ones — and offer review-by-commit. Treat ~500 LOC / ~10 files as a signal to reach for that commit structure, not a hard cap; the ceiling rises as LLM-assisted review matures. For staged multi-commit refactors (e.g. removing a dependency across many files/modules), order stages easiest-to-hardest and independently compile/test each stage (see Build & test's fast-iteration guidance) before moving to the next, so a failure is isolated to the stage that caused it. - **Keep docs in step with code.** When you change code, update the docs that describe it in the same change — a module's `README.md`, `ARCHITECTURE.md`, or an ADR — so a doc never outlives the API it documents (see REVIEW.md, Code quality). If the doc fix is out of scope, file a ticket rather than let it drift. - `.androidide_root` is a sentinel file tests use to locate the project root — don't delete it. diff --git a/REVIEW.md b/REVIEW.md index 39de210c00..4d1d9b2e50 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -22,7 +22,7 @@ A review isn't done because it *looks* fine; it's done when you can **show what | §4 Security | Which untrusted inputs were validated; secrets checked | | §5 Tests & coverage | JaCoCo numbers for new non-UI code (line & branch) | | §7 Code quality | Duplication/cohesion pass done; no reimplementation of existing helpers | -| §8–§9 A11y & help | contentDescription + long-press help on new interactive elements | +| §8–§9 A11y & help | contentDescription + long-press help on new interactive elements; font scale 1.0/2.0 verified on new or changed screens | | §10 Architecture | Checklist below, each item pass/fail | | §13 Plugins | API-surface touched? impact check result | @@ -40,6 +40,7 @@ Keep it proportional — a two-line change needs a two-line ledger. - [ ] **Docs:** public classes/functions have KDoc/Javadoc explaining *why*, not *what*; any module `README`/`ARCHITECTURE.md`/ADR the change affects is updated in the same PR. - [ ] **Strings** are in the **`:resources`** module's `strings.xml` (not per-module, not inline literals) — keeps localization centralized. - [ ] **Accessibility:** every actionable view has a `contentDescription` (XML *or* programmatic); decorative views are marked `importantForAccessibility="no"`. +- [ ] **Font scale:** new or changed screens verified at **2.0** — nothing clipped, nothing unreachable — or explicitly noted as not applicable. - [ ] **Contextual help:** new interactive elements (and any new screen/panel) have long-press help wired to the 3-tier tooltip system. - [ ] **Analytics:** meaningful user/build actions emit an event (see below). - [ ] **Scope/size:** PR is focused on one ticket/use case; if large, it's split into **reviewable commits** (mechanical separate from behavioral) rather than force-split into multiple PRs (`CLAUDE.md`). @@ -142,7 +143,7 @@ Keep event names/params stable and low-cardinality; **no PII, file paths with us - **Strings in `strings.xml`.** User-facing text must be a string resource, never an inline literal — lint flags `HardcodedText`, and externalized strings feed our Crowdin translation flow. Use plurals/`getQuantityString` and positional args for formatting. Log messages and analytics keys are *not* user-facing and stay in code. - **Dependencies:** don't add one without checking `gradle/libs.versions.toml` first — we probably already have it (`CLAUDE.md`). -## 8. Accessibility — every actionable view speaks +## 8. Accessibility — every actionable view speaks, and every screen scales CoGo serves visually-impaired developers, so TalkBack support is a correctness requirement, not a nice-to-have (pattern set by ADFA-2667). New UI is Compose ([§10](#10-architecture-alignment) / [ADR 0009](docs/adr/0009-jetpack-compose-for-new-ui.md)), so each rule gives the View and Compose form — the requirement is the same in either. @@ -159,6 +160,15 @@ CoGo serves visually-impaired developers, so TalkBack support is a correctness r - **Externalize, with the `cd_` convention.** Content descriptions live in `strings.xml` as `cd_*` — greppable, translatable, reusable; check for an existing one first. `HardcodedText` lint does **not** catch Compose literals, so reviewers must. - **Bonus — it stabilizes tests.** Screen-reader semantics are what UI tests match on (`ACTION_CLICK` for Views, `onNodeWithContentDescription(…)` for Compose), so a11y and reliable instrumentation tests are the same work. +**Text scales, so layouts must too.** Low vision means large system fonts as often as it means TalkBack. A screen isn't done until it works at **2x**. + +- **Verify new or changed screens at font scale 1.0 and 2.0**, and put the result in the PR — a screenshot at 2.0, or one line saying what you checked. Recipe in `CLAUDE.md` (Build & test → Emulator / device). "No visual change" or "no text on this surface" is a valid one-line opt-out; silence is not. +- **Spacing in `dp`, text in `sp`.** An `sp` dimension used as a margin or padding grows with the font scale and squeezes the text it was meant to frame — as `layout-land/fragment_onboarding_greeting.xml` does today with `@dimen/_32sp`. +- **Don't box text in a fixed size.** A control sized `40dp x 40dp` can't hold a label that doubled. Let the container wrap its content and set a `minWidth`/`minHeight` for the touch target instead of a fixed one. + - *Compose:* the same trap is `Modifier.height(44.dp)`/`.size(36.dp)` on chrome that contains text — use `defaultMinSize` and let it grow. +- **Give growth somewhere to go.** Content that can reflow past the viewport needs a `NestedScrollView` (Compose: `verticalScroll`/`LazyColumn`). Only 14 of the 108 layouts in `app/src/main/res/layout/` have one today — don't add to the pile. +- **`maxLines`/`singleLine`/`ellipsize` are a decision, not a default.** Clamping is fine for a preview line, wrong for anything the user must read to proceed. `ellipsize="none"` with `maxLines` clips mid-glyph and is almost never what you want. + ## 9. Contextual help — long-press works everywhere Help in CoGo is reached by **long-press**, anywhere: a progressive three-tier experience — **Tiers 1 & 2 are tooltips** (anchored popups from `idetooltips`), **Tier 3 is a full help web page** via the tooltip's "See More" link. A long-press should never be met with silence.