Skip to content

Fix/id focus state - #2362

Merged
usmanvrtx merged 3 commits into
android_TV_implementationfrom
fix/id-focus-state
Sep 10, 2026
Merged

Fix/id focus state#2362
usmanvrtx merged 3 commits into
android_TV_implementationfrom
fix/id-focus-state

Conversation

@usmanvrtx

@usmanvrtx usmanvrtx commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Description

This PR fixes ${id.hasFocus} resetting to false when a focused TV widget is rebuilt after navigating back.

The previous widget controller is available through the widget ID context. The replacement controller now inherits its hasFocus value before bindings are evaluated, preventing ID-bound focus styles from rendering unfocused during the rebuild.

Related Issue

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)

What Has Changed

Preserve ID-bound focus state during rebuilds

  • Look up the previous widget instance using its YAML id.
  • When the previous controller has focus, copy hasFocus to the replacement controller before bindings run.
  • Keep the existing TV focus node, navigation, scrolling, actions, and binding-dispatch behavior unchanged.

Behavior Compatibility

  • No changes to D-pad traversal, focus coordinates, navigation, actions, or scrolling.
  • No expected mobile behavior change: hasFocus is set by the existing TV focus wrappers.
  • The change applies only when an ID-bound widget is rebuilt and the previous controller is focused.

How to Test

  1. From modules/ensemble, run:

    flutter analyze lib/framework/widget/view_util.dart

@usmanvrtx usmanvrtx self-assigned this Sep 10, 2026
@TheNoumanDev

Copy link
Copy Markdown
Member

Code review(medium · 2 findings)
modules/ensemble/lib/framework/widget/view_util.dart
● 388 [correctness] The rebuild copy writes hasFocus = true onto the new controller without verifying the rebuilt widget's focus node is actually focused, so when the rebuild produces a new element instead of reusing the old one, ${id.hasFocus} is stuck true permanently.
● 382 [correctness] getContextById walks the parent-scope chain, and the new previousContext is HasController branch acts on outer-scope hits that the previous EnsembleController-only check ignored for old-style widgets.

Scope: the branch is now fix/id-focus-state (PR #2362, base android_TV_implementation) — 17 added / 6 removed lines in view_util.dart only. Note: 23c57fb added resync logic in box_wrapper.dart, and a2b0dcd reverted all of it, so box_wrapper is net-unchanged in the PR. Analyzes clean apart from the pre-existing unused_local_variable at line 434.

Two findings, both on the new block at view_util.dart:382-390. The core issue is that the copy is one-directional — it only ever writes true, and never re-checks the rebuilt node's actual state. That's correct for the case the comment describes (Flutter reusing the element, FocusNode persists), but wrong when the rebuild produces a new element, and the initState/didUpdateWidget resync that covered exactly that case was removed in the second commit with nothing put in its place. The blast radius is limited to ${id.hasFocus} bindings since the focus ring reads live focus state — but that binding is the point of the PR.

The second is narrower: hoisting the lookup and widening the type check to HasController means getContextById's parent-scope walk can now match an outer-scope widget sharing the same id, which the old EnsembleController-only check silently ignored for legacy widgets.

The rest of the diff is clean — the previousContext EnsembleController refactor is behavior-preserving, hoisting getContextById out of the widgetInstance != null branch has no side effects (pure map walk), CustomWidgetModel still returns early before the new code, and running the copy before _updateWidgetBindings means skipping the ModelChangeEvent dispatch is right.

The focus-state copy in view_util carries hasFocus onto a rebuilt
controller, but it is one-directional: when the rebuild produces a new
element whose FocusNode is not actually focused, nothing clears the
copied true and the ${id.hasFocus} binding stays true permanently.

Re-assert the controller against the live focus node in both TV focus
wrapper states (initState post-frame for inherited state, didUpdateWidget
on controller swap), deferring the ModelChangeEvent dispatch to post-frame.

Also scope the view_util copy to ids registered in the current scope so
an ancestor-scope hit with the same id (a collision, not the widget being
rebuilt) cannot transfer focus onto the rebuilt widget.
@usmanvrtx usmanvrtx closed this Sep 10, 2026
@usmanvrtx usmanvrtx reopened this Sep 10, 2026
@usmanvrtx
usmanvrtx merged commit fa967c2 into android_TV_implementation Sep 10, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants