Fix resonant explosion of scroll disconnect when scrolling a pv in a list #180246
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a scrolling issue with platform views by ensuring touch event coordinates are correctly translated. The change from using potentially stale left and top fields to calling getLeft() and getTop() for ACTION_MOVE events is a good step. The addition of requestUnbufferedDispatch(event) for smoother scrolling is also a nice improvement.
My main feedback is to apply the coordinate fix more consistently. The reasoning for using getLeft() and getTop() applies to all touch events, not just ACTION_MOVE. I've left a suggestion to simplify the logic and make it more robust.
Additionally, it appears the canDragViews test in FlutterMutatorViewTest.java may no longer be valid with these changes and will likely need to be updated to align with the new behavior. The test currently seems to assert the old behavior of using coordinates from the start of the gesture.
| return super.onTouchEvent(event); | ||
| } | ||
|
|
||
| requestUnbufferedDispatch(event); |
There was a problem hiding this comment.
This may have an impact on platform views that themselves are handling touches, as we will be sending them unbuffered inputs after we pipe them through the framework. We could alternatively make this
if (event.getAction() == MotionEvent.ACTION_MOVE) {
requestUnbufferedDispatch(event);
}as currently I think the move case is the place where we get the benefit, and this would then not have an impact on taps.
There was a problem hiding this comment.
I removed this, as it is a logically separate change. Can consider in a different PR.
This comment was marked as resolved.
This comment was marked as resolved.
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Confirmed this works with TLHC/VD |
Removed unbuffered dispatch request from onTouchEvent.
I don't know if there is an issue for this, but without the fix it looks like (in particular see the end of the video)
Screen_recording_20251223_140853.mp4
and with the fix it looks like
Screen_recording_20251223_140519.mp4
Basically, we were using the value passed in by the engine in
onDisplayPlatformView(2)to try to calculate the offset for this touch. However, we aren't guaranteed a call toonDisplayPlatformViewfor each call toFlutterMutatorView.onTouchEvent, and in practice we don't normally get them lined up like that, so the value is stale. Also, the touches are frequently changing by sub pixel values, but the values passed toonDisplayPlatformVieware received as ints (we could maybe fix this, if it matters elsewhere). So this is fragile, comparing whole pixel to sub pixel, and also uses stale values. We can instead just ask android where the view is, I believe (this is what the PR does, and it works AFAICT).I don't know how to test this.