Fix janky scrolls on iPhone X/XS #36616
Conversation
|
I tried this with #28113 (comment), and it looks to have fixed scroll performance after startup (#31086) but not at startup (#28113). |
Fixes flutter#31086 Tests added: * Miss at most one frame for irregular scroll drag events * Delay at most one event for faster-than-vsync events * Handles iPhone XS irregular events with different base delays
goderbauer
left a comment
There was a problem hiding this comment.
This looks fine to me, but since scrolling is somewhat delicate, I'd like @Hixie to take a look as well.
|
Ping @Hixie ... |
| // That's because `_isScrollFrameInProgress` will always be false at this | ||
| // point since it will be cleared by the end of a frame (VSYNC) through | ||
| // `_checkPendingDragCallback`. This is the case for all Android/iOS devices | ||
| // before iPhone X/XS. |
There was a problem hiding this comment.
meta note: this is a commit message, not a comment. The comment should talk about what we do now, not what we used to do. Similarly it shouldn't link to a GitHub comment to explain the situation, it should just explain it inline.
|
This seems like it's fixing an iOS-specific problem (at least the comments are very iOS-specific). Would it make more sense to solve this on the iOS embedder side? I'm not a huge fan of intentionally delaying input messages. This seems like it could be really bad for games, in particular. |
|
I'm discussing with Chinmay to see how to do this on the iOS embedder. BTW, the approach that we'll do on the iOS embedder is probably going to be similar to this. The delay won't happen to any input events that are delivered to us regularly at 60Hz (even with this framework patch). For events that are delivered at a faster rate (currently that's probably only iPad Pro at 120Hz), I think we (either the embedder or the framework) need to send that frequency information to the embedder/framework in order to handle all cases. |
|
@liyuqian Any updates? |
|
We're still working on porting the solution to the iOS embedder so it won't
affect other platforms. If you need a quick fix, please feel free to cherry
pick this pull request into your local Flutter checkout.
…On Wed, Aug 7, 2019 at 12:32 AM GO ***@***.***> wrote:
@liyuqian <https://github.com/liyuqian> Any updates?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#36616?email_source=notifications&email_token=AFPMGMD2BNE2MZM434FFDM3QDJ3APA5CNFSM4IFPUQDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3XPKVI#issuecomment-518976853>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFPMGMCBTJT46GVNY7T2WI3QDJ3APANCNFSM4IFPUQDA>
.
|
|
@liyuqian I am closing this one since it sounds like we agreed to take a different approach. Feel free to re-open if this is not true. |
|
Hello. Is this pull request merged to master? |
|
@nullxx0 : please see #31086 (comment) |
Fixes flutter/flutter#31086 This patch is a lower level implementation of flutter/flutter#36616 that would only impact iOS engine, and host unittests.
Fixes flutter/flutter#31086 This patch is a lower level implementation of flutter/flutter#36616 that would only impact iOS engine, and host unittests.
Fixes flutter/flutter#31086 This patch is a lower level implementation of flutter/flutter#36616 that would only impact iOS engine, and host unittests.
) This reverts commit c2879ca. Additionally, we fix flutter/flutter#40863 by adding a secondary VSYNC callback. Unit tests are updated to provide VSYNC mocking and check the fix of flutter/flutter#40863. The root cause of having flutter/flutter#40863 is the false assumption that each input event must trigger a new frame. That was true in the framework PR flutter/flutter#36616 because the input events there are all scrolling move events. When the PR was ported to the engine, we can no longer distinguish different types of events, and tap events may no longer trigger a new frame. Therefore, this PR directly hooks into the `VsyncWaiter` and uses its (newly added) secondary callback to dispatch the pending input event.
Fix janky scrolls on iPhone X/XS
Fixes #31086
Tests added:
Many
await tester.pump()are added to existing unit tests that usegesture.moveByto declare that those input events aren't faster than VSYNC (and the tests shall pass).