Fix losing Exit event on scroll

Fixes JetBrains/compose-multiplatform#1324 (comment) Compose doesn't work well if we send an event with different coordinates without sending Move event before it: ``` Column { Box(size=10) Box(size=10) } ``` If we send these events: Move(5,5) -> Scroll(5,15) -> Move(5,15) Then during the scroll event, HitPathTracker forgets about the first Box, and never send Exit to it. Instead it sends the Scroll event to it. We should send events in this order: Move(5,5) -> Move(5,15) -> Scroll(5,15) -> Move(5,15) With synthetic events things more complicated, as we always send events with the same position. I suppose the proper fix should be in Compose core itself, but it would a very huge fix. Reproducer: ``` import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.size import androidx.compose.ui.ExperimentalComposeUiApi import androidx.compose.ui.ImageComposeScene import androidx.compose.ui.Modifier import androidx.compose.ui.geometry.Offset import androidx.compose.ui.input.pointer.PointerEventType import androidx.compose.ui.input.pointer.onPointerEvent import androidx.compose.ui.unit.Density import androidx.compose.ui.unit.dp import androidx.compose.ui.use import com.google.common.truth.Truth.assertThat import org.junit.Test import org.junit.runner.RunWith import org.junit.runners.JUnit4 @OptIn(ExperimentalComposeUiApi::class) @RunWith(JUnit4::class) class MouseHoverFilterTest { // bug: JetBrains/compose-multiplatform#1324 (comment) @test fun `move from one component to another, between another non-move event`() = ImageComposeScene( width = 100, height = 100, density = Density(1f) ).use { scene -> var enterCount1 = 0 var exitCount1 = 0 var enterCount2 = 0 var exitCount2 = 0 scene.setContent { Column { Box( modifier = Modifier .pointerMove( onEnter = { enterCount1++ }, onExit = { exitCount1++ } ) .size(10.dp, 10.dp) ) Box( modifier = Modifier .pointerMove( onEnter = { enterCount2++ }, onExit = { exitCount2++ } ) .size(10.dp, 10.dp) ) } } scene.sendPointerEvent(PointerEventType.Enter, Offset(5f, 5f)) assertThat(enterCount1).isEqualTo(1) assertThat(exitCount1).isEqualTo(0) assertThat(enterCount2).isEqualTo(0) assertThat(exitCount2).isEqualTo(0) // Compose doesn't work well if we send an event with different type and different coordinates, without sending move event before it scene.sendPointerEvent(PointerEventType.Scroll, Offset(5f, 15f)) scene.sendPointerEvent(PointerEventType.Move, Offset(5f, 15f)) assertThat(enterCount1).isEqualTo(1) assertThat(exitCount1).isEqualTo(1) assertThat(enterCount2).isEqualTo(1) assertThat(exitCount2).isEqualTo(0) } } ``` Another issue: JetBrains/compose-multiplatform#1480
JetBrains · Nov 27, 2021 · 9b5449c · 9b5449c
1 parent 8bc17fc
commit 9b5449c
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 31 deletions.
diff --git a/compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/ImageComposeScene.desktop.kt b/compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/ImageComposeScene.desktop.kt
@@ -181,6 +181,12 @@ class ImageComposeScene(
     /**
      * Send pointer event to the content.
      *
+     * Don't send non-Move event with a different position without sending Move event first.
+     * Otherwise hover can lose Exit/Enter events.
+     *
+     * Do: Move(5,5) -> Move(15,5) -> Scroll(15,5) -> Press(15,5) -> Move(20,5) -> Release(20,5)
+     * Don't: Move(5,5) -> Scroll(15,5) -> Press(15,5) -> Release(20,5)
+     *
      * @param eventType Indicates the primary reason that the event was sent.
      * @param position The [Offset] of the current pointer event, relative to the content.
      * @param timeMillis The time of the current pointer event, in milliseconds. The start (`0`) time
@@ -190,7 +196,7 @@ class ImageComposeScene(
      * @param buttons Contains the state of pointer buttons (e.g. mouse and stylus buttons).
      * @param keyboardModifiers Contains the state of modifier keys, such as Shift, Control, and Alt, as well as the state
      * of the lock keys, such as Caps Lock and Num Lock.
-     * @param mouseEvent The original native event.
+     * @param nativeEvent The original native event.
      */
     @OptIn(ExperimentalComposeUiApi::class)
     fun sendPointerEvent(

diff --git a/compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeLayer.desktop.kt b/compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeLayer.desktop.kt
@@ -205,21 +205,12 @@ internal class ComposeLayer {
         }
 
         override fun scheduleSyntheticMoveEvent() {
+            needSendSyntheticMoveSent = true
             SwingUtilities.invokeLater {
-                val lastMouseEvent = lastMouseEvent ?: return@invokeLater
-                val source = lastMouseEvent.source as Component
-                source.dispatchEvent(
-                    MouseEvent(
-                        source,
-                        MouseEvent.MOUSE_MOVED,
-                        System.nanoTime(),
-                        lastMouseEvent.modifiersEx,
-                        lastMouseEvent.x,
-                        lastMouseEvent.y,
-                        0,
-                        false
-                    )
-                )
+                if (isDisposed) return@invokeLater
+                catchExceptions {
+                    flushSyntheticMoveEvent()
+                }
             }
         }
     }
@@ -228,6 +219,7 @@ internal class ComposeLayer {
         _component.skikoView = object : SkikoView {
             override fun onRender(canvas: Canvas, width: Int, height: Int, nanoTime: Long) {
                 catchExceptions {
+                    flushSyntheticMoveEvent()
                     scene.render(canvas, nanoTime)
                 }
             }
@@ -274,12 +266,33 @@ internal class ComposeLayer {
     }
 
     private var lastMouseEvent: MouseEvent? = null
+    private var needSendSyntheticMoveSent = false
+
+    private fun flushSyntheticMoveEvent() {
+        val lastMouseEvent = lastMouseEvent ?: return
+        if (needSendSyntheticMoveSent) {
+            needSendSyntheticMoveSent = false
+            val source = lastMouseEvent.source as Component
+            val event = MouseEvent(
+                source,
+                MouseEvent.MOUSE_MOVED,
+                System.nanoTime(),
+                lastMouseEvent.modifiersEx,
+                lastMouseEvent.x,
+                lastMouseEvent.y,
+                0,
+                false
+            )
+            scene.onMouseEvent(density, event)
+        }
+    }
 
     private fun onMouseEvent(event: MouseEvent) {
         // AWT can send events after the window is disposed
         if (isDisposed) return
         lastMouseEvent = event
         catchExceptions {
+            flushSyntheticMoveEvent()
             scene.onMouseEvent(density, event)
         }
     }
@@ -288,6 +301,7 @@ internal class ComposeLayer {
         if (isDisposed) return
         lastMouseEvent = event
         catchExceptions {
+            flushSyntheticMoveEvent()
             scene.onMouseWheelEvent(density, event)
         }
     }

diff --git a/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/input/mouse/MouseHoverFilterTest.kt b/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/input/mouse/MouseHoverFilterTest.kt
@@ -17,24 +17,20 @@
 package androidx.compose.ui.input.mouse
 
 import androidx.compose.foundation.layout.Box
+import androidx.compose.foundation.layout.Column
 import androidx.compose.foundation.layout.size
 import androidx.compose.ui.ExperimentalComposeUiApi
 import androidx.compose.ui.ImageComposeScene
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.geometry.Offset
 import androidx.compose.ui.input.pointer.PointerEventType
 import androidx.compose.ui.input.pointer.onPointerEvent
-import androidx.compose.ui.input.pointer.pointerInput
 import androidx.compose.ui.unit.Density
 import androidx.compose.ui.unit.dp
 import androidx.compose.ui.use
 import com.google.common.truth.Truth.assertThat
-import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.ExperimentalCoroutinesApi
-import kotlinx.coroutines.delay
-import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.test.TestCoroutineDispatcher
-import kotlinx.coroutines.test.TestCoroutineScope
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.junit.runners.JUnit4
@@ -126,6 +122,51 @@ class MouseHoverFilterTest {
         assertThat(moveCount).isEqualTo(0)
     }
 
+    @Test
+    fun `move from one component to another`() = ImageComposeScene(
+        width = 100,
+        height = 100,
+        density = Density(1f)
+    ).use { scene ->
+        var enterCount1 = 0
+        var exitCount1 = 0
+        var enterCount2 = 0
+        var exitCount2 = 0
+
+        scene.setContent {
+            Column {
+                Box(
+                    modifier = Modifier
+                        .pointerMove(
+                            onEnter = { enterCount1++ },
+                            onExit = { exitCount1++ }
+                        )
+                        .size(10.dp, 10.dp)
+                )
+                Box(
+                    modifier = Modifier
+                        .pointerMove(
+                            onEnter = { enterCount2++ },
+                            onExit = { exitCount2++ }
+                        )
+                        .size(10.dp, 10.dp)
+                )
+            }
+        }
+
+        scene.sendPointerEvent(PointerEventType.Enter, Offset(5f, 5f))
+        assertThat(enterCount1).isEqualTo(1)
+        assertThat(exitCount1).isEqualTo(0)
+        assertThat(enterCount2).isEqualTo(0)
+        assertThat(exitCount2).isEqualTo(0)
+
+        scene.sendPointerEvent(PointerEventType.Move, Offset(5f, 15f))
+        assertThat(enterCount1).isEqualTo(1)
+        assertThat(exitCount1).isEqualTo(1)
+        assertThat(enterCount2).isEqualTo(1)
+        assertThat(exitCount2).isEqualTo(0)
+    }
+
     @OptIn(ExperimentalCoroutinesApi::class)
     @Test
     fun `send multiple move events with paused dispatcher`() {
@@ -184,9 +225,9 @@ class MouseHoverFilterTest {
 
 @OptIn(ExperimentalComposeUiApi::class)
 private fun Modifier.pointerMove(
-    onMove: () -> Unit,
-    onExit: () -> Unit,
-    onEnter: () -> Unit,
+    onMove: () -> Unit = {},
+    onExit: () -> Unit = {},
+    onEnter: () -> Unit = {},
 ): Modifier = this
     .onPointerEvent(PointerEventType.Move) { onMove() }
     .onPointerEvent(PointerEventType.Exit) { onExit() }

diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt
@@ -80,8 +80,8 @@ internal val LocalComposeScene = staticCompositionLocalOf<ComposeScene> {
 class ComposeScene internal constructor(
     coroutineContext: CoroutineContext = Dispatchers.Unconfined,
     internal val component: PlatformComponent,
-    density: Density,
-    private val invalidate: () -> Unit
+    density: Density = Density(1f),
+    private val invalidate: () -> Unit = {}
 ) {
     /**
      * Constructs [ComposeScene]
@@ -372,10 +372,15 @@ class ComposeScene internal constructor(
     ) = this != null && targetOwner != null && list.indexOf(this) > list.indexOf(targetOwner)
 
     // TODO(demin): return Boolean (when it is consumed).
-    //  see ComposeLayer todo about AWTDebounceEventQueue
     /**
      * Send pointer event to the content.
      *
+     * Don't send non-Move event with a different position without sending Move event first.
+     * Otherwise hover can lose Exit/Enter events.
+     *
+     * Do: Move(5,5) -> Move(15,5) -> Scroll(15,5) -> Press(15,5) -> Move(20,5) -> Release(20,5)
+     * Don't: Move(5,5) -> Scroll(15,5) -> Press(15,5) -> Release(20,5)
+     *
      * @param eventType Indicates the primary reason that the event was sent.
      * @param position The [Offset] of the current pointer event, relative to the content.
      * @param scrollDelta scroll delta for the PointerEventType.Scroll event

diff --git a/...ose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/input/mouse/MouseScrollFilter.skiko.kt b/...ose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/input/mouse/MouseScrollFilter.skiko.kt
@@ -114,7 +114,7 @@ fun Modifier.mouseScrollFilter(
     //  the more proper behaviour would be to batch multiple scroll events into the single one
     while (true) {
         val event = awaitScrollEvent()
-        val mouseEvent = (event.mouseEvent as? MouseWheelEvent) ?: continue
+        val mouseEvent = event.mouseEvent as? MouseWheelEvent
         val mouseChange = event.changes.find { it.type == PointerType.Mouse }
         if (mouseChange != null && !mouseChange.isConsumed) {
             val legacyEvent = mouseEvent.toLegacyEvent(mouseChange.scrollDelta)
@@ -133,17 +133,17 @@ private suspend fun PointerInputScope.awaitScrollEvent() = awaitPointerEventScop
     event
 }
 
-private fun MouseWheelEvent.toLegacyEvent(scrollDelta: Offset): MouseScrollEvent {
+private fun MouseWheelEvent?.toLegacyEvent(scrollDelta: Offset): MouseScrollEvent {
     val value = if (scrollDelta.x != 0f) scrollDelta.x else scrollDelta.y
+    val scrollType = this?.scrollType ?: MouseWheelEvent.WHEEL_UNIT_SCROLL
+    val scrollAmount = this?.scrollAmount ?: 1
     return MouseScrollEvent(
         delta = if (scrollType == MouseWheelEvent.WHEEL_BLOCK_SCROLL) {
             MouseScrollUnit.Page(value * scrollAmount)
         } else {
             MouseScrollUnit.Line(value * scrollAmount)
         },
-
-        // There are no other way to detect horizontal scrolling in AWT
-        orientation = if (isShiftDown || scrollDelta.x != 0f) {
+        orientation = if (scrollDelta.x != 0f) {
             MouseScrollOrientation.Horizontal
         } else {
             MouseScrollOrientation.Vertical