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

[showModalBottomSheet] fix: showModalBottomSheet does not move along keyboard #71636

Merged
merged 7 commits into from Jan 22, 2021
Merged

[showModalBottomSheet] fix: showModalBottomSheet does not move along keyboard #71636

merged 7 commits into from Jan 22, 2021

Conversation

ZainUrRehmanKhan
Copy link
Contributor

@ZainUrRehmanKhan ZainUrRehmanKhan commented Dec 3, 2020

Description

This issue was found in release 1.22, 1.23, 1.24 that the showModelBottomSheet was not moving along the keyboard, the bottom sheet contains a TextField which open up the keyboard on focus, so if the height of bottom sheet is less then the height of keyboard then the bottom sheet was hidden behind the keyboard.

showModelBottomSheet with TextField unfocused:

unfocus

When we Focus on TextField - Before Changes:
Bottom Sheet is hidden behind the keyboard.

before

When we Focus on TextField - After Changes:

after

Related Issues

#71418

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.

@flutter-dashboard
Copy link

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.

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

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Dec 3, 2020
@google-cla google-cla bot added the cla: yes label Dec 3, 2020
@ZainUrRehmanKhan ZainUrRehmanKhan changed the title [showModalBottomSheet] fix: showModalBottomSheet does not move along keyboard #71418 [showModalBottomSheet] fix: showModalBottomSheet does not move along keyboard Dec 3, 2020
@ZainUrRehmanKhan
Copy link
Contributor Author

Kindly review my pull request.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

The change looks solid. Please add a unit test to make sure someone doesn't accidentally revert it (probably a golden test).

packages/flutter/lib/src/material/bottom_sheet.dart Outdated Show resolved Hide resolved
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.For more guidance, visit Writing a golden file test for package:flutter.

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

@ZainUrRehmanKhan
Copy link
Contributor Author

I'm writing a test for this PR.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

Tiny issues. Everything else looks great!

packages/flutter/test/material/bottom_sheet_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/bottom_sheet_test.dart Outdated Show resolved Hide resolved
ZainUrRehmanKhan and others added 2 commits January 12, 2021 09:56
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
@ZainUrRehmanKhan
Copy link
Contributor Author

Suggested changes done.

@ZainUrRehmanKhan
Copy link
Contributor Author

ZainUrRehmanKhan commented Jan 20, 2021

@dkwingsmt Updated my branch to the latest master. That should resolve some failing tests.

@ZainUrRehmanKhan
Copy link
Contributor Author

?

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM

@davidmartos96
Copy link
Contributor

@dkwingsmt Could we have a way to make this change opt out? I currently have some modal bottom sheets where I place the textfields (usually for filtering results) at the top of a quite large bottomSheet, so the keyboard will not hide it. With this PR everything will be pushed above the keyboard which may be considered somewhat breaking. What are your thoughts?

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Jan 23, 2021

@davidmartos96 You are right. Although to make a modal bottom sheet hidden by the on-screen keyboard does not really sound correct to me. A displayed modal bottom sheet is supposed to prevent the user from interacting with the rest of the app. Will normal showBottomSheet suit your needs?

@davidmartos96
Copy link
Contributor

@dkwingsmt I may have not expressed myself correctly. Sorry about that.
The usecase I'm referring to is a TextField inside the modal bottom sheet, but where the bottom sheet will be about 70% tall of the screen height. I have some of those modal sheets in my app, but I display the text field at the top knowing that up to now the keyboard would hide the lower portion of the bottom sheet (and I'm fine with that in my usecase).
With the PR I worry that all the bottom sheet will be pushed too much overflowing the screen height. I agree with the changes made in the PR but I would suggest including an option similar to resizeToAvoidBottomInset in the Scaffold so that this behavior can be opted out (https://api.flutter.dev/flutter/material/Scaffold/resizeToAvoidBottomInset.html)
If the Flutter team agrees to such new option I could open a PR with it.

@dkwingsmt
Copy link
Contributor

Can you achieve the same result by manually applying a negative bottom padding? Actually, you can get the height of your sheet and the screen height to calculate the exact amount of underflow for you to avoid overflowing.

@davidmartos96
Copy link
Contributor

@dkwingsmt I'm not sure I follow you with the negative padding. Does Flutter support negative padding?

@Peng-Qian
Copy link

Peng-Qian commented Mar 3, 2021

Personally, I think this is a bad idea to force ModalBottomSheet to resize itself. I am experiencing the same issue as @davidmartos96 says.

At least we need a parameter to control if the ModalBottomSheet should resize or not (just like Scaffold). And why padding should wrap BottomSheet to force resize to avoid keyboard? (we can just add padding inside ModalBottomSheet to achieve the same effect.)

@dkwingsmt
Copy link
Contributor

@HansMuller Are you more familiar with how Material Design defines ModelBottomSheet? Can you take a look at the requests above?

@Peng-Qian
Copy link

Peng-Qian commented Mar 3, 2021

@dkwingsmt Thanks for your quick response!

Please imagine ModalBottomSheet takes 5/6 of full-screen height and it used to pick a country(e.g. New Zealand), so it will be a scrollable list and contains many items, and on the top of this list, there is a search bar that contains a TextField.

If the ModalBottonSheet forcibly resizes as the keyboard shows, It will push the search bar to the very top of the screen, it not only breaks the UI looking but also break the feature. Because the phone notification bar will overlay on it.

Moreover, padding when the keyboard shows is a brutal way to avoid keyboard overlay. Please check out this https://pub.dev/packages/keyboard_avoider package. I don't think flutter should limit us on how to design the Application.

dkwingsmt added a commit to dkwingsmt/flutter that referenced this pull request Mar 4, 2021
@dkwingsmt
Copy link
Contributor

dkwingsmt commented Mar 4, 2021

We're reverting this PR in #77286. Various sources have reported that the previous behavior is the desired one.

fluttergithubbot pushed a commit that referenced this pull request Mar 5, 2021
@totroi
Copy link

totroi commented May 7, 2021

We're reverting this PR in #77286. Various sources have reported that the previous behavior is the desired one.

Any tips on what to do to have the behavior of the original commit (moving up with the keyboard)?

Maybe an specific modal could be created, like showModalBottomSheetFlexible ? I'd prefer to have it behave that way

@totroi
Copy link

totroi commented May 7, 2021

We're reverting this PR in #77286. Various sources have reported that the previous behavior is the desired one.

Any tips on what to do to have the behavior of the original commit (moving up with the keyboard)?

Maybe an specific modal could be created, like showModalBottomSheetFlexible ? I'd prefer to have it behave that way

Actually it you be much better if one could simply define the "height" of it. Being able to elongate it up just a little would solve my problem.

And also being able to define which children would move upwards with the keyboard, and which would get hidden behind the keyboard. That would be ideal.

@davidmartos96
Copy link
Contributor

@totroi You can get the behavior by wrapping the bottom sheet builder with:

Padding(
  padding: MediaQuery.of(context).viewInsets
)

That's essentially what the reverted PR was doing

@totroi
Copy link

totroi commented May 7, 2021

@totroi You can get the behavior by wrapping the bottom sheet builder with:

Padding(
  padding: MediaQuery.of(context).viewInsets
)

That's essentially what the reverted PR was doing

Thanks! Problem is I'm seeking to move a Card inside the modal up, and move the modal up just the amount required for the Card to move. Like about 30 pixels...

I managed to shrink all the paddings inside the Card widget and it looks okay, but the best result would be those 30 pixels up whenever the keyboard is opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants