Fix unexpected pointer change issue and Add test case #43949

LoveJello · 2023-07-24T08:16:03Z

Fix issue 129765 and Add test case, see issue
Fixes flutter/flutter#129765

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2023-07-24T08:16:07Z

This pull request was opened against a branch other than main. Since Flutter pull requests should not normally be opened against branches other than main, I have changed the base to main. If this was intended, you may modify the base back to master. See the Release Process for information about how other branches get updated.

Reviewers: Use caution before merging pull requests to branches other than main, unless this is an intentional hotfix/cherrypick.

google-cla · 2023-07-24T08:16:08Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

moffatman

My question -- what happens to the data in the ACTION_SCROLL here? Is it important we should turn that into an actual scroll update? Or it's OK just to throw it away and ignore this event? Looks like your change now will just ignore it, the new event (PAN_ZOOM_UPDATE) will (should?) have fields of zero-- being a no-op.

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

LoveJello · 2023-07-25T02:16:58Z

My question -- what happens to the data in the ACTION_SCROLL here? Is it important we should turn that into an actual scroll update? Or it's OK just to throw it away and ignore this event? Looks like your change now will just ignore it, the new event (PAN_ZOOM_UPDATE) will (should?) have fields of zero-- being a no-op.

Map scroll event for PointerChange.HOVER and finaly PointerChange.PAN_ZOOM_UPDATE. just as your advise before.

@@ -396,12 +411,12 @@ public class AndroidTouchProcessor {
private int getPointerChangeForPanZoom(int pointerChange) {
if (pointerChange == PointerChange.DOWN) {
return PointerChange.PAN_ZOOM_START;

} else if (pointerChange == PointerChange.MOVE) {

} else if (pointerChange == PointerChange.MOVE || pointerChange == PointerChange.HOVER) {
return PointerChange.PAN_ZOOM_UPDATE;
} else if (pointerChange == PointerChange.UP || pointerChange == PointerChange.CANCEL) {
return PointerChange.PAN_ZOOM_END;
}

LoveJello · 2023-07-28T03:07:52Z

@moffatman Any update or comment?

moffatman · 2023-07-28T03:12:08Z

shell/platform/android/test/io/flutter/embedding/android/AndroidTouchProcessorTest.java

+        .verify(mockRenderer)
+        .dispatchPointerDataPacket(packetCaptor.capture(), packetSizeCaptor.capture());
+    packet = packetCaptor.getValue();
+    assertEquals(0.0, readPointerPhysicalX(packet));


Can you test, what are the deviceKind, signalKind, and pointerChange here?

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

moffatman · 2023-07-28T03:23:40Z

Does the ACTION_SCROLL generic motion event contain any useful data? Such as some scroll delta which isn't in any other event? Maybe it's best just to skip this event altogether. Follow same approach as if (pointerChange == -1), check this pan-zoom mapping earlier and return without modifying the packet if we get this weird event.

I would like to see, if you can print out the exact events and field values from the real device? Thanks :)

Also, you will have to use the formatting script I think to get all the checks to pass.

LoveJello · 2023-07-28T07:17:08Z

Does the ACTION_SCROLL generic motion event contain any useful data? Such as some scroll delta which isn't in any other event? Maybe it's best just to skip this event altogether. Follow same approach as if (pointerChange == -1), check this pan-zoom mapping earlier and return without modifying the packet if we get this weird event.

I would like to see, if you can print out the exact events and field values from the real device? Thanks :)

Also, you will have to use the formatting script I think to get all the checks to pass.

For events in that wired device, please refer to flutter/flutter#129765. Copy them here:

06-29 19:19:42.562 4919 4919 V AndroidTouchProcessor: onGenericMotionEvent, event MotionEvent { action=ACTION_HOVER_ENTER, actionButton=0, id[0]=0, x[0]=1296.2535, y[0]=398.5768, toolType[0]=TOOL_TYPE_MOUSE, buttonState=
0, classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=628215, downTime=144832, deviceId=7, source=0x2002, displayId=0 }
06-29 19:19:42.567 4919 4919 V AndroidTouchProcessor: onGenericMotionEvent, event MotionEvent { action=ACTION_HOVER_MOVE, actionButton=0, id[0]=0, x[0]=1296.2535, y[0]=398.5768, toolType[0]=TOOL_TYPE_MOUSE, buttonState=0
, classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=628215, downTime=144832, deviceId=7, source=0x2002, displayId=0 }
06-29 19:19:42.575 4919 4919 V AndroidTouchProcessor: onGenericMotionEvent, event MotionEvent { action=ACTION_HOVER_EXIT, actionButton=0, id[0]=0, x[0]=1296.2535, y[0]=398.5768, toolType[0]=TOOL_TYPE_MOUSE, buttonState=0
, classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=628222, downTime=628222, deviceId=7, source=0x2002, displayId=0 }
06-29 19:19:42.578 4919 4919 V AndroidTouchProcessor: onTouchEvent, event MotionEvent { action=ACTION_DOWN, actionButton=0, id[0]=0, x[0]=1296.2535, y[0]=398.5768, toolType[0]=TOOL_TYPE_MOUSE, buttonState=0, classificati
on=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=628222, downTime=628222, deviceId=7, source=0x2002, displayId=0 }
06-29 19:19:42.673 4919 4919 V AndroidTouchProcessor: onTouchEvent, event MotionEvent { action=ACTION_MOVE, actionButton=0, id[0]=0, x[0]=1277.6, y[0]=448.00885, toolType[0]=TOOL_TYPE_MOUSE, buttonState=0, classification
=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=628328, downTime=628222, deviceId=7, source=0x2002, displayId=0 }
06-29 19:19:42.675 4919 4919 V AndroidTouchProcessor: onGenericMotionEvent, event MotionEvent { action=ACTION_SCROLL, actionButton=0, id[0]=0, x[0]=0.0, y[0]=0.0, toolType[0]=TOOL_TYPE_MOUSE, buttonState=0, classificatio
n=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=628328, downTime=628222, deviceId=7, source=0x2002, displayId=0 }

drop this ACTION_SCROLL is not a good choice I think .

LoveJello · 2023-07-28T07:26:01Z

Also, you will have to use the formatting script I think to get all the checks to pass.

Already done. That Linux linux_unopt case is not related to this patch, and I don't known how to fix it.

moffatman · 2023-07-28T14:04:33Z

Yes, it is related to your patch, see the mismatch in formatting here: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8774350301251168225/+/u/test:_test:format_and_dart_test__3_/stdout

moffatman · 2023-07-28T14:06:06Z

Looking at the log above, there is no useful info in the ACTION_SCROLL. All fields zero. What happens afterwards? Is there ACTION_UP on this "scroll pointer"?

LoveJello · 2023-07-31T03:06:10Z

Yes, it is related to your patch, see the mismatch in formatting here: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8774350301251168225/+/u/test:_test:format_and_dart_test__3_/stdout

Done.

LoveJello · 2023-07-31T03:15:24Z

Looking at the log above, there is no useful info in the ACTION_SCROLL. All fields zero. What happens afterwards? Is there ACTION_UP on this "scroll pointer"?

Sorry, I don't have that device any more.

Drop that event or send that event to flutter, it's a question...

moffatman · 2023-07-31T04:08:08Z

OK. I propose we can just drop the event. At the beginning of addPointerForIndex, we can just check isTrackpadPan. Then if getPointerChangeForPanZoom is -1, return without creating the packet.

LoveJello · 2023-08-01T02:44:50Z

OK. I propose we can just drop the event. At the beginning of addPointerForIndex, we can just check isTrackpadPan. Then if getPointerChangeForPanZoom is -1, return without creating the packet.

Done.

moffatman · 2023-08-01T04:29:47Z

Btw, I think you have to rebase to fix the test failure

LoveJello · 2023-08-01T08:29:47Z

Btw, I think you have to rebase to fix the test failure

Done.

moffatman · 2023-08-02T05:35:09Z

I just tried running locally, you don't need the changes for testing, you can add when(event.isFromSource(InputDevice.SOURCE_CLASS_POINTER)).thenReturn(true); into the list of mocks for MotionEventMocker::mockEvent

Signed-off-by: Xiaohu Hu <huxiaohu2007@gmail.com>

LoveJello · 2023-08-02T09:56:14Z

when(event.isFromSource(InputDevice.SOURCE_CLASS_POINTER)).thenReturn(true);

Yes,you are right. I have changed the code .

moffatman

Looks good, thanks for all the changes 👍

chinmaygarde · 2023-08-03T20:04:08Z

cc @reidbaker for second approval.

LoveJello · 2023-08-08T08:57:15Z

cc @reidbaker for second approval.

Any update ?

reidbaker · 2023-08-08T21:27:24Z

Sorry I wasn't actually added as a reviewer and I triage pings slower than review requests.

I don't see anything obviously wrong but I am going to defer to @Piinks. I am not as comfortable as I should be with the scroll logic.

Thank you for your contribution

LoveJello · 2023-08-10T12:57:07Z

Sorry I wasn't actually added as a reviewer and I triage pings slower than review requests.

I don't see anything obviously wrong but I am going to defer to @Piinks. I am not as comfortable as I should be with the scroll logic.

Thank you for your contribution

@Piinks Any update or comment ?

Piinks

Hey @LoveJello welcome, thanks for contributing! I see you found and reported the issue as well, many thanks . :)

LGTM!

…ter/engine#43949)

