Fixes scrollingContainer from 'blocking' LayoutModifierNode overscrol…

…l implementations

LayoutModifierNodes cannot meaningfully delegate to other LayoutModifierNodes currently, so this just means that the delegated node never gets its measure invoked. Instead we can use a separate clip modifier to allow for delegation to work in this case.

Fixes: b/392060494
Bug: b/392666252
Test: ScrollingContainerTest
Change-Id: Ic70a39893f473df6688b7287141d7fdb48420c9e

compose/foundation/foundation/src/androidInstrumentedTest/kotlin/androidx/compose/foundation/ScrollTest.kt

@@ -669,8 +669,12 @@ class ScrollTest(private val config: Config) {
                     Horizontal -> Modifier.horizontalScroll(state)
                 }.toList()
 
-            val scrollableContainer = modifiers[0] as InspectableValue
-            val scroll = modifiers[1] as InspectableValue
+            val clip = modifiers[0] as InspectableValue
+            val scrollableContainer = modifiers[1] as InspectableValue
+            val scroll = modifiers[2] as InspectableValue
+
+            assertThat(clip.nameFallback).isEqualTo("graphicsLayer")
+
             assertThat(scrollableContainer.nameFallback).isEqualTo("scrollingContainer")
             assertThat(scrollableContainer.valueOverride).isNull()
             assertThat(scrollableContainer.inspectableElements.map { it.name }.asIterable())
@@ -703,8 +707,11 @@ class ScrollTest(private val config: Config) {
                     Horizontal -> Modifier.horizontalScroll(state, overscrollEffect = null)
                 }.toList()
 
-            val scrollableContainer = modifiers[0] as InspectableValue
-            val scroll = modifiers[1] as InspectableValue
+            val clip = modifiers[0] as InspectableValue
+            val scrollableContainer = modifiers[1] as InspectableValue
+            val scroll = modifiers[2] as InspectableValue
+
+            assertThat(clip.nameFallback).isEqualTo("graphicsLayer")
 
             assertThat(scrollableContainer.nameFallback).isEqualTo("scrollingContainer")
             assertThat(scrollableContainer.valueOverride).isNull()

compose/foundation/foundation/src/androidInstrumentedTest/kotlin/androidx/compose/foundation/ScrollingContainerTest.kt

@@ -28,26 +28,39 @@ import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.setValue
 import androidx.compose.testutils.assertShape
+import androidx.compose.testutils.toList
+import androidx.compose.ui.CombinedModifier
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.draw.drawBehind
 import androidx.compose.ui.geometry.Offset
 import androidx.compose.ui.geometry.Size
 import androidx.compose.ui.graphics.Color
 import androidx.compose.ui.graphics.RectangleShape
+import androidx.compose.ui.input.nestedscroll.NestedScrollSource
+import androidx.compose.ui.layout.Measurable
+import androidx.compose.ui.layout.MeasureResult
+import androidx.compose.ui.layout.MeasureScope
+import androidx.compose.ui.node.DelegatableNode
 import androidx.compose.ui.node.DelegatingNode
+import androidx.compose.ui.node.LayoutModifierNode
 import androidx.compose.ui.node.ModifierNodeElement
 import androidx.compose.ui.platform.InspectableValue
 import androidx.compose.ui.platform.LocalLayoutDirection
 import androidx.compose.ui.platform.isDebugInspectorInfoEnabled
 import androidx.compose.ui.platform.testTag
 import androidx.compose.ui.test.captureToImage
+import androidx.compose.ui.test.getUnclippedBoundsInRoot
 import androidx.compose.ui.test.junit4.createComposeRule
 import androidx.compose.ui.test.onNodeWithTag
 import androidx.compose.ui.test.performTouchInput
 import androidx.compose.ui.test.swipeLeft
+import androidx.compose.ui.unit.Constraints
+import androidx.compose.ui.unit.IntOffset
 import androidx.compose.ui.unit.LayoutDirection.Ltr
 import androidx.compose.ui.unit.LayoutDirection.Rtl
+import androidx.compose.ui.unit.Velocity
 import androidx.compose.ui.unit.dp
+import androidx.compose.ui.unit.size
 import androidx.test.filters.MediumTest
 import androidx.test.filters.SdkSuppress
 import com.google.common.truth.Truth.assertThat
