Skip to content

[TV] Hide the top bar on nested detail screens - #5700

Open
sztomek wants to merge 6 commits into
mainfrom
feat/tv-deep-destination-top-bar
Open

[TV] Hide the top bar on nested detail screens#5700
sztomek wants to merge 6 commits into
mainfrom
feat/tv-deep-destination-top-bar

Conversation

@sztomek

@sztomek sztomek commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

On Android TV the top bar (profile + tab row + logo) was shown on every screen, including nested detail screens. On the Apple TV app the tab bar is hidden on pushed detail screens (.toolbar(.hidden, for: .tabBar) on each detail view, plus the profile/logo accessory hidden via a navigation-depth flag). This brings Android TV to parity: the top bar is now hidden on the nested detail screens so they use the full height, and it reappears when you back out.

Detail screens that now hide the top bar:

  • Podcast details (opened from Home, Your Podcasts, or a folder)
  • Folder details
  • Playlist details

How it works

  • A small TvTopBarVisibility state holder (counter-based) is provided through a CompositionLocal (LocalTvTopBarVisibility) in TvScaffold, mirroring the existing TvToast/LocalTvToastHostState pattern in the module.
  • Each nested detail branch in TvHomeScreen, TvYourPodcastsScreen, and TvPlaylistsScreen calls a declarative HideTvTopBar() effect (a DisposableEffect that increments/decrements the counter), so visibility is driven at the navigation layer and the detail composables stay unchanged and reusable — analogous to iOS driving it from the navigation stack rather than from inside the detail views.

Animations (matching Apple TV)

The Apple TV app keeps this deliberately subtle: the tab bar/accessory just fades away and the detail is a separate full-screen layer (the content underneath never reflows), and tab switches are a simple .easeInOut crossfade (MainTabView.swift:93). Matched on Android by:

  • Rendering TvTopBar as a top overlay inside a Box (not a Column row), so the detail screen fills the whole area. The bar fades in/out (AnimatedVisibility with fadeIn/fadeOut, easeInOut ~300ms) instead of the earlier vertical expand/shrink that squished the bar and shoved the content.
  • The top-level content gets an animated top inset equal to the (measured) bar height, so grids sit below the bar when it's shown and the content smoothly reclaims the space as the bar fades — without a jump. When the bar is hidden, the detail screens keep a small 32dp top inset for breathing room instead of sitting flush against the top edge.
  • Tab switches use a Crossfade with an easeInOut curve, mirroring iOS.
  • TvTabBar previously requested focus on the selected tab on every composition-entry. Because the bar is disposed/re-created as it hides and shows, that would steal focus back to the tab bar every time you backed out of a detail. The initial-focus request is now driven once, via a flag owned by the never-disposed TvScaffold.

Layout tweak

While in here, evened out the horizontal padding on the two-pane detail screens (podcast + playlist details): they were start = 32.dp, end = 56.dp (right edge 24dp wider than the left); both are now 32.dp, matching the app's horizontal rhythm.

Fixes POC-792 https://linear.app/a8c/issue/POC-792/hide-top-bar-on-nested-destinations-like-playlist-details-and-podcast

Testing Instructions

  1. Launch the TV app and sign in (so Your Podcasts / Playlists have content).
  2. On Home, Your Podcasts, and Playlists confirm the top bar is visible.
  3. Open a podcast (from Home or Your Podcasts), a folder, and a playlist. In each detail screen the top bar should be hidden, the content should fill the height (with a small top margin), and the left/right edges should have matching padding.
  4. Press Back — the top bar fades back in and the top-level grid/list is shown again.
  5. Switch between tabs and confirm the content crossfades.
  6. Confirm the Up Next tab (which has no detail screen) always keeps the top bar.

Screenshots or Screencast

Screenshot_20260805_140909

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
  • Any jetpack compose components I added or changed are covered by compose previews
  • I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics.

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@dangermattic

dangermattic commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ Class TvTopBarVisibility is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by 🚫 Danger

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @sztomek's task in 6m 12s —— View job


