[TV] Search screen: system on-screen keyboard + browse categories - #5717
[TV] Search screen: system on-screen keyboard + browse categories#5717sztomek wants to merge 10 commits into
Conversation
|
Claude finished @sztomek's task in 3m 3s —— View job Review: TV Search groundwork — extract content rows renderer
Verdict: looks good to merge. This is a genuine no-op move and the renames are complete. Three non-blocking notes, one of which I'd suggest handling now because it's the exact thing the extraction is for. VerificationI compared the removed Other checks:
Inline comments
Minor, no comment posted
What I couldn't verifyI wasn't able to run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
sztomek
changed the title
sztomek
changed the title
c462e11 to
16ca072
Compare
sztomek
changed the title
16ca072 to
8abd445
Compare
sztomek
changed the title
b4f145e to
81331ea
Compare
|
Claude finished @sztomek's task in 5m 34s —— View job Review: TV Search screen — on-screen keyboard + browse categories
Verdict: solid, one thing I'd want checked on a device before merge. The screen layer is clean, the stateful/stateless split is right, previews are there, strings are localized, and the Findings1. 2. 3. 4. 5. Minor, no inline comment
What I couldn't verifyI didn't run |
81331ea to
b607b74
Compare
|
Claude finished @sztomek's task in 5m 18s —— View job Review: TV Search screen — re-review after
|
ae186ef to
b42ccfd
Compare
sztomek
changed the title
geekygecko
left a comment
geekygecko
left a comment
There was a problem hiding this comment.
This interaction doesn't feel quite right. When navigating left and right through the top menu, moving to Search immediately jumps into the page and focuses the search field.
I think the user should have to press Enter on the search field before the keyboard appears. Instead, when they navigate to the Search page, pressing Down once could highlight the search field in some way, for example by changing the magnifying glass from grey to white.
From there, pressing Enter would open the keyboard, or instead pressing Down again would continue into the Discover content.
|
Claude finished @sztomek's task in 4m 30s —— View job Review: TV Search screen — re-review after the resting/editing rework
Verdict: the interaction rework is the right shape and resolves @geekygecko's feedback — but there's one rendering bug I'd fix before merge. The branch is now off The interaction feedback is addressed@geekygecko asked for: no auto-jump into the page, Down to highlight the field, Enter to open the keyboard, Down again to continue into content. That is exactly what the new field does:
Also worth noting: the previous Findings1. 2. 3. 4. Unchanged from previous passes
Checked this pass, nothing to report
What I couldn't verifyGradle isn't runnable in this job, so |
| Box(contentAlignment = Alignment.CenterStart) { | ||
| if (query.isEmpty()) { | ||
| Text( | ||
| text = stringResource(LR.string.search), | ||
| style = MaterialTheme.tvTypography.title3, | ||
| color = MaterialTheme.tvColors.textSecondary, | ||
| ) | ||
| } | ||
| innerTextField?.invoke() | ||
| } |
There was a problem hiding this comment.
The typed query disappears as soon as you leave editing mode.
In the resting branch the field is rendered with innerTextField = null (L106). This Box then contains the placeholder only when query.isEmpty(), and nothing else — so with a non-empty query it renders an empty Box. Type huberman, press Enter (ImeAction.Search → stopEditing()) or Back, and the field collapses to just the magnifying glass. The state is still there (query is hoisted in TvSearchScreen), it's simply never drawn.
The preview is the giveaway: TvSearchFieldPreview passes query = "huberman" and editing starts false, so the preview renders a blank field rather than the query it's clearly meant to show.
| Box(contentAlignment = Alignment.CenterStart) { | |
| if (query.isEmpty()) { | |
| Text( | |
| text = stringResource(LR.string.search), | |
| style = MaterialTheme.tvTypography.title3, | |
| color = MaterialTheme.tvColors.textSecondary, | |
| ) | |
| } | |
| innerTextField?.invoke() | |
| } | |
| Box(contentAlignment = Alignment.CenterStart) { | |
| if (query.isEmpty()) { | |
| Text( | |
| text = stringResource(LR.string.search), | |
| style = MaterialTheme.tvTypography.title3, | |
| color = MaterialTheme.tvColors.textSecondary, | |
| ) | |
| } | |
| if (innerTextField != null) { | |
| innerTextField() | |
| } else { | |
| Text( | |
| text = query, | |
| style = MaterialTheme.tvTypography.title3, | |
| color = contentColor, | |
| maxLines = 1, | |
| ) | |
| } | |
| } |
That also gives contentColor a second use — right now it's computed from focus state in both branches but only ever applied to the icon, so the grey→white transition @geekygecko asked for lands on the icon alone and not on the text beside it.
| LaunchedEffect(restoreRestFocus) { | ||
| if (restoreRestFocus) { | ||
| runCatching { restFocusRequester.requestFocus() } | ||
| restoreRestFocus = false | ||
| } | ||
| } |
There was a problem hiding this comment.
Both requestFocus() calls here skip the withFrameNanos {} the rest of the TV module uses, and this one is the riskier of the two.
restFocusRequester is attached to a node that is composed in this very recomposition — LaunchedEffect bodies are dispatched on AndroidUiDispatcher, which drains before the measure/layout pass, so the target hasn't been placed yet when requestFocus() runs. runCatching swallows the resulting failure, and because the BasicTextField that held focus was just removed from composition, focus ends up nowhere: after dismissing the keyboard the D-pad has no anchor on the screen.
Every other focus restore in tv/ guards against exactly this — TvEmptyState.kt:41-44, TvNowPlayingScreen.kt:179-184, and TvTabBar.kt:65-73 / TvEpisodeListFocus.kt:96-99 (which retry across several frames):
| LaunchedEffect(restoreRestFocus) { | |
| if (restoreRestFocus) { | |
| runCatching { restFocusRequester.requestFocus() } | |
| restoreRestFocus = false | |
| } | |
| } | |
| LaunchedEffect(restoreRestFocus) { | |
| if (restoreRestFocus) { | |
| withFrameNanos {} | |
| runCatching { restFocusRequester.requestFocus() } | |
| restoreRestFocus = false | |
| } | |
| } |
(needs androidx.compose.runtime.withFrameNanos.)
The same applies to the editing-entry effect at L65-68: fieldFocusRequester is likewise brand new, and keyboardController?.show() on the next line only does anything once the field actually holds focus — so if the request fails, the user presses Center on the field and no IME appears. Worth a frame there too.
| runCatching { fieldFocusRequester.requestFocus() } | ||
| keyboardController?.show() | ||
| } | ||
| BackHandler(enabled = true) { stopEditing() } |
There was a problem hiding this comment.
editing is tied to composition, not to focus, so this BackHandler can stay armed while focus is somewhere else entirely.
Nothing sets editing = false when the field loses focus. The system IME normally keeps focus inside its own window, so in the common case this doesn't bite — but with a physical keyboard attached (the leanback IME often doesn't show at all), the user can D-pad Down from the text field into the Browse categories row while editing is still true. From there:
- Back no longer leaves the screen — it calls
stopEditing(), - which sets
restoreRestFocus = true, - which yanks focus back up to the search field from a row the user deliberately moved to.
Tying the two together closes it, and also handles the D-pad-away case generally:
modifier = modifier
.focusRequester(fieldFocusRequester)
.onFocusChanged { if (!it.isFocused) stopEditing() }(with a guard so the initial unfocused composition doesn't immediately cancel editing before requestFocus() lands). Worth a device check with a USB keyboard plugged in, since that's the configuration where it's reachable.
Two smaller notes on the same state machine:
editingandrestoreRestFocusareremember, notrememberSaveable, so a config change drops you back to the resting field whilequery(which is saveable) survives. Same asymmetry as before, just moved.- On the leanback IME, Back is usually consumed by the IME window to dismiss the keyboard, so this handler likely needs a second Back press to fire. That leaves an intermediate state — keyboard hidden,
editingstilltrue— which is the same state theonFocusChangedabove would clean up.
| .clickable( | ||
| interactionSource = interactionSource, | ||
| indication = null, | ||
| ) { editing = true }, |
There was a problem hiding this comment.
Nit, accessibility: the resting field is a bare clickable with no role and no label — the icon is contentDescription = null and the placeholder Text only exists while the query is empty. TalkBack announces "Search, double tap to activate" by accident when empty, and nothing at all once a query is set (see the rendering issue above). role = Role.Button plus onClickLabel = stringResource(LR.string.search) would make the affordance explicit.
Also cosmetic: the resting Row wraps its content, while the BasicTextField in the editing branch fills the available width. The focusable target therefore changes size between the two states, and since the only focus affordance is the icon tint (no background or border), the field can be easy to spot as "focused" only if you're looking at the icon. @geekygecko's suggestion was specifically the grey→white magnifying glass, so this may be exactly the intent — flagging just in case the Figma also has a container treatment.
|
Claude finished @sztomek's task in 2m 42s —— View job Review: TV Search screen — re-review after
|
|
thanks for your feedback @geekygecko i took your advice and made changes: Screen_recording_20260813_143102.mp4please take another look when you can! 🙏 |
|
Claude finished @sztomek's task in 4m 2s —— View job Review: TV Search screen — re-review after
|
Review: TV Search screen — re-review after
|
Addresses review feedback that entering the Search tab jumped straight into the field and popped the on-screen keyboard. The field now rests as a focusable element whose magnifier brightens on focus; Enter opens the keyboard and Down continues into the browse categories. The field is also slightly smaller.
a38b57e to
e740e43
Compare
Generated by 🚫 Danger |
|
Claude finished @sztomek's task in 2m 35s —— View job Review: TV Search screen — re-review after the latest push
Verdict: this push is a history rewrite, not new work — the working tree is unchanged from what I reviewed 4 minutes ago, so both findings from that pass are still open and unaddressed. No new findings. What actually changedThe branch was squashed. The four commits I'd been reviewing incrementally ( The resulting file content lines up exactly with the state described in my 13:36 pass — So the squash is clean and loses nothing. Worth noting only because the two inline threads I opened at 13:36 are anchored to the pre-squash SHAs; the code they point at is still there verbatim. Still open from the last pass1. LaunchedEffect(editing) {
if (editing) {
runCatching { fieldFocusRequester.requestFocus() } // L70 — node composed this pass
keyboardController?.show()
2. Also still open, previously commented
Checked this pass, nothing to report
What I couldn't verify
|
3 participants
Description
The Android TV search screen: an editable query field backed by Android TV's default system on-screen keyboard (the leanback IME, per Manage on-screen keyboards), plus a Browse categories row.
Search field + keyboard
search/TvSearchField.kt— an editableBasicTextField(search icon + placeholder + cursor) withImeAction.Search, single line. It auto-focuses and shows the system keyboard on entry, so text comes from the leanback IME (or a connected physical keyboard).search/TvSearchScreen.kt— holds the query state and drives it from the field'sonValueChange; split into a stateful screen (hiltViewModel) and a preview-friendly statelessTvSearchContent. Wrapped inverticalScrollso the categories row is reachable by D-pad.home/TvScaffold.kt— theTvTab.Searchtab rendersTvSearchScreen.Browse categories (new, display-only)
Modeled on the Apple TV category tiles.
component/TvCategoryTile.kt— aTvTile-based rounded card with a centered tinted icon (category.icon, rendered as a monochrome mask — same treatment as the mobile category pills) over the category name; icon/label swaptextSecondary→textPrimaryon focus.search/TvSearchViewModel.kt—@HiltViewModel, loads the category list and exposescategories: StateFlow<List<DiscoverCategory>>(empty on failure). + unit tests.TvRow(title = "Browse categories")ofTvCategoryTiles (shown only once categories load).Scope: display-only — category tiles have no click navigation and no analytics (both intentionally deferred). Autocomplete suggestions and search results are follow-ups (
ImprovedSearchManageris already reachable fromtv/).Fixes POC-799 https://linear.app/a8c/issue/POC-799/search-skeleton-ui
Figma: Ftk3KwnfqaK4g57yCN63p0-fi-2595_2028
Conversation: p1786523963874079-slack-C0ATWH7BNH3
Testing Instructions
./gradlew :tv:testDebugUnitTestpasses.Screenshots or Screencast
Checklist
./gradlew spotlessApply)modules/services/localization/src/main/res/values/strings.xml