Propagate composition locals to layers in the (re)composition phase (#…

…1233)
JetBrains · Apr 4, 2024 · b3621de · b3621de
1 parent 3d9809a
commit b3621de
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 13 deletions.
diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneLayer.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneLayer.skiko.kt
@@ -19,12 +19,9 @@ package androidx.compose.ui.scene
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.CompositionLocalContext
 import androidx.compose.runtime.DisposableEffect
-import androidx.compose.runtime.SideEffect
 import androidx.compose.runtime.currentCompositionLocalContext
-import androidx.compose.runtime.getValue
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.rememberCompositionContext
-import androidx.compose.runtime.rememberUpdatedState
 import androidx.compose.ui.InternalComposeUiApi
 import androidx.compose.ui.graphics.Color
 import androidx.compose.ui.input.key.KeyEvent
@@ -172,27 +169,24 @@ internal fun rememberComposeSceneLayer(
     val density = LocalDensity.current
     val layoutDirection = LocalLayoutDirection.current
     val parentComposition = rememberCompositionContext()
-    val compositionLocalContext by rememberUpdatedState(currentCompositionLocalContext)
+    val compositionLocalContext = currentCompositionLocalContext
     val layer = remember {
         scene.createLayer(
             density = density,
             layoutDirection = layoutDirection,
             focusable = focusable,
             compositionContext = parentComposition,
-        ).also {
-            it.compositionLocalContext = compositionLocalContext
-        }
+        )
     }
     layer.focusable = focusable
+    layer.compositionLocalContext = compositionLocalContext
+    layer.density = density
+    layer.layoutDirection = layoutDirection
+
     DisposableEffect(Unit) {
         onDispose {
             layer.close()
         }
     }
-    SideEffect {
-        layer.density = density
-        layer.layoutDirection = layoutDirection
-        layer.compositionLocalContext = compositionLocalContext
-    }
     return layer
 }
diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/MultiLayerComposeScene.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/MultiLayerComposeScene.skiko.kt
@@ -516,7 +516,13 @@ private class MultiLayerComposeSceneImpl(
          * This scenario is important when user code relies on hover events to show tooltips.
          */
         override var boundsInWindow: IntRect by mutableStateOf(IntRect.Zero)
+
+        @Deprecated(
+            message = "Should not be used in this implementation",
+            level = DeprecationLevel.ERROR
+        )
         override var compositionLocalContext: CompositionLocalContext? = null
+
         override var scrimColor: Color? by mutableStateOf(null)
         override var focusable: Boolean = focusable
             set(value) {
@@ -581,7 +587,14 @@ private class MultiLayerComposeSceneImpl(
             composition?.dispose()
             composition = owner.setContent(
                 parent = this@AttachedComposeSceneLayer.compositionContext,
-                { this@AttachedComposeSceneLayer.compositionLocalContext }
+                {
+                    /*
+                     * Do not use `compositionLocalContext` here - composition locals already
+                     * available from `compositionContext` and explicitly overriding it might cause
+                     * issues. See https://github.com/JetBrains/compose-multiplatform/issues/4558
+                     */
+                    null
+                }
             ) {
                 owner.setRootModifier(background then keyInput)
                 content()

diff --git a/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/window/PopupTest.kt b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/window/PopupTest.kt
@@ -20,6 +20,7 @@ import androidx.compose.foundation.layout.Box
 import androidx.compose.foundation.layout.fillMaxSize
 import androidx.compose.foundation.layout.offset
 import androidx.compose.foundation.layout.size
+import androidx.compose.material.Text
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.CompositionLocalProvider
 import androidx.compose.runtime.DisposableEffect
@@ -58,6 +59,7 @@ import androidx.compose.ui.test.assertIsDisplayed
 import androidx.compose.ui.test.assertPositionInRootIsEqualTo
 import androidx.compose.ui.test.assertWidthIsEqualTo
 import androidx.compose.ui.test.onNodeWithTag
+import androidx.compose.ui.test.runComposeUiTest
 import androidx.compose.ui.test.runSkikoComposeUiTest
 import androidx.compose.ui.touch
 import androidx.compose.ui.unit.Density
@@ -125,6 +127,41 @@ class PopupTest {
         assertThat(actualLocalValue).isEqualTo(3)
     }
 
+    // https://github.com/JetBrains/compose-multiplatform/issues/4558
+    @Test
+    fun changeInStaticCompositionLocalVisibleImmediatelyInPopup() = runComposeUiTest {
+        // Test that when the provided value of a staticCompositionLocalOf changes, the change is
+        // seen correctly in the content of a popup.
+        // The `text` variable is needed in order to cause a recomposition of the popup content in
+        // the same frame as CompositionLocalProvider is recomposed. Without this the bug doesn't
+        // reproduce because first CompositionLocalProvider is recomposed and that triggers
+        // recomposition of the popup content, which happens afterward.
+
+        val compositionLocal = staticCompositionLocalOf<Int> {
+            error("not set")
+        }
+        var providedValue by mutableStateOf(1)
+        var text by mutableStateOf("1")
+        var actualLocalValue = 0
+
+        setContent {
+            CompositionLocalProvider(compositionLocal provides providedValue) {
+                Popup {
+                    actualLocalValue = compositionLocal.current.also {
+                        println(it)
+                    }
+                    Text(text)
+                }
+            }
+        }
+
+        assertThat(actualLocalValue).isEqualTo(providedValue)
+        text = "2"
+        providedValue = 2
+        waitForIdle()
+        assertThat(actualLocalValue).isEqualTo(providedValue)
+    }
+
     // https://github.com/JetBrains/compose-multiplatform/issues/3142
     @Test
     fun passLayoutDirectionToPopup() = runSkikoComposeUiTest {