@@ -73,21 +86,25 @@ class ScrollingContainerTest {
     @Test
     fun testInspectorValue() {
         rule.setContent {
-            val modifier =
-                Modifier.scrollingContainer(
-                    rememberScrollState(),
-                    orientation = Horizontal,
-                    enabled = true,
-                    reverseScrolling = false,
-                    flingBehavior = null,
-                    interactionSource = null,
-                    useLocalOverscrollFactory = false,
-                    overscrollEffect = null,
-                    bringIntoViewSpec = null
-                ) as InspectableValue
-            assertThat(modifier.nameFallback).isEqualTo("scrollingContainer")
-            assertThat(modifier.valueOverride).isNull()
-            assertThat(modifier.inspectableElements.map { it.name }.asIterable())
+            val modifiers =
+                (Modifier.scrollingContainer(
+                        rememberScrollState(),
+                        orientation = Horizontal,
+                        enabled = true,
+                        reverseScrolling = false,
+                        flingBehavior = null,
+                        interactionSource = null,
+                        useLocalOverscrollFactory = false,
+                        overscrollEffect = null,
+                        bringIntoViewSpec = null
+                    ) as CombinedModifier)
+                    .toList()
+            val clip = modifiers[0] as InspectableValue
+            assertThat(clip.nameFallback).isEqualTo("graphicsLayer")
+            val scrollingContainer = modifiers[1] as InspectableValue
+            assertThat(scrollingContainer.nameFallback).isEqualTo("scrollingContainer")
+            assertThat(scrollingContainer.valueOverride).isNull()
+            assertThat(scrollingContainer.inspectableElements.map { it.name }.asIterable())
                 .containsExactly(
                     "state",
                     "orientation",
@@ -641,6 +658,72 @@ class ScrollingContainerTest {
         }
     }
 
+    /**
+     * Test for b/392060494
+     *
+     * Currently LayoutModifierNodes cannot delegate measurement to other LayoutModifierNodes. So if
+     * scrollingContainer is a LayoutModifierNode, it prevents overscroll implementations from using
+     * LayoutModifierNode internally. This test ensures that an overscroll implementation using
+     * LayoutModifierNode works inside scrollingContainer.
+     */
+    @Test
+    fun doesNotIgnoreOverscrollEffectNodeLayout() {
+        val expectedOffset = IntOffset(20, 20)
+        val overscrollEffect =
+            object : OverscrollEffect {
+                override val isInProgress = false
+
+                override suspend fun applyToFling(
+                    velocity: Velocity,
+                    performFling: suspend (Velocity) -> Velocity
+                ) {}
+
+                override fun applyToScroll(
+                    delta: Offset,
+                    source: NestedScrollSource,
+                    performScroll: (Offset) -> Offset
+                ): Offset = performScroll(delta)
+
+                override val node: DelegatableNode =
+                    object : Modifier.Node(), LayoutModifierNode {
+                        override fun MeasureScope.measure(
+                            measurable: Measurable,
+                            constraints: Constraints
+                        ): MeasureResult {
+                            val placeable = measurable.measure(constraints)
+                            return layout(placeable.width, placeable.height) {
+                                placeable.placeRelative(expectedOffset)
+                            }
+                        }
+                    }
+            }
+
+        rule.setContent {
+            Box(
+                Modifier.scrollingContainer(
+                    rememberScrollState(),
+                    orientation = Horizontal,
+                    enabled = true,
+                    reverseScrolling = false,
+                    flingBehavior = null,
+                    interactionSource = null,
+                    useLocalOverscrollFactory = false,
+                    overscrollEffect = overscrollEffect,
+                    bringIntoViewSpec = null
+                )
+            ) {
+                Box(Modifier.size(20.dp).background(Color.Red).testTag("content"))
+            }
+        }
+
+        rule.runOnIdle { assertThat(overscrollEffect.node.node.isAttached).isTrue() }
+        val bounds = rule.onNodeWithTag("content").getUnclippedBoundsInRoot()
+        with(rule.density) {
+            assertThat(bounds.left.toPx()).isEqualTo(expectedOffset.x)
+            assertThat(bounds.top.toPx()).isEqualTo(expectedOffset.y)
+        }
+    }
+
     private fun Modifier.drawOutsideOfBounds() = drawBehind {
         val inflate = 20.dp.roundToPx().toFloat()
         drawRect(

compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/ClipScrollableContainer.kt

@@ -61,7 +61,7 @@ fun Modifier.clipScrollableContainer(orientation: Orientation) =
  */
 internal val MaxSupportedElevation = 30.dp
 
-internal object HorizontalScrollableClipShape : Shape {
+private object HorizontalScrollableClipShape : Shape {
     override fun createOutline(
         size: Size,
         layoutDirection: LayoutDirection,
@@ -79,7 +79,7 @@ internal object HorizontalScrollableClipShape : Shape {
     }
 }
 
-internal object VerticalScrollableClipShape : Shape {
+private object VerticalScrollableClipShape : Shape {
     override fun createOutline(
         size: Size,
         layoutDirection: LayoutDirection,

compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/ScrollingContainer.kt

@@ -24,22 +24,15 @@ import androidx.compose.foundation.gestures.ScrollableNode
 import androidx.compose.foundation.gestures.ScrollableState
 import androidx.compose.foundation.interaction.MutableInteractionSource
 import androidx.compose.ui.Modifier
-import androidx.compose.ui.graphics.GraphicsLayerScope
-import androidx.compose.ui.layout.Measurable
-import androidx.compose.ui.layout.MeasureResult
-import androidx.compose.ui.layout.MeasureScope
 import androidx.compose.ui.node.CompositionLocalConsumerModifierNode
 import androidx.compose.ui.node.DelegatableNode
 import androidx.compose.ui.node.DelegatingNode
-import androidx.compose.ui.node.LayoutModifierNode
 import androidx.compose.ui.node.ModifierNodeElement
 import androidx.compose.ui.node.ObserverModifierNode
 import androidx.compose.ui.node.currentValueOf
-import androidx.compose.ui.node.invalidatePlacement
 import androidx.compose.ui.node.observeReads
 import androidx.compose.ui.node.requireLayoutDirection
 import androidx.compose.ui.platform.InspectorInfo
-import androidx.compose.ui.unit.Constraints
 import androidx.compose.ui.unit.LayoutDirection
 
 // TODO b/316559454 to make it public
@@ -60,19 +53,20 @@ internal fun Modifier.scrollingContainer(
     overscrollEffect: OverscrollEffect?,
     bringIntoViewSpec: BringIntoViewSpec? = null
 ): Modifier {
-    return this.then(
-        ScrollingContainerElement(
-            state = state,
-            orientation = orientation,
-            enabled = enabled,
-            reverseScrolling = reverseScrolling,
-            flingBehavior = flingBehavior,
-            interactionSource = interactionSource,
-            bringIntoViewSpec = bringIntoViewSpec,
-            useLocalOverscrollFactory = useLocalOverscrollFactory,
-            overscrollEffect = overscrollEffect
+    return clipScrollableContainer(orientation)
+        .then(
+            ScrollingContainerElement(
+                state = state,
+                orientation = orientation,
+                enabled = enabled,
+                reverseScrolling = reverseScrolling,
+                flingBehavior = flingBehavior,
+                interactionSource = interactionSource,
+                bringIntoViewSpec = bringIntoViewSpec,
+                useLocalOverscrollFactory = useLocalOverscrollFactory,
+                overscrollEffect = overscrollEffect
+            )
         )
-    )
 }
 
 /**
@@ -175,11 +169,7 @@ private class ScrollingContainerNode(
     private var bringIntoViewSpec: BringIntoViewSpec?,
     private var useLocalOverscrollFactory: Boolean,
     private var userProvidedOverscrollEffect: OverscrollEffect?
-) :
-    DelegatingNode(),
-    LayoutModifierNode,
-    CompositionLocalConsumerModifierNode,
-    ObserverModifierNode {
+) : DelegatingNode(), CompositionLocalConsumerModifierNode, ObserverModifierNode {
     override val shouldAutoInvalidate = false
     private var scrollableNode: ScrollableNode? = null
     private var overscrollNode: DelegatableNode? = null
@@ -194,15 +184,6 @@ private class ScrollingContainerNode(
             userProvidedOverscrollEffect
         }
 
-    // Needs to be mutated to properly update the underlying layer, which relies on instance
-    // equality
-    private var layerBlock: GraphicsLayerScope.() -> Unit = {
-        clip = true
-        shape =
-            if (orientation == Orientation.Vertical) VerticalScrollableClipShape
-            else HorizontalScrollableClipShape
-    }
-
     override fun onAttach() {
         shouldReverseDirection = shouldReverseDirection()
         attachOverscrollNodeIfNeeded()
@@ -227,17 +208,6 @@ private class ScrollingContainerNode(
         overscrollNode?.let { undelegate(it) }
     }
 
-    override fun MeasureScope.measure(
-        measurable: Measurable,
-        constraints: Constraints
-    ): MeasureResult {
-        val placeable = measurable.measure(constraints)
-        // Note: this is functionally the same as Modifier.clip, but inlined to reduce nodes.
-        return layout(placeable.width, placeable.height) {
-            placeable.placeWithLayer(0, 0, layerBlock = layerBlock)
-        }
-    }
-
     override fun onLayoutDirectionChange() {
         val reverseDirection = shouldReverseDirection()
         if (shouldReverseDirection != reverseDirection) {
@@ -268,16 +238,7 @@ private class ScrollingContainerNode(
         bringIntoViewSpec: BringIntoViewSpec?
     ) {
         this.state = state
-        if (this.orientation != orientation) {
-            this.orientation = orientation
-            this.layerBlock = {
-                clip = true
-                shape =
-                    if (orientation == Orientation.Vertical) VerticalScrollableClipShape
-                    else HorizontalScrollableClipShape
-            }
-            invalidatePlacement()
-        }
+        this.orientation = orientation
         var useLocalOverscrollFactoryChanged = false
         if (this.useLocalOverscrollFactory != useLocalOverscrollFactory) {
             useLocalOverscrollFactoryChanged = true