Accessibility fixes: disable animations when screen reader detected; add CLI flag to disable them manually #4014
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
aibrahim-oai
left a comment
aibrahim-oai
left a comment
There was a problem hiding this comment.
Very useful! Please address the comments and solve merge conflicts.
| use crate::accessibility::animations_enabled; | ||
| use crate::onboarding::onboarding_screen::KeyboardHandler; | ||
| use crate::onboarding::onboarding_screen::StepStateProvider; | ||
| use crate::shimmer::shimmer_spans; |
There was a problem hiding this comment.
Can you keep the shimmer spans API the same by putting your changes behind shimmer_spans
| let area = Rect::new(0, 0, MIN_ANIMATION_WIDTH, MIN_ANIMATION_HEIGHT); | ||
| let mut buf = Buffer::empty(area); | ||
| (&widget).render(area, &mut buf); | ||
| const SCREEN_READER_ENV_VARS: [&str; 5] = [ |
There was a problem hiding this comment.
let's just have a config flag to disable animations and update the readme to explain the usage of it. Let's not try to detect it.
codex-rs/tui/src/accessibility.rs
Outdated
| @@ -0,0 +1,280 @@ | |||
| use std::sync::OnceLock; | |||
There was a problem hiding this comment.
Let's just have a flag for disabling the animations
| start: Instant, | ||
| enabled: bool, | ||
| } | ||
|
|
There was a problem hiding this comment.
let's not render the ascii at all when no animation is set on instead of this flag.
There was a problem hiding this comment.
Alright, I think I've addressed the comments here, change wise. May have to update the readme but sorry it took a while.
The welcome screen now instantiates the disabled animation variant when --no-animations is set, skips scheduling/rendering entirely when no frames exist, and the onboarding tests were refreshed to assert the new behavior.
I've removed the overengineered screen reader detection and such. Should I just remove the CLI flag entirely? I figured most users want to see the ascii animation by default.
There was a problem hiding this comment.
Should I just remove the CLI flag entirely? I figured most users want to see the ascii animation by default.
I don't get this part. Isn't the animation on by default and the flag is for disabling it? If you removed the flag, how can we disable animations?
- Remove screen reader detection logic and environment variable checks - Simplify shimmer_spans to check animations flag internally - Remove shimmer_spans_with_animation_control helper function - Update all call sites to use simplified shimmer_spans API - Simplify tests to focus on CLI flag behavior only - Remove is_screen_reader_active public export
Resolved conflicts in: - Cargo.lock: accepted upstream schannel 0.1.28 - tui/Cargo.toml: accepted upstream workspace dependency format - tui/src/cli.rs: merged both --no-animations and --add-dir flags - tui/src/lib.rs: merged both accessibility and additional_dirs modules - tui/src/shimmer.rs: merged all imports (animations_enabled, color blend, terminal palette) - tui/src/status_indicator_widget.rs: merged both animations_enabled and show_interrupt_hint fields - tui/src/new_model_popup.rs: removed (deleted upstream) Preserved PR review simplifications: - CLI-only approach for animation control (no screen reader detection) - Uses AtomicBool for thread-safe CLI flag state All tests pass (460/460 when run sequentially).
|
@codex review this |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Check if animations are enabled (i.e., not disabled by CLI flag). | ||
| pub(crate) fn animations_enabled() -> bool { | ||
| !animations_disabled_by_cli() | ||
| } |
There was a problem hiding this comment.
The new accessibility module only tracks the CLI switch. animations_enabled() (lines 21‑24) simply returns !animations_disabled_by_cli() and nothing in this commit ever sets that atomic based on screen reader state or environment variables. As a result, the feature described in the commit title—automatically disabling animations when a screen reader is active—never triggers; unless the user explicitly passes --no-animations, the onboarding animation, shimmer, and status widgets keep animating and the original accessibility issue remains unresolved.
Useful? React with 👍 / 👎.
|
@codex Address your feedback: Remove the CLI switch and the animation and shimmer entirely as screen reader detection is not feasible. |
|
Note To use Codex here, create an environment for this repo. |
|
It looks like there are a few small merge conflicts that need to be resolved. |
|
Thank you, I'll try to get to them when I can. Feel free to help me resolve if possible.
…________________________________
From: Eric Traut ***@***.***>
Sent: Sunday, November 9, 2025 1:13 PM
To: openai/codex ***@***.***>
Cc: Orinks ***@***.***>; Author ***@***.***>
Subject: Re: [openai/codex] Accessibility fixes: disable animations when screen reader detected; add CLI flag to disable them manually (PR #4014)
[https://avatars.githubusercontent.com/u/223546144?s=20&v=4]etraut-openai left a comment (openai/codex#4014)<#4014 (comment)>
It looks like there are a few small merge conflicts that need to be resolved.
—
Reply to this email directly, view it on GitHub<#4014 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AJFLE3E2H2OWMLK6I6WGXA333575RAVCNFSM6AAAAACHDH74H2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKMBYGY2TCMZTGI>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
This PR adds support for a new feature flag `tui.animations`. By default, the TUI uses animations in its welcome screen, "working" spinners, and "shimmer" effects. This animations can interfere with screen readers, so it's good to provide a way to disable them. This change is inspired by [a PR](#4014) contributed by @Orinks. That PR has faltered a bit, but I think the core idea is sound. This version incorporates feedback from @aibrahim-oai. In particular: 1. It uses a feature flag (`tui.animations`) rather than the unqualified CLI key `no-animations`. Feature flags are the preferred way to expose boolean switches. They are also exposed via CLI command switches. 2. It includes more complete documentation. 3. It disables a few animations that the other PR omitted.
This PR adds support for a new feature flag `tui.animations`. By default, the TUI uses animations in its welcome screen, "working" spinners, and "shimmer" effects. This animations can interfere with screen readers, so it's good to provide a way to disable them. This change is inspired by [a PR](openai#4014) contributed by @Orinks. That PR has faltered a bit, but I think the core idea is sound. This version incorporates feedback from @aibrahim-oai. In particular: 1. It uses a feature flag (`tui.animations`) rather than the unqualified CLI key `no-animations`. Feature flags are the preferred way to expose boolean switches. They are also exposed via CLI command switches. 2. It includes more complete documentation. 3. It disables a few animations that the other PR omitted.
3 participants
Related to issue #3933
I took a shot at solving this. Wasn't sure of the best way to handle screen reader detection, as its platform specific. I'm not certain environment vars are the best way.
✅ TUI Accessibility Implementation Complete
Summary
All accessibility improvements for screen readers have been successfully implemented in the Codex TUI. The implementation addresses the core issue where ASCII animations were causing screen readers to continuously announce changing content during the onboarding process.
What Was Implemented
1. ✅ Screen Reader Detection Infrastructure
Location: /home/usr/codex/codex-rs/tui/src/accessibility.rs
•
Environment Variable Detection: Checks for NVDA_RUNNING, JAWS, ORCA_RUNNING, SPEECHD_RUNNING, ACCESSIBILITY_ENABLED
•
Caching System: Results are cached for application lifetime using OnceLock<RwLock<Option>>
•
Robust Parsing: Handles various truthy/falsy values ("0", "false", "no", "off", "disabled")
•
Comprehensive Tests: 13 test cases covering all scenarios including edge cases
2. ✅ Animation Control System
Location: /home/usr/codex/codex-rs/tui/src/ascii_animation.rs
•
Enabled/Disabled State: set_enabled(bool) and is_enabled() methods
•
Frame Scheduling Control: schedule_next_frame() skips scheduling when disabled
•
Empty Frame Return: current_frame() returns empty string when disabled
•
Graceful Degradation: All animation methods handle disabled state properly
3. ✅ Welcome Screen Integration
Location: /home/usr/codex/codex-rs/tui/src/onboarding/welcome.rs
•
Automatic Detection: Integrates with animations_enabled() function
•
Animation Disabling: Sets animation to disabled state when screen reader detected
•
No Content Changes: Preserves existing welcome text without additions
•
Test Coverage: Comprehensive tests verify screen reader behavior
4. ✅ CLI Flag Support
Location: /home/usr/codex/codex-rs/tui/src/cli.rs and /home/usr/codex/codex-rs/tui/src/lib.rs
•
--no-animations Flag: Already defined in CLI structure
•
Global State Management: Uses AtomicBool for thread-safe CLI flag storage
•
Integration: Wired through set_cli_animations_disabled() in main function
•
Combined Logic: Works alongside screen reader detection
5. ✅ Status Indicator Widget
Location: /home/usr/codex/codex-rs/tui/src/status_indicator_widget.rs
•
Shimmer Effect Control: Respects animation settings via animations_enabled()
•
Frame Rate Adjustment: Reduces from 32ms to 1s when animations disabled
•
Graceful Fallback: Shows plain text instead of animated shimmer
How It Works
Detection Flow
pub(crate) fn animations_enabled() -> bool {
!(is_screen_reader_active() || animations_disabled_by_cli())
}
Integration Points
1.
CLI Processing: set_cli_animations_disabled(cli.no_animations) in main
2.
Widget Creation: WelcomeWidget::new() checks animations_enabled()
3.
Animation Control: AsciiAnimation::set_enabled(animations_enabled())
4.
Frame Scheduling: Skipped when !self.enabled in schedule_next_frame()
Testing Coverage
Screen Reader Detection Tests
•
✅ Environment variable detection for all 5 supported variables
•
✅ Empty and whitespace value handling
•
✅ False-like value parsing ("0", "false", "no", etc.)
•
✅ Caching behavior verification
•
✅ Multiple variables set scenarios
Animation Control Tests
•
✅ Animation disabling when screen reader detected
•
✅ Animation disabling when CLI flag set
•
✅ Frame scheduling prevention when disabled
•
✅ Empty frame return when disabled
•
✅ Render behavior with/without animations
User Experience
Before (Problematic)
•
Screen readers announced every ASCII frame change (every 80ms)
•
Continuous stream of meaningless ASCII art announcements
•
Poor accessibility experience during onboarding
After (Fixed)
•
Screen readers get single announcement of welcome text
•
No ASCII animation content announced
•
Clean, accessible onboarding experience
•
Manual override available via --no-animations flag
Environment Variables Supported
VARIABLE
SCREEN READER
PLATFORM
NVDA_RUNNING
NVDA
Windows
JAWS
JAWS
Windows
ORCA_RUNNING
Orca
Linux
SPEECHD_RUNNING
Speech Dispatcher
Linux
ACCESSIBILITY_ENABLED
Generic
Cross-platform
Usage Examples
Automatic Detection
Screen reader automatically detected
NVDA_RUNNING=1 codex
Manual Override
Force disable animations
codex --no-animations
Combined Usage
Both detection and manual override work together
ORCA_RUNNING=1 codex --no-animations
Implementation Quality
•
Thread-Safe: Uses AtomicBool and RwLock for concurrent access
•
Performance: Cached detection avoids repeated environment variable reads
•
Robust: Handles edge cases and malformed environment values
•
Tested: Comprehensive test suite with 13+ test cases
•
Minimal Impact: No changes to existing functionality when animations enabled
•
Backwards Compatible: Existing behavior preserved for non-screen-reader users
The implementation successfully solves the original problem of screen readers being overwhelmed by ASCII animations while maintaining the visual experience for sighted users.