…132332) flutter/engine@a9be77e...16b01b9 2023-08-10 skia-flutter-autoroll@skia.org Roll Dart SDK from 46da53e7abe2 to a2eac00da6b8 (1 revision) (flutter/engine#44601) 2023-08-10 huxiaohu2007@gmail.com Fix unexpected pointer change issue and Add test case (flutter/engine#43949) 2023-08-10 47866232+chunhtai@users.noreply.github.com Reland "Android a11y bridge sets importantness" (flutter/engine#44589) 2023-08-10 john@johnmccutchan.com Support for Android Platform Views under Impeller/Vulkan (flutter/engine#44571) 2023-08-10 109111084+yaakovschectman@users.noreply.github.com Reintroduce Windows lifecycle with guard for posthumous `OnWindowStateEvent` (flutter/engine#44344) 2023-08-10 skia-flutter-autoroll@skia.org Roll Skia from 92e6f52b0fa8 to b6492f5ce8c3 (1 revision) (flutter/engine#44597) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC chinmaygarde@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Fix issue 129765 and Add test case, see issue Fixes flutter/flutter#129765 - [â��] 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. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

flutter-dashboard bot changed the base branch from master to main July 24, 2023 08:16

github-actions bot added the platform-android label Jul 24, 2023

LoveJello force-pushed the master branch from 3a62212 to 499b626 Compare July 24, 2023 09:18

moffatman reviewed Jul 24, 2023

View reviewed changes

LoveJello force-pushed the master branch 3 times, most recently from fcbf66f to f0f00ec Compare July 28, 2023 02:52

moffatman reviewed Jul 28, 2023

View reviewed changes

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

LoveJello force-pushed the master branch from f0f00ec to 5b92f4b Compare July 28, 2023 06:07

LoveJello force-pushed the master branch from 5b92f4b to 7ad20c5 Compare July 31, 2023 03:01

LoveJello force-pushed the master branch from 7ad20c5 to 027e8d0 Compare August 1, 2023 02:42

LoveJello force-pushed the master branch from 027e8d0 to f2b91b4 Compare August 1, 2023 02:47

LoveJello force-pushed the master branch from c5d4fc8 to 154fc5a Compare August 1, 2023 07:50

LoveJello force-pushed the master branch from 154fc5a to 8c2ba5d Compare August 2, 2023 03:38

Fix unexpected pointer change (#129765) and add test case

c653777

Signed-off-by: Xiaohu Hu <huxiaohu2007@gmail.com>

LoveJello force-pushed the master branch from 8c2ba5d to c653777 Compare August 2, 2023 09:56

moffatman approved these changes Aug 2, 2023

View reviewed changes

reidbaker requested review from Piinks and reidbaker August 8, 2023 21:26

Piinks approved these changes Aug 10, 2023

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 10, 2023

auto-submit bot merged commit 0795adf into flutter:main Aug 10, 2023

engine-flutter-autoroll mentioned this pull request Aug 10, 2023

Roll Flutter Engine from a9be77e6f475 to 16b01b98af20 (6 revisions) flutter/flutter#132332

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 10, 2023

0795adf91 Fix unexpected pointer change issue and Add test case (flut…

e3997c8

…ter/engine#43949)

reidbaker mentioned this pull request Aug 16, 2023

i82973 scroll mouse wheel support #44724

Merged

14 tasks

Fix unexpected pointer change issue and Add test case #43949

Fix unexpected pointer change issue and Add test case #43949

Uh oh!

Conversation

LoveJello commented Jul 24, 2023 • edited by Piinks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jul 24, 2023

Uh oh!

google-cla bot commented Jul 24, 2023

Uh oh!

moffatman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LoveJello commented Jul 25, 2023

Uh oh!

LoveJello commented Jul 28, 2023

Uh oh!

moffatman Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

LoveJello Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

moffatman commented Jul 28, 2023

Uh oh!

LoveJello commented Jul 28, 2023

Uh oh!

LoveJello commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moffatman commented Jul 28, 2023

Uh oh!

moffatman commented Jul 28, 2023

Uh oh!

LoveJello commented Jul 31, 2023

Uh oh!

LoveJello commented Jul 31, 2023

Uh oh!

moffatman commented Jul 31, 2023

Uh oh!

LoveJello commented Aug 1, 2023

Uh oh!

moffatman commented Aug 1, 2023

Uh oh!

LoveJello commented Aug 1, 2023

Uh oh!

moffatman commented Aug 2, 2023

Uh oh!

LoveJello commented Aug 2, 2023

Uh oh!

moffatman left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Aug 3, 2023

Uh oh!

LoveJello commented Aug 8, 2023

Uh oh!

reidbaker commented Aug 8, 2023

Uh oh!

LoveJello commented Aug 10, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LoveJello commented Jul 24, 2023 •

edited by Piinks

Loading

LoveJello commented Jul 28, 2023 •

edited

Loading