Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[iOS] Ensure the keyboard bottom inset is zero when keyboard dismiss. #31982

Merged
merged 4 commits into from
Mar 31, 2022
Merged

[iOS] Ensure the keyboard bottom inset is zero when keyboard dismiss. #31982

merged 4 commits into from
Mar 31, 2022

Conversation

luckysmg
Copy link
Contributor

@luckysmg luckysmg commented Mar 12, 2022

This PR is to ensure the viewport metrics is correct when keyboard dismiss.

Related issue:
flutter/flutter#99951
flutter/flutter#100220

Demo code for first issue
import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';

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

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: const MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({Key? key, required this.title}) : super(key: key);
  
  final String title;

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  @override
  Widget build(BuildContext context) {
    return GestureDetector(
      onTap: (){
        FocusScope.of(context).unfocus();
      },
      child: Scaffold(
        backgroundColor: Colors.white,
        appBar: AppBar(
          title: Text(widget.title),
        ),
        body: Container(
          color: Colors.blue.shade100,
          child: Column(
            children: [
              const SizedBox(height: 48),
              const Padding(
                padding: EdgeInsets.symmetric(horizontal: 16),
                child: Text(
                  '1. Change iOS keyboard to Bitmoji or Slyder keyboard.\n'
                      '2. Tap on the button below.\n'
                      '3. Navigate back to previous route.\n'
                      '4. Check that shaded blue(Scaffold.body) not covering entire screen.\n'
                      '5. Compare the behavior with OS keyboard',
                  style: TextStyle(fontSize: 20),
                ),
              ),
              const TextField(autofocus: true),
              const Spacer(),
              ElevatedButton(
                onPressed: () {
                  FocusScope.of(context).unfocus();
                  Navigator.of(context).push(CupertinoPageRoute(
                    builder: (context) {
                      return const MyHomePage(title: 'Another Page');
                    },
                  ));
                },
                child: const Text('Hello', style: TextStyle(fontSize: 30)),
              )
            ],
          ),
        ),
        bottomNavigationBar: BottomNavigationBar(
          items: [Icons.abc, Icons.train]
              .map((e) => BottomNavigationBarItem(icon: Icon(e), label: 'hi'))
              .toList(),
        ),
      ),
    );
  }
}

Preview for flutter/flutter#99951

Before.MP4
After.mp4


Preview for flutter/flutter#100220

Before2.mp4
After2.mp4

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

@luckysmg luckysmg changed the title [iOS] Ensure the keyboard bottom inset is 0 when keyboard dismiss. [iOS] Ensure the keyboard bottom inset is zero when keyboard dismiss. Mar 12, 2022
@coldstar96
Copy link

Just out of curiosity, why does keyboard with shorter height properly animates without this change?

@luckysmg
Copy link
Contributor Author

luckysmg commented Mar 14, 2022

@coldstar96
Reason: When custom keyboard is dismissed ,the keyboard frame of CGRect given by custom keyboard will intersect with screen....
So the targetBottomInset will be bottom * scale instead of 0.(I have debug the engine code and prove this)

self.targetViewInsetBottom = bottom * scale;

I think maybe there are some issues in custom keyboard app. But making this change in engine side will make it more universal

@coldstar96
Copy link

coldstar96 commented Mar 14, 2022

It's unfortunate that iOS keyboard notification is not called with consistency. I've been through so many weird behaviors with iOS framework that this is no surprise. Thanks for the explanation!

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

@coldstar96 were you able to confirm this fixes the issue since you can repro?

Very minor nits.
@gaaclarke can you give this a sanity check since you reviewed #29281 ?

Comment on lines 218 to 219
@"UIKeyboardAnimationDurationUserInfoKey" : [NSNumber numberWithDouble:0.25],
@"UIKeyboardIsLocalUserInfoKey" : [NSNumber numberWithBool:isLocal]
Copy link
Member

Choose a reason for hiding this comment

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

Use boxed literals

Suggested change
@"UIKeyboardAnimationDurationUserInfoKey" : [NSNumber numberWithDouble:0.25],
@"UIKeyboardIsLocalUserInfoKey" : [NSNumber numberWithBool:isLocal]
@"UIKeyboardAnimationDurationUserInfoKey" : @(0.25),
@"UIKeyboardIsLocalUserInfoKey" : @(isLocal)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

This looks good to me. @jmagman can you LGTM this?

@jmagman
Copy link
Member

jmagman commented Mar 25, 2022

@cyanglaz is reviewing.

@jmagman jmagman requested a review from cyanglaz March 25, 2022 00:28
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Did some debugging, It looks like the root cause of the issue is the keyboardFrame that we get from keyboardWillChangeFrame is a not CGRectZero when the keyboard is going to hide. This might be a bug from iOS.

With the above behavior, I think this fix is a good workaround.

Note: I cannot reproduce the issue on iPhone. I can confirm the iPad issue is fixed with the PR.

@luckysmg
Copy link
Contributor Author

luckysmg commented Mar 30, 2022

Hi @cyanglaz ,What's your iPhone version ? My iPhone can reproduce it.
iPhone 13 pro. iOS 15.4.With custom keyboard app Slyder

@luckysmg luckysmg requested a review from cyanglaz March 31, 2022 03:09
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM % nits

Comment on lines +1185 to +1196
if (@available(iOS 9, *)) {
// Ignore keyboard notifications related to other apps.
id isLocal = info[UIKeyboardIsLocalUserInfoKey];
if (isLocal && ![isLocal boolValue]) {
return;
}
}

// Ignore keyboard notifications if engine’s viewController is not current viewController.
if ([_engine.get() viewController] != self) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nits: This code is the same as in keyboardWillChangeFrame, maybe we can refactor it into a method.

@cyanglaz
Copy link
Contributor

cc @jmagman or @chinmaygarde for a second LGTM

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

Thanks @luckysmg and @cyanglaz!

@jmagman jmagman added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 31, 2022
@fluttergithubbot fluttergithubbot merged commit 7ebc773 into flutter:main Mar 31, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 31, 2022
@luckysmg luckysmg deleted the fix_ios_keyboard branch April 1, 2022 00:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
platform-ios waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants