Fix(accessibility): add screen reader support to RewindViewer#20750
Fix(accessibility): add screen reader support to RewindViewer#20750Adib234 merged 10 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the accessibility of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request aims to add screen reader support for the /rewind command. The approach of adding a conditional rendering path for screen reader users is correct. However, the implementation for the screen reader view currently renders a static list of text, which is not interactive. My review includes a critical fix to make the list interactive by using the BaseSelectionList component, which provides the necessary keyboard navigation and selection functionality and aligns with existing UI patterns. This change ensures that the accessibility feature works as intended.
| {items.map((item, idx) => ( | ||
| <Text key={item.key}> | ||
| {idx + 1}.{' '} | ||
| {item.value.id === 'current-position' | ||
| ? 'Stay at current position' | ||
| : getCleanedRewindText(item.value)} | ||
| </Text> | ||
| ))} |
There was a problem hiding this comment.
The current implementation for the screen reader view renders a static list of items using items.map(). This makes the list non-interactive, as the key press handlers for navigation (arrow keys) and selection (Enter) from BaseSelectionList are not being used. As a result, screen reader users can see the list but cannot interact with it, which prevents them from using the rewind functionality and negates the goal of this accessibility fix.
To address this, you should use the BaseSelectionList component for the screen reader view as well, but with a simplified renderItem function that produces plain text output. This will provide the necessary keyboard interactivity.
As a follow-up, you could extract the onSelect handler into a named function to avoid duplicating it between the screen reader and visual rendering paths.
<BaseSelectionList
items={items}
initialIndex={items.length - 1}
isFocused={true}
showNumbers={false}
wrapAround={false}
onSelect={(item: MessageRecord) => {
if (item?.id) {
if (item.id === 'current-position') {
onExit();
} else {
selectMessage(item.id);
}
}
}}
renderItem={(itemWrapper) => {
const item = itemWrapper.value;
const text =
item.id === 'current-position'
? 'Stay at current position'
: getCleanedRewindText(item);
return (
<Text>
{itemWrapper.index + 1}. {text}
</Text>
);
}}
/>
References
- Maintain consistency with existing UI behavior across components. Defer non-standard UX pattern improvements to be addressed holistically rather than in a single component.
There was a problem hiding this comment.
Updated successfully
|
Great work on improving the accessibility of the /rewind command. Here are a few points to address before merging:
|
|
Hi @Adib234 , addressed all 6 points from your review:
All 14 tests passing. Let me know if anything needs further adjustment. |
PR 20750 - Review FindingsThank you for the accessibility improvements! Here are some important issues that should be addressed:
Fixing these will ensure the feature actually works as intended for screen reader users and is properly tested. |
|
Hi @Adib234, I have addressed all 3 points:
Let me know if anything needs further adjustment. |
|
Hi @Famous077, thank you for the PR! The accessibility improvements for I have a few points for cleanup and correctness:
Once these are addressed, the PR should be in great shape! |
|
Hi @Adib234 , Can you check and review now. I have changed everything as per you suggestions. let me know if anything else need to be implemented. |
Summary:
The /rewind command was completely inaccessible to screen reader users (NVDA, Braille displays). Once triggered, users were trapped in a silent visual-only state with no audible feedback and no standard way to exit — forcing them to use Ctrl+C twice to recover. This fix makes the /rewind interface accessible by rendering a screen reader-friendly text view when screen reader mode is active.
Details:
Added a conditional rendering path in RewindViewer.tsx using the useIsScreenReaderEnabled() hook from ink — the same pattern already used in Notifications.tsx. When a screen reader is detected, the visual BaseSelectionList is replaced with a simple numbered text list of conversation points, along with clear navigation instructions. This mirrors the accessible behavior of the /resume command which already works correctly with screen readers.
Related Issues
Fixed issue : #20285
How to Validate
Set screenReaderMode: true in ~/.gemini/settings.json
Run gemini in interactive mode
Type /rewind and press Enter
Verify a numbered text list of conversation points is displayed
Verify navigation instructions are visible
Press Esc to confirm exit works correctly
Without screen reader mode, confirm the original visual interface is unchanged
Pre-Merge Checklist