ChromeOS/Android trackpad gestures #31595

moffatman · 2022-02-21T21:07:53Z

Start sending PointerPanZoom events from ChromeOS/Android when trackpad is used

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

flutter-dashboard · 2022-02-21T21:07:56Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

blasten

Thanks for the contribution! It will really help to get some unit tests and comments for each case that this PR is handling.

chinmaygarde · 2022-03-17T20:27:12Z

@moffatman Any updates on the tests or are you seeking specific guidance from @blasten ?

moffatman · 2022-03-17T20:39:04Z

@chinmaygarde Framework side needs to be merged first so I haven't spent any time on adding tests here yet (also this platform is the least priority, trackpad gestures already functionally work). Should get around to it within the next week.

justinmc · 2022-03-30T17:25:26Z

If the framework PR is merged without this will it just gracefully not receive pan/zoom events?

moffatman · 2022-03-30T18:12:39Z

@justinmc Framework PR needs to be merged first as merging only engine changes will break scrolling. If only the framework PR is merged there should be no changes to existing behaviour. In fact the Web engine will never get a PR, as it's not possible to get trackpad gestures to work there. So there is no rush or synchronization needed in merging the engine PRs after the framework lands. This specific one I have been slow on amending since I can't build for android on my ARM mac.

justinmc · 2022-03-30T18:24:02Z

Sounds good, thanks for the clarification.

moffatman · 2022-03-31T22:47:14Z

@blasten
I want to try making the following change to robolectric to fix the failure, can't seem to get the tests to use a locally-built version though.

diff --git a/shadows/framework/src/main/java/org/robolectric/shadows/ShadowMotionEvent.java b/shadows/framework/src/main/java/org/robolectric/shadows/ShadowMotionEvent.java
index 1ee385479..682938376 100644
--- a/shadows/framework/src/main/java/org/robolectric/shadows/ShadowMotionEvent.java
+++ b/shadows/framework/src/main/java/org/robolectric/shadows/ShadowMotionEvent.java
@@ -52,7 +52,7 @@ import org.robolectric.util.ReflectionHelpers;
  */
 @SuppressWarnings({"UnusedDeclaration"})
 @Implements(MotionEvent.class)
