Repository navigation
Conversation
trypsynth
left a comment
There was a problem hiding this comment.
Thanks, this is a nice idea, and matching the item by its content when an app rebuilds the screen does something TalkBack's own restore can't. Before it can merge, the cost needs to come down a lot, because it runs all the time rather than only around a Back.
Must fix: main-thread work (ScreenFocusRestorer.kt)
- Every
TYPE_WINDOW_CONTENT_CHANGEDcallsschedule()(around :266), and each run callsobserveScreen(context())before checking whether a restore is pending (:379-380).context()readsrootInActiveWindowand walks up to 100 nodes. During any animation, progress bar or chat that's several walks a second, in every app, with no Back pending. onUserInteraction()callscontext()whenever a retry is queued, and it runs on every hover enter, so touch exploration pays for it.- Each swipe walks the tree two or three times, and
describe()loops through up to 100 siblings per level.rememberBeforeActivationdoes this before every click, which delays activation. - None of this is turned off on watches, where these calls are slowest (see #152).
Suggestions: do nothing on content changes unless a Back is pending; keep the focused node and only describe it when Back or an activation actually happens; skip context() in onUserInteraction and on hover; and turn the feature off on watches.
Should fix
isBackControlmatches the English labels "Back" and "Navigate up" (around :171), so app Back buttons aren't recognised in other languages. Matching by resource ID, or by the framework's localized "Navigate up" string, would work everywhere.- In split screen, focus outside
rootInActiveWindowcancels the attempt and is never saved. - The tests replace
readContextwith a stub, so the tree walking and the app calls, which are the costly and risky part, aren't covered. - The
differences.mdsection sits above the intro paragraph; it belongs under Navigation. The summary string's "Off by default." and "allow Backtalk to" don't follow Google style, and the Czech strings are missing fromvalues-cs/strings_backtalk.xml. - CI on this PR ran while master's workflow was broken (lint only), so it was never built or unit tested there. A rebase onto current master will run the full workflow.
7eeb5e6 to
644aa99
Compare
|
Thanks for going through this. I've addressed the points in your review in commit 644aa99:
Validation passed: 667 phone unit tests, 479 TalkBack Wear unit tests, including 120 focus tests; I tested the revised isolated debug APK and it still appears to work. RedReader, Edge, Chrome, and Firefox had also been confirmed during the earlier testing, though I haven't separately reported every app case for this latest revision. Restoration remains Back-only, scrolling stays off by default, and user input cancels pending restoration. |
Returning with Back can lose accessibility focus when an app rebuilds its list or a browser reconstructs a page. Backtalk now remembers the last focused item on up to 20 screens during the current service session and restores a strong identity/content match when returning, including after opening another app.
Restoration is Back-only and phone-only. User input cancels it; weak or missing matches fall back to existing initial focus. Visible-item restoration is always active. The Advanced Settings option Scroll to restore previous focus is disabled by default and permits at most ten supported scroll actions within a three-second restoration deadline. Ordinary TalkBack window restoration remains unchanged on Wear and TV.
Addresses the maintainer review: ordinary focus tracking caches a lightweight identity, detailed bookmarks are captured at activation or Back, idle content changes and hover cancellation perform no window-root reads, and each restore attempt reads context once. Ancestor/sibling work is bounded. Departing focus uses its own window for split-screen support; Back controls use resource IDs and localized Navigate-up descriptions. The setting is hidden on Wear/TV, Czech translations are included, and documentation is under Navigation. Rebased onto master to preserve the direct-touch fix and restored build/test workflow.
Validation:
testPhoneDebugUnitTest: 667 tests passed.:talkback:testWearDebugUnitTest: 479 tests passed.:lintPhoneDebug,assemblePhoneDebug,assembleWearDebug, andgit diff --checkpassed.Debug configuration, APKs, local guidance, and unrelated session work are excluded. The separate suppressed-speech PR is untouched.