-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Revert "Fix NavigationBar indicator overlay color (#164484)" #169497
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
Revert "Fix NavigationBar indicator overlay color (#164484)" #169497
Conversation
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Weird. It's not due to this is it? #161698
This reverts commit c6c5186.
25df240 to
b58a872
Compare
Thanks!
I wondered too if it could be a side effect of #161698 but I don't think so as I tried the fix from #163419 and the bug was still there. In the meantime, this PR is probably a good candidate for CP, WDYT? |
…lutter#169497) ## Description This PR reverts the change from [Fix NavigationBar indicator overlay color](flutter#164484) as it leads to several regressions. The change was very small: replacing a Container (which obscured the overlay) with an Ink. Unfortunately this leads to some rendering issues which seem related to the Ink painting not being fully in sync with the animation logic implemented by `NavigationIndicator`. I investigated this but did not find an obvious solution. So I would prefer to revert flutter#164484 as the issue it fixed has less impact than the regression. cc @justinmc for review and also in case you have some clue about why an Ink would cause such problems compared to a Container. ## Related Issue Fixes [[Flutter 3.32.0] Active NavigationBar item not selected when switching items programmatically](flutter#169249) Fixes [NavigationDrawer active indicator offset with SvgPicture](flutter#169436) Fixes flutter#164484 (comment) Reopens flutter#163871
## Description This PR fixes NavigationBar lacking visual feedback on the active destination indicator. This is a reland of #164484 which was reverted in #169497. After investigation, I narrowed down the regression introduced in #164484 to the usage of a RepaintBoundary. I added one test to verify that the regression which led to the revert of #164484 is no more reproducible. ### Before: The navigation indicator does not change color when hovered or focused: https://github.com/user-attachments/assets/a1e67dee-4a38-4711-ba90-bdcd9bed3226 ### After: The navigation indicator color changes (slightly darker): https://github.com/user-attachments/assets/1b1cc335-2cf4-4c41-9c53-696537707c72 ## Related Issue Fixes [NavigationBar lacks visual feedback when focused or hovered](#163871) ## Tests - Updates several helper functions which are used by several tests. - Updates several test: adding an Ink widget changes the coordinates used in several tests because these coordinates are now relative to the Ink offset. - Add one test to check that active destination moves to the correct destination (that was not the case after #164484). This new test is a golden test.
## Description This PR fixes NavigationBar lacking visual feedback on the active destination indicator. This is a reland of flutter#164484 which was reverted in flutter#169497. After investigation, I narrowed down the regression introduced in flutter#164484 to the usage of a RepaintBoundary. I added one test to verify that the regression which led to the revert of flutter#164484 is no more reproducible. ### Before: The navigation indicator does not change color when hovered or focused: https://github.com/user-attachments/assets/a1e67dee-4a38-4711-ba90-bdcd9bed3226 ### After: The navigation indicator color changes (slightly darker): https://github.com/user-attachments/assets/1b1cc335-2cf4-4c41-9c53-696537707c72 ## Related Issue Fixes [NavigationBar lacks visual feedback when focused or hovered](flutter#163871) ## Tests - Updates several helper functions which are used by several tests. - Updates several test: adding an Ink widget changes the coordinates used in several tests because these coordinates are now relative to the Ink offset. - Add one test to check that active destination moves to the correct destination (that was not the case after flutter#164484). This new test is a golden test.
## Description This PR fixes NavigationBar lacking visual feedback on the active destination indicator. This is a reland of flutter#164484 which was reverted in flutter#169497. After investigation, I narrowed down the regression introduced in flutter#164484 to the usage of a RepaintBoundary. I added one test to verify that the regression which led to the revert of flutter#164484 is no more reproducible. ### Before: The navigation indicator does not change color when hovered or focused: https://github.com/user-attachments/assets/a1e67dee-4a38-4711-ba90-bdcd9bed3226 ### After: The navigation indicator color changes (slightly darker): https://github.com/user-attachments/assets/1b1cc335-2cf4-4c41-9c53-696537707c72 ## Related Issue Fixes [NavigationBar lacks visual feedback when focused or hovered](flutter#163871) ## Tests - Updates several helper functions which are used by several tests. - Updates several test: adding an Ink widget changes the coordinates used in several tests because these coordinates are now relative to the Ink offset. - Add one test to check that active destination moves to the correct destination (that was not the case after flutter#164484). This new test is a golden test.
## Description This PR fixes NavigationBar lacking visual feedback on the active destination indicator. This is a reland of flutter#164484 which was reverted in flutter#169497. After investigation, I narrowed down the regression introduced in flutter#164484 to the usage of a RepaintBoundary. I added one test to verify that the regression which led to the revert of flutter#164484 is no more reproducible. ### Before: The navigation indicator does not change color when hovered or focused: https://github.com/user-attachments/assets/a1e67dee-4a38-4711-ba90-bdcd9bed3226 ### After: The navigation indicator color changes (slightly darker): https://github.com/user-attachments/assets/1b1cc335-2cf4-4c41-9c53-696537707c72 ## Related Issue Fixes [NavigationBar lacks visual feedback when focused or hovered](flutter#163871) ## Tests - Updates several helper functions which are used by several tests. - Updates several test: adding an Ink widget changes the coordinates used in several tests because these coordinates are now relative to the Ink offset. - Add one test to check that active destination moves to the correct destination (that was not the case after flutter#164484). This new test is a golden test.
Description
This PR reverts the change from Fix NavigationBar indicator overlay color as it leads to several regressions.
The change was very small: replacing a Container (which obscured the overlay) with an Ink. Unfortunately this leads to some rendering issues which seem related to the Ink painting not being fully in sync with the animation logic implemented by
NavigationIndicator.I investigated this but did not find an obvious solution. So I would prefer to revert #164484 as the issue it fixed has less impact than the regression.
cc @justinmc for review and also in case you have some clue about why an Ink would cause such problems compared to a Container.
Related Issue
Fixes [Flutter 3.32.0] Active NavigationBar item not selected when switching items programmatically
Fixes NavigationDrawer active indicator offset with SvgPicture
Fixes #164484 (comment)
Reopens #163871