Same behaviour items like list in leanback [275157318]

Verified

Bug

Status Update

No update yet.

Description

al...@gmail.com

created issue #1

Mar 26, 2023 10:22AM

Component used: TV

Version used: 1.0.0-alpha05

When set some TvLazyRow into TvLazyColumn, the below problem is occurred

Item is focused according to changing direction by D-PAD. When changes from a current row to another, leanback keeps state current row item and starts from first index in new row list, but in compose android framework finds nearest focusable item and this item is focused

Comments

rv...@google.com <rv...@google.com> #2Mar 27, 2023 10:18AM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

on...@jacon.cz <on...@jacon.cz> #3Mar 29, 2023 06:04PM

Have you tried benchmarking this? We are seeing huge performance drop when we wrap TvLazyRow in FocusGroup, especially when navigating between two rows.

Results with FocusGroup:

NavigationBenchmark_navigateLazyRowAndColumnsCompilationNone
frameDurationCpuMs   P50   30,1,   P90  107,6,   P95  129,5,   P99  201,8

NavigationBenchmark_navigateLazyRowAndColumnsCompilationPartial
frameDurationCpuMs   P50   29,6,   P90  118,0,   P95  142,1,   P99  188,8

Results without FocusGroup:

NavigationBenchmark_navigateLazyRowAndColumnsCompilationNone
frameDurationCpuMs   P50   28,2,   P90   60,5,   P95   75,4,   P99  136,3

NavigationBenchmark_navigateLazyRowAndColumnsCompilationPartial
frameDurationCpuMs   P50  18,8,   P90  29,4,   P95  38,4,   P99  48,4

Samples (mostly copy-pasted from androidx.tv):

with FocusGroup https://github.com/okycelt/compose-tv/tree/a9b08cbd646064b3c9e176db245ca008ef7ffe67
without FocusGroup https://github.com/okycelt/compose-tv/tree/aa73d646cfd8bfe859e21e85ac45b40a49aff835

By the way, it would be nice if FocusGroup could automatically focus the item with Modifier.initiallyFocused().

Either way, thanks for the sample FocusGroup.

al...@gmail.com <al...@gmail.com> #4Apr 4, 2023 01:37PM

That's right. I've seen a huge performance drop when select other row. I think your solution is not good for this situation.

rv...@google.com <rv...@google.com> #5Apr 4, 2023 02:14PM

Thanks for the feedback. Since this is just a sample, we haven't benchmarked it. We know that this is not the best solution. You can use the sample as a starter and do any enhancements which you feel are necessary.

We are still investigating better ways to do this. We will post here once we have an update.

rv...@google.com <rv...@google.com> Apr 5, 2023 05:13AM

Status: Assigned (reopened)

ap...@google.com <ap...@google.com> #6Jun 2, 2023 11:25PM

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

commit ee78f0dd46533f9b9cd7e0e588d1c09b19f87785
Author: Ralston Da Silva <ralu@google.com>
Date: Fri Jun 02 13:40:26 2023

FocusRestorer API

Adds an api to restore focus to a previously focused child

Bug: 283243426
Bug: 235681940
Bug: 235691035
Bug: 272302679
Bug: 275157318
Test: ./gradlew compose:ui:ui:cC -P android.testInstrumentationRunnerArguments.package=androidx.compose.ui.focus.FocusRestorerTest
Test: ./gradlew compose:ui:ui:cC -P android.testInstrumentationRunnerArguments.package=androidx.compose.ui.focus.RestoreFocusTest
Relnote: "Added FocusRequester.saveFocusedChild and FocusRequester.restoreFocusedChild"
Change-Id: Ic557eaacc4b5c5fe9f908c5afcc86fc5e84f2bac

M compose/ui/ui/api/current.txt
M compose/ui/ui/api/restricted_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/FocusRestorationDemo.kt
M compose/ui/ui/samples/src/main/java/androidx/compose/ui/samples/FocusSamples.kt
A compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/focus/FocusRestorerTest.kt
A compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/focus/RestoreFocusTest.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/FocusRequesterModifierNode.kt
A compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusRestorer.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTargetNode.kt

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

vr...@gmail.com <vr...@gmail.com> #7Jun 7, 2023 10:46PM

Which version will this change be part of?

al...@gmail.com <al...@gmail.com> #8Jul 25, 2023 07:53AM

When we use FocusGroup composable fun, we can not use BackHandler function. We removed FocusGroup from my screen and after that BackHandler ables to run.

vi...@google.com <vi...@google.com> #9Jul 25, 2023 09:27AM

FocusGroup was a stop-gap, which is no longer preferred. FocusRestorer is part of compose-foundation 1.6.0-alpha*.

You can find examples of how to use FocusRestorer in FocusRestorationDemo and FocusRestorerSample.

Backhandler should work with FocusRestorer.

Also, please refer to this example to move focus to the first element in an unexplored row.

Message last modified on Jul 25, 2023 09:33AM

[Deleted User] <[Deleted User]> #10Aug 30, 2023 09:00AM

I continue to have problems with moving focus to first element in an unexplored row if I scroll the list. For example: I have a TvLazyColumn and it contains a list of TvLazyRow. For the first Rows all is ok, the focus goes on the first element and if I move up and down the focus is restored. But, if I scroll the entire list, for one of the new rows the focus is moved to another position like the position of an older row. It rememeber the focus.

Row 1 focus ok Row 2 focus ok Row 3 focus ok .... Row 10 it get focus of row 1 Row 11 gets focus of row 2 and so on

rv...@google.com <rv...@google.com> #11May 12, 2024 05:21AM

Verified by rv...@google.com.

Focus restoration APIs are now available and you can use them as shown here: https://stackoverflow.com/questions/76880923/how-to-always-focus-on-the-first-item-while-focussing-on-a-tvlazyrow

@Angelo, please file a new ticket for this as this is a different issue.