Fix non-vd android platform view input event offsets #52532

gmackall · 2024-05-02T22:42:03Z

Fixes flutter/flutter#146570, which tracks a regression from #49268 regarding platform view inputs in some specific cases.

This PR translates the input event location to be the same as the location we calculated before #49268, returning to the previous behavior, while maintaining the input event's verified status (I checked this manually with the InputManager).

Tested manually with the reproduction in the linked issue.

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 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.

…ains verification

gmackall · 2024-05-02T22:55:01Z

Not sure if I need to consider a different element than pointerCoords[0]?

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java

johnmccutchan · 2024-05-03T04:15:23Z

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java

+
+ originalEvent.offsetLocation(xOffset, yOffset);
+ }
+


Could we add a roboelectric test programmatically invokes translateNonVirtualDisplayMotionEvent and toMotionEvent and checks their return values?

I wrote a test that invokes toMotionEvent in both the vd and non-vd case, and expects that they have equivalent x and y coords. This isn't exactly what you asked for, but does it make sense to you?

…oords

gmackall · 2024-05-07T16:31:03Z

I also deleted a test that had been skipped for 4 years, which seems to have sort of confused the diff (the test was the itUsesActionEventTypeFromMotionEventForHybridPlatformViews). I tried unskipping it and it did not pass.

johnmccutchan

Great fix. Just a bunch of nits and some questions.

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java

