Skip to content

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

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

Fix TalkBack seeking on the full player progress bar#5547
joashrajin wants to merge 3 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 can't seek with the full player progress bar: each accessibility swipe moves the bar, but the position immediately snaps back (reported in #2264 as "progression leaps back to the zero timepoint").

Root cause: TalkBack seek actions (ACTION_SET_PROGRESS) and keyboard arrow keys change the SeekBar progress via onProgressChanged(fromUser = true) without the onStartTrackingTouch/onStopTrackingTouch callbacks a finger drag produces. PlayerSeekBar only committed a seek from onStopTrackingTouch, so for accessibility seeks:

  • seeking was never set, so the next playback position update (setCurrentTime) immediately reverted the bar, and
  • the seek was never sent to the player at all.

This PR handles non-touch progress changes in PlayerSeekBar:

  • A fromUser change with no active touch tracking marks seeking = true (stopping the snap-back) and commits the seek through the existing onSeekPositionChangeStop path after a short debounce, so consecutive TalkBack swipes accumulate into a single seek.
  • A pending non-touch commit is cancelled if a real drag starts or the view detaches.
  • Touch-drag behaviour is unchanged.

This benefits both consumers of PlayerSeekBar: the full screen player (via the Compose nowplaying/PlayerSeekBar wrapper) and the full screen video player.

References #2264 — this resolves the progress bar item. The missing-labels item from that issue 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

Verified on-device (Samsung, Android SDK 34) by dispatching AccessibilityAction.ACTION_SET_PROGRESS — the exact action TalkBack sends when adjusting a slider — at the live full-player seek bar:

  • Seeked from 695s → 995s (+5 min); progress held at the target ([995, 995, 995, 996, 996, 997]) and playback continued from there (media session position advanced 693687ms → 1009800ms → 1042829ms) instead of snapping back. Before this change the samples revert toward the original position on the next playback tick.

To reproduce manually with TalkBack:

  1. Enable TalkBack (Settings → Accessibility → TalkBack)
  2. Play an episode and open the full screen player
  3. Focus the progress bar and swipe up/down to seek
  • Verify the position advances with each swipe and does not jump back to the previous position
  • Verify playback resumes from the new position shortly after the last swipe
  1. Repeat on the full screen video player with a video episode
  • Verify the same behaviour
  1. With TalkBack off, drag the progress bar with a finger
  • Verify normal drag-seeking is unchanged

Screenshots or Screencast

⚠️ TODO (manual): Optional — add a TalkBack screencast of seeking on the full player, or 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 — the change lives in an Android View; the player module has no Robolectric setup, so a JVM unit test isn't practical without adding that infrastructure. Behaviour was instead verified on-device via the accessibility action pathway (see Testing Instructions).
  • 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 Compose changes
  • I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics. — n/a, no new analytics (the existing trackPlaybackSeek now also fires for accessibility seeks)

I have tested any UI changes...

  • with different themes — n/a
  • with a landscape orientation — n/a
  • with the device set to have a large display and font size — n/a
  • for accessibility with TalkBack — verified the seek bar commits accessibility seeks on-device (see Testing Instructions)

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

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/PlayerSeekBarPlayerHeaderFragment, 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-159onDetachedFromWindow() 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 onSeekPositionChangeStartonSeekPositionChanging. 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

Copy link
Copy Markdown
Collaborator
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