fix(editor): sync TalkBack accessibility focus with the text cursor#1509
Open
makhlwf wants to merge 2 commits into
Open
fix(editor): sync TalkBack accessibility focus with the text cursor#1509makhlwf wants to merge 2 commits into
makhlwf wants to merge 2 commits into
Conversation
The editor wraps Rosemoe's CodeEditor, a custom-drawn View. Its accessibility node reported the whole document as a single generic editable node (class name CodeEditor) and advertised no text-movement granularities, ACTION_SET_SELECTION or granularity actions. TalkBack therefore had no way to move the real text caret with the reading position: double-tapping a word/line placed the caret at the node's centre and subsequent typing landed on the wrong line. Make IDEEditor behave like a standard EditText for screen readers: - Report as android.widget.EditText and advertise CHARACTER/WORD/LINE movement granularities plus ACTION_SET_SELECTION and the next/previous-at-granularity actions. - Handle those actions by converting absolute char indices to (line, column) via the editor's Indexer and moving the real caret, then emit TYPE_VIEW_TEXT_TRAVERSED_AT_MOVEMENT_GRANULARITY so the spoken segment and caret stay in sync. - Track the last touch-exploration hover point and, on ACTION_CLICK, place the caret there so a double-tap lands on the explored word/line (only active while touch exploration is on, so sighted use is unaffected). Text-segment iteration lives in EditorAccessibilitySegments (character and word mirror the framework's AccessibilityIterators; line walks logical '\n'-separated lines) and is covered by unit tests.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Author
|
Can someone please build it for me so I can test it? I don't think there is a GitHub action to build it. I followed the documentation, but I need a device to be able to build it, and I don't think I can get one. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 Walkthrough
WalkthroughIDEEditor now exposes EditText-style accessibility semantics, supports TalkBack selection and movement actions, synchronizes caret placement from touch exploration, and announces traversed text using new character, word, and line segment calculations. ChangesAccessibility cursor support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TalkBack
participant IDEEditor
participant EditorAccessibilitySegments
participant ContentIndexer
TalkBack->>IDEEditor: Request movement action
IDEEditor->>EditorAccessibilitySegments: Calculate next or previous segment
EditorAccessibilitySegments-->>IDEEditor: Return segment indices
IDEEditor->>ContentIndexer: Convert indices to line and column
ContentIndexer-->>IDEEditor: Return editor positions
IDEEditor-->>TalkBack: Update selection and announce traversed text
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt (2)
701-726: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
sendTextTraversedEventcopies the entire document on every granularity move.
event.text.add(text.toString())(line 724) makes a full string copy of the document each time, on top of the copies already made insideEditorAccessibilitySegments. This only runs while an accessibility service is enabled, so impact is limited to that population, but for large files it compounds the per-navigation cost flagged inEditorAccessibilitySegments.kt.🤖 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 `@editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt` around lines 701 - 726, The sendTextTraversedEvent method copies the entire document for every accessibility navigation event. Remove the event.text.add(text.toString()) operation and preserve the event metadata and dispatch flow without creating a full-document string copy.
612-615: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer narrower exception types over broad
catch (e: Exception).Based on learnings, this project prefers catching only the specific exception types expected from the call (e.g.
IndexOutOfBoundsException/IllegalArgumentExceptionfromsetSelection/setSelectionRegion/getCharPosition) rather than a catch-all, to fail fast during development and surface unexpected errors instead of silently swallowing them.Also applies to: 695-698
🤖 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 `@editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt` around lines 612 - 615, Replace the broad catch blocks around the accessibility cursor-placement logic in IDEEditor with only the expected IndexOutOfBoundsException and IllegalArgumentException types from setSelection, setSelectionRegion, or getCharPosition; preserve the existing logging and false-return behavior for those cases while allowing unexpected exceptions to propagate.Source: Learnings
editor/src/test/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegmentsTest.kt (1)
73-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a mid-word offset case for WORD granularity.
All current word tests use offsets that already sit at word/whitespace boundaries. Adding a case like
following("hello", word, 2)would have caught the mid-word truncation bug flagged inEditorAccessibilitySegments.kt(word skip-loop doesn't snap back to the enclosing word's start when the offset is already inside a word).🤖 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 `@editor/src/test/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegmentsTest.kt` around lines 73 - 105, The WORD navigation tests lack coverage for offsets inside an existing word. Add a mid-word case in the `word following` tests, such as calling `following("hello", word, 2)` and asserting the complete word range `[0, 5]`, so `EditorAccessibilitySegments` preserves the enclosing word instead of truncating it.editor/src/main/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegments.kt (1)
92-92: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFresh
BreakIterator+ fulltext.toString()copy on every character/word move.Each call to
characterFollowing/characterPreceding/wordFollowing/wordPrecedingallocates a newBreakIteratorand copies the entire document viatext.toString(). Since these are invoked on every single TalkBack character/word navigation action, this is an O(n) allocation per keystroke-equivalent gesture — noticeable on large source files. Consider caching the last(text, BreakIterator)pair (invalidate on text-identity change) or backing the iterator with aCharacterIteratoradapter over theCharSequenceinstead of copying toString.Also applies to: 106-106, 124-124, 139-139
🤖 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 `@editor/src/main/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegments.kt` at line 92, Update the character and word navigation methods around the BreakIterator usages to avoid creating a new iterator and copying the entire CharSequence on every call. Cache and reuse the BreakIterator with the last text identity, invalidating it when the source text changes, or use a CharacterIterator adapter backed directly by the CharSequence; apply the same optimization to characterFollowing, characterPreceding, wordFollowing, and wordPreceding.
🤖 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 `@editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt`:
- Around line 550-564: Update IDEEditor’s getAccessibilityClassName() to return
the EditText class name only when !isReleased, isEnabled, and isEditable,
matching createAccessibilityNodeInfo(). For all other states, return the
superclass accessibility class name so read-only or released editors do not
expose edit-box semantics in accessibility events.
In
`@editor/src/main/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegments.kt`:
- Around line 121-148: Update wordFollowing and wordPreceding to first snap
mid-word offsets to the enclosing word’s start or end boundary, so navigation
never returns a truncated range such as [2, 5] or [0, 2] for “hello”. Preserve
existing scanning for whitespace and punctuation, and add a regression test in
EditorAccessibilitySegmentsTest.kt covering mid-word offsets.
---
Nitpick comments:
In `@editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt`:
- Around line 701-726: The sendTextTraversedEvent method copies the entire
document for every accessibility navigation event. Remove the
event.text.add(text.toString()) operation and preserve the event metadata and
dispatch flow without creating a full-document string copy.
- Around line 612-615: Replace the broad catch blocks around the accessibility
cursor-placement logic in IDEEditor with only the expected
IndexOutOfBoundsException and IllegalArgumentException types from setSelection,
setSelectionRegion, or getCharPosition; preserve the existing logging and
false-return behavior for those cases while allowing unexpected exceptions to
propagate.
In
`@editor/src/main/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegments.kt`:
- Line 92: Update the character and word navigation methods around the
BreakIterator usages to avoid creating a new iterator and copying the entire
CharSequence on every call. Cache and reuse the BreakIterator with the last text
identity, invalidating it when the source text changes, or use a
CharacterIterator adapter backed directly by the CharSequence; apply the same
optimization to characterFollowing, characterPreceding, wordFollowing, and
wordPreceding.
In
`@editor/src/test/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegmentsTest.kt`:
- Around line 73-105: The WORD navigation tests lack coverage for offsets inside
an existing word. Add a mid-word case in the `word following` tests, such as
calling `following("hello", word, 2)` and asserting the complete word range `[0,
5]`, so `EditorAccessibilitySegments` preserves the enclosing word instead of
truncating it.
🪄 Autofix (Beta)
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
Run ID: b2210ce8-0e06-4975-846d-db11ef1fa261
📒 Files selected for processing (3)
editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kteditor/src/main/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegments.kteditor/src/test/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegmentsTest.kt
- getAccessibilityClassName() now mirrors the !isReleased && isEnabled && isEditable guard used by createAccessibilityNodeInfo(), so a read-only or released editor is no longer announced as an "Edit box". - Word granularity navigation snaps a mid-word offset to the enclosing word's start/end, so navigation traverses the whole word (e.g. "hello"@2 -> [0,5]) instead of a truncated range. Adds regression tests for mid-word offsets. - Narrow the accessibility catch blocks to IndexOutOfBoundsException / IllegalArgumentException instead of a broad Exception, per project convention.
Author
|
Thanks for the review @coderabbitai. Addressed in Major (both fixed)
Nitpicks
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
On the two deferred items, your reasoning holds up:
Nice work getting 🐰✨ |
makhlwf
force-pushed
the
adfa-editor-a11y-cursor-sync
branch
from
7ca9295 to
e88685a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The code editor is very hard to use with TalkBack. Double-tapping a word/line fails to sync the text cursor with the accessibility focus — the caret disappears or lands in the wrong place, and typing is inserted on the wrong line (e.g. line 8 instead of line 3). Users are forced into the screen reader's native edit box as a workaround.
Root cause: the editor wraps Rosemoe's
CodeEditor, a custom-drawnView. Its accessibility node reports the whole document as a single generic editable node (class nameCodeEditor) and advertises no text-movement granularities,ACTION_SET_SELECTION, or granularity actions. So TalkBack has no way to move the real text caret in step with the reading position; a double-tap sendsACTION_CLICKto the node centre.Fix
Make
IDEEditorbehave like a standardEditTextfor screen readers:android.widget.EditTextand advertisesCHARACTER/WORD/LINEmovement granularities plusACTION_SET_SELECTIONand the next/previous-at-granularity actions on the node.(line, column)via the editor'sIndexerand moving the real caret (setSelection/setSelectionRegion), then emitsTYPE_VIEW_TEXT_TRAVERSED_AT_MOVEMENT_GRANULARITYso the spoken segment and the caret stay in sync.dispatchHoverEvent) and, onACTION_CLICK, places the caret at that on-screen point viagetPointPositionOnScreen. Gated on touch-exploration being enabled, so normal (sighted) use is unaffected.Text-segment iteration lives in the new
EditorAccessibilitySegments(character and word mirror the framework'sAccessibilityIterators; line walks logical\n-separated lines).Testing
EditorAccessibilitySegmentsTest(17 tests, Robolectric + Truth) covering following/preceding for character, word and line, plus empty-text / boundary / unsupported-granularity guards. Green via:editor:testV8DebugUnitTest.:editormodule compiles clean (no new warnings).Not covered here
On-device TalkBack behaviour (double-tap placement + granular navigation on real hardware) was not verified in CI — the wiring follows the documented Android
EditTextaccessibility contract and is unit-tested, but a manual TalkBack pass is recommended before release.