fix(editor): sync TalkBack accessibility focus with the text cursor#1509
fix(editor): sync TalkBack accessibility focus with the text cursor#1509makhlwf wants to merge 5 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}
|
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIDEEditor now exposes EditText-style accessibility semantics, supports TalkBack selection and movement actions, synchronizes caret placement from touch exploration, and announces text and selection changes using character, word, line, and paragraph 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)
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}
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.
|
Thanks for the review @coderabbitai. Addressed in Major (both fixed)
Nitpicks
|
|
Tip For best results, initiate chat on the files or code changes.
On the two deferred items, your reasoning holds up:
Nice work getting 🐰✨ |
7ca9295 to
e88685a
Compare
… events Two TalkBack gaps surfaced in on-device testing: 1. Line and paragraph navigation didn't work. PARAGRAPH granularity was never advertised, and the editor never emitted TYPE_VIEW_TEXT_SELECTION_CHANGED, so the screen reader could not track the caret across the larger granularities. Advertise + implement PARAGRAPH (non-empty logical lines, blank lines skipped) and fire a selection-changed event on every selection change. 2. Inserted/deleted characters were not announced. Emit TYPE_VIEW_TEXT_CHANGED on content changes with fromIndex, added/removed counts and beforeText reconstructed from the change, so TalkBack speaks typed and deleted text like a standard EditText. Both event senders are no-ops when no accessibility service is enabled. Adds unit tests for paragraph navigation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/utils/EditorAccessibilitySegments.kt`:
- Around line 210-234: Update paragraphFollowing and paragraphPreceding in
editor/src/main/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegments.kt
to mirror lineFollowing/linePreceding for mid-paragraph offsets, advancing to
the next paragraph or truncating to the preceding paragraph as appropriate. Add
regression tests in
editor/src/test/java/com/itsaky/androidide/editor/utils/EditorAccessibilitySegmentsTest.kt
covering paragraphFollowing("abc\ndef\nghi", 5) → [8,11] and
paragraphPreceding("abc\ndef\nghi", 5) → [4,7].
- Around line 210-234: Update paragraphPreceding so that after locating the
preceding paragraph’s start, it scans end forward to the next newline or text
length instead of returning the raw offset. Preserve the existing handling for
trailing newlines, out-of-range offsets, and missing preceding paragraphs.
🪄 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: 0b81e6ae-8aad-420e-b085-6121ce76b375
📒 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
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt
paragraphPreceding returned the raw offset as its end bound, so a mid-paragraph offset yielded a truncated range (e.g. "abc\ndef\nghi"@5 gave [4,5] instead of [4,7]). paragraphFollowing also snapped to the current line instead of advancing. Rework both to mirror lineFollowing/linePreceding (advance to the next line forward; return the whole enclosing line backward) while still skipping blank lines, and add mid-paragraph regression tests.
|
I'm starting to look at this. |
|
Hi @makhlwf — first, thank you. This is thoughtful, careful work on something that genuinely matters, and it's clear you put real care into it: the code is clean, the You asked if someone could build and test this on a device, since you couldn't — so I did. I rebased your branch onto current What checked out:
What happened with TalkBack actually running, though, is where I think we hit a wall:
I dug into why, and I don't think it's a bug in your handlers — I think it's the underlying model. The editor exposes its whole document to accessibility as a single, flat node (that comes from Rosemoe's base
The direction that I think actually gets there is exposing a virtual view hierarchy via The good news: your One gentle process suggestion for next time, and it would have helped here: try to work in tiny, incremental baby steps spread across several small PRs, rather than one large change. This PR is 700+ lines, which makes it hard to tease apart — when something doesn't behave as expected on-device, it's tough to isolate which piece is responsible, and it's a lot to review and land as a unit. If this had arrived as a sequence of small PRs — say, (1) just the pure I know this isn't the outcome you were hoping for after the effort you put in, and I'm sorry about that — but please don't let it discourage you. This was a real, useful contribution: it surfaced the actual problem clearly and moved our understanding forward, and I'd genuinely welcome more from you. I can't promise you a test device, but I'm glad to build and run your future iterations on a real device with TalkBack and report back what I see — exactly like I did here — so the lack of hardware doesn't have to block you. Happy to pair on the provider-based approach or look at an upstream Rosemoe issue together if you'd like to keep going. Thank you again. 🙏 (One small logistics note: the branch currently has a merge conflict with |
|
Hi @hal-eisen-adfa, thank you for building this and testing it on a real device with TalkBack. I really appreciate the detailed notes. I want to clarify the intended scope and testing flow, because I think a few of the observations may be measuring a different interaction model than the one this PR is trying to support. The granularity is meant to be controlled from TalkBack only. Most of the changes I made are deliberately on the TalkBack/accessibility side, because I do not want to change behavior for normal users or affect interactions people are already used to. So the scope here is accessibility only, specifically screen-reader behavior. On focus reading the whole editor: that is also the normal behavior I expect from a large editable text node. If you focus a text box in a notebook app, or a long paragraph you wrote in a normal editor, TalkBack will often announce the whole text/content for that focused text field. So I do not think reading the full document on initial focus means this PR is failing by itself. On the line focus + double-tap point: I do not think the intended workflow is "focus a line, then double-tap to place the cursor on that focused line." The workflow requested by the initial users was closer to this:
That is the flow I implemented and tested in the code: TalkBack movement granularity actions update the accessibility/editor selection, and activation places the cursor at that current accessibility position. So if the test was mainly normal swipe focus traversal between views, or focusing what TalkBack treats as a line and then double-tapping, that is not quite the behavior this PR is trying to provide. I agree that a virtual view hierarchy with Could you please re-review with the TalkBack granularity flow above if that is not what you tested? If you have specific testing instructions or a different expected TalkBack gesture sequence, please share them and I will look into it. I really want this work to land if the intended TalkBack path behaves correctly, because it would help users start getting used to this editor being more accessible. |
|
Hi @hal-eisen-adfa, I handled the branch conflict you mentioned. I merged current The resolution keeps both sides of the editor changes:
I also reran |
2 participants
Closes #1497
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.