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

Fix viewInset.bottom and viewPadding.bottom… #21730

Merged
merged 11 commits into from
Oct 30, 2020
Merged

Fix viewInset.bottom and viewPadding.bottom… #21730

merged 11 commits into from
Oct 30, 2020

Conversation

LasseRosenow
Copy link
Contributor

@LasseRosenow LasseRosenow commented Oct 9, 2020

… for pre android api 30 devices, to enable edge-to-edge navigation on android 10.

Description

Currently the flutter engine sets the bottom inset for the navigation bar into viewInset.bottom and viewpadding.bottom is always 0 on pre api 30 devices.

If you are using edge-to-edge on api 30 everything works perfectly fine, but on api 29, which is supporting edge-to-edge, we get some issues:

The padding.bottom value is calculated like this: max(0, (viewPadding.bottom - viewInset.bottom))
But if viewPadding.bottom is 0 and viewInset.bottom contains the system navigation bar height, the resulting padding.bottom will be 0.

So for example the BottomNavigationBar widget will not apply bottom padding to have its content on over and not behind the system navigation bar.

Related Issues

Tests

I added the following tests:

Replace this with a list of the tests that you added as part of this PR. A
change in behaviour with no test covering it will likely get reverted
accidentally sooner or later. PRs must include tests for all
changed/updated/fixed behaviors. See testing the engine for instructions on
writing and running engine tests.

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 C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

@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.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Oct 9, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@LasseRosenow
Copy link
Contributor Author

The pr is ready for review.

@GaryQian can you review this?

@xster xster requested a review from GaryQian October 12, 2020 18:31
@GaryQian
Copy link
Contributor

This does indeed look like it brings the behavior in line with what is described in https://api.flutter.dev/flutter/widgets/MediaQueryData-class.html. Thanks for the catch.

@GaryQian
Copy link
Contributor

The change will also need to be replicated in the equivalent code in shell/platform/android/io/flutter/view/FlutterView.java

@GaryQian
Copy link
Contributor

This PR also needs tests. The best place to put it would probably be in shell/platform/android/test/io/flutter/embedding/android/FlutterViewTest.java, where systemInsetGetInsetsFullscreenLegacy is likely a good example test to base a new one off of.

Copy link
Contributor

@GaryQian GaryQian left a comment

Choose a reason for hiding this comment

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

See comments above :)

@LasseRosenow
Copy link
Contributor Author

Okay thanks for the review. :)

I will look into it this afternoon

@LasseRosenow
Copy link
Contributor Author

@GaryQian

I updated all the files based on your feedback.

Regarding the tests, I think that the tests that the available tests should cover my pr, so I only updated them to expect the correct bottomPadding values.

Unfortunately I am struggling, getting the engine compiled and running the tests on my windows pc, so I could not check if my changes to the test file are correct. Would you like to do that? :) If you don't have enough time, I would try to things working on my side, but I don't know how long this will take

@GaryQian
Copy link
Contributor

I may be able to find some time to look at the tests later this week. In the meantime, it should be possible to look at the pre-submit check logs for details on why tests failed. The relevant tests will generally finish running/fail about 10 min after pushing.

You should make sure the tests are changed only for the tests specified for API 29 or below.

@LasseRosenow
Copy link
Contributor Author

LasseRosenow commented Oct 21, 2020

Thats very nice of you. :)
It took me a bit but I figured out how to find the test outputs on github :)
It seems the ones that are failing are tests that are not specified for any api version. I will try to duplicate those tests and create one for api29 and one for api30

@LasseRosenow
Copy link
Contributor Author

The android tests seem to be working now.

@google-cla
Copy link

google-cla bot commented Oct 21, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@chinmaygarde
Copy link
Member

This needs to be rebased on ToT as the failing presubmit has been fixed on master. Also @GaryQian, can you take another look and LGTM please. Thanks.

@LasseRosenow
Copy link
Contributor Author

Okay I rebased it :)

Copy link
Contributor

@GaryQian GaryQian left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for getting the tests done, and I apologize for not getting to it last week. In any case, thank you for the contribution!

@GaryQian GaryQian added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. affects: engine labels Oct 29, 2020
@fluttergithubbot fluttergithubbot merged commit 7d6180c into flutter:master Oct 30, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020
chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020
@flutter-dashboard
Copy link

This pull request was opened against a branch other than main. Since Flutter pull requests should not normally be opened against branches other than main, I have changed the base to main. If this was intended, you may modify the base back to master. See the Release Process for information about how other branches get updated.

Reviewers: Use caution before merging pull requests to branches other than main, unless this is an intentional hotfix/cherrypick.

1 similar comment
@flutter-dashboard
Copy link

This pull request was opened against a branch other than main. Since Flutter pull requests should not normally be opened against branches other than main, I have changed the base to main. If this was intended, you may modify the base back to master. See the Release Process for information about how other branches get updated.

Reviewers: Use caution before merging pull requests to branches other than main, unless this is an intentional hotfix/cherrypick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: engine cla: yes platform-android 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.

5 participants