Demo: use the system on-screen keyboard for TV search - #5727
Conversation
|
Claude finished @sztomek's task in 2m 34s —— View job Review
I've reviewed this as a spike whose job is to produce a fair demo for design, not as merge-ready code — so the bar I applied is "does the recording show the system IME at its best". Mostly yes; a few things would make the comparison misleading. Blocking for the demo1. Auto-focus can re-fire and steal focus back. 2. 3. No focus indication on the field. Worth fixing, cheap
No concerns
I did not build or run the TV app, so the auto-focus re-fire and the caret/placeholder overlap are read off the code rather than observed on a device. |
Generated by 🚫 Danger |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (autoFocus) { | ||
| LaunchedEffect(Unit) { | ||
| runCatching { focusRequester.requestFocus() } | ||
| keyboardController?.show() | ||
| } | ||
| } |
There was a problem hiding this comment.
Auto-focus can re-fire and steal focus back from the content rows.
TvSearchField is composed inside a LazyColumn item { } (TvSearchScreen.kt:64-74), so when the user D-pads down far enough to browse the categories/discover rows, the header item is disposed. Scrolling back up recomposes it, focusRequester is remembered fresh, and LaunchedEffect(Unit) runs again → focus jumps into the field and the system IME pops open over the browsing UI.
The version being replaced guarded against exactly this with hasAutoFocused hoisted into TvSearchContent (outside the LazyColumn), and that guard is gone here. Either hoist a "has auto-focused" flag back up into TvSearchContent, or hold it in rememberSaveable next to query in TvSearchScreen so it survives item disposal.
Two smaller things in this block:
runCatching { … }silently swallowsFocusRequester is not initialized. The failure mode it converts a crash into is "the field never focuses and the keyboard never appears" — i.e. exactly the thing this spike is trying to demo, failing invisibly. At minimum log it (Timber.e).keyboardController?.show()is redundant and racy:BasicTextFieldrequests the IME itself once it actually gains focus, and this call runs in the same frame asrequestFocus(), before the focus event has been dispatched or the text input session started. If you want a safety net, hang it off an observed focus change (interactionSource.collectIsFocusedAsState()/Modifier.onFocusChanged) rather than firing it blind.
| keyboardOptions = KeyboardOptions( | ||
| capitalization = KeyboardCapitalization.None, | ||
| autoCorrectEnabled = false, | ||
| imeAction = ImeAction.Search, | ||
| ), |
There was a problem hiding this comment.
ImeAction.Search is declared but there is no keyboardActions, so pressing Search on the leanback IME does nothing and the keyboard stays up. On a phone that's a minor annoyance; on TV the IME is a full-screen overlay, so there's no obvious way to dismiss it and get to the results — which will read badly in the demo recording. Worth adding before you record:
keyboardActions = KeyboardActions(onSearch = {
keyboardController?.hide()
focusManager.clearFocus()
}),Also: autoCorrectEnabled = false likely suppresses the IME's suggestion strip, but the testing instructions ask reviewers to "confirm … autocomplete suggestions show". If suggestions are part of what you want design to compare, this flag is working against you.
| Box(contentAlignment = Alignment.CenterStart) { | ||
| if (query.isEmpty()) { | ||
| Text( | ||
| text = stringResource(LR.string.search), | ||
| style = MaterialTheme.tvTypography.title2, | ||
| color = MaterialTheme.tvColors.textSecondary, | ||
| ) | ||
| } | ||
| innerTextField() | ||
| } |
There was a problem hiding this comment.
Two layout nits in the decoration box:
-
Cursor sits on top of the placeholder.
Box(contentAlignment = Alignment.CenterStart)left-aligns both the placeholder andinnerTextField(). While the field is focused and empty — which is the state the screen opens in, givenautoFocus = true— the blinking caret is drawn over the "S" of Search. Material'sTextFieldhides the placeholder on focus for this reason; consider doing the same, or offsetting the placeholder. -
The field wraps its content instead of filling the row.
modifierarrives with no width constraint, so the focusable/editable region is only as wide as the text (roughly zero when empty). That makes the focus target and any focus indicator you add effectively invisible on an empty query.Modifier.weight(1f)on theBox(orfillMaxWidth()at the call site) gives the field a stable, full-width hit area.
Separately: the old implementation kept the caret visible for long queries via horizontalScroll + scrollTo(maxValue). BasicTextField with singleLine = true handles that internally, so nothing is lost there — just flagging that I checked.
| TvSearchField( | ||
| query = query, | ||
| onQueryChange = onQueryChange, | ||
| autoFocus = true, | ||
| ) |
There was a problem hiding this comment.
With the custom keyboard gone, this text field is the only focusable element in the header, and BasicTextField draws no focus decoration of its own. On TV, D-pad focus has to be visible at all times — once the user moves down into the categories row there's no indication that the field above is focusable, and no highlight when focus returns to it. Worth adding a border/background driven by interactionSource.collectIsFocusedAsState() before showing this to design, otherwise the comparison is partly measuring "our keyboard has focus states, the system one doesn't".
Also note TvSearchScreen still only uses query to hide the discover rows (line 99) — nothing is dispatched to TvSearchViewModel, so typing won't produce search results in the demo. That's pre-existing on feat/tv-home-auth-feeds, not introduced here, but it does limit what the recording can show.
14ede72 to
425f5e8
Compare
|
Demo served its purpose — design approved the system on-screen keyboard. The change has been folded into the search stack at #5717 (custom keyboard removed across the stack), so this demo PR is redundant. Closing and deleting the branch. p1786523963874079-slack-C0ATWH7BNH3 |
2 participants
Description
Temporary demo / spike — not intended to merge.
Temporarily swaps our custom on-screen keyboard on the TV Search screen for Android TV's default system on-screen keyboard (the leanback IME described in Manage on-screen keyboards), so we can record a demo for design to compare the two approaches.
TvSearchFieldbecomes a real editableBasicTextField(search icon + placeholder preserved,ImeAction.Search, single line) that auto-focuses and shows the system keyboard on entry.TvSearchScreennow drives the query from the field'sonValueChangeand no longer renders our hand-rolledTvSearchKeyboard.The custom keyboard code (
TvSearchKeyboard.ktand its unit test) is left in the tree, unused, so it's trivial to restore once the demo has served its purpose.Branch is off
feat/tv-home-auth-feeds(which carries the TV search work), so this diff shows only the keyboard swap.Testing Instructions
./gradlew :tv:installDebugScreenshots or Screencast
Demo video:
scratchpad/tv-search-system-keyboard-demo.mp4Screenshot:
scratchpad/tv-search-system-keyboard.pngChecklist
./gradlew spotlessApplyto automatically apply formatting/linting)TvSearchKeyboardStateTestretainedmodules/services/localization/.../strings.xml— reuses existingsearchstringTvSearchFieldPreviewupdatedI have tested any UI changes...