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

Please ensure this works with LazyColumn, LazyRow and LazyVerticalGrid when this change is implemented. Thanks!

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

Thanks Nick, LazyList is the internal component that all these composables are built out of.

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

If I could add a small additional feature request here:

Hopefully the implementation of focus scrolling in lazy lists leaves room for the easy customization / overriding of the behavior, since TV UI often has unique snapping requirements around the focused item.

I've started to come up with my own compose API for this, and while I'm happy with the results so far, I'm hoping that any future built-in scrolling functionality will play nicely with it, or as a better alternative - provide all of the necessary configurations.

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

To add a specific case to #4's comment on configurability:

In View Android apps, scrolling typically occurs once focus is moved to an item outside the viewport. With such behavior, however, the user is given no indication that there are more items to see. To solve this, it would be useful to be able to specify a scroll offset or similar that triggers scrolling even before reaching an item that is outside of the scroll viewport. This could for instance be a fixed dp value or .5 "list item heights" ahead. That way, the user would be informed that there is more to see.

In any case, glad to see this being of high priority!

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

Looks like focus support for LazyLists did not make it to 1.1.0-beta01.

Custom implementation that scrolls a LazyList to bring the focused element to view still almost works, but occasionally the focus moves to a random input node. The random input node can be uninitialized causing an exception at:

2021-11-01 09:54:51.247 E/MessageQueue-JNI: java.lang.IllegalStateException: KeyEvent can't be processed because this key input node is not active.
    at androidx.compose.ui.input.key.KeyInputModifier.processKeyInput-ZmokQxo(KeyInputModifier.kt:75)
    at androidx.compose.ui.platform.AndroidComposeView.sendKeyEvent-ZmokQxo(AndroidComposeView.android.kt:439)
    at androidx.compose.ui.platform.AndroidComposeView.dispatchKeyEvent(AndroidComposeView.android.kt:446)

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

Apologies for pinging this thread. I'm also having the IllegalState exception issue from above. Should this be pulled out into a separate ticket as opposed to part of this one or is it all sort of the same thing? From our perspective we can get by with a custom implementation similar to above if it's stable (until the out of the box implementation is sorted later on).

As an aside thanks for all your hard work on this. Compose is a pleasure to work with!

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

Whoever accepted this issue, you probably have 67 thousand issues, but since the error message is the same, can you please check if this focus changing keyevent stuff problem is the same as with BasicTextfield ?

https://issuetracker.google.com/issues/215186908
<3

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

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

commit ef81023fc0de83421e4779f6bef8e14d4ef6a158
Author: Ralston Da Silva <ralu@google.com>
Date: Mon Feb 14 18:00:04 2022

Refactor focus search to accept a lambda

Refactoring the focus search code so that it runs a lambda
once it finds the next item. This is needed so that we can
request focus on the next item when the next item is beyond
visible bounds. We need this because beyond bounds layout
requests return a block and the items are guaranteed to be
available only within the scope of the block.

For more info, see go/compose-focus-beyondbounds

Bug: 184670295
Test: Internal refactoring, existing moveFocus() tests
Change-Id: I6545ef1a094f5bbe37eb6f861d1ea5ab6a7ec926

M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/OneDimensionalFocusSearch.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTraversal.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/TwoDimensionalFocusSearch.kt

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

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

Marked as fixed.

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

commit 3b4ce572c2fa394843ff0082cc985c8d02228560
Author: Ralston Da Silva <ralu@google.com>
Date: Tue Feb 15 16:57:08 2022

BeyondBoundsLayout ModifierLocal

This CL adds a BeyondBoundsLayout modifier local.

Bug: 184670295
Test: ./gradlew compose:ui:ui:cC -P android.testInstrumentationRunnerArguments.class=androidx.compose.ui.layout.BeyondBoundsLayoutTest
Relnote: Added a BeyondBoundsLayout modifier local
Change-Id: If8b51c6e08a375d1c733588e53c9b07474c0855c

A compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/layout/BeyondBoundsLayoutTest.kt
M compose/ui/ui/api/restricted_current.txt
M compose/ui/ui/api/current.txt
A compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/layout/BeyondBoundsLayout.kt
M compose/ui/ui/api/public_plus_experimental_current.txt

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