Review: [TV] Hide the top bar on nested detail screens

  • Gather context (read diff + related files)
  • Analyze for bugs / quality / performance
  • Post review

Overall this is a clean, well-scoped change and the approach (state holder + CompositionLocal, driven from the navigation branch rather than from inside the detail composables) is the right shape for this module — it mirrors the existing TvToastHostState/LocalTvToastHostState pattern and keeps the detail screens reusable. The counter (rather than a boolean) is also the correct choice: it makes the folder → podcast transition in TvYourPodcastsScreen safe, because the outgoing branch's onDispose and the incoming branch's enterDetail() both land in the same apply pass, so the bar doesn't flash between two detail screens.

A few things I'd want addressed or at least verified on a device before merge — all of them are about focus, which is the part of this change with real user impact on TV.


1. didFocusTopBar isn't saved across activity recreation — the bug this PR fixes comes back

tv/.../home/TvScaffold.kt:46

var didFocusTopBar by remember { mutableStateOf(false) }

The detail-screen state that drives HideTvTopBar() is saved (openedPodcastUuid/openedFolder/openedPlaylist are all rememberSaveable), but this flag isn't. So after a configuration change or process-death restore while a detail screen is open:

  1. didFocusTopBar restores as falseautoFocusSelectedTab = true.
  2. First composition still has detailCount == 0, so the top bar is composed and TvTabBar requests focus on the selected tab.
  3. The detail branch's DisposableEffect then fires, the bar animates out and is disposed — taking the focused node with it.

That races with TvPodcastDetailsScreen.kt:280 / TvPlaylistDetailsScreen.kt:172, which request focus on the first episode, and can leave the restored detail screen with nothing focused. rememberSaveable here also expresses the intent better ("the tab bar has already had its one initial focus"). Fix this →

Related nit: because TvTopBarVisibility is also plain remember, a restore with a detail open draws one frame with the bar visible and then shrinks it away. Cosmetic, but visible.

2. Disposing the top bar throws away TabRow's focus-restoration state — please verify tab selection isn't silently changed

TvTabBar.kt:69 uses Modifier.focusRestorer(), and TvTabBar.kt:85 selects a tab on focus:

Tab(selected = ..., onFocus = { onTabSelect(index) }, ...)

On main the tab row was never disposed, so its restorer remembered which tab child last had focus. Now AnimatedVisibility disposes the whole bar on every detail entry, so when it comes back the restorer starts empty. If the first D-pad press into the tab row lands on the first tab rather than the previously selected one, onFocus fires and switches the user to Home — i.e. backing out of a playlist could kick you off the Playlists tab. Worth explicitly testing: Playlists → open a playlist → Back → D-pad Up and confirm the selected tab is unchanged.

If it does reproduce, an alternative that sidesteps both this and issue #1: keep TvTopBar composed and animate it out instead of removing it — e.g. an animated height/alpha plus Modifier.focusProperties { canFocus = false } on the container while hidden. That preserves TabRow state, keeps the "focus once on launch" LaunchedEffect(Unit) as it was, and lets you drop the whole autoFocusSelectedTab/onSelectedTabFocus/didFocusTopBar plumbing.

3. Known limitation is cheap to fix, and this PR changes its failure mode

Previously LaunchedEffect(Unit) { requestFocus() } re-ran when the bar re-entered composition, so backing out always left something focused (the tab bar). After this change nothing is focused until the next D-pad input. Since TvPodcastGridScaffold.kt:63-71 and TvRow.kt:79-92 already restore the last focused item via focusProperties { onEnter = ... }, simply requesting focus on the tab-content container when the bar becomes visible again (focusRestorer()/a FocusRequester on the Box at TvScaffold.kt:142) should land focus back on the tile you opened the detail from — resolving the limitation described in the PR body rather than deferring it.

4. requestFocus() should be guarded, and the flag can get stuck

TvTabBar.kt:55-60

LaunchedEffect(autoFocusSelectedTab) {
    if (autoFocusSelectedTab) {
        focusRequester.requestFocus()
        currentOnSelectedTabFocus()
    }
}

The rest of the module wraps this in runCatching (TvPodcastGridScaffold.kt:52, TvEmptyState.kt:45, TvPodcastDetailsScreen.kt:280) because an unattached FocusRequester throws — and here the requester is only attached to the tab where index == selectedTabIndex, so an empty/out-of-range tabs state on the first frame throws from a coroutine, which crashes. It also means onSelectedTabFocus() is skipped on failure, so didFocusTopBar never flips and the request is retried indefinitely on every re-entry. Pre-existing for the throw itself, but the PR is editing this line. Fix this →

rememberUpdatedState is also unnecessary here — the effect body never suspends before invoking the callback, and it restarts whenever the key changes.

5. Missing test (CI is red on this)

Danger is failing on TvTopBarVisibility. TvToastHostState has tv/src/test/.../TvToastHostStateTest.kt; a symmetrical TvTopBarVisibilityTest covering isVisible for nested enter/exit is a handful of lines and cheaper than the unit-tests-exemption label. Add the test →

Nits

  • Naming: onSelectedTabFocus doesn't observe focus — it reports that the initial focus request was issued (onInitialFocusRequested reads truer), and didFocusTopBar is about the tab bar, not the top bar.
  • Defaults: autoFocusSelectedTab = true / onSelectedTabFocus = {} on TvTopBar.kt:47-48 and TvTabBar.kt:50-51 mean a future call site silently steals focus. There's exactly one production caller each; making them required (previews pass explicitly) keeps the behaviour visible.
  • Discoverability: nothing enforces that a new detail branch calls HideTvTopBar(). A short KDoc on it — call from the navigation branch, not from inside the detail composable, and why — would help; longer term this belongs in a real nav graph for the TV app.
  • Generality: detailCount/enterDetail()/exitDetail() bake "detail screen" into a generic visibility holder; hideRequestCount + requestHidden()/releaseHidden() would age better if anything else ever needs to hide the bar.
  • Preview: TvScaffoldPreview only covers isTopBarVisible = true; a hidden-state preview would document the full-height layout the PR is adding.

Not verified: I couldn't compile the module or run spotlessCheck in this environment (Gradle invocation isn't permitted for this job), so items 2 and 3 in particular need on-device confirmation.
Branch

@sztomek
sztomek force-pushed the feat/tv-deep-destination-top-bar branch from e0eb07a to b4343cf Compare August 5, 2026 11:57
@sztomek
sztomek force-pushed the feat/tv-deep-destination-top-bar branch from b4343cf to c127887 Compare August 5, 2026 11:57
@sztomek sztomek added this to the 8.19 milestone Aug 5, 2026
@sztomek
sztomek marked this pull request as ready for review August 5, 2026 12:12
@sztomek
sztomek requested a review from a team as a code owner August 5, 2026 12:12
@sztomek
sztomek requested review from geekygecko and removed request for a team August 5, 2026 12:12
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @sztomek's task in 7m 21s —— View job


Review: [TV] Hide the top bar on nested detail screens

  • Read the PR diff and related TV module files
  • Analyze for bugs, focus behaviour, quality, performance
  • Post review

The shape of this is right: a @Stable state holder + staticCompositionLocalOf mirrors the existing TvToastHostState/LocalTvToastHostState pattern, driving visibility from the navigation branch keeps the detail composables reusable, and a counter rather than a boolean is the correct choice — it makes the folder → podcast transition in TvYourPodcastsScreen safe, since Compose dispatches onForgotten before onRemembered in the same apply pass, so the count dips to 0 and back to 1 without the bar flashing. The 56dp → 32dp end-padding change also checks out against the module's rhythm (TvRow.kt:50 uses horizontal = 32.dp), and 32dp still comfortably clears the 1.02× focus scale of TvEpisodeRow.

The concerns below are almost all about focus and the cost of the new animations — the two things that actually bite on TV hardware.


1. Disposing the top bar throws away TabRow's focus-restoration state, which can silently switch the user's tab

