engine icon indicating copy to clipboard operation
engine copied to clipboard

[iOS] Avoid keyboard animation junk and laggy on Promotion devices.

Open luckysmg opened this issue 2 years ago • 14 comments

If use CADisplaylink directly, the refresh rate of CADisplaylink is not correct It doesn't match the refresh using for flutter rendering, and this will cause some junk and laggy when start keyboard animation.

So using VsyncClient will fix this (See VsyncClient.setMaxRefreshRateIfEnabled)

List which issues are fixed by this PR. You must list at least one issue. https://github.com/flutter/flutter/issues/109435

Pre-launch Checklist

  • [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] 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.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I signed the [CLA].
  • [x] All existing tests are passing.

luckysmg avatar Jul 24 '22 13:07 luckysmg

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

flutter-dashboard[bot] avatar Jul 24 '22 13:07 flutter-dashboard[bot]

Tests has added.

luckysmg avatar Jul 26 '22 08:07 luckysmg

@cyanglaz could you take a look?

jmagman avatar Jul 27 '22 20:07 jmagman

Thanks for updating it to use vsync client. Let's also mention the purpose of using vsync client in the PR description.

Done

luckysmg avatar Aug 03 '22 16:08 luckysmg

Does this fix the linked issue? The description just says it "optimizes" it.

Yes, optimizes it. Maybe that issue is due to diffrence thread. The VsyncClient for keyboard animation in FlutterViewController must run in mainLoop, because the properties like view.presentationLayermust be accessd from iOS main thread. But VsyncClient for flutter UI rendering is run in the runloop of UITaskRunner.

So maybe this PR is to optimize it (At least to ensure the refresh rate that keyboard animation take matches the refresh rate using for rendering). And this optimization will be more obvious on iPhone that have Promotion,eg:iPhone13Pro ).

You could have some debuging using physical device that have promotion to test this PR's optimization. Here is example dart code:

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: const TextFieldPage(),
    );
  }
}


class TextFieldPage extends StatefulWidget {
  const TextFieldPage({Key? key}) : super(key: key);

  @override
  State<TextFieldPage> createState() => _TextFieldPageState();
}

class _TextFieldPageState extends State<TextFieldPage> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Column(
        mainAxisAlignment: MainAxisAlignment.end,
        children: [
          const Spacer(),
          const Text('Keyboard animation', style: TextStyle(fontSize: 26)),
          const Spacer(),
          SizedBox(
              height: 60 + MediaQuery.of(context).padding.bottom,
              child:  const CupertinoTextField(
                decoration: BoxDecoration(color: Colors.red),
              )),
        ],
      ),
    );
  }
}


luckysmg avatar Aug 04 '22 06:08 luckysmg

I think there is a bit of a language problem. "Optimize" means to me making something more efficient. Since the linked issue isn't an efficiency problem, it is a visual glitch. Does this PR remove the visual glitch?

gaaclarke avatar Aug 04 '22 23:08 gaaclarke

I think there is a bit of a language problem. "Optimize" means to me making something more efficient. Since the linked issue isn't an efficiency problem, it is a visual glitch. Does this PR remove the visual glitch?

Oh, It will reduce some visual glitch on high refresh rate devices. But it still has a little little bit.... I have modified the desciption.

luckysmg avatar Aug 04 '22 23:08 luckysmg

Would you mind take a look again?😄 @gaaclarke.

luckysmg avatar Aug 05 '22 21:08 luckysmg

I think this looks good. This section of code is pretty tricky and this change makes it even more complicated. I'd like to make sure that we are actually getting the benefit we want before merging it. @cyanglaz did you test this locally?

I think this PR is more suitable for this desc: https://github.com/flutter/flutter/issues/101653#issuecomment-1169809142

luckysmg avatar Aug 10 '22 16:08 luckysmg

And If you want to run locally to test the benefit. You can use this demo code😄 It can be run directly without any setup: flutter_demo.zip

