Repository navigation
android: expose GPUI's accessibility tree to TalkBack - #25
Conversation
|
I added two fixes after more testing, each as its own commit:
I found both while porting this to the current Zed GPUI for itsbalamurali/gpui-mobile, whose example only runs as a NativeActivity. Verified with TalkBack on a OnePlus CPH2581 (Android 16), and with repeated activate/deactivate cycles on redroid. |
a37bff0 to
d7e7a73
Compare
|
Rebased onto
One thing I noticed while doing this, not changed here: Checked with |
huacnlee
left a comment
There was a problem hiding this comment.
Thanks, getting GPUI's tree in front of TalkBack is a big step. A few issues need fixing before this can merge. These come from reading the diff together with accesskit_android 0.7.5; I have not reproduced them on a device.
1. Wrong Activity after going back (accessibility.rs, init)
init (also run from set_active(true)) attaches through activity_as_ptr(), which is the Activity that most recently called set_host_activity. With host A, then host B, then Back: A resumes, but HOST_ACTIVITY.current is still B, so A's adapter is installed on B's finishing content view and A goes back to being one opaque surface for TalkBack. The adapter should be tied to the window's own host Activity/view.
2. The isEnabled() check still races with raising events (accessibility.rs, update)
update_if_active only posts events.raise to the UI thread, and that runnable doesn't check again. If TalkBack is turned off between the GPUI-thread isEnabled() call and the runnable running, AccessibilityManager throws, the unwrap inside the crate's extern "system" callback panics across FFI, and the process aborts. That is the same crash this workaround is meant to prevent. The check has to happen on the UI thread, right before sending (or the exception has to be caught there).
3. BRIDGE is never cleared when the window closes (window.rs)
After HostDestroyed closes the active window, BRIDGE still holds that window's callbacks and an adapter installed on its view. A later TalkBack activation calls the dead window's activation and gets a stale tree, and the next init calls deactivation on the closed window. Please clear the bridge (dropping the adapter) when the window that owns it is dropped.
4. Abort if the view already has an AccessibilityDelegate (accessibility.rs, attach)
InjectingAdapter::new posts a runnable that panics with "host already has an accessibility delegate". Because it runs inside an extern "system" callback, this aborts the process. A host app, or an androidx/Compose layer, can easily have set its own delegate on android.R.id.content. Please check for an existing delegate first and fall back (log and skip) rather than crash.
5. attach is never retried after a failure (accessibility.rs:85)
init returns early when the same callbacks are already registered, even if adapter is None because the earlier attach failed (VM/Activity not ready, or a JNI error). Accessibility then stays off for that window. It should retry when adapter.is_none().
6. Attaching during onCreate happens before setContentView (jni.rs, activity_changed)
With AppCompat, the android.R.id.content id moves to the subDecor's ContentFrameLayout after setContentView, so the delegate can end up on an outer frame, which offsets node bounds. Consider attaching lazily, or when the surface is created.
Non-blocking:
updatemakes a JNIisEnabled()call every frame while holding the globalBRIDGElock, which also blocksactivity_changedon the UI thread. AnAccessibilityStateChangeListener, or a cachedAtomicBool, would avoid this.with_host_viewuses thejni0.21 thataccesskit_androidre-exports and rebuilds theJavaVMon every attach, while the crate already uses newerjniand caches the VM. That means two jni major versions in the build and two JNI styles to maintain.- In
jni.rs, the hover early return comes after the platform-viewhit_test, so every hover event during touch exploration pays for a registry lookup that is thrown away. Move the check above it.
GPUI 0.3.7 builds an AccessKit tree and hands it to PlatformWindow::a11y_init / a11y_tree_update, which the Android window left as no-ops. Wire them to accesskit_android's InjectingAdapter, which installs an AccessibilityDelegate on the existing host View, so hosts need no Java changes. accesskit_android 0.7.5 is the newest release on accesskit 0.24 (the version gpui-pre 0.3.7 uses). It raises accessibility events without checking AccessibilityManager.isEnabled(), and Android throws on the UI thread when no service is listening, so updates are skipped while accessibility is off (0.9 has this check but needs accesskit 0.25). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Only the sibling window and jni modules call it, matching document_picker and text_input. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
InjectingAdapter is already Send (its JavaVM, WeakRef, global JClass and Arc<Mutex<_>> fields all are in jni 0.21), so the bridge state can live in the static Mutex without it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Once accessibility turns off, tell GPUI to stop building trees and swap in an adapter that is not yet active. Before, the adapter stayed active after TalkBack was turned off, so turning it back on never re-activated GPUI and TalkBack read a stale tree until something repainted; GPUI also kept building trees nobody read. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
On the android-activity (NativeActivity) path every motion event goes to the native input queue first, and hover events were reported as handled even though GPUI skips them. With TalkBack on, touch exploration arrives as hover events, so the decor view's accessibility delegate never saw them and TalkBack found nothing under the finger. Report them unhandled so Android passes them on, as touches on platform views already are. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Since longbridge#26 each host Activity has its own window, and going back re-attaches an earlier one. The bridge kept the last window's callbacks and view, so the returning window got no screen reader. Each window now keeps its callbacks, takes the adapter over when it becomes active, and the window it replaces is told to stop building trees. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nding accesskit_android's jni 0.21 API returns an error for a Java exception but leaves it pending, and the bridge never cleared it, so the next JNI call on the render thread ran with an exception pending. Describe and clear it after the host-view lookups, AccessibilityManager.isEnabled and each tree update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Touch exploration sends a stream of hover events; each paid for a platform-view registry lookup that was then thrown away. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After HostDestroyed, BRIDGE still held the closed window's callbacks and an adapter on its view: a later screen reader got the dead window's tree, and the next init deactivated a closed window. Closing the window, or dropping it, now drops the bridge and its adapter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
init returned early whenever the window's callbacks were already registered, even if the first attach had failed, so accessibility stayed off for that window. It now attaches again while there is no adapter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
InjectingAdapter panics on the UI thread when its view already has a delegate, and the panic aborts the process because it runs inside an extern "system" callback. A host app or an androidx layer can set one on android.R.id.content. attach now checks first, logs and leaves the view alone; an earlier adapter's delegate is fine, its removal is queued first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… in onCreate set_host_activity moved the adapter in onCreate, which can run before setContentView; with AppCompat, android.R.id.content then moves to the subDecor's ContentFrameLayout and the delegate sits on an outer frame with offset bounds. The adapter is now attached whenever the window becomes active (on resume and when its surface arrives), and moves when the Activity behind the window is no longer the one it is on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The adapter went on the Activity that most recently called set_host_activity. With host A, then host B, then Back, A resumed but its adapter was installed on B's finishing view, and TalkBack saw A as one opaque surface again. host::set_activity(env, host, activity) does what set_host_activity does and also remembers the Activity per HostId; each window knows its host, so the adapter goes on that host's own Activity. Hosts that only call set_host_activity keep the old behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
with_host_view rebuilt a JavaVM on every attach, and update made its JNI isEnabled() call while holding the BRIDGE lock. Both are cached once, and the check runs before the lock is taken. The docs now say what the check cannot cover: a screen reader turned off between it and the adapter's UI-thread runnable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d7e7a73 to
31d79d8
Compare
|
Thanks for the careful review! I rebased onto
Non-blocking:
Checked: |
|
Device results for 31d79d8, on a OnePlus CPH2581 (Android 16), with the lab app (one host Activity):
The lab has a single host Activity, so I couldn't stack two GPUI hosts on the device. The A → B → Back case is covered by reading the code, not by a device run. |
huacnlee
left a comment
There was a problem hiding this comment.
Reviewed 31d79d8. The per-host Activity registration, window callback ownership, bridge cleanup, and attach retry changes address the earlier lifecycle concerns. Two correctness issues still need fixing before merge: the TalkBack shutdown race can still abort the process, and per-frame adapter updates leak JNI local references. Details are inline. CI is green, but these findings come from source inspection of this PR and accesskit_android 0.7.5; I have not reproduced them on an Android device.
| let Some(adapter) = bridge.adapter.as_mut() else { | ||
| return; | ||
| }; | ||
| if enabled { |
There was a problem hiding this comment.
[P1] Check accessibility state where events are actually raised on the UI thread
The GPUI-thread isEnabled() check still races with the runnable posted by InjectingAdapter::update_if_active. If TalkBack turns off after this check but before that runnable runs, AccessibilityManager can throw; accesskit_android 0.7.5 unwraps the JNI error inside its extern "system" callback, aborting the process. clear_exception below runs on the GPUI thread, so it cannot clear an exception raised later on the UI thread and cannot recover from that panic. The new module comment acknowledges the race, but it remains a crash path. Please backport the UI-thread guard into a compatible adapter release (or use a patched/low-level adapter that checks immediately before raising events) before merging. Device runs that happen not to hit the race do not establish that it is fixed.
| return; | ||
| }; | ||
| if enabled { | ||
| adapter.inner.update_if_active(|| tree); |
There was a problem hiding this comment.
[P2] Release JNI local references around every adapter update
InjectingAdapter::update_if_active in accesskit_android 0.7.5 creates a local host reference via upgrade_local and, when posting events, a local Runnable reference. Neither is explicitly deleted by that implementation. This call runs on the permanently attached render thread outside a local-reference frame, so those references accumulate across frames and retain Java objects, including old Activity views. with_raw_activity only establishes a frame for attach/is_current; it does not cover this update call. Please obtain the current JNIEnv and wrap update_if_active in with_local_frame so all local references created by each update are released when the update returns. The adapter drop path also creates local references, so cover that path when replacing or clearing the bridge as well. Android documents that references on an attached native thread are not automatically freed until it detaches: https://developer.android.com/ndk/guides/jni-tips#local-and-global-references
InjectingAdapter::update_if_active and its Drop create local references (the host view, the posted Runnable) and never delete them. The GPUI thread stays attached, so they piled up every frame and kept old views alive. Run both inside a local reference frame. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the second pass! P2, local references (fafa6f0): P1, the
Which would you prefer? With 2, part of this PR's workaround could probably go as well, since 0.9 checks |
| if enabled { | ||
| in_local_frame(|| adapter.inner.update_if_active(|| tree)); | ||
| // Raising events calls into Java; don't leave its exception to the next caller. | ||
| if let Ok(mut env) = vm().and_then(|vm| vm.get_env().map_err(|e| e.to_string())) { | ||
| let _ = clear_exception(&mut env, Ok(())); |
There was a problem hiding this comment.
[P1] 在实际发送事件的 UI 线程修复 TalkBack 关闭竞态
这里的 isEnabled() 检查发生在 GPUI 线程,而 accesskit_android 0.7.5 的 update_if_active() 只将 events.raise 排到 UI 线程。如果 TalkBack 在检查后、runnable 执行前关闭,requestSendAccessibilityEvent 的 JNI 调用仍可能抛异常,依赖中的 unwrap() 会在 extern "system" 回调里 panic,导致整个进程 abort。这里的 clear_exception() 在 GPUI 线程执行,无法清理 UI 线程的异常。因此目前的检查和异常清理仍没有解决此前的崩溃问题。请回移适配器发送事件前的 isEnabled() 检查,或升级到兼容的已修复版本后再合并。
| let delegate = env | ||
| .call_method( | ||
| view, | ||
| "getAccessibilityDelegate", | ||
| "()Landroid/view/View$AccessibilityDelegate;", | ||
| &[], | ||
| )? |
There was a problem hiding this comment.
[P2] 为 API 26–28 提供兼容的 delegate 检查和挂载路径
项目 README 和示例 minSdk 均声明支持 API 26+,但 View.getAccessibilityDelegate() 从 API 29 才提供。在 API 26–28 上,这个无条件 JNI 调用会发生方法查找错误,attach() 随后记录警告并退出;每次重试都会失败,TalkBack 因而仍只能看到不透明表面。accesskit_android 0.7.5 的 InjectingAdapter 在安装和移除 delegate 时也调用该方法,所以只跳过这里的检查还不够。请同时修复依赖内部的兼容路径。Android API 文档:https://developer.android.com/reference/android/view/View#getAccessibilityDelegate()
GPUI 0.3.7 already builds an AccessKit tree and hands it to
PlatformWindow::a11y_init/a11y_tree_update, but the Android window leaves both as no-ops, so TalkBack only sees one opaque surface. This PR connects them toaccesskit_android'sInjectingAdapter. That adapter installs anAccessibilityDelegateon the existing hostView, so hosts don't need any Java changes.It's one new module (
src/android/accessibility.rs) plus two hooks inwindow.rsand two small changes injni.rs, about 250 lines in total.How it works
a11y_initstores GPUI's callbacks and attaches the adapter to the host view. That's the content view on the host-driven path, where theSurfaceViewlives, and the decor view on theandroid-activitypath.a11y_tree_updateforwards each tree to the adapter. The adapter posts its work to the UI thread itself.set_host_activitymoves the adapter onto the new Activity's views when the Activity is recreated.android-activitypath, hover events are now reported as unhandled. GPUI doesn't use hover on mobile, and TalkBack's touch exploration arrives as hover events, which the delegate on the decor view needs to see.Two things worth flagging
accesskit_android0.7.5 is the newest release built onaccesskit0.24, which is the versiongpui-pre0.3.7 uses. It can move to 0.9 when GPUI moves toaccesskit0.25.AccessibilityManager.isEnabled(). Android throws on the UI thread when no service is listening, so the app crashed as soon as the screen reader disconnected. This PR checks first. Once accessibility is off, it calls GPUI'sdeactivationcallback and attaches a fresh adapter, so turning the screen reader back on activates GPUI again instead of reading a stale tree. 0.9 does theisEnabled()check internally, so most of this can go away with the upgrade.Testing
Tested with the host-driven path in a small lab app on GPUI Kit 0.7.0.
OnePlus CPH2581 (Android 16), TalkBack. Touch exploration lands on the right controls, double-tap toggles a checkbox, and turning TalkBack off with the volume keys no longer crashes the app:
Redroid (Android 14),
uiautomator dump. Every node's bounds drawn over the rendering. Kit's checkboxes, radios, switches and sliders come through as their native Android classes with labels and checked state:android-activity(NativeActivity) path, same phone. I tested the same bridge ported to the NativeActivity example in itsbalamurali/gpui-mobile#69, because the lab app uses the host-driven path. That's where the hover fix above came from: before it, touch exploration never reached the delegate. With it, TalkBack reads the controls and double-tap toggles them. One limitation remains: Android doesn't draw its green focus ring, because NativeActivity takes over the window surface that the ring would be drawn on.Happy to change the approach if you'd prefer something different, for example a Java-side
AccessibilityNodeProviderin the host instead of the injected delegate.🤖 Generated with Claude Code