Merge "Fix focus transaction issue" into androidx-mainandroidx-main

5 files changed, 47 insertions, 17 deletions

diff --git a/compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/focus/FocusTransactionsTest.kt b/compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/focus/FocusTransactionsTest.kt
index 5c098dec901..bd71cacfefe 100644
--- a/compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/focus/FocusTransactionsTest.kt
+++ b/compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/focus/FocusTransactionsTest.kt
@@ -22,7 +22,6 @@ import androidx.compose.foundation.layout.size
 import androidx.compose.ui.ExperimentalComposeUiApi
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.focus.FocusRequester.Companion.Cancel
-import androidx.compose.ui.focus.FocusStateImpl.Active
 import androidx.compose.ui.focus.FocusStateImpl.ActiveParent
 import androidx.compose.ui.focus.FocusStateImpl.Inactive
 import androidx.compose.ui.input.InputMode.Companion.Keyboard
@@ -92,6 +91,40 @@ class FocusTransactionsTest {
         }
     }
 
+    @OptIn(ExperimentalComposeUiApi::class)
+    @Test
+    fun reentrantRequestFocus_byCallingRequestFocusWithinOnFocusChanged2() {
+        // Arrange.
+        val (item1, item2) = FocusRequester.createRefs()
+        var (item1Focused, item2Focused) = List(2) { false }
+        rule.setFocusableContent {
+            Box(
+                Modifier
+                    .focusRequester(item1)
+                    .onFocusChanged {
+                        item1Focused = it.isFocused
+                        if (item1Focused) item2.requestFocus()
+                    }
+                    .focusTarget()
+            )
+            Box(
+                Modifier
+                    .focusRequester(item2)
+                    .onFocusChanged { item2Focused = it.isFocused }
+                    .focusTarget()
+            )
+        }
+
+        // Act.
+        rule.runOnIdle { item1.requestFocus() }
+
+        // Assert.
+        rule.runOnIdle {
+            assertThat(item1Focused).isFalse()
+            assertThat(item2Focused).isTrue()
+        }
+    }
+
     @Test
     fun cancelTakeFocus_fromOnFocusChanged() {
         // Arrange.
@@ -129,10 +162,8 @@ class FocusTransactionsTest {
         // Assert.
         rule.runOnIdle {
             assertThat(focusState1).isEqualTo(Inactive)
-            // TODO(b/312524818): When a focus transaction is cancelled, we should re-notify
-            //  all the focus event modifiers that were called in the previous transaction.
-            assertThat(focusState2).isEqualTo(Active) // Should be Inactive.
-            assertThat(focusState3).isEqualTo(Active) // Should be Inactive.
+            assertThat(focusState2).isEqualTo(Inactive)
+            assertThat(focusState3).isEqualTo(Inactive)
 
             val root = view as AndroidComposeView
 
diff --git a/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusOwnerImpl.kt b/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusOwnerImpl.kt
index 09e2c4737c5..3666a3e46d2 100644
--- a/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusOwnerImpl.kt
+++ b/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusOwnerImpl.kt
@@ -132,11 +132,9 @@ internal class FocusOwnerImpl(
      * [FocusTargetNode] was found or if the focus search was cancelled.
      */
     override fun takeFocus(focusDirection: FocusDirection, previouslyFocusedRect: Rect?): Boolean {
-        return focusTransactionManager.withExistingTransaction {
-            focusSearch(focusDirection, previouslyFocusedRect) {
-                it.requestFocus(focusDirection) ?: false
-            } ?: false
-        }
+        return focusSearch(focusDirection, previouslyFocusedRect) {
+            it.requestFocus(focusDirection) ?: false
+        } ?: false
     }
 
     /**
diff --git a/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTransactionManager.kt b/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTransactionManager.kt
index 59327a038ae..ab8d02b50da 100644
--- a/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTransactionManager.kt
+++ b/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTransactionManager.kt
@@ -89,12 +89,13 @@ internal class FocusTransactionManager {
         }
         states.clear()
         ongoingTransaction = false
+        cancellationListener.clear()
     }
 
     private fun cancelTransaction() {
-        cancellationListener.forEach { it() }
-        cancellationListener.clear()
         states.clear()
         ongoingTransaction = false
+        cancellationListener.forEach { it() }
+        cancellationListener.clear()
     }
 }
diff --git a/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTransactions.kt b/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTransactions.kt
index 6078b915791..24eaca0f682 100644
--- a/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTransactions.kt
+++ b/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/focus/FocusTransactions.kt
@@ -41,7 +41,9 @@ import androidx.compose.ui.node.requireOwner
 internal fun FocusTargetNode.requestFocus(): Boolean = requestFocus(Enter) ?: false
 
 internal fun FocusTargetNode.requestFocus(focusDirection: FocusDirection): Boolean? {
-    return requireTransactionManager().withNewTransaction {
+    return requireTransactionManager().withNewTransaction(
+        onCancelled = { if (node.isAttached) refreshFocusEventNodes() }
+    ) {
         when (performCustomRequestFocus(focusDirection)) {
             None -> performRequestFocus()
             Redirected -> true
diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt
index dc830200a27..8cbcca37f6e 100644
--- a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt
+++ b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt
@@ -206,10 +206,8 @@ internal class SkiaBasedOwner(
     init {
         snapshotObserver.startObserving()
         root.attach(this)
-        focusOwner.focusTransactionManager.withNewTransaction {
-            // TODO instead of taking focus here, call this when the owner gets focused.
-            focusOwner.takeFocus(Enter, previouslyFocusedRect = null)
-        }
+        // TODO instead of taking focus here, call this when the owner gets focused.
+        focusOwner.takeFocus(Enter, previouslyFocusedRect = null)
     }
 
     fun dispose() {