[Flutter] Use the correct place for the system navigation bar color adjustment #68102

jacek-marchwicki · 2020-10-14T13:02:35Z

Description

Use the correct place for the system navigation bar color adjustment

Before the fix: the center of the screen were used to adjust navigation bar color.
After the fix: the center of the bottom navigation bar is used to adjust navigation bar color.

Before	After the fix

For more you can read the following issue: #66429

For example code that shows differences before and after the change you can use https://github.com/jacek-marchwicki/flutter-insets-bug2

Keep in mind, that window.padding isn't calculated correctly for translucent navigation bar on Android (issue: #63761) and this is not fixed by the PR.

If you have further questions related to this PR, I'm happy to try answering all of them.

Related Issues

Tests

I added the following tests:

'statusBarColor isn't set for unannotated view',
'statusBarColor is set for annotated view',
'statusBarColor isn't set when view covers less than half of the system status bar',
'statusBarColor is set when view covers more than half of tye system status bar',
'systemNavigationBarColor isn't set for non Android device',
'systemNavigationBarColor isn't set for unannotated view',
'systemNavigationBarColor is set for annotated view',
'systemNavigationBarColor isn't set when view covers less than half of navigation bar',
'systemNavigationBarColor is set when view covers more than half of navigation bar'

Checklist

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 ///). NOT NEED
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR. (I'll wait for the CI build to pass, because unchanged master branch analyze execution is failing on my computer. But I got success on stable branch)
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.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

jacek-marchwicki · 2020-10-14T13:05:06Z

packages/flutter/lib/src/rendering/view.dart

info: bottom padding shouldn't be divided by _window.devicePixelRatio because everywhere we use native pixels. We should just divide the top padding by two so we get the status bar center.
Before this calculations worked correctly only on 2x screens.

packages/flutter/lib/src/rendering/view.dart

jacek-marchwicki · 2020-10-14T13:09:43Z

packages/flutter/test/rendering/view_chrome_style_test.dart

info: I couldn't use view_test.dart file because TestRenderingFlutterBinding it is used there. Those tests uses TestWidgetsFlutterBinding.
Different types of bindings can't be used in the same file.

pedromassango · 2020-10-14T15:03:11Z

Thanks for this PR. Does this PR also fixes the issues mentioned in #66429 (comment)?

jacek-marchwicki · 2020-10-14T20:43:33Z

@pedromassango yes it fixes the issue :)

jacek-marchwicki · 2020-10-14T20:51:38Z

I see some CI failures on this PR:

"Linux docs" failing - looks like not related to this PR issue /b/s/w/ir/kitchen-checkout/flutter/recipe_modules/firebase/resources/firebase_deploy.sh: line 10: firebase: command not found
"flutter-build" failing - I can't get details at the build. It displays some flutter dashboard
"flutter-gold" pending - it is still pending after 8 hours. Unfortunately, I can't get details here too :(

Can you guys help me solve those problems?

pedromassango · 2020-10-14T21:28:47Z

Those failing tests are probably unrelated to the changes so don’t worry about them. They will get fixed once the fix lands into master and you rebase your branch.

I would suggest you to verify and add then into Relates issues section so they will also get closed automatically. This avoid having opened issues that were fixed already.

jacek-marchwicki · 2020-10-16T07:49:55Z

⬆️ FYI: I've just rebased my change to the current master branch and now all tests passes!
Unfortunately, I still see a lot of flutter analyze issues on my computer unrelated to the change.

jonahwilliams · 2020-10-16T15:39:51Z

Since you branched from master when there was an analysis failure, you'll need to pull in the fixes to your local branch. git fetch upstream master -> git pull upstream master

packages/flutter/test/rendering/view_chrome_style_test.dart

jonahwilliams · 2020-10-16T15:42:04Z

packages/flutter/test/rendering/view_chrome_style_test.dart

Not sure if this is necessary, doesn't each testWidgets get its own binding/render tree? @goderbauer

@jonahwilliams Looks like it's necessary, testWidgets doesn't clear those values by itself.

packages/flutter/test/rendering/view_chrome_style_test.dart

jonahwilliams · 2020-10-16T15:43:17Z

packages/flutter/lib/src/rendering/view.dart

Explanations make sense, could you leave a detailed comment in the code here?

Can you look at the updated?

goderbauer

LGTM

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 14, 2020

google-cla bot added the cla: yes label Oct 14, 2020

jacek-marchwicki commented Oct 14, 2020

View reviewed changes

packages/flutter/lib/src/rendering/view.dart Outdated Show resolved Hide resolved

jacek-marchwicki commented Oct 14, 2020

View reviewed changes

HansMuller changed the title ~~[Flutter] Use the correct pleace for the system navigation bar color adjustment~~ [Flutter] Use the correct place for the system navigation bar color adjustment Oct 15, 2020

HansMuller requested a review from goderbauer October 15, 2020 23:55

jonahwilliams self-requested a review October 16, 2020 00:04

jacek-marchwicki force-pushed the system-navigation-bar-style-fix branch from 07cd7dc to 30118d4 Compare October 16, 2020 07:42

jonahwilliams reviewed Oct 16, 2020

View reviewed changes

jacek-marchwicki force-pushed the system-navigation-bar-style-fix branch from 30118d4 to 126e792 Compare October 19, 2020 12:13

Use the correct pleace for the system navigation bar color adjustment

2c22116

jacek-marchwicki force-pushed the system-navigation-bar-style-fix branch from 126e792 to 2c22116 Compare October 19, 2020 14:53

goderbauer requested a review from jonahwilliams October 28, 2020 21:49

goderbauer approved these changes Oct 30, 2020

View reviewed changes

goderbauer added the waiting for tree to go green label Oct 30, 2020

fluttergithubbot merged commit 32b9af6 into flutter:master Oct 30, 2020

[Flutter] Use the correct place for the system navigation bar color adjustment #68102

[Flutter] Use the correct place for the system navigation bar color adjustment #68102

Uh oh!

Conversation

jacek-marchwicki commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

jacek-marchwicki Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jacek-marchwicki Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

pedromassango commented Oct 14, 2020

Uh oh!

jacek-marchwicki commented Oct 14, 2020

Uh oh!

jacek-marchwicki commented Oct 14, 2020

Uh oh!

pedromassango commented Oct 14, 2020

Uh oh!

jacek-marchwicki commented Oct 16, 2020

Uh oh!

jonahwilliams commented Oct 16, 2020

Uh oh!

Uh oh!

jonahwilliams Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

jacek-marchwicki Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonahwilliams Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

jacek-marchwicki Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jacek-marchwicki commented Oct 14, 2020 •

edited

Loading