-public class ShadowMotionEvent {
+public class ShadowMotionEvent extends ShadowInputEvent {
 
   private static NativeObjRegistry<NativeInput.MotionEvent> nativeMotionEventRegistry =
       new NativeObjRegistry<>(NativeInput.MotionEvent.class);

blasten · 2022-04-05T20:47:44Z

@moffatman that's weird. I think the workaround requires to supply a shadow implementation that extends InputEvent.

For more, see http://robolectric.org/extending/

moffatman · 2022-04-06T02:15:21Z

@blasten Making a new shadow implementation will be a lot of code since we cannot extend the existing one as we wish to change its parent class. I updated the test to just use Mockito mock instead of shadowed object, and it's running fine now.

chinmaygarde · 2022-04-14T20:18:51Z

Ping @blasten. He may be busy with some firefighting ATM though.

dkwingsmt · 2022-04-14T23:00:46Z

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java

    packet.putLong(timeStamp); // time_stamp
-    packet.putLong(pointerChange); // change
-    packet.putLong(pointerKind); // kind
+    if (ongoingPans.containsKey(event.getPointerId(pointerIndex))) {


Extract this recurring condition as a boolean.

Or do you think it makes sense to extract the trackpad related data to another function? Because it seems that there are contains exclusive logic on both side, such as adding to and removing from ongoingPans, and many if clauses.

If we define a "data packet" class (as commented by a TODO on L207 of this file), then we don't have to worry about assignment order or packet structure. We can assign the common fields first, then split exclusive parts into two functions.

Just extracted the condition for now. It would be nice to use a data packet if it could be used in the test class as well. I don't know that much about how the java code interoperates with the rest of the engine, but I assume we can't change the signature on FlutterRenderer::dispatchPointerDataPacket ?

Nope, but the "data packet" class is actually just a builder. Check out https://github.com/flutter/engine/blob/main/lib/ui/window/pointer_data_packet.h . Although I don't really think the one I linked helps much in this scenario... What might really makes it prettier is something like a struct where you assign each field and then get data() out of it. In this way we can ensure that the data packet structure is correct no matter how you assign the properties. But just my two cents. See if you like the idea.

chinmaygarde · 2022-04-21T20:19:25Z

@dkwingsmt I think this is good for another review. Thanks.

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java

dkwingsmt

LGTM except for a comment

dkwingsmt · 2022-05-11T10:36:06Z

@blasten Would you like to make a second review?

dkwingsmt · 2022-05-12T00:44:30Z

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java

+    if (pointerKind == PointerDeviceKind.MOUSE) {
+      buttons = event.getButtonState() & 0x1F;
+      if (buttons == 0
+          && event.getSource() == InputDevice.SOURCE_MOUSE
+          && pointerChange == PointerChange.DOWN) {
+        // Some implementations translate trackpad scrolling into a mouse down-move-up event
+        // sequence with buttons: 0, such as ARC on a Chromebook. See #11420, a legacy
+        // implementation that uses the same condition but converts differently.
+        ongoingPans.put(event.getPointerId(pointerIndex), viewToScreenCoords);
+      }


I just tested on a real device and found out that ChromeOS now converts trackpad scrolling into events with tool type "finger" instead of "mouse". I've therefore fixed the logic as below. This also makes these events especially distinctive.

Suggested change

if (pointerKind == PointerDeviceKind.MOUSE) {

buttons = event.getButtonState() & 0x1F;

if (buttons == 0

&& event.getSource() == InputDevice.SOURCE_MOUSE

&& pointerChange == PointerChange.DOWN) {

// Some implementations translate trackpad scrolling into a mouse down-move-up event

// sequence with buttons: 0, such as ARC on a Chromebook. See #11420, a legacy

// implementation that uses the same condition but converts differently.

ongoingPans.put(event.getPointerId(pointerIndex), viewToScreenCoords);

}

if (pointerKind == PointerDeviceKind.MOUSE) {

buttons = event.getButtonState() & 0x1F;

} else if (pointerKind == PointerDeviceKind.TOUCH) {

buttons = 0;

// Chromebook with ARC translates trackpad scrolling into a touch down-move-up event

// sequence, which can be identified by a distinctive combination of input device "mouse",

// tool type "finger", and buttons 0.

if (event.getButtonState() == 0

&& event.getSource() == InputDevice.SOURCE_MOUSE

&& pointerChange == PointerChange.DOWN) {

ongoingPans.put(event.getPointerId(pointerIndex), viewToScreenCoords);

}

As a result, running the demo at https://docs.flutter.dev/cookbook/effects/drag-a-widget#interactive-example, moving the cursor onto the menu and scrolling the trackpad now correctly scrolls the list instead of dragging the items. :D

Any concern that we have no idea which proportion of devices this will work on? I guess it's based on undocumented behaviour anyway, and Chrome OS is always up to date.

We're required to make Flutter work on ChromeOS, so the portion of device shouldn't matter. Sure the behavior might change, and it's best we run an integration test, but I don't think we can achieve this in a short while. Maybe leave a TODO?

Also maybe it's better to use switch instead.

Never mind, I'll handle this change in another PR. The current code passes all tests anyway.

dkwingsmt · 2022-05-19T20:44:50Z

@blasten Would you like to make a second review of this PR?

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java

blasten · 2022-05-21T02:28:34Z

This change LGTM once the two minor comments are resolved

zanderso · 2022-06-02T20:07:19Z

@moffatman From PR review triage: it looks like the next step for this PR is to respond to the comments from @blasten

chinmaygarde · 2022-06-09T20:13:49Z

@blasten is OOO for the rest of this week.

blasten

LGTM

This reverts commit 984ef27.

…34046)

flutter-dashboard bot added the platform-android label Feb 21, 2022

flutter-dashboard bot added the needs tests label Feb 21, 2022

blasten suggested changes Mar 7, 2022

View reviewed changes

moffatman force-pushed the trackpad_gestures_android branch from f13d0e9 to c17c6b4 Compare March 31, 2022 22:42

moffatman force-pushed the trackpad_gestures_android branch from 1631a78 to ef8d9db Compare April 6, 2022 00:54

moffatman force-pushed the trackpad_gestures_android branch 2 times, most recently from 35d6f96 to cae15af Compare April 6, 2022 13:05

moffatman requested a review from blasten April 6, 2022 14:27

dkwingsmt suggested changes Apr 14, 2022

View reviewed changes

dkwingsmt reviewed May 11, 2022

View reviewed changes

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java Show resolved Hide resolved

dkwingsmt approved these changes May 11, 2022

View reviewed changes

moffatman force-pushed the trackpad_gestures_android branch from e0ee901 to f290c83 Compare May 11, 2022 18:57

dkwingsmt reviewed May 12, 2022

View reviewed changes

blasten reviewed May 21, 2022

View reviewed changes

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java Show resolved Hide resolved

blasten reviewed May 21, 2022

View reviewed changes

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java Outdated Show resolved Hide resolved

dkwingsmt approved these changes Jun 2, 2022

View reviewed changes

dkwingsmt requested a review from blasten June 2, 2022 22:32

moffatman force-pushed the trackpad_gestures_android branch from d0d72b3 to cc49d6d Compare June 2, 2022 22:33

moffatman added 6 commits June 2, 2022 19:11

Android trackpad gestures

6a21ca9

Add test

5120130

Use mock instead of shadow

b41a92b

Address feedback

eff543c

Add comment

c7ee3db

Feedback 2

8df97aa

moffatman force-pushed the trackpad_gestures_android branch from cc49d6d to 8df97aa Compare June 2, 2022 23:11

blasten approved these changes Jun 13, 2022

View reviewed changes

blasten added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 13, 2022

fluttergithubbot merged commit 984ef27 into flutter:main Jun 14, 2022

dkwingsmt added a commit that referenced this pull request Jun 14, 2022

Revert "ChromeOS/Android trackpad gestures (#31595)"

2597c44

This reverts commit 984ef27.

dkwingsmt mentioned this pull request Jun 14, 2022

Revert "ChromeOS/Android trackpad gestures" #34046

Merged

dkwingsmt added a commit that referenced this pull request Jun 14, 2022

Revert "ChromeOS/Android trackpad gestures (#31595)" (#34046)

bacea15

engine-flutter-autoroll mentioned this pull request Jun 14, 2022

Roll Flutter Engine from edc2aa4e894b to 5ad8879327be (23 revisions) flutter/flutter#105978

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 14, 2022

984ef27 ChromeOS/Android trackpad gestures (flutter/engine#31595)

a6dc92c

moffatman mentioned this pull request Jun 15, 2022

Reland "ChromeOS/Android trackpad gestures" #34060

Merged

8 tasks

houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022

ChromeOS/Android trackpad gestures (flutter#31595)

4c2206f

houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022

Revert "ChromeOS/Android trackpad gestures (flutter#31595)" (flutter#…

76b0758

…34046)

ChromeOS/Android trackpad gestures #31595

ChromeOS/Android trackpad gestures #31595

Uh oh!

Conversation

moffatman commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 21, 2022

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Mar 17, 2022

Uh oh!

moffatman commented Mar 17, 2022

Uh oh!

justinmc commented Mar 30, 2022

Uh oh!

moffatman commented Mar 30, 2022

Uh oh!

justinmc commented Mar 30, 2022

Uh oh!

moffatman commented Mar 31, 2022

Uh oh!

blasten commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moffatman commented Apr 6, 2022

Uh oh!

chinmaygarde commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkwingsmt Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

moffatman Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Apr 21, 2022

Uh oh!

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented May 11, 2022

Uh oh!

dkwingsmt May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moffatman May 12, 2022

Choose a reason for hiding this comment

Uh oh!

dkwingsmt May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt May 12, 2022

Choose a reason for hiding this comment

Uh oh!

dkwingsmt May 13, 2022

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented May 19, 2022

Uh oh!

Uh oh!

Uh oh!

blasten commented May 21, 2022

Uh oh!

zanderso commented Jun 2, 2022

Uh oh!

chinmaygarde commented Jun 9, 2022

moffatman commented Feb 21, 2022 •

edited

Loading

blasten commented Apr 5, 2022 •

edited

Loading

chinmaygarde commented Apr 14, 2022 •

edited

Loading

dkwingsmt Apr 20, 2022 •

edited

Loading

dkwingsmt May 12, 2022 •

edited

Loading

dkwingsmt May 12, 2022 •

edited

Loading