Repository navigation
Repaint NavigationIndicator's host Material when its animation ticks - #187802
theprantadutta wants to merge 3 commits into
Conversation
NavigationIndicator draws its background with an Ink widget, which paints on the nearest ancestor Material rather than on its own render object. When the selected index changes without a pointer interaction (for example, programmatically alongside an IndexedStack), nothing notifies that Material, so it keeps showing the indicator's previous frame; in a NavigationDrawer the highlight visibly sticks to the old destination until a hover, click, or resize forces a repaint. Notify the indicator's MaterialInkController whenever the indicator animation ticks, the same way ink features such as InkHighlight request repaints when their state changes. Fixes flutter#180359
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that the navigation indicator repaints when the selected index changes programmatically by calling Material.of(context).markNeedsPaint() during animation ticks, and adds a regression test to verify this behavior. Feedback suggests using Material.maybeOf(context)?.markNeedsPaint() instead to prevent potential runtime exceptions in environments where a Material ancestor is not present.
github-actions
Bot
added
framework
|
Regarding the code freeze check: this is a one-line fix for a regression introduced by #164484 (the indicator stopped repainting on programmatic selection changes), and the approach was verified by dkwingsmt in #180359 before this PR was opened. If it qualifies for |
Piinks
added
the
Decoupling: Not ready to port yet
|
I've marked this PR as not ready to port to flutter/packages yet. |
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
Fantastic documentation and unit test! Thank you very much!
|
@QuncCccccc Can you make a 2nd review? |
QuncCccccc
left a comment
QuncCccccc
left a comment
There was a problem hiding this comment.
LGTM with one super nit. Thanks a lot for your contribution!
| testWidgets('Indicator repaints when selectedIndex changes without pointer interaction', ( | ||
| WidgetTester tester, | ||
| ) async { | ||
| // Regression test for https://github.com/flutter/flutter/issues/180359. |
There was a problem hiding this comment.
super nit: I usually will put this comment above the test:)
There was a problem hiding this comment.
Moved the regression test reference above the test in 1e5b7df — thanks, and thanks for the second review. Understood on the freeze; I'll watch for the porting instructions once the decoupling lands.
|
Thanks for your contribution! This PR has reached completion, however we're not able to merge this PR for the moment due to the Material/Cupertino code freeze (#184093). As soon as the decoupling is complete, we will post a way to port this PR to the new packages to land. In the meantime, I'll convert this PR to a draft and apply a label so that we can quickly find it when the freeze is lifted. (Feel free to address the nit comment if you feel like to, either here in the draft or in the new PR.) |
dkwingsmt
added
the
waiting for code freeze
1e5b7df
|
If you would like to return to this, material_ui and cupertino_ui are now accepting contributions. :) |
dkwingsmt
added
Decoupling: Port to flutter/packages
|
Thanks @dkwingsmt — ported to |
…without pointer input (#13010) Port of flutter/flutter#187802 to `material_ui`, following the process in flutter/flutter#188444. The original PR was approved by dkwingsmt and QuncCccccc but couldn't land because of the Material/Cupertino code freeze. `NavigationIndicator` draws its indicator with an `Ink`, which paints on the nearest ancestor `Material` rather than locally. When the indicator animates without an accompanying pointer interaction — for example when `selectedIndex` is changed programmatically — nothing tells that `Material` to repaint, so it keeps showing the indicator's previous frame. The fix calls `Material.of(context).markNeedsPaint()` on each animation tick. The regression test pumps a `NavigationDrawer`, changes `selectedIndex` from outside without any pointer events, and verifies the indicator repaints. I checked it fails with the one-line fix removed and passes with it, in its new location under `packages/material_ui/test/`. The two original commits were cherry-picked without merge commits, as the porting guide describes; rename detection placed them at `lib/src/navigation_bar.dart` and `test/navigation_drawer_test.dart` with no conflicts. The only new commit adds the pending changelog entry (`version: patch`). Fixes flutter/flutter#180359 ## Pre-Review Checklist I used AI tooling while working on this, and reviewed and tested all of it myself. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
4 participants
NavigationIndicatordraws its background with anInkwidget, which paints on the nearest ancestorMaterialrather than on its own render object. When the selected index changes without a pointer interaction (for example, programmatically alongside anIndexedStack), nothing notifies thatMaterial, so it keeps showing the indicator's previous frame. In aNavigationDrawer— where the hostMaterialsits outside the destinationListView— the highlight visibly sticks to the old destination until a hover, click, or window resize forces a repaint.This PR applies the fix dkwingsmt verified in the issue thread: notify the indicator's
MaterialInkControllerwhenever the indicator animation ticks. On the idiomatic-ness question raised there:markNeedsPaint()is part of the publicMaterialInkControllerinterface, documented as "Notifies the controller that one of its ink features needs to repaint", and it is the same mechanism ink features likeInkHighlightandInkSplashuse when their state changes — so this seemed like the most direct expression of the intent.The regression test deliberately avoids the
paintsmatcher (which forces a repaint and would mask the staleness). Instead it changesselectedIndexprogrammatically, pumps a frame up toEnginePhase.layout, and asserts the indicator's hostMaterialrender object is marked as needing paint. The test fails without the one-line fix and passes with it. The fullnavigation_drawer_test.dart,navigation_bar_test.dart, andnavigation_rail_test.dartsuites pass (NavigationIndicatoris shared by all three widgets).Fixes #180359
Pre-launch Checklist
///).