Skip to content

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

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented May 27, 2025

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

@bleroux bleroux requested a review from justinmc May 27, 2025 09:06
@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels May 27, 2025
@flutter-dashboard
Copy link

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 package:flutter.

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

Changes reported for pull request #169497 at sha 25df240

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label May 27, 2025
Copy link
Contributor

@justinmc justinmc left a 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

@bleroux bleroux force-pushed the revert_fix_NavigationBar_indicator_overlay_color branch from 25df240 to b58a872 Compare June 2, 2025 06:07
@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 2, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Jun 2, 2025
@bleroux
Copy link
Contributor Author

bleroux commented Jun 2, 2025

LGTM 👍

Thanks!

Weird. It's not due to this is it? #161698

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.
I will search if there are known similar issues for Ink.

In the meantime, this PR is probably a good candidate for CP, WDYT?

Merged via the queue into flutter:master with commit 0b3ba80 Jun 2, 2025
76 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 2, 2025
@bleroux bleroux deleted the revert_fix_NavigationBar_indicator_overlay_color branch June 2, 2025 09:46
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
@bleroux bleroux added the cp: stable cherry pick this pull request to stable release candidate branch label Jun 5, 2025
flutteractionsbot pushed a commit to flutteractionsbot/flutter that referenced this pull request Jun 5, 2025
…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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cp: stable cherry pick this pull request to stable release candidate branch f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files
Projects
None yet
2 participants