TvTabBar.kt:69 puts Modifier.focusRestorer() on the TabRow, and TvTabBar.kt:85 selects a tab on focus:

Tab(selected = index == selectedTabIndex, onFocus = { onTabSelect(index) }, ...)

focusRestorer() keeps "which child last had focus" in its modifier node. On main the bar lived in a Column and was never disposed, so after walking down from the Playlists tab into the grid, pressing Up restored focus to the Playlists tab. Now AnimatedVisibility (TvScaffold.kt:154) disposes the entire bar on every detail entry, so when it comes back the restorer has nothing saved and onRestoreFailed defaults to FocusRequester.Default — i.e. ordinary focus search. For a directional Up search that resolves geometrically, so the tab that gets focused is whichever one sits above the item you were on, not the one that's selected. Because focus is selection here, that fires onTabSelect(otherIndex).

Repro to run on device: Playlists → open a playlist → Back → D-pad Up. If focus lands on anything but the Playlists tab, you've just been moved to another tab.

The alternative that fixes this at the root: keep TvTopBar composed and animate its alpha (e.g. animateFloatAsState + Modifier.graphicsLayer), deactivating it while hidden with Modifier.focusProperties { canFocus = false } on the container. That preserves the restorer node, keeps the D-pad from reaching an invisible bar, and lets you delete the whole autoFocusSelectedTab / onSelectedTabFocus / didFocusTopBar chain — LaunchedEffect(Unit) { requestFocus() } goes back to firing exactly once, as on main. Fix this →

Secondary, and only partly this PR's problem: after Back, nothing is focused until the next D-pad press (same as main — the bar's LaunchedEffect(Unit) never re-ran there either, since it was never disposed). Since TvPodcastGridScaffold and TvRow already restore their last focused item via focusProperties { onEnter = ... }, requesting focus on the tab-content container when the bar becomes visible again would land you back on the tile you opened — which also makes issue 1 unreachable in the common path.

2. Animating padding remeasures the whole tab subtree for 300ms on every detail open/close

TvScaffold.kt:124-149: contentTopPadding is animated and applied as Modifier.padding(top = …) on the Crossfade, so for 300ms every frame remeasures and relayouts the entire current screen — the podcast grid, the episode LazyColumn, the two-pane detail. On low-end TV boxes that's the most expensive thing this PR adds, and it's exactly the reflow the PR body says Apple TV doesn't do ("the content underneath never reflows").

A placement-only animation gets the same visual for free: keep a static padding(top = topBarHeight) and animate Modifier.offset { IntOffset(0, -offsetPx) } (the lambda overload runs in the placement phase and skips remeasure), or animate graphicsLayer { translationY }. Fix this →

Related nit on the same block: DEFAULT_TOP_BAR_HEIGHT = 82.dp (TvScaffold.kt:182) hardcodes a value TvTopBar derives from its own padding + the 44dp tab height. If the measured height ever differs from 82dp, the very first onSizeChanged will animate the content inset at launch. Worth snapping the first measured value instead of animating it.

3. Crossfade + focus-driven tab selection composes several screens at once

Tab selection happens on focus, so walking the tab row from Home to Search selects every tab in between. Crossfade keeps each in-flight target composed until its animation finishes, so a quick traversal can have 3–4 screens alive simultaneously — each with its own hiltViewModel() and Room/flow subscriptions — and each incoming screen fires its own initial requestFocus() while the outgoing one still holds focus. Please try holding D-pad Right across the whole tab row on a device and watch for jank and focus jumping. If it misbehaves, debouncing onTabSelect (or animating only on committed selection) is cheaper than the crossfade.

4. didFocusTopBar isn't saved across activity recreation

TvScaffold.kt:53

var didFocusTopBar by remember { mutableStateOf(false) }