You can test according to this: https://github.com/flutter/flutter/issues/101653#issuecomment-1169809142

The comparison is very obvious.

luckysmg avatar Aug 10 '22 16:08 luckysmg

Hi @gaaclarke @cyanglaz I have merged main into this branch, so that maybe it will not take so much time to compile code when you switching to this branch and test locally😄.

luckysmg avatar Aug 11 '22 09:08 luckysmg

Hi @gaaclarke @cyanglaz Sorry.I think I didn't made things very clear before. Here is coming:

This PR is to mainly avoid the keyboard animation junk and laggy on Promotion devices (eg:iPhone13Pro)

The visual glitch in https://github.com/flutter/flutter/issues/105687 . I think it is due to the cause below probably: On the engine side,we use CADisplaylink or VsyncClient to track the animation interpolation manually, but it is not a iOS native view that use the way like:UIView.animate(block:),so it maybe have some diffrence...

So why's this visual glitch related with this PR? Because the CADisplayLink's refresh rate using in FlutterViewController's keyboard animation doesn't match the refresh rate of VsyncClient using for rendering on 120HZ devices. And that mismatch will cause some junk and laggy,which will enlarge the visual glitch. (That's why 60hz devices are fine and keyboard animation is smoother, because both of them refresh rate are 60HZ)

In brief, this PR is to mainly avoid the keyboard animation junk and laggy, and will reduce the visual glitch.....

Before ,I wanted to show you a video that can show the difference before and after applying this PR. But the recorded screen video on iPhone is usually 45FPS-55FPS, it is not enough and it is very difficult to see the differences before and after applying this PR. But it is easy to see it directly with our eyes😄. And that's why I recommend you to test it locally to see the comparsion😄

luckysmg avatar Aug 12 '22 02:08 luckysmg

@luckysmg Great work. I guess this PR is most important for improving frame rate performance on devices with promotions so far.

setAllowPauseAfterVsync method can help to improve scroll and gestures performance as I described in this comment. https://github.com/flutter/flutter/issues/101653#issuecomment-1134786994

IMHO, we don't need to pause CADisplaylink during a keyboard animation and any user's gestures (like scroll and etc)

rotorgames avatar Aug 15 '22 01:08 rotorgames

@rotorgames Oh yeah, I'll also take a look that problem you said.

luckysmg avatar Aug 15 '22 02:08 luckysmg

Here is the corresponding flutter framework ref to run locally: ab23233fa8212b2813d92191e520d3cc88ffd362

luckysmg avatar Aug 18 '22 01:08 luckysmg

The issue with keyboard animation on flutter is also that the curve is different from native. It is strange that nodoby mentioned this so far but it seems I couldnt find an open issue for that.

Native video below:

https://user-images.githubusercontent.com/53510751/201529532-b1e3e89f-68ee-4c19-9471-25d0e26e5635.MOV

delfme avatar Nov 13 '22 15:11 delfme

As from video above, keyboard and textfield move up almost in sync. While in flutter they are not in sync and visual result looks weird to the end user

delfme avatar Nov 13 '22 15:11 delfme

As from video above, keyboard and textfield move up almost in sync. While in flutter they are not in sync and visual result looks weird to the end user

See

https://github.com/flutter/flutter/issues/105687#issuecomment-1246153912

luckysmg avatar Nov 13 '22 15:11 luckysmg

Hello @luckysmg , Thank you for the PR. Is this feature merged to latest stable flutter version(3.3.9)?

dayoul avatar Dec 06 '22 08:12 dayoul

No, but it should be in beta channel

luckysmg avatar Dec 06 '22 08:12 luckysmg

This is available in 3.4.0-17.0.pre, see https://github.com/flutter/flutter/wiki/Where's-my-Commit%3F#finding-the-framework-commit-that-contains-engine-commit-x

jmagman avatar Dec 06 '22 18:12 jmagman

@jmagman oh I didn't know how to check the corresponding version. Thanks for the reference!

dayoul avatar Dec 07 '22 02:12 dayoul