Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 20 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,12 @@ public static class PlatformViewTouch {
public final int action;
/** The number of pointers (e.g, fingers) involved in the touch event. */
public final int pointerCount;
/** Properties for each pointer, encoded in a raw format. */
/**
* Properties for each pointer, encoded in a raw format.
* Expected to be formatted as a List[List[Integer]], where each inner list has two items:
* - An id, at index 0, corresponding to {@link android.view.MotionEvent.PointerProperties#id}
* - A tool type, at index 1, corresponding to {@link android.view.MotionEvent.PointerProperties#toolType}.
* */
@NonNull public final Object rawPointerPropertiesList;
/** Coordinates for each pointer, encoded in a raw format. */
@NonNull public final Object rawPointerCoords;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

package io.flutter.plugin.platform;

import static android.view.MotionEvent.PointerCoords;
import static android.view.MotionEvent.PointerProperties;
import static io.flutter.Build.API_LEVELS;

import android.annotation.TargetApi;
Expand Down Expand Up @@ -668,32 +666,53 @@ public long configureForTextureLayerComposition(
return textureId;
}

/**
* Translates an original touch event to have the same locations as the ones that Flutter
* calculates (because original + flutter's - original = flutter's).
*
* @param originalEvent The saved original input event.
* @param pointerCoords The coordinates that Flutter thinks the touch is happening at.
*/
private static void translateMotionEvent(
MotionEvent originalEvent, PointerCoords[] pointerCoords) {
if (pointerCoords.length < 1) {
return;
}

float xOffset = pointerCoords[0].x - originalEvent.getX();
float yOffset = pointerCoords[0].y - originalEvent.getY();

originalEvent.offsetLocation(xOffset, yOffset);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. MotionEvent (O) is delivered directly to the PlatformView. We stash it on the side in our "motionEventTracker".
  2. Transform MotionEvent into a Flutter TouchEvent
  3. TouchEvent is delivered to Flutter Widgets and eventually to the PlatformView widget.
  4. 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.

Copy link
Member Author

@gmackall gmackall May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@gmackall gmackall May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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)

// The framework converts them to be local to a widget, given that
// motion events operate on local coords, we need to replace these in the tracked
// event with their local counterparts.
// Compute this early so it can be used as input to translateNonVirtualDisplayMotionEvent.
PointerCoords[] pointerCoords =
parsePointerCoordsList(touch.rawPointerCoords, density)
.toArray(new PointerCoords[touch.pointerCount]);

if (!usingVirtualDiplay && trackedEvent != null) {
// We have the original event, deliver it as it will pass the verifiable
// We have the original event, deliver it after offsetting as it will pass the verifiable
// input check.
translateMotionEvent(trackedEvent, pointerCoords);
return trackedEvent;
}
// We are in virtual display mode or don't have a reference to the original MotionEvent.
// In this case we manually recreate a MotionEvent to be delivered. This MotionEvent
// will fail the verifiable input check.

// Pointer coordinates in the tracked events are global to FlutterView
// framework converts them to be local to a widget, given that
// motion events operate on local coords, we need to replace these in the tracked
// event with their local counterparts.
PointerProperties[] pointerProperties =
parsePointerPropertiesList(touch.rawPointerPropertiesList)
.toArray(new PointerProperties[touch.pointerCount]);
PointerCoords[] pointerCoords =
parsePointerCoordsList(touch.rawPointerCoords, density)
.toArray(new PointerCoords[touch.pointerCount]);

// TODO (kaushikiska) : warn that we are potentially using an untracked
// event in the platform views.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -361,34 +361,48 @@ public void itUsesActionEventTypeFromFrameworkEventAsActionChanged() {
assertNotEquals(resolvedEvent.getAction(), frameWorkTouch.action);
}

@Ignore
@Test
public void itUsesActionEventTypeFromMotionEventForHybridPlatformViews() {
MotionEventTracker motionEventTracker = MotionEventTracker.getInstance();
PlatformViewsController platformViewsController = new PlatformViewsController();

MotionEvent original =
MotionEvent.obtain(
100, // downTime
100, // eventTime
1, // action
0, // x
0, // y
0 // metaState
);

// track an event that will later get passed to us from framework
private MotionEvent makePlatformViewTouchAndInvokeToMotionEvent(
PlatformViewsController platformViewsController,
MotionEventTracker motionEventTracker,
MotionEvent original,
boolean usingVirtualDisplays) {
MotionEventTracker.MotionEventId motionEventId = motionEventTracker.track(original);

PlatformViewTouch frameWorkTouch =
// Construct a PlatformViewTouch.rawPointerPropertiesList by doing the inverse of
// PlatformViewsController.parsePointerPropertiesList.
List<List<Integer>> pointerProperties =
Arrays.asList(
Arrays.asList(
original.getPointerId(0),
original.getToolType(0)
)
);
// Construct a PlatformViewTouch.rawPointerCoords by doing the inverse of
// PlatformViewsController.parsePointerCoordsList.
List<List<Double>> pointerCoordinates =
Arrays.asList(
Arrays.asList(
(double) original.getOrientation(),
(double) original.getPressure(),
(double) original.getSize(),
(double) original.getToolMajor(),
(double) original.getToolMinor(),
(double) original.getTouchMajor(),
(double) original.getTouchMinor(),
(double) original.getX(),
(double) original.getY()
)
);
// Make a platform view touch from the motion event.
PlatformViewTouch frameWorkTouchNonVd =
new PlatformViewTouch(
0, // viewId
original.getDownTime(),
original.getEventTime(),
2, // action
original.getAction(),
1, // pointerCount
Arrays.asList(Arrays.asList(0, 0)), // pointer properties
Arrays.asList(Arrays.asList(0., 1., 2., 3., 4., 5., 6., 7., 8.)), // pointer coords
pointerProperties, // pointer properties
pointerCoordinates, // pointer coords
original.getMetaState(),
original.getButtonState(),
original.getXPrecision(),
Expand All @@ -399,11 +413,38 @@ public void itUsesActionEventTypeFromMotionEventForHybridPlatformViews() {
original.getFlags(),
motionEventId.getId());

MotionEvent resolvedEvent =
platformViewsController.toMotionEvent(
/*density=*/ 1, frameWorkTouch, /*usingVirtualDisplay=*/ false);
return platformViewsController.toMotionEvent(
1, // density
frameWorkTouchNonVd,
usingVirtualDisplays);
}

assertEquals(resolvedEvent.getAction(), frameWorkTouch.action);
@Test
public void toMotionEvent_returnsSameCoordsForVdAndNonVd() {
MotionEventTracker motionEventTracker = MotionEventTracker.getInstance();
PlatformViewsController platformViewsController = new PlatformViewsController();

MotionEvent original =
MotionEvent.obtain(
10, // downTime
10, // eventTime
261, // action
1, // x
1, // y
0 // metaState
);

MotionEvent resolvedNonVdEvent =
makePlatformViewTouchAndInvokeToMotionEvent(
platformViewsController, motionEventTracker, original, false);

MotionEvent resolvedVdEvent =
makePlatformViewTouchAndInvokeToMotionEvent(
platformViewsController, motionEventTracker, original, true);

assertEquals(resolvedVdEvent.getEventTime(), resolvedNonVdEvent.getEventTime());
assertEquals(resolvedVdEvent.getX(), resolvedNonVdEvent.getX(), 0.001f);
assertEquals(resolvedVdEvent.getY(), resolvedNonVdEvent.getY(), 0.001f);
}

@Test
Expand Down