johnmccutchan · 2024-05-08T14:31:15Z

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java

 @VisibleForTesting
 public MotionEvent toMotionEvent(
 float density, PlatformViewsChannel.PlatformViewTouch touch, boolean usingVirtualDiplay) {
 MotionEventTracker.MotionEventId motionEventId =
 MotionEventTracker.MotionEventId.from(touch.motionEventId);
 MotionEvent trackedEvent = motionEventTracker.pop(motionEventId);

+ // Pointer coordinates in the tracked events are global to FlutterView


I have a deeper question about this change:

If the original MotionEvent had view-local coordinates why can't we just deliver the original MotionEvent here? Let me explain below:

When someone touches a Platform View the following happens:

MotionEvent (O) is delivered directly to the PlatformView. We stash it on the side in our "motionEventTracker".

Transform MotionEvent into a Flutter TouchEvent

TouchEvent is delivered to Flutter Widgets and eventually to the PlatformView widget.

We end up back in this code to (finally) deliver the event to the Android View.

My intuition would be that in step (1) the events in O have the correct coordinates. And then in (4) we deliver O to the view.

In what cases do we see a non-zero offset? Help me understand intuitively what's happening here.

There is something weird going on here for sure. The reporting issue has a repro, which includes an example of a temporary fix that forces the platform view to refresh. In investigating why this temporarily fixes the issue, I printed out the original motion events and our calculated locations of platform view touches (and some more locations from framework code).

It turns out refreshing the webview changes the location of the original motion event that we store. Still need to investigate further as to why this would be the case, but it wasn't what I was originally expecting at all.

It looks like probably the reason why we can't deliver the motion events as is in (4) is that the Platform view in question believes it is at (0,0) (thats the output of platformView.getView.getX() and getY()). Still not sure why that is though!

Ok, I think I actually know the answer here now. MotionEvents in general are relative to the view they are delivered to. So this particular motion event is global to the FlutterView, but still relative to a view.

When we deliver the motion event in (4), we are then delivering it to an inner view, and as such need to make it relative to the inner view, which is why we need to offset. Does that make sense?

(Talked with @johnmccutchan offline and we still aren't sure this reasoning is 100% correct, but landing regardless as the fix works better than what we have)

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java

shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java

…event

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java

…er/engine#52532)

…148367) flutter/engine@55c62ff...d35a1a6 2024-05-15 [email protected] Roll FreeType from af4c2d86 to bfc3453f (flutter/engine#52829) 2024-05-14 [email protected] [Impeller] immediately encode blit commands for Vulkan. (flutter/engine#52794) 2024-05-14 [email protected] Fix non-vd android platform view input event offsets (flutter/engine#52532) 2024-05-14 [email protected] Roll Dart SDK from 0380354e2448 to 0b77fbab8cf5 (1 revision) (flutter/engine#52827) 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 [email protected],[email protected],[email protected] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Fixes flutter/flutter#146570, which tracks a regression from flutter#49268 regarding platform view inputs in some specific cases. This PR translates the input event location to be the same as the location we calculated before flutter#49268, returning to the previous behavior, while maintaining the input event's verified status (I checked this manually with the `InputManager`). Tested manually with the reproduction in the linked issue. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Gray Mackall added 2 commits May 2, 2024 15:39

apply the needed offset to the (non vd) pv inputs in a way that maint…

e51a149

…ains verification

format

3358956

github-actions bot added the platform-android label May 2, 2024

gmackall requested a review from johnmccutchan May 2, 2024 22:42

gmackall changed the title ~~Fix platform view input offsets~~ Fix non-vd android platform view input event offsets May 2, 2024

johnmccutchan reviewed May 3, 2024

View reviewed changes

gmackall and others added 9 commits May 6, 2024 12:13

Merge branch 'main' into fix_pv_inputs

dd40623

Merge branch 'main' into fix_pv_inputs

e09eeed

add debug warning

d1208fa

format

e560f49

docs

d263563

fix npe

00801ed

write a test that confirms that vd and non vd have the same x and y c…

b91c1cd

…oords

delete test that has been skipped for ~4 yrs

dab6725

format

ad39109

gmackall marked this pull request as ready for review May 7, 2024 16:27

gmackall requested a review from johnmccutchan May 7, 2024 16:32

johnmccutchan reviewed May 8, 2024

View reviewed changes

reidbaker self-requested a review May 8, 2024 17:45

Gray Mackall and others added 5 commits May 9, 2024 10:00

partial review

616e8bf

yield to the formatting gods

356adf2

Merge branch 'main' into fix_pv_inputs

5e936b6

make the pointer coords and properties list from the original motion …

f118c61

…event

list<list<object>> -> list<list<double>>

e190144

reidbaker reviewed May 9, 2024

View reviewed changes

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java Outdated Show resolved Hide resolved

reidbaker approved these changes May 9, 2024

View reviewed changes

gmackall marked this pull request as draft May 9, 2024 21:08

remove throwing, per comment

90c65d9

gmackall marked this pull request as ready for review May 13, 2024 19:33

Merge branch 'main' into fix_pv_inputs

3f6970b

gmackall mentioned this pull request May 14, 2024

Webview sometimes displays blank on launch, until scroll flutter/flutter#148344

Open

johnmccutchan approved these changes May 14, 2024

View reviewed changes

Gray Mackall and others added 2 commits May 14, 2024 15:24

remove unused input manager and buildConfig import

4fec506

Merge branch 'main' into fix_pv_inputs

26ab85b

gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label May 14, 2024

auto-submit bot merged commit 70de32b into flutter:main May 14, 2024
29 checks passed

engine-flutter-autoroll mentioned this pull request May 15, 2024

Roll Flutter Engine from 55c62ff82c7e to 70de32b3e13f (2 revisions) flutter/flutter#148366

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2024

70de32b3e Fix non-vd android platform view input event offsets (flutt…

f02b318

…er/engine#52532)

engine-flutter-autoroll mentioned this pull request May 15, 2024

Roll Flutter Engine from 55c62ff82c7e to d35a1a603c80 (4 revisions) flutter/flutter#148367

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2024

70de32b3e Fix non-vd android platform view input event offsets (flutt…

85692e9

…er/engine#52532)

gmackall added the cp: stable cherry pick to the stable release candidate branch label May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix non-vd android platform view input event offsets #52532

Fix non-vd android platform view input event offsets #52532

gmackall commented May 2, 2024 •

edited

gmackall commented May 2, 2024

johnmccutchan May 3, 2024

gmackall May 7, 2024

gmackall commented May 7, 2024 •

edited

johnmccutchan left a comment

johnmccutchan May 8, 2024

gmackall May 13, 2024 •

edited

gmackall May 14, 2024 •

edited

gmackall May 14, 2024

gmackall May 14, 2024

Fix non-vd android platform view input event offsets #52532

Fix non-vd android platform view input event offsets #52532

Conversation

gmackall commented May 2, 2024 • edited

Pre-launch Checklist

gmackall commented May 2, 2024

johnmccutchan May 3, 2024

Choose a reason for hiding this comment

gmackall May 7, 2024

Choose a reason for hiding this comment

gmackall commented May 7, 2024 • edited

johnmccutchan left a comment

Choose a reason for hiding this comment

johnmccutchan May 8, 2024

Choose a reason for hiding this comment

gmackall May 13, 2024 • edited

Choose a reason for hiding this comment

gmackall May 14, 2024 • edited

Choose a reason for hiding this comment

gmackall May 14, 2024

Choose a reason for hiding this comment

gmackall May 14, 2024

Choose a reason for hiding this comment

gmackall commented May 2, 2024 •

edited

gmackall commented May 7, 2024 •

edited

gmackall May 13, 2024 •

edited

gmackall May 14, 2024 •

edited