The detail state that drives HideTvTopBar() is saved (openedPodcastUuid / openedFolder / openedPlaylist are all rememberSaveable, and TvScaffold sits in a NavHost entry so its saveable state is restored). After a config change or process-death restore with a detail open: didFocusTopBar restores as false → the bar requests focus on the first frame → the HideTvTopBar() DisposableEffect then disposes the bar, taking the focused node with it, racing TvPodcastDetailsScreen.kt:280's own request. rememberSaveable also states the intent better ("the tab bar already had its one initial focus"). Moot if you take the always-composed approach in issue 1. Fix this →

Cosmetic sibling: TvTopBarVisibility isn't saveable either, so a restore with a detail open draws one frame with the bar before hiding it.

5. The toast anchor was tuned to the top bar's height

TvOnboardingNavHost.kt:87-92 pins TvToastHost to TopEnd with padding(top = 80.dp) — i.e. just under the ~82dp bar. Podcast and playlist detail screens do show toasts (TvEpisodeActionsModal at TvPodcastDetailsScreen.kt:341, TvPlaylistDetailsScreen.kt:267), and there's now no bar in that band, so the toast floats lower over live content than intended. Note TvToastHost lives outside TvScaffold, so LocalTvTopBarVisibility isn't in scope there — if you want it to react, the provider needs hoisting next to TvToastHostState. Low priority, but worth a look while you're on a device.

6. requestFocus() is unguarded, and a failure permanently re-arms the request

TvTabBar.kt:55-60

LaunchedEffect(autoFocusSelectedTab) {
    if (autoFocusSelectedTab) {
        focusRequester.requestFocus()
        currentOnSelectedTabFocus()
    }
}

The rest of the module wraps this in runCatching { … }.onFailure { Timber.e(it, …) } (TvPodcastGridScaffold.kt:52, TvEmptyState.kt:45, TvPodcastDetailsScreen.kt:280) because an unattached FocusRequester throws — and from a LaunchedEffect that's a crash. It isn't reachable today (tabs is always TvTab.entries and selectedTabIndex is always in range, so the requester is always attached), but there's a second reason to guard it: on failure onSelectedTabFocus() is skipped, so didFocusTopBar never flips and the focus request re-arms every time the bar re-enters composition. Fix this →

rememberUpdatedState is unnecessary here — the effect never suspends before invoking the callback, and it restarts whenever its key changes.

7. isVisible can latch to hidden forever

TvTopBarVisibility.kt:15val isVisible get() = detailCount == 0. One unbalanced exitDetail() drives the count negative and the bar never comes back. detailCount <= 0 costs nothing and makes the failure mode "bar shows" instead of "bar gone for the rest of the session". Fix this →

Nits

  • Test: the unit-tests-exemption label clears CI, but TvTopBarVisibility is the one genuinely unit-testable piece here, and TvToastHostStateTest.kt gives you the template. A ~15-line TvTopBarVisibilityTest would pin the nesting invariant that makes folder → podcast safe. Add the test →
  • Preview: TvScaffoldPreview only passes isTopBarVisible = true. The hidden state is what this PR adds — a second preview would document the full-height layout (and the checklist claims preview coverage).
  • Discoverability: nothing enforces that a future detail branch calls HideTvTopBar(). A short KDoc ("call from the navigation branch, not inside the detail composable, so detail screens stay reusable") would carry the intent until the TV app has a real nav graph.
  • Naming: onSelectedTabFocus doesn't observe focus — it reports that the initial request was issued (onInitialFocusRequested); didFocusTopBar is about the tab bar, not the top bar.
  • Defaults: autoFocusSelectedTab = true / onSelectedTabFocus = {} on TvTopBar.kt:47-48 and TvTabBar.kt:50-51 mean a future call site silently steals focus. One production caller each — making them required (previews pass explicitly) keeps that visible.
  • Generality: detailCount/enterDetail()/exitDetail() bake "detail screen" into an otherwise generic holder; hideRequestCount + requestHidden()/releaseHidden() would age better.

Not verified: I did not build the module or run spotlessCheck (Gradle isn't available to this job), so issues 1 and 3 in particular need on-device confirmation rather than being taken on my reasoning alone.
Branch

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants