Merge "Feature Carousel should allow internal focus movement before triggering slide changes" into androidx-main
diff --git a/tv/integration-tests/playground/src/main/java/androidx/tv/integration/playground/FeaturedCarousel.kt b/tv/integration-tests/playground/src/main/java/androidx/tv/integration/playground/FeaturedCarousel.kt
index 705ef91..6f6d535 100644
--- a/tv/integration-tests/playground/src/main/java/androidx/tv/integration/playground/FeaturedCarousel.kt
+++ b/tv/integration-tests/playground/src/main/java/androidx/tv/integration/playground/FeaturedCarousel.kt
@@ -151,7 +151,7 @@
}
@Composable
-private fun OverlayButton(modifier: Modifier = Modifier, text: String = "Test Button") {
+private fun OverlayButton(modifier: Modifier = Modifier, text: String = "Play") {
var isFocused by remember { mutableStateOf(false) }
Button(
diff --git a/tv/tv-material/src/androidTest/java/androidx/tv/material3/CarouselTest.kt b/tv/tv-material/src/androidTest/java/androidx/tv/material3/CarouselTest.kt
index b8eb11f..1c863ef 100644
--- a/tv/tv-material/src/androidTest/java/androidx/tv/material3/CarouselTest.kt
+++ b/tv/tv-material/src/androidTest/java/androidx/tv/material3/CarouselTest.kt
@@ -67,6 +67,7 @@
import androidx.compose.ui.unit.dp
import androidx.test.platform.app.InstrumentationRegistry
import com.google.common.truth.Truth.assertThat
+import kotlin.math.abs
import kotlinx.coroutines.delay
import org.junit.Rule
import org.junit.Test
@@ -364,7 +365,7 @@
}
@Test
- fun carousel_withCarouselItem_parentContainerGainsFocus_onBackPress() {
+ fun carousel_withCarouselItem_parentContainerGainsFocusOnBackPress() {
rule.setContent {
Box(modifier = Modifier
.testTag("box-container")
@@ -508,7 +509,7 @@
}
@Test
- fun carousel_manualScrolling_withFocusableItemsOnTop() {
+ fun carousel_manualScrollingWithFocusableItemsOnTop_focusStaysWithinCarousel() {
rule.setContent {
Column {
Row(horizontalArrangement = Arrangement.spacedBy(10.dp)) {
@@ -545,8 +546,7 @@
// Check that item 2 is in view and button 2 has focus
rule.onNodeWithText("Button-2").assertIsDisplayed()
- // TODO: Fix button 2 isn't gaining focus
- // rule.onNodeWithText("Button-2").assertIsFocused()
+ rule.onNodeWithText("Button-2").assertIsFocused()
// Check if the first focusable element in parent has focus
rule.onNodeWithText("Row-button-1").assertIsNotFocused()
@@ -566,15 +566,23 @@
}
@Test
- fun carousel_manualScrolling_fastMultipleKeyPresses() {
+ fun carousel_manualScrollingFastMultipleKeyPresses_focusStaysWithinCarousel() {
val carouselState = CarouselState()
val tabs = listOf("Tab 1", "Tab 2", "Tab 3")
+ var numberOfTimesTabGainedFocus = 0
rule.setContent {
var selectedTabIndex by remember { mutableStateOf(0) }
Column {
- TabRow(selectedTabIndex = selectedTabIndex) {
+ TabRow(
+ modifier = Modifier.onFocusChanged {
+ if (it.hasFocus || it.isFocused) {
+ numberOfTimesTabGainedFocus++
+ }
+ },
+ selectedTabIndex = selectedTabIndex
+ ) {
tabs.forEachIndexed { index, tab ->
Tab(
selected = index == selectedTabIndex,
@@ -592,22 +600,25 @@
}
rule.waitForIdle()
- rule.onNodeWithTag("pager").performSemanticsAction(SemanticsActions.RequestFocus)
+ rule.onNodeWithText("Play 0").performSemanticsAction(SemanticsActions.RequestFocus)
rule.waitForIdle()
val itemProgression = listOf(6, 3, -4, 3, -6, 5, 3)
+ // reset the counter at test start.
+ numberOfTimesTabGainedFocus = 0
itemProgression.forEach {
- if (it < 0) {
- performKeyPress(NativeKeyEvent.KEYCODE_DPAD_LEFT, it * -1)
- } else {
- performKeyPress(NativeKeyEvent.KEYCODE_DPAD_RIGHT, it)
- }
+ performKeyPress(
+ if (it < 0) NativeKeyEvent.KEYCODE_DPAD_LEFT else NativeKeyEvent.KEYCODE_DPAD_RIGHT,
+ abs(it)
+ )
+ rule.waitForIdle()
}
rule.mainClock.advanceTimeBy(animationTime)
val finalItem = itemProgression.sum()
+ assertThat(numberOfTimesTabGainedFocus).isEqualTo(0)
rule.onNodeWithText("Play $finalItem", useUnmergedTree = true).assertIsFocused()
performKeyPress(NativeKeyEvent.KEYCODE_DPAD_RIGHT, 3)
@@ -618,7 +629,7 @@
}
@Test
- fun carousel_manualScrolling_onDpadLongPress() {
+ fun carousel_manualScrollingDpadLongPress_moveOnlyOneSlide() {
rule.setContent {
SampleCarousel(itemCount = 6) { index ->
SampleButton("Button ${index + 1}")
@@ -663,7 +674,7 @@
}
@Test
- fun carousel_manualScrolling_ltr() {
+ fun carousel_manualScrollingLtr_RightMovesToNextSlideLeftMovesToPrevSlide() {
rule.setContent {
SampleCarousel { index ->
SampleButton("Button ${index + 1}")
@@ -709,7 +720,7 @@
}
@Test
- fun carousel_manualScrolling_rtl() {
+ fun carousel_manualScrollingRtl_LeftMovesToNextSlideRightMovesToPrevSlide() {
rule.setContent {
CompositionLocalProvider(
LocalLayoutDirection provides LayoutDirection.Rtl
@@ -789,6 +800,27 @@
rule.waitUntil(timeoutMillis = 5000) { itemChanges > minSuccessfulItemChanges }
}
+
+ @Test
+ fun carousel_slideWithTwoButtonsInARow_focusMovesWithinSlideAndChangesSlideOnlyOnFocusExit() {
+ rule.setContent {
+ // No AutoScrolling
+ SampleCarousel(timeToDisplayItemMillis = Long.MAX_VALUE) {
+ Row {
+ SampleButton("Left Button ${it + 1}")
+ SampleButton("Right Button ${it + 1}")
+ }
+ }
+ }
+
+ rule.onNodeWithText("Left Button 1").performSemanticsAction(SemanticsActions.RequestFocus)
+ performKeyPress(KeyEvent.KEYCODE_DPAD_RIGHT)
+ // focus should have moved from left to right button
+ rule.onNodeWithText("Right Button 1").assertIsFocused()
+ performKeyPress(KeyEvent.KEYCODE_DPAD_RIGHT)
+ // slide should have changed.
+ rule.onNodeWithText("Left Button 2").assertIsFocused()
+ }
}
@OptIn(ExperimentalTvMaterial3Api::class)
diff --git a/tv/tv-material/src/main/java/androidx/tv/material3/Carousel.kt b/tv/tv-material/src/main/java/androidx/tv/material3/Carousel.kt
index bf97f92..c8b28a0 100644
--- a/tv/tv-material/src/main/java/androidx/tv/material3/Carousel.kt
+++ b/tv/tv-material/src/main/java/androidx/tv/material3/Carousel.kt
@@ -16,9 +16,6 @@
package androidx.tv.material3
-import android.view.KeyEvent.KEYCODE_BACK
-import android.view.KeyEvent.KEYCODE_DPAD_LEFT
-import android.view.KeyEvent.KEYCODE_DPAD_RIGHT
import androidx.compose.animation.AnimatedContent
import androidx.compose.animation.AnimatedVisibilityScope
import androidx.compose.animation.ContentTransform
@@ -49,15 +46,18 @@
import androidx.compose.ui.ExperimentalComposeUiApi
import androidx.compose.ui.Modifier
import androidx.compose.ui.focus.FocusDirection
+import androidx.compose.ui.focus.FocusDirection.Companion.Left
+import androidx.compose.ui.focus.FocusDirection.Companion.Right
import androidx.compose.ui.focus.FocusManager
import androidx.compose.ui.focus.FocusRequester
import androidx.compose.ui.focus.FocusState
+import androidx.compose.ui.focus.focusProperties
import androidx.compose.ui.focus.focusRequester
import androidx.compose.ui.focus.onFocusChanged
import androidx.compose.ui.graphics.Color
+import androidx.compose.ui.input.key.Key
import androidx.compose.ui.input.key.KeyEventType.Companion.KeyUp
import androidx.compose.ui.input.key.key
-import androidx.compose.ui.input.key.nativeKeyCode
import androidx.compose.ui.input.key.onKeyEvent
import androidx.compose.ui.input.key.type
import androidx.compose.ui.platform.LocalFocusManager
@@ -137,7 +137,6 @@
.focusRequester(carouselOuterBoxFocusRequester)
.onFocusChanged {
focusState = it
-
// When the carousel gains focus for the first time
if (it.isFocused && isAutoScrollActive) {
focusManager.moveFocus(FocusDirection.Enter)
@@ -148,8 +147,8 @@
outerBoxFocusRequester = carouselOuterBoxFocusRequester,
focusManager = focusManager,
itemCount = itemCount,
- isLtr = isLtr,
- )
+ isLtr = isLtr
+ ) { focusState }
.focusable()
) {
AnimatedContent(
@@ -160,7 +159,8 @@
} else {
contentTransformStartToEnd
}
- }
+ },
+ label = "CarouselAnimation"
) { activeItemIndex ->
LaunchedEffect(Unit) {
this@AnimatedContent.onAnimationCompletion {
@@ -232,69 +232,93 @@
outerBoxFocusRequester: FocusRequester,
focusManager: FocusManager,
itemCount: Int,
- isLtr: Boolean
+ isLtr: Boolean,
+ currentCarouselBoxFocusState: () -> FocusState?
): Modifier = onKeyEvent {
- // Ignore KeyUp action type
- if (it.type == KeyUp) {
- return@onKeyEvent KeyEventPropagation.ContinuePropagation
+ fun showPreviousItem() {
+ carouselState.moveToPreviousItem(itemCount)
+ outerBoxFocusRequester.requestFocus()
+ }
+ fun showNextItem() {
+ carouselState.moveToNextItem(itemCount)
+ outerBoxFocusRequester.requestFocus()
}
- val showPreviousItemAndGetKeyEventPropagation = {
- if (carouselState.isFirstItem()) {
- KeyEventPropagation.ContinuePropagation
- } else {
- carouselState.moveToPreviousItem(itemCount)
- outerBoxFocusRequester.requestFocus()
- KeyEventPropagation.StopPropagation
- }
- }
- val showNextItemAndGetKeyEventPropagation = {
- if (carouselState.isLastItem(itemCount)) {
- KeyEventPropagation.ContinuePropagation
- } else {
- carouselState.moveToNextItem(itemCount)
- outerBoxFocusRequester.requestFocus()
- KeyEventPropagation.StopPropagation
+ fun updateItemBasedOnLayout(direction: FocusDirection, isLtr: Boolean) {
+ when (direction) {
+ Left -> if (isLtr) showPreviousItem() else showNextItem()
+ Right -> if (isLtr) showNextItem() else showPreviousItem()
}
}
- when (it.key.nativeKeyCode) {
- KEYCODE_BACK -> {
+ fun handledHorizontalFocusMove(direction: FocusDirection): Boolean =
+ when {
+ it.nativeKeyEvent.repeatCount > 0 ->
+ // Ignore long press key event for manual scrolling
+ KeyEventPropagation.StopPropagation
+
+ currentCarouselBoxFocusState()?.isFocused == true ->
+ // if carousel box has focus, do not trigger focus search as it can cause focus to
+ // move out of Carousel unintentionally.
+ if (shouldFocusExitCarousel(direction, carouselState, itemCount, isLtr)) {
+ KeyEventPropagation.ContinuePropagation
+ } else {
+ updateItemBasedOnLayout(direction, isLtr)
+ KeyEventPropagation.StopPropagation
+ }
+
+ !focusManager.moveFocus(direction) -> {
+ // if focus search was unsuccessful, interpret as input for slide change
+ updateItemBasedOnLayout(direction, isLtr)
+ KeyEventPropagation.StopPropagation
+ }
+ else -> KeyEventPropagation.StopPropagation
+ }
+
+ when {
+ // Ignore KeyUp action type
+ it.type == KeyUp -> KeyEventPropagation.ContinuePropagation
+ it.key == Key.Back -> {
focusManager.moveFocus(FocusDirection.Exit)
KeyEventPropagation.ContinuePropagation
}
- KEYCODE_DPAD_LEFT -> {
- // Ignore long press key event for manual scrolling
- if (it.nativeKeyEvent.repeatCount > 0) {
- return@onKeyEvent KeyEventPropagation.StopPropagation
- }
-
- if (isLtr) {
- showPreviousItemAndGetKeyEventPropagation()
- } else {
- showNextItemAndGetKeyEventPropagation()
- }
- }
-
- KEYCODE_DPAD_RIGHT -> {
- // Ignore long press key event for manual scrolling
- if (it.nativeKeyEvent.repeatCount > 0) {
- return@onKeyEvent KeyEventPropagation.StopPropagation
- }
-
- if (isLtr) {
- showNextItemAndGetKeyEventPropagation()
- } else {
- showPreviousItemAndGetKeyEventPropagation()
- }
- }
+ it.key == Key.DirectionLeft -> handledHorizontalFocusMove(Left)
+ it.key == Key.DirectionRight -> handledHorizontalFocusMove(Right)
else -> KeyEventPropagation.ContinuePropagation
}
+}.focusProperties {
+ // allow exit along horizontal axis only for first and last slide.
+ exit = {
+ when {
+ shouldFocusExitCarousel(it, carouselState, itemCount, isLtr) ->
+ FocusRequester.Default
+ else -> FocusRequester.Cancel
+ }
+ }
}
@OptIn(ExperimentalTvMaterial3Api::class)
+private fun shouldFocusExitCarousel(
+ focusDirection: FocusDirection,
+ carouselState: CarouselState,
+ itemCount: Int,
+ isLtr: Boolean
+): Boolean =
+ when {
+ // LTR: Don't exit if not first item
+ focusDirection == Left && isLtr && !carouselState.isFirstItem() -> false
+ // RTL: Don't exit if it is not the last item
+ focusDirection == Left && !isLtr && !carouselState.isLastItem(itemCount) -> false
+ // LTR: Don't exit if not last item
+ focusDirection == Right && isLtr && !carouselState.isLastItem(itemCount) -> false
+ // RTL: Don't exit if it is not the first item
+ focusDirection == Right && !isLtr && !carouselState.isFirstItem() -> false
+ else -> true
+ }
+
+@OptIn(ExperimentalTvMaterial3Api::class)
@Composable
private fun CarouselStateUpdater(carouselState: CarouselState, itemCount: Int) {
LaunchedEffect(carouselState, itemCount) {