Skip to content

fix: NavigationBarDestination.selected_icon renders wrongly when set to a Control#6468

Merged
FeodorFitsner merged 22 commits intorelease/v0.85.0from
fix-nav-destination-selected-icon
May 6, 2026
Merged

fix: NavigationBarDestination.selected_icon renders wrongly when set to a Control#6468
FeodorFitsner merged 22 commits intorelease/v0.85.0from
fix-nav-destination-selected-icon

Conversation

@ndonkoHenri
Copy link
Copy Markdown
Contributor

@ndonkoHenri ndonkoHenri commented May 5, 2026

Fix #6460

Summary by Sourcery

Fix NavigationBarDestination selected icon rendering when provided as a control and update tests and changelog accordingly.

Bug Fixes:

  • Ensure NavigationBarDestination.selected_icon renders correctly when passed as an Icon control instead of icon data.

Documentation:

  • Document the NavigationBarDestination.selected_icon rendering fix in the changelog.

Tests:

  • Extend navigation bar integration test to cover a selected destination using an Icon control as selected_icon.

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

We've reviewed this pull request using the Sourcery rules engine

@ndonkoHenri ndonkoHenri changed the title fix:NavigationBarDestination.selected_icon renders wrongly when set to a Control fix: NavigationBarDestination.selected_icon renders wrongly when set to a Control May 5, 2026
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 5, 2026

Deploying flet-website-v2 with  Cloudflare Pages  Cloudflare Pages

Latest commit: dfcf014
Status: ✅  Deploy successful!
Preview URL: https://42f0510d.flet-website-v2.pages.dev
Branch Preview URL: https://fix-nav-destination-selected.flet-website-v2.pages.dev

View logs

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes #6460 where NavigationBarDestination.selected_icon rendered incorrectly when provided as an Icon control (e.g., ft.Icon(...)) by ensuring the Flutter-side implementation treats selected_icon consistently as “icon-or-widget” rather than always attempting to parse icon data.

Changes:

  • Update NavigationBarDestinationControl to build selectedIcon via control.buildIconOrWidget("selected_icon"), avoiding getIconData() on non-int values.
  • Adjust the Material NavigationBar integration screenshot test to select the destination that uses a Control-typed selected_icon.
  • Add a root changelog entry documenting the fix.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
sdk/python/packages/flet/integration_tests/controls/material/test_navigation_bar.py Updates screenshot coverage to exercise selected_icon when provided as an ft.Icon control (and ensures it’s visibly selected).
packages/flet/lib/src/controls/navigation_bar_destination.dart Fixes selected icon rendering by using the shared buildIconOrWidget() path for selected_icon.
CHANGELOG.md Documents the bugfix for NavigationBarDestination.selected_icon when provided as an Icon control.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 5, 2026

Deploying flet-examples with  Cloudflare Pages  Cloudflare Pages

Latest commit: dfcf014
Status: ✅  Deploy successful!
Preview URL: https://e8809b29.flet-examples.pages.dev
Branch Preview URL: https://fix-nav-destination-selected.flet-examples.pages.dev

View logs

@ndonkoHenri ndonkoHenri linked an issue May 5, 2026 that may be closed by this pull request
1 task
ndonkoHenri and others added 5 commits May 6, 2026 00:47
Avoids the `!debugNeedsPaint` assertion in RenderRepaintBoundary.toImage
when the boundary is still dirty at capture time on slower runners.

Also temporarily narrows macos-integration-tests workflow to
examples/controls/core::test_row.py to validate the fix in CI.
The macOS runner upgrade (macos-14 → -15 → -26) increased the default
viewport size, making the Row alignment screenshots flake with
`!debugNeedsPaint` because the screenshot package's 20ms post-delay was
no longer enough to flush a paint of the larger surface.

Calling resize_page(800, 1000) restores deterministic paint workload.
Reverts the earlier endOfFrame attempt, which deadlocks under the
flutter_test fake scheduler.
@ndonkoHenri ndonkoHenri linked an issue May 6, 2026 that may be closed by this pull request
1 task
The screenshot package waits 20ms before calling RenderRepaintBoundary.toImage().
On macos-26 runners that's too short — the capture invocation itself can
schedule a frame that's still painting when toImage() runs, tripping the
`!debugNeedsPaint` assertion.

Pass an explicit 200ms delay from the testing harness for both
take_page_controls_screenshot and assert_control_screenshot.
To capture more diagnostic output for the test_alignment screenshot
race investigation. Revert before merge.
The OS window resize triggered by resize_page propagates asynchronously.
Without an explicit settle, it races into the capture's post-delay and
re-dirties the RepaintBoundary right before toImage() runs, tripping
`!debugNeedsPaint`.

CI debug log showed the window UPDATE_CONTROL_PROPS arriving 1ms after
the capture invocation was sent.
The resize_page workaround introduced its own race: macOS window resize
is OS-async beyond Flutter's pump cycle, the requested height is clamped
to the runner's display, and the late-arriving resize event re-dirties
the boundary anyway. Revert and rely on the 200ms capture delay only.
Use pump_times to outlast any scrollbar-fade or SafeArea inset
adjustment that might keep the boundary dirty for ~1.5s.

Diagnostic — narrowing down the !debugNeedsPaint cause.
The page is already scrollable; the inner Column.scroll=AUTO produces a
double-scroll layout that combined with intrinsic_width wrapping in the
test harness keeps the RepaintBoundary perpetually dirty, tripping
!debugNeedsPaint when the screenshot is captured.
Drops the 200ms capture delay, pump_times=10 in test_alignment, and
DEBUG-level pytest logging — none were the actual fix; the inner
double-scroll in the alignment example was. Workflow stays narrowed to
test_row.py for one more validation run before reverting that too.
PR #6450 wrapped the scrollable child in a LayoutBuilder to read viewport
size for the min-height constraint. LayoutBuilder reports 0 for intrinsic
dimensions, which collapses any ancestor IntrinsicWidth / IntrinsicHeight
and leaves the layout in an unstable state — the test harness's
intrinsic_width=True wrapper then perpetually marks the RepaintBoundary
dirty, tripping !debugNeedsPaint when toImage() runs.

MediaQuery.sizeOf exposes the same viewport size through a regular
InheritedWidget, transparent to intrinsic queries, while
SingleChildScrollView already forwards cross-axis intrinsic queries to
its child. Net effect: vertical alignment in scrollable Page/View still
works (the original #6450 intent), and IntrinsicWidth/Height ancestors
are no longer broken.

Restores scroll=AUTO on the alignment example to validate the fix.
…yBoxes

PR #6450 used a LayoutBuilder to read the parent's constraints and apply
the viewport's max-extent as a min-height on the scroll-view child, so
vertical alignment works in scrollable Page/View when content is shorter
than the viewport. LayoutBuilder, however, reports 0 for intrinsic
dimensions, which collapses any ancestor IntrinsicWidth / IntrinsicHeight
and leaves the layout perpetually dirty (test_alignment was tripping
!debugNeedsPaint in toImage()).

Replace the LayoutBuilder with two cooperating RenderProxyBoxes that
forward intrinsic queries to their child:

* _OuterConstraintsReader sits outside Scrollbar/SingleChildScrollView
  and captures its incoming constraints during performLayout.
* _InnerConstraintsEnforcer sits inside SingleChildScrollView and reads
  the captured value back, applying it as a min on the scroll-view child.

Same render output as the original LayoutBuilder (including correct
no-op behavior for nested-in-unbounded-scroll cases), but transparent to
ancestor IntrinsicWidth/Height.
…t change

The inner enforcer lives inside SingleChildScrollView, which provides
unbounded constraints in the scroll axis to its child. On window resize
those constraints don't change, so Flutter's layout pipeline skips the
inner enforcer's performLayout — and it keeps applying the stale min
captured on first layout. Result: vertical alignment looks correct
initially but freezes at the original viewport size after a resize.

Track the inner enforcer in the constraints holder; when the outer
reader sees its incoming constraints change, mark the inner dirty via
invokeLayoutCallback (which enables mutations during layout). The inner
then re-runs performLayout later in the same pass with the updated
holder value.
Revert the temporary narrowing that scoped runs to test_row.py while
diagnosing the !debugNeedsPaint regression introduced by PR #6450.
@FeodorFitsner FeodorFitsner merged commit 2a1ded7 into release/v0.85.0 May 6, 2026
16 of 28 checks passed
@FeodorFitsner FeodorFitsner deleted the fix-nav-destination-selected-icon branch May 6, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants