Invert performMouseInput { scroll(2f) } [224992993]

Fixed

Bug

Status Update

No update yet.

Description

ig...@jetbrains.com

created issue #1

Mar 16, 2022 07:38PM

Jetpack Compose version: 3621f0a7 Jetpack Compose component used: performMouseInput, scroll

In https://android-review.googlesource.com/c/platform/frameworks/support/+/1913489 we inverted vertical mouse scroll, because on Android when we scroll down (the top touch point of the wheel moves to the user), scrollX is positive, but it should be negative.

In tests when we make artificial scroll event, we should invert x axis too (to avoid confusion).

See for example the test: https://android-review.googlesource.com/c/platform/frameworks/support/+/1994356/3/compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/input/pointer/OnPointerEventTest.kt#155

Now it is:

rule.onNodeWithTag(tag).performMouseInput {
    scroll(2f)
}
assertThat(events.last().changes.first().scrollDelta).isEqualTo(Offset(0f, -2f))

But it should be:

rule.onNodeWithTag(tag).performMouseInput {
    scroll(2f)
}
assertThat(events.last().changes.first().scrollDelta).isEqualTo(Offset(0f, 2f))

Comments

ig...@jetbrains.com <ig...@jetbrains.com> #2Mar 16, 2022 07:40PM

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

je...@google.com <je...@google.com> #3Apr 22, 2022 10:36AM

scroll(2f) should generate a MotionEvent who's y scroll axis is 2f, not -2f. Whether that should be called "upward" or "downward" depends on the sign of the y scroll axis for MotionEvents generated by an actual mouse.

If this means that performMouseInput { scroll(2f) } scrolls in opposite directions on different platforms, that would be very unfortunate indeed.

Is the sign of raw scroll events of the same mouse the same on Android and on Desktop, or do the platforms produce different signs for the same scroll rotation of the same mouse?

ig...@jetbrains.com <ig...@jetbrains.com> #4Apr 22, 2022 02:06PM

Is the sign of raw scroll events of the same mouse the same on Android and on Desktop, or do the platforms produce different signs for the same scroll rotation of the same mouse

Platforms produce different signs. Desktop and new Api for web produce the same sign, Android and old api for web - opposite sign.

If we scroll mouse wheel downward on desktop, we will have positive delta, and scroll down the scrollable list. If we scroll downward on Android, we will have negative delta in raw native android event, but positive delta in Compose PointerEvent (we inverted in https://android-review.googlesource.com/c/platform/frameworks/support/+/1913489), and so the list will also be scrolled down.

scroll(2f) should generate a MotionEvent who's y scroll axis is 2f, not -2f

I think, that delta in a raw native event shouldn't matter, because our test API abstract away from platform details. What is matter, is that sign in scroll(2f) and sign in PointerEvent.scrollDelta should be the same.

Message last modified on Apr 22, 2022 02:07PM

ig...@jetbrains.com <ig...@jetbrains.com> #5Apr 22, 2022 02:14PM

Platforms produce different signs

Because of that, we have to invert signs on some platform, so behaviour is the same on all platforms:

call test scroll -> [start of platform code] -> invert delta -> nativeScroll -> invert delta -> [end of platform code] -> scroll the list

We chose to invert the delta on Android, because two other platforms have opposite sign (desktop/web), and because the usual coordinate system starts from top to bottom.

Message last modified on Apr 22, 2022 02:24PM

je...@google.com <je...@google.com> #6Apr 22, 2022 04:55PM

On second thought, I think you're right. It does make more sense if scroll(2f) yields a scrollDelta of 2f. Will update my review accordingly

ap...@google.com <ap...@google.com> #7Apr 28, 2022 07:12PM

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

commit f7459926705b1c79810fa321844f060d2d4276f1
Author: Igor Demin <igor.demin@jetbrains.com>
Date: Thu Apr 21 17:35:32 2022

Invert delta in MouseInjectionScope.scroll for vertical scroll

Similar to

https://android-review.googlesource.com/c/platform/frameworks/support/+/1913489

Bug: 224992993
RelNote: MouseInjectionScope.scroll(delta = someDelta) is now inverted on Android if we scroll vertically (if someDelta is positive, it will scroll downward)
Test: ./gradlew buildOnServer
Change-Id: Ifb697a9ae8bc05a2d54d0a6cb4018713e156baf8

M compose/ui/ui-test/src/commonMain/kotlin/androidx/compose/ui/test/InputDispatcher.kt
M compose/ui/ui-test/src/test/kotlin/androidx/compose/ui/test/inputdispatcher/MouseEventsTest.kt
M compose/ui/ui-test/src/androidMain/kotlin/androidx/compose/ui/test/AndroidInputDispatcher.android.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ScrollableTest.kt
M compose/ui/ui-test/src/commonMain/kotlin/androidx/compose/ui/test/MouseInjectionScope.kt

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

je...@google.com <je...@google.com> Apr 4, 2024 06:18PM

Marked as fixed.