Ship feature/android-talkback-87 - #127
Merged
Merged
Conversation
C++ JNI bridge (accessibility_android.cpp): - Walks the View tree, collects Views with non-none AccessRole - Serves role, label, value, range, and table info by index to Kotlin - Handles click, increment, and decrement actions from TalkBack - Mirrors the iOS VoiceOver bridge pattern (flatten tree, query by index) Kotlin delegate (PulpAccessibility.kt): - Fix role constants to match C++ enum (none=0, slider=1, toggle=2, label=3, group=4, meter=5, image=6) - Add meter → ProgressBar and image → ImageView mappings - Add group → ViewGroup mapping - Remove non-existent ROLE_BUTTON and ROLE_TEXT constants The delegate was already wired into PulpSurfaceView. With this change, TalkBack can discover and announce Pulp widgets, navigate via swipe gestures, and adjust slider values via scroll actions. Testing requires an Android device/emulator with TalkBack enabled. The C++ code is behind #ifdef __ANDROID__ and doesn't affect other platforms.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ee05b9b9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+115
to
+116
| const val ROLE_TOGGLE = 2 | ||
| const val ROLE_LABEL = 3 |
There was a problem hiding this comment.
This change reassigns ROLE_TOGGLE/ROLE_LABEL (and adds new IDs) to match View::AccessRole, but the JNI method actually used at runtime (Java_com_pulp_accessibility_PulpAccessibilityDelegate_nativeGetNodeRole in core/render/platform/android/gpu_surface_android.cpp) still returns the legacy mapped IDs (toggle=3, label=2, meter=1, image=2). Because Android wiring still compiles gpu_surface_android.cpp (not the new core/view/platform/android/accessibility_android.cpp), TalkBack now interprets many controls as the wrong widget type (e.g., toggles announced as labels, labels/images announced as toggles).
Useful? React with 👍 / 👎.
danielraffel
added a commit
that referenced
this pull request
Apr 12, 2026
1. measure_text uses SkShaper when shaping is enabled (#125 review) - Aligned with fill_text: both now use HarfBuzz for consistent kerning/ligature widths. Fixes draw-vs-measure mismatch for strings like "AV", "ffi". 2. SDF text: UTF-8 bounds checking + missing glyph fallback (#126 review) - Guard multibyte decode against truncated input (no UB on malformed) - Fall back to fill_text() if any glyph is missing from the atlas rather than silently skipping characters 3. Remove duplicate accessibility JNI from gpu_surface_android.cpp (#127 review) - Old code had wrong role mappings (toggle=3, label=2, meter=1) - New code in core/view/platform/android/accessibility_android.cpp uses C++ enum values directly (correct) - Eliminates linker duplicate symbol conflict on Android 4. Add MIDI include path for Android audio target (#128 review) - core/midi/include added to pulp-audio's include directories in PulpAndroid.cmake so oboe_device.cpp can find buffer.hpp and android_midi_fifo.hpp
danielraffel
added a commit
that referenced
this pull request
Apr 12, 2026
1. measure_text uses SkShaper when shaping is enabled (#125 review) - Aligned with fill_text: both now use HarfBuzz for consistent kerning/ligature widths. Fixes draw-vs-measure mismatch for strings like "AV", "ffi". 2. SDF text: UTF-8 bounds checking + missing glyph fallback (#126 review) - Guard multibyte decode against truncated input (no UB on malformed) - Fall back to fill_text() if any glyph is missing from the atlas rather than silently skipping characters 3. Remove duplicate accessibility JNI from gpu_surface_android.cpp (#127 review) - Old code had wrong role mappings (toggle=3, label=2, meter=1) - New code in core/view/platform/android/accessibility_android.cpp uses C++ enum values directly (correct) - Eliminates linker duplicate symbol conflict on Android 4. Add MIDI include path for Android audio target (#128 review) - core/midi/include added to pulp-audio's include directories in PulpAndroid.cmake so oboe_device.cpp can find buffer.hpp and android_midi_fifo.hpp
danielraffel
added a commit
that referenced
this pull request
Apr 12, 2026
PR #127 landed accessibility_android.cpp with two API mismatches that broke the android-build CI check: 1. View::children() doesn't exist; use child_count() + child_at(i) to iterate children, matching the iOS VoiceOver bridge pattern. 2. View::simulate_click() requires a Point in root coordinates. Walk the parent chain to convert the view's local centre into root coords, same technique accessibility_ios.mm uses for accessibilityFrame. No behaviour change on other platforms (file is guarded by __ANDROID__).
2 tasks
danielraffel
added a commit
that referenced
this pull request
Apr 12, 2026
* Fix Android build: accessibility bridge uses correct View API PR #127 landed accessibility_android.cpp with two API mismatches that broke the android-build CI check: 1. View::children() doesn't exist; use child_count() + child_at(i) to iterate children, matching the iOS VoiceOver bridge pattern. 2. View::simulate_click() requires a Point in root coordinates. Walk the parent chain to convert the view's local centre into root coords, same technique accessibility_ios.mm uses for accessibilityFrame. No behaviour change on other platforms (file is guarded by __ANDROID__). * Address Codex P1: dispatch simulate_click from root view simulate_click(root_pos) runs hit_test from 'this'; calling it on 'target' (which isn't at origin) causes the hit test to miss and the TalkBack ACTION_CLICK to silently no-op for nested controls. Compute the target's centre in root coordinates and dispatch via g_root_view instead, preserving root-space hit-testing.
danielraffel
added a commit
that referenced
this pull request
Apr 13, 2026
…pulp-view PR #132's P1 review never actually landed on main. accessibility_android.cpp defines all Java_com_pulp_accessibility_PulpAccessibilityDelegate_native* entry points inside pulp-view (static lib). Nothing inside libpulp.so references them — the only callers are on the Kotlin side (PulpAccessibility.kt) — so the Android linker is free to strip them under default dead-code elimination. At runtime TalkBack's first node-tree walk then hits UnsatisfiedLinkError. PR #148 tried to fix this by whole-archiving pulp-view. That pulled SDL3's libSDL3.a in transitively, and SDL3 defines its own JNI_OnLoad, colliding with core/platform/src/android/jni_bridge.cpp:41. #148 was closed. This PR uses the narrower `-Wl,--undefined=<symbol>` approach: force- reference each of the nine TalkBack JNI exports in pulp-jni's link options. The linker then keeps the accessibility_android.cpp object file alive without dragging any other sections of pulp-view (or its SDL3 transitive closure) into libpulp.so. No behaviour change on non-Android builds (the whole pulp-jni target is gated on ANDROID). On Android, the nine forced references grow libpulp.so by the single accessibility_android.cpp object file — same footprint PR #127 intended when it first landed TalkBack.
danielraffel
added a commit
that referenced
this pull request
Apr 14, 2026
…pulp-view PR #132's P1 review never actually landed on main. accessibility_android.cpp defines all Java_com_pulp_accessibility_PulpAccessibilityDelegate_native* entry points inside pulp-view (static lib). Nothing inside libpulp.so references them — the only callers are on the Kotlin side (PulpAccessibility.kt) — so the Android linker is free to strip them under default dead-code elimination. At runtime TalkBack's first node-tree walk then hits UnsatisfiedLinkError. PR #148 tried to fix this by whole-archiving pulp-view. That pulled SDL3's libSDL3.a in transitively, and SDL3 defines its own JNI_OnLoad, colliding with core/platform/src/android/jni_bridge.cpp:41. #148 was closed. This PR uses the narrower `-Wl,--undefined=<symbol>` approach: force- reference each of the nine TalkBack JNI exports in pulp-jni's link options. The linker then keeps the accessibility_android.cpp object file alive without dragging any other sections of pulp-view (or its SDL3 transitive closure) into libpulp.so. No behaviour change on non-Android builds (the whole pulp-jni target is gated on ANDROID). On Android, the nine forced references grow libpulp.so by the single accessibility_android.cpp object file — same footprint PR #127 intended when it first landed TalkBack.
danielraffel
added a commit
that referenced
this pull request
Apr 14, 2026
…pulp-view (#151) * Fix Android: force-keep TalkBack JNI symbols without whole-archiving pulp-view PR #132's P1 review never actually landed on main. accessibility_android.cpp defines all Java_com_pulp_accessibility_PulpAccessibilityDelegate_native* entry points inside pulp-view (static lib). Nothing inside libpulp.so references them — the only callers are on the Kotlin side (PulpAccessibility.kt) — so the Android linker is free to strip them under default dead-code elimination. At runtime TalkBack's first node-tree walk then hits UnsatisfiedLinkError. PR #148 tried to fix this by whole-archiving pulp-view. That pulled SDL3's libSDL3.a in transitively, and SDL3 defines its own JNI_OnLoad, colliding with core/platform/src/android/jni_bridge.cpp:41. #148 was closed. This PR uses the narrower `-Wl,--undefined=<symbol>` approach: force- reference each of the nine TalkBack JNI exports in pulp-jni's link options. The linker then keeps the accessibility_android.cpp object file alive without dragging any other sections of pulp-view (or its SDL3 transitive closure) into libpulp.so. No behaviour change on non-Android builds (the whole pulp-jni target is gated on ANDROID). On Android, the nine forced references grow libpulp.so by the single accessibility_android.cpp object file — same footprint PR #127 intended when it first landed TalkBack. * Fix macOS AU v2 plugin bundles: pin CXX_STANDARD=23 on ${target}_AU ViewBridge Phase 3 (#146) added au_v2_cocoa_view.mm to each plugin's ${target}_AU MODULE target via PulpUtils.cmake's pulp_add_plugin helper. The .mm compiles inside that AU target, not pulp-format, so it does NOT inherit pulp-format's `target_compile_features(... PUBLIC cxx_std_23)` PUBLIC setting — CMake only propagates compile features when the consumer links against the target, not when its sources are merely `add_library(... src1 src2 ...)`-ed in alongside. AUUtility.h in AudioUnitSDK 1.4 uses std::expected / std::unexpected (C++23). Apple clang only exposes those at -std=c++23. Result on github-hosted macos-14 runners (clang 17): every plugin AU target fails to build with 'no template named unexpected in namespace std'. Pin CXX_STANDARD=23 + CXX_STANDARD_REQUIRED=ON on ${target}_AU directly. Same rationale documented at core/format/CMakeLists.txt L76 for the pulp-format target. * Fix macOS AU v2 (follow-up): .mm files need OBJCXX_STANDARD=23 too CXX_STANDARD=23 alone doesn't apply to .mm sources — CMake treats them as Objective-C++ (OBJCXX), a separate language with its own OBJCXX_STANDARD property. Previous commit 1e3b4f9 only set the C++ standard, so au_v2_cocoa_view.mm still compiled at the default C++17 and AUUtility.h's std::expected still failed. Add OBJCXX_STANDARD=23 + OBJCXX_STANDARD_REQUIRED=ON alongside the existing CXX_STANDARD so both C++ and Objective-C++ TUs in ${target}_AU pick up the required standard.
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.
Automated by Shipyard