Modifier.onFocusChanged does not work if called after Modifier.focusModifier() [186567354]

Fixed

Bug

Status Update

No update yet.

Description

hv...@gmail.com

created issue #1

Apr 28, 2021 07:33AM

Jetpack Compose release version: 1.0.0-beta04
Android Studio Build: #AI-203.7148.57.2031.7242491

Steps to Reproduce:

Add focusModifier() before onFocusChanged {}
onFocusChanged is not called even if the element reaches focus
The required order is not documented in either of the modifiers

Sample project that demonstrates the bug can be found at https://github.com/hvrauhal/compose-focusmodifier-bug

In the following example BoxWithFocusModifiersInFaultyOrder is the not working version:

@Composable
private fun FocusDemo() {
    Column {
        Text("Use arrows to move focus over the following items:")
        BoxWithFocusModifiersInGoodOrder()
        BoxWithFocusModifiersInFaultyOrder()
        BoxWithFocusModifiersInGoodOrder()
        BoxWithFocusModifiersInFaultyOrder()
    }
}

@Composable
private fun BoxWithFocusModifiersInGoodOrder() {
    var color by remember { mutableStateOf(Color.Yellow) }
    Box(modifier = Modifier
        .onFocusChanged {
            color = if (it.isFocused) Color.Red else Color.Blue
        }
        .focusModifier()
        .background(color)
    ) {
        Text (text = "onFocusChanged is invoked", style = TextStyle(Color.White))
    }
}

@Composable
private fun BoxWithFocusModifiersInFaultyOrder() {
    var color by remember { mutableStateOf(Color.Yellow) }
    Box(modifier = Modifier
        .focusModifier()
        .onFocusChanged {
            color = if (it.isFocused) Color.Red else Color.Blue
        }
        .background(color)
    ) {
        Text("onFocusChanged is NOT invoked", style = TextStyle(Color.White))
    }
}

Comments

hv...@gmail.com <hv...@gmail.com> #2Apr 28, 2021 08:24AM

Hi. Thanks for reporting this. Fixed in this changelist. This should be available in alpha-04

Message last modified on Apr 28, 2021 08:25AM

so...@google.com <so...@google.com> Apr 28, 2021 09:27AM

Assigned to ra...@google.com.

hv...@gmail.com <hv...@gmail.com> #3May 3, 2021 09:16AM

Project: platform/frameworks/support
Branch: androidx-main

commit e782987543a9f8ccd485e970ddc74564b24378db
Author: Vighnesh Raut <vighnesh.raut13@gmail.com>
Date: Mon Jan 02 15:27:40 2023

fix: tab row crashes when only 1 tab is added

Bug:

b/264018028

Test: Added unit test

Change-Id: I6381dbac304fc1d69d3708c6655f8b595668e93f

M tv/tv-material/src/androidTest/java/androidx/tv/material/TabRowTest.kt
M tv/tv-material/src/main/java/androidx/tv/material/TabRow.kt

https://android-review.googlesource.com/2373449

ra...@google.com <ra...@google.com> May 3, 2021 06:44PM

Accepted by ra...@google.com.

ra...@google.com <ra...@google.com> #4May 3, 2021 08:50PM

deleted

hv...@gmail.com <hv...@gmail.com> #5May 4, 2021 10:53AM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.tv:tv-material:1.0.0-alpha04

ra...@google.com <ra...@google.com> #6May 5, 2021 10:34PM

I can't think of an actual use-case for multiple focusModifiers per layout node, but the nature of our system allows the addition of multiple focus modifiers. So this is not a realistic scenario, but if we want to make your example work, you would need another focus requester. Focus requesters are similar to onFocusChanged modifiers. Both of them request/observe the next focusModifier. Your example could be changed to:

Modifier
   .focusRequester(focusRequester1)
   .onFocusChanged { ... }
   .focusModifier()
   .border(10.dp)
   .focusRequester(focusRequester2)
   .onFocusChanged { ... }
   .focusModifier()

Now, calling focusRequester1.requestFocus() will change the state of the first focusModifier. focusRequester2.requestFocus() will change the state of the second focusModifier.

I guess the way to think of it is that the focus state is part of the focus modifier. When you call requestFocus(), the focusModifier's state is changed to "focused".

hv...@gmail.com <hv...@gmail.com> #7May 6, 2021 07:12AM

Thank you for the explanation! I tried that out and it meets expectations. One thing to note is that the platform provided d-pad navigation brings focus to the first focusRequester.

ra...@google.com <ra...@google.com> #8May 7, 2021 08:07AM

Oh, I didn't realize that you were using the d-pad. Yes, the d-pad will bring focus to the first focus modifier. You can think of the second modifier as a child of the first, imagine they were defined across different composables:

@Composable
fun FancyFocusableBox(){
   FocusableBox(Modifier
       .onFocusChanged {...}
       .focusModifier()
   )
}

@Composable
fun FocusableBox(modifier: Modifier){
   Box(modifier
       .onFocusChanged {...}
       .focusModifier()
   )
}

So coming back to your example, the d-pad will initially bring focus to the first modifier. If you then press d-pad center, focus will move to the next modifier.

ap...@google.com <ap...@google.com> #9Jun 18, 2021 03:45PM

Project: platform/frameworks/support
Branch: androidx-main

commit 94f406c27f67b2298be236e1c7f7ce11bf532785
Author: Ralston Da Silva <ralu@google.com>
Date: Wed Jun 09 14:44:08 2021

Focus Cleanup

Add Samples
Update Documentation
Remove internal references from public docs

Bug: 170154986
Bug: 190386715
Bug: 186567354
Bug: 168510304
Test: N/A
Relnote: "Removed deprecated experimental `FocusManager#moveFocusIn` and `FocusManager#moveFocusOut`"
Change-Id: I227d79e985d993d52d383d22439abaecbce9f593

M compose/ui/ui/api/public_plus_experimental_1.0.0-beta10.txt
M compose/ui/ui/api/public_plus_experimental_current.txt
M compose/ui/ui/integration-tests/ui-demos/src/main/java/androidx/compose/ui/demos/UiDemos.kt
A compose/ui/ui/integration-tests/ui-demos/src/main/java/androidx/compose/ui/demos/focus/CaptureFocusDemo.kt
M compose/ui/ui/integration-tests/ui-demos/src/main/java/androidx/compose/ui/demos/focus/CustomFocusOrderDemo.kt
M compose/ui/ui/samples/src/main/java/androidx/compose/ui/samples/FocusSamples.kt
A compose/ui/ui/samples/src/main/java/androidx/compose/ui/samples/KeyInputSamples.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/input/key/Key.android.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/input/key/KeyEvent.android.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusChangedModifier.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusManager.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusModifier.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusOrderModifier.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusRequester.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusRequesterModifier.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusState.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTraversal.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/key/Key.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/key/KeyEvent.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/key/KeyInputModifier.kt

https://android-review.googlesource.com/1732475

ra...@google.com <ra...@google.com> #10Jun 18, 2021 03:49PM

Marked as fixed.

Updated the documentation. Closing this bug.