Skip to content

Fix TalkBack seeking on the full player progress bar - #5547

Draft
joashrajin wants to merge 5 commits into
mainfrom
fix/2264-talkback-seekbar-seek-commit
Draft

joashrajin wants to merge 5 commits into
mainfrom
fix/2264-talkback-seekbar-seek-commit

Conversation

@joashrajin

@joashrajin joashrajin commented Jul 9, 2026 •

Copy link
Copy Markdown
Contributor

Description

TalkBack users could move the full-player progress bar, but the position immediately snapped back and the seek was never committed (reported in #2264 as “progression leaps back to the zero timepoint”).

Root cause

TalkBack ACTION_SET_PROGRESS actions and keyboard arrows update a native SeekBar through onProgressChanged(fromUser = true) without the touch-only onStartTrackingTouch / onStopTrackingTouch callbacks. The player seek bar therefore did not enter its seeking state or send the requested position to playback.

A deeper review also found that a simple delayed commit was not sufficient: an older async completion could clear a newer interaction, a delayed request could target the next episode, video reported completion before playback actually moved, detach could silently discard an accepted accessibility action, and seek analytics could race the playback-state mutation.

What changed

  • Model touch, TalkBack, and keyboard seeks as explicit, generation-scoped sessions.
  • Debounce consecutive non-touch changes while capturing the target, duration bound, listener, and episode UUID for that session.
  • Keep touch takeover paired as one start/stop interaction, and prevent stale gesture events after an episode change from targeting the replacement episode.
  • Flush accepted non-touch actions on detach and explicitly cancel lifecycle-invalidated touch/in-flight sessions.
  • Guard playback seeks against the current queue episode, playback state, and player instance; serialize UI seeks and make the latest request win.
  • Deliver UI seek completions on the main thread and ignore stale completions without unlocking newer interactions.
  • Wait for the real playback completion in full-screen video and prevent stale callbacks from restarting its hide-controls timer.
  • Capture seek analytics from the validated episode before mutating its playback position.
  • Preserve the legacy seekIfPlayingToTimeMs overload’s synchronous guard, callback ABI, and sync-caller behavior.
  • Add focused Robolectric coverage for native accessibility actions, real touch events, detach/reattach, episode changes, stale completions, and video timer generations.

This benefits both consumers of PlayerSeekBar: the Compose full-screen player and the full-screen video player.

References #2264 — this resolves the progress bar item. The missing-labels item was already fixed in #2742, and the bottom-navigation item needs separate on-device confirmation, so this PR intentionally does not auto-close the issue.

Testing Instructions

Automated validation:

./gradlew :modules:features:player:testDebugUnitTest
./gradlew :modules:features:player:lintRelease
./gradlew spotlessCheck

The new tests cover:

  • repeated ACTION_SET_PROGRESS changes coalescing to the latest target
  • stale async completion versus a newer seek session
  • non-touch-to-touch takeover without duplicate callbacks
  • episode changes during pending and active gestures
  • detach flush plus detach/reattach accessibility input
  • playback updates remaining blocked only until the matching completion
  • stale video completion/cancellation timer behavior
  • forwarding the expected episode UUID, position, and analytics source

The original accessibility path was also verified on-device (Samsung, Android SDK 34) by dispatching AccessibilityAction.ACTION_SET_PROGRESS at the live full-player seek bar: a seek from 695s to 995s held at the target and playback continued from there instead of snapping back.

Optional manual smoke test:

  1. Enable TalkBack.
  2. Play an episode and open the full-screen player.
  3. Focus the progress bar and swipe up/down repeatedly.
    • The position accumulates and does not jump back.
    • Playback resumes from the final position after the debounce.
  4. Repeat with a video episode in the full-screen video player.
    • The same behavior holds.
  5. Disable TalkBack and drag the progress bar normally.
    • Touch dragging remains unchanged.
  6. Optionally rotate or move to the next episode during a seek.
    • No stale seek is applied to the replacement episode.

Screenshots or Screencast

n/a — no visual changes.

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml — n/a, no new strings
  • Any Jetpack Compose components I added or changed are covered by Compose previews — n/a, no visual Compose change
  • I have updated (or requested that someone edit) the Event Horizon schema to reflect new or changed analytics — n/a, the existing seek event/schema is unchanged

I have tested any UI changes...

  • with different themes — n/a, no visual changes
  • with a landscape orientation — covered structurally by detach/reattach tests; optional manual smoke above
  • with the device set to have a large display and font size — n/a, no visual changes
  • for accessibility with TalkBack — live accessibility action verified on-device; native action path covered by Robolectric

Accessibility seek actions change SeekBar progress without touch
tracking callbacks, so the seek was never sent to the player and the
next playback position update snapped the bar back (#2264).
@claude

claude Bot commented Jul 9, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @joashrajin's task in 2m 22s —— View job


Code Review: TalkBack seeking on the full player progress bar

  • Read the changed file (PlayerSeekBar.kt)
  • Analyzed the accessibility/non-touch seek logic
  • Traced both consumers (nowplaying/PlayerSeekBar → PlayerHeaderFragment, and VideoFragment)
  • Verified the seekComplete / seeking reset paths

Nice fix — the root-cause analysis in the description is spot on, and the approach (mark seeking on non-touch changes + debounce a single committed seek) is the right one. The fromUser guard at line 124 correctly prevents programmatic setCurrentTime updates from re-triggering the commit path, and the debounce accumulation is good UX for consecutive TalkBack swipes. A few observations, none blocking:

1. seeking can be left stuck true after detach (minor edge case)

PlayerSeekBar.kt:156-159 — onDetachedFromWindow() cancels the pending commitNonTouchSeek, but if a non-touch seek was already in flight, seeking stays true:

override fun onDetachedFromWindow() {
    removeCallbacks(commitNonTouchSeek)
    super.onDetachedFromWindow()
}

Two ways to reach a stuck state:

  • The view detaches after onNonTouchSeek() set seeking = true but before the 750ms callback fires — the callback is removed, so seekComplete never runs and seeking is never reset.
  • The commit already fired and we're awaiting the async seekComplete from playbackManager.seekToTimeMs(...) (PlayerViewModel.kt:593), then the view detaches — the callback may resolve against a detached view.

If the same instance were ever reattached and reused, setCurrentTime would early-return forever (line 63) and the bar would freeze. In practice the risk is low because the Compose AndroidView instance isn't reused after leaving composition and VideoFragment recreates its view, so I'd call this defensive rather than a live bug. Resetting the flag on detach would make it robust regardless:

override fun onDetachedFromWindow() {
    removeCallbacks(commitNonTouchSeek)
    seeking = false
    super.onDetachedFromWindow()
}

Fix this →

2. Callback ordering is inverted for non-touch seeks (minor)

For a finger drag the order is onSeekPositionChangeStart → onSeekPositionChanging. For a non-touch seek the order is reversed: onProgressChanged calls onSeekPositionChanging(currentTime) at line 129 before onNonTouchSeek() invokes onSeekPositionChangeStart() at line 144.

This is harmless today — both consumers treat onSeekPositionChanging as a no-op (nowplaying/PlayerSeekBar.kt:38, VideoFragment.kt:163) — but it's an implicit contract inversion that could bite a future listener that assumes "start precedes changing." Consider calling onNonTouchSeek() (which fires start) before dispatching the changing callback, or documenting the ordering on the interface.

3. Property placement / init ordering (readability, not a bug)

commitNonTouchSeek (line 150) is declared after the init { setupSeekBar() } block (line 53). This is safe because setupSeekBar() only registers the listener and never dereferences commitNonTouchSeek during construction — it's only touched later inside callbacks. Still, moving the commitNonTouchSeek property and the two onNonTouchSeek/onDetachedFromWindow members up near the other fields would make the construction-time safety obvious at a glance.

4. Testing note (agree with your reasoning)

Agree a JVM unit test isn't practical without Robolectric here. If you wanted lightweight regression coverage later, the debounce/seeking state machine could be extracted into a plain class and unit-tested independently of the View — optional, not for this PR.


Summary: The change is correct and well-scoped, cleanup (removeCallbacks on touch-start and detach) is handled, and analytics/seek paths are wired through both consumers. Item #1 is the only one I'd suggest addressing before merge as cheap insurance; #2 and #3 are polish. 👍

@dangermattic

dangermattic commented Jul 9, 2026 •

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

@joashrajin joashrajin added [Type] Bug Not functioning as intended. [Area] Accessibility labels Jul 9, 2026
@joashrajin joashrajin self-assigned this Jul 9, 2026
@joashrajin
joashrajin requested a review from Copilot July 9, 2026 08:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an accessibility bug where TalkBack (and other non-touch interactions like keyboard arrows) could move the full player seek bar but the UI would snap back and the seek wouldn’t actually be committed, by adding a debounced non-touch seek commit path inside PlayerSeekBar.

Changes:

  • Track touch vs non-touch seek interactions and debounce non-touch progress changes into a single committed seek.
  • Cancel pending non-touch commits when touch-drag starts and when the view detaches.
  • Add a changelog entry for the TalkBack seek fix.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
modules/features/player/src/main/java/au/com/shiftyjelly/pocketcasts/player/view/PlayerSeekBar.kt Adds non-touch seek handling (TalkBack/keyboard) with debounced commit and cancellation hooks.
CHANGELOG.md Notes the user-facing TalkBack seek fix in 8.17.

Comment on lines 116 to 121
override fun onStartTrackingTouch(seekBar: SeekBar) {
touchSeeking = true
removeCallbacks(commitNonTouchSeek)
changeListener?.onSeekPositionChangeStart()
seeking = true
}
Comment on lines +156 to +159
override fun onDetachedFromWindow() {
removeCallbacks(commitNonTouchSeek)
super.onDetachedFromWindow()
}
Address review feedback on the non-touch seek path:
- Fire onSeekPositionChangeStart once per seek session so a touch drag
  interrupting an in-flight TalkBack/keyboard seek can't emit two starts
  for one stop, and start is dispatched before the changing callback.
- Reset seeking/touchSeeking in onDetachedFromWindow so a reused view
  instance can't get stuck ignoring position updates.
@joashrajin

Copy link
Copy Markdown
Contributor Author

Thanks for the review — addressed both correctness points in da7fccf:

1. seeking stuck true after detach. onDetachedFromWindow() now resets seeking and touchSeeking (in addition to cancelling the pending commit), so a reused instance can't get stuck ignoring setCurrentTime/setChapters updates.

2. Double onSeekPositionChangeStart when a touch drag interrupts an in-flight non-touch seek. Start is now dispatched exactly once per seek session via a shared beginSeekIfNeeded() used by both onStartTrackingTouch and the non-touch path. If a drag begins while a TalkBack/keyboard seek is pending, the drag cancels the pending commit and beginSeekIfNeeded() no-ops (session already active), so the single onStopTrackingTouch keeps start/stop paired. This also orders start before the changing callback for non-touch seeks, addressing the ordering nit.

Left the state machine in the View (not extracted for unit testing) for this PR; compiles + spotlessApply clean. The primary fix was verified on-device via ACTION_SET_PROGRESS; these are defensive edge-case hardening not exercised by that path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Area] Accessibility [Type] Bug Not functioning as intended.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants