-
Notifications
You must be signed in to change notification settings - Fork 38
chore: add accessibility guidelines to AGENTS.md - WPB-26264 #4830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,6 +101,38 @@ Code generation: Sourcery generates mocks (`AutoMockable`), SwiftGen generates s | |
| - Code review uses [conventional comments](https://conventionalcomments.org/) | ||
| - Cherry-picked commits are marked with 🍒 | ||
| - All colors in UI code must come from `WireDesign.ColorTheme` or `WireDesign.BaseColorPalette` | ||
| - New UI elements require `accessibilityLabel` for VoiceOver / `accessibilityIdentifier` for UI tests | ||
| - Push notifications only work with the App Store-signed build (Apple security restriction) | ||
|
|
||
| ## Accessibility | ||
|
|
||
| Every new screen or UI element must pass the following checklist before merging: | ||
|
|
||
| ### Identifiers and labels | ||
| - Set `accessibilityIdentifier` on every new element — **never remove existing ones** (QA automation depends on them) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Do we have a separate file for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure what you mean
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WireUI -> ... -> Locators |
||
| - Set `accessibilityLabel` for VoiceOver on every interactive or informative element | ||
| - Set `accessibilityHint` to describe the result of an action (e.g. "Double tap to open profile"); omit it when the label already makes the outcome obvious | ||
| - Set `accessibilityValue` for elements with dynamic state (e.g. a text field's current content) | ||
| - VoiceOver reads in order: **label → value → trait → hint** | ||
| - Put all VoiceOver strings in `Accessibility.strings`, not `Localizable.strings` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: The |
||
|
|
||
| ### Traits | ||
| - Assign the correct trait to every custom element (all native elements already have traits): `.button`, `.header`, `.staticText`, `.link`, `.selected`, etc. | ||
| - Combine traits when needed (e.g. `.button` + `.selected` for a selected toggleable button) | ||
|
|
||
| ### Hiding elements | ||
| - Hide purely decorative elements with `accessibilityHidden(true)` / `isAccessibilityElement = false` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: "purely decorative elements" is a very subjective definition. Perhaps examples or an explanation of how this phrase might be used in a real-life situation are needed. |
||
| - When a container element expresses the full meaning, hide its children and set the label/trait/hint on the container (use `.accessibilityElement(children: .ignore)` in SwiftUI) | ||
|
|
||
| ### Navigation bar | ||
| - All navigation bar buttons must have alternative text | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I wouldn't use "All navigation bar buttons", because some of them as "Next", "Start", etc. maybe don't need it. When defining them, we should always collaborate with the product team. |
||
| - Back buttons must describe the destination (e.g. "Go back to Settings", not just "Back") in the accessibilityLabel (no button's title visible) | ||
|
|
||
| ### Dynamic content | ||
| - When content changes dynamically (list updates, search results, state transitions), post a UIAccessibility notification: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I would advise caution with these examples. I understand that these were generated by AI using our code, but
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did use AI to generate this but I've tried to be as close to our guidelines as possible by leaning heavily on this document https://wearezeta.atlassian.net/wiki/spaces/IOS/pages/654475313/Development+guide. If you're saying |
||
| - `.layoutChanged` — an element appeared or disappeared | ||
| - `.screenChanged` — the major portion of the screen changed | ||
| - `.announcement` — a brief status message (e.g. "Loading…") | ||
|
|
||
| ### Dynamic type | ||
| - UI elements that display text must use the API that supports dynamic type / large fonts | ||
| - Push notifications only work with the App Store-signed build (Apple security restriction) | ||
| - Test with the largest accessibility font size (Settings → Accessibility → Larger Text) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: We also have snapshot tests for dynamic text. You can mention them here. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I wonder if this section can be pulled into context on demand so we don't stuff too much info in the context. Not sure how big should be this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pasting Claude's opinion on this
With a 1M context window, AGENTS.md is essentially free — even at 100× its current size (600 KB) it would only consume ~150K tokens. The context window is not the binding constraint here.
The practical limits are different:
Human readability — AGENTS.md is read by engineers, not just the AI. Once it exceeds ~300–400 lines it becomes hard to scan and maintain. That's probably the real ceiling (~15–20 KB).
Signal dilution — the longer it gets, the more the AI has to weight competing instructions. Dense, specific rules work better than long explanations. The accessibility section we just added is already on the edge of this — it's detailed because the topic is complex, but adding many more sections like it would start to dilute the more critical conventions at the top.
What to watch for — if you find yourself adding large "how-to" or tutorial content to AGENTS.md, that's a sign it belongs in a separate skill or referenced doc instead. AGENTS.md should stay as a checklist/ruleset, not a guide.
At the current growth rate, you have plenty of room — but keep sections tight.