From 09e0b06e494b39d73de4dfc8ed637331b89a974e Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Thu, 20 Aug 2026 22:54:45 +0200 Subject: [PATCH] fix(tv): unbreak collapsed-rail D-pad traversal, action-bar focus survival, and text-field back handling With the rail collapsed and Libraries expanded, DOWN from the header landed on a mounted but focus-excluded library node and was swallowed, trapping traversal; the focus order now matches the render exclusion and traversal scans past non-focusable candidates. An action bar rebuilding on count change disposed every node, silently dropping focus and never notifying the TV host; bindings are now reused by stable identity (external focus node, else debugLabel) and the transition is reported. Positional reuse remains the only option for an unlabeled action, but it applies only while the action-list shape is provably unchanged - catalog detail enrichment inserting Request before a focused Trailer used to hand Trailer's focused binding, focus ring and next Select included, to Request. Those actions also carry stable debugLabels so their focus survives enrichment. The text field consumed back on KeyDown outside the shared handler, letting one press act twice. It now marks the back coordinator like handleBackKeyAction does and keeps the down-only shape solely on TV, where the closing IME session swallows the matching KeyUp. Because onBack can move focus (empty search field -> sidebar), the orphaned KeyUp would otherwise drive app-level back on the new focus chain, so the field arms BackKeyUpSuppressor; the suppressor gained a hardware-level KeyUp observer that clears the armed state in a microtask after the press ends, removing the long-documented pinning hazard where an unconsumed KeyUp left it swallowing the next press. --- lib/focus/dpad_navigator.dart | 46 ++++-- lib/focus/focusable_action_bar.dart | 137 +++++++++++++++--- lib/focus/focusable_text_field.dart | 27 +++- lib/focus/key_event_utils.dart | 17 +-- lib/screens/catalog_item_detail_screen.dart | 6 + lib/widgets/side_navigation_rail.dart | 28 ++-- ...pad_select_long_press_controller_test.dart | 4 + test/focus/focusable_action_bar_test.dart | 125 ++++++++++++++++ test/focus/focusable_text_field_test.dart | 47 ++++++ .../catalog_item_detail_screen_test.dart | 10 +- test/widgets/focusable_text_field_test.dart | 133 +++++++++++++++++ test/widgets/side_navigation_rail_test.dart | 64 ++++++++ 12 files changed, 586 insertions(+), 58 deletions(-) diff --git a/lib/focus/dpad_navigator.dart b/lib/focus/dpad_navigator.dart index 64877fcfc..121734687 100644 --- a/lib/focus/dpad_navigator.dart +++ b/lib/focus/dpad_navigator.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:ui' as ui; import 'package:flutter/services.dart'; @@ -70,9 +71,18 @@ extension DpadKeyExtension on LogicalKeyboardKey { bool get isDownKey => this == LogicalKeyboardKey.arrowDown; } -/// Base class for suppressing key-up events after a key category triggers an -/// action (e.g. opening a sheet). While suppressed, all events for the matched -/// key category are consumed; suppression auto-clears on [KeyUpEvent]. +/// Base class for suppressing key events after a key category triggers an +/// action (e.g. opening a sheet). While suppressed, every event of the matched +/// key category is consumed — including a [KeyDownEvent], because an armer +/// running in the [HardwareKeyboard] handler phase (the hotkey recorder) arms +/// against the very KeyDown that is then re-dispatched through the focus tree. +/// +/// Suppression ends with the physical press: a global [HardwareKeyboard] +/// observer watches for the matching [KeyUpEvent] and clears the armed state +/// in a microtask — after that KeyUp's own synchronous focus dispatch, so +/// focus-phase consumers still swallow it — which guarantees a stale armed +/// state (a KeyUp delivered to a focus chain that never consulted us) can +/// never swallow the next press. class _KeyUpSuppressor { final bool Function(LogicalKeyboardKey) _keyMatcher; @@ -80,7 +90,27 @@ class _KeyUpSuppressor { bool _suppressed = false; - void suppress() => _suppressed = true; + void suppress() { + // Re-registered on every arm because flutter_test's HardwareKeyboard + // clearState() drops handlers between tests; remove-then-add keeps exactly + // one live registration. The observer only reads state and returns false, + // so it can never consume an event, and it is intentionally never removed + // outside re-registration. + HardwareKeyboard.instance + ..removeHandler(_observeKeyUp) + ..addHandler(_observeKeyUp); + _suppressed = true; + } + + bool _observeKeyUp(KeyEvent event) { + if (_suppressed && event is KeyUpEvent && _keyMatcher(event.logicalKey)) { + // The hardware phase runs before the same event's focus dispatch; + // clearing in a microtask keeps the armed state visible to the KeyUp's + // focus-phase consumers while ending it before any later event. + scheduleMicrotask(clearSuppression); + } + return false; + } void clearSuppression() => _suppressed = false; @@ -88,11 +118,9 @@ class _KeyUpSuppressor { /// category and suppression is active. Clears suppression on [KeyUpEvent]. bool consumeIfSuppressed(KeyEvent event) { if (!_suppressed) return false; - if (_keyMatcher(event.logicalKey)) { - if (event is KeyUpEvent) _suppressed = false; - return true; - } - return false; + if (!_keyMatcher(event.logicalKey)) return false; + if (event is KeyUpEvent) _suppressed = false; + return true; } } diff --git a/lib/focus/focusable_action_bar.dart b/lib/focus/focusable_action_bar.dart index 9692888e6..b52bb52c8 100644 --- a/lib/focus/focusable_action_bar.dart +++ b/lib/focus/focusable_action_bar.dart @@ -130,8 +130,7 @@ class FocusableActionBarState extends State { void didUpdateWidget(FocusableActionBar oldWidget) { super.didUpdateWidget(oldWidget); if (_shouldRebuildFocusNodes(oldWidget)) { - _disposeNodes(); - _initNodes(); + _rebindNodes(oldWidget.actions); } } @@ -144,31 +143,120 @@ class FocusableActionBarState extends State { return false; } - void _initNodes() { - _focusBindings = []; - _focusNodes = []; - _focusStates = List.filled(widget.actions.length, false); - for (var i = 0; i < widget.actions.length; i++) { - final index = i; - final binding = OwnedFocusNodeBinding(); - binding.bind( - externalNode: widget.actions[i].focusNode, - debugLabel: widget.actions[i].debugLabel ?? 'ActionBar[$i]', - listener: () { - final hasFocus = _focusNodes[index].hasFocus; - if (_focusStates[index] != hasFocus) { - setState(() => _focusStates[index] = hasFocus); - } - _notifyRowFocusIfChanged(); - }, - ); - _focusBindings.add(binding); - _focusNodes.add(binding.node); - _focusStates[i] = binding.node.hasFocus; + /// Stable identity for matching an action across list changes: the supplied + /// [FocusableAction.focusNode] first, else [FocusableAction.debugLabel]. + /// Unlabeled actions have no identity and only ever match positionally. + static Object? _actionIdentity(FocusableAction action) => action.focusNode ?? action.debugLabel; + + /// Whether the action-list shape is unchanged: same length and every + /// labeled identity present in both lists sits at the index it had. Only + /// then does an unlabeled action's position provably still refer to the + /// same conceptual action. + bool _unlabeledPositionsStable(List oldActions) { + if (oldActions.length != widget.actions.length) return false; + for (var i = 0; i < oldActions.length; i++) { + final identity = _actionIdentity(oldActions[i]); + if (identity == null) continue; + for (var j = 0; j < widget.actions.length; j++) { + if (j != i && _actionIdentity(widget.actions[j]) == identity) return false; + } } + return true; + } + + OwnedFocusNodeBinding _createBinding(FocusableAction action, int index) { + final binding = OwnedFocusNodeBinding(); + binding.bind( + externalNode: action.focusNode, + debugLabel: action.debugLabel ?? 'ActionBar[$index]', + listener: () => _handleBindingFocusChange(binding), + ); + return binding; + } + + /// Index resolved at call time: a binding survives action-list changes, so + /// a listener must not capture the slot it was created for. + void _handleBindingFocusChange(OwnedFocusNodeBinding binding) { + final index = _focusBindings.indexOf(binding); + if (index != -1) { + final hasFocus = binding.node.hasFocus; + if (_focusStates[index] != hasFocus) { + setState(() => _focusStates[index] = hasFocus); + } + } + _notifyRowFocusIfChanged(); + } + + void _initNodes() { + _focusBindings = [for (var i = 0; i < widget.actions.length; i++) _createBinding(widget.actions[i], i)]; + _focusNodes = [for (final binding in _focusBindings) binding.node]; + _focusStates = [for (final node in _focusNodes) node.hasFocus]; _hasAnyFocus = _focusNodes.any((node) => node.hasFocus); } + /// Rebind on an action-list change, reusing the binding of every action + /// whose identity survives so its focus node is not disposed out from under + /// the user (media detail's watchlist action arrives asynchronously and + /// used to drop D-pad focus with the wholesale rebuild). A removed focused + /// action is disposed with its listener already detached, so the genuine + /// row-focus loss is reported through [_notifyRowFocusIfChanged] here. + void _rebindNodes(List oldActions) { + final oldBindings = _focusBindings; + final focusedOldIndex = _focusNodes.indexWhere((node) => node.hasFocus); + + final claimed = List.filled(oldBindings.length, false); + final unlabeledPositionsStable = _unlabeledPositionsStable(oldActions); + int? matchOldIndex(int newIndex) { + final identity = _actionIdentity(widget.actions[newIndex]); + if (identity != null) { + for (var i = 0; i < oldActions.length; i++) { + if (!claimed[i] && _actionIdentity(oldActions[i]) == identity) return i; + } + return null; + } + // Positional reuse is the only option for an unlabeled action, but it + // is only provably correct while the list shape is unchanged; across an + // insertion/removal its successor is unknowable, so fall through to a + // fresh binding — genuinely dropping focus beats silently retargeting + // the focused binding (and its next Select) onto a different action. + if (unlabeledPositionsStable && !claimed[newIndex] && _actionIdentity(oldActions[newIndex]) == null) { + return newIndex; + } + return null; + } + + var focusedNewIndex = -1; + final newBindings = []; + for (var i = 0; i < widget.actions.length; i++) { + final oldIndex = matchOldIndex(i); + if (oldIndex == null) { + newBindings.add(_createBinding(widget.actions[i], i)); + continue; + } + claimed[oldIndex] = true; + if (oldIndex == focusedOldIndex) focusedNewIndex = i; + newBindings.add(oldBindings[oldIndex]); + } + + _focusBindings = newBindings; + _focusNodes = [for (final binding in newBindings) binding.node]; + _focusStates = [for (final node in _focusNodes) node.hasFocus]; + + for (var i = 0; i < oldBindings.length; i++) { + if (!claimed[i]) oldBindings[i].dispose(); + } + + // The focused action survived: keep focus on it. The keyed row keeps its + // Focus element attached across reorders, so this only re-requests focus + // if the framework unfocused the node during widget churn. + if (focusedNewIndex != -1 && + widget.actions[focusedNewIndex].onPressed != null && + !_focusNodes[focusedNewIndex].hasFocus) { + _focusNodes[focusedNewIndex].requestFocus(); + } + _notifyRowFocusIfChanged(); + } + void _notifyRowFocusIfChanged() { final hasAnyFocus = _focusNodes.any((node) => node.hasFocus); if (_hasAnyFocus == hasAnyFocus) return; @@ -223,6 +311,9 @@ class FocusableActionBarState extends State { final customChild = action.builder?.call(context, buildState); return Focus( + // Keyed on the binding so an insertion moves the element with its node + // instead of detaching (and thereby unfocusing) it slot by slot. + key: ObjectKey(_focusBindings[index]), focusNode: _focusNodes[index], canRequestFocus: enabled, autofocus: action.autofocus && enabled, diff --git a/lib/focus/focusable_text_field.dart b/lib/focus/focusable_text_field.dart index ecf5a84d7..a5d2cba54 100644 --- a/lib/focus/focusable_text_field.dart +++ b/lib/focus/focusable_text_field.dart @@ -227,9 +227,30 @@ KeyEventResult _handleInputKey({ if (result != KeyEventResult.ignored) return finish(result, 'custom-tv-hardware-keyboard'); } - if (onBack != null && key.isBackKey) { - if (event is KeyDownEvent) onBack(); - return finish(KeyEventResult.handled, 'onBack'); + if (onBack != null && event.logicalKey.isBackKey) { + // On TV the native text-input path can swallow the matching KeyUp (the + // closing IME session eats it), so back fires on KeyDown — the same + // down-only shape as [handleBackKeyAction]'s Apple TV branch, coordinator + // mark included so a parallel back dispatch still dedupes. Elsewhere the + // shared handler's KeyUp semantics apply. + if (PlatformDetector.isTV()) { + if (BackKeyUpSuppressor.consumeIfSuppressed(event)) return finish(KeyEventResult.handled, 'onBack'); + if (event is KeyDownEvent) { + BackKeyCoordinator.markHandled(); + onBack(); + // onBack may move focus (empty search field -> sidebar); the matching + // KeyUp is then delivered to the NEW focus chain, whose shared + // handlers act on KeyUp — a second back action. Arm the suppressor + // (after onBack, so a modal opened by it cannot clear the arming) so + // whichever chain receives the KeyUp swallows it. This cannot pin: the + // suppressor's hardware observer clears the armed state once the + // physical press ends — see _KeyUpSuppressor. + BackKeyUpSuppressor.suppressBackUntilKeyUp(); + } + return finish(KeyEventResult.handled, 'onBack'); + } + final backResult = handleBackKeyAction(event, onBack); + if (backResult != KeyEventResult.ignored) return finish(backResult, 'onBack'); } // Enter/numpad enter are left to TextField.onSubmitted. Handle only diff --git a/lib/focus/key_event_utils.dart b/lib/focus/key_event_utils.dart index e9e0bd47d..bc3032264 100644 --- a/lib/focus/key_event_utils.dart +++ b/lib/focus/key_event_utils.dart @@ -76,11 +76,10 @@ KeyEventResult handleBackKeyAction(KeyEvent event, VoidCallback onBack) { // AppleTV back (Siri Remote Menu via engine-synthesized escape): run onBack // on KeyDown only; consume KeyUp silently. Some engine paths report Menu as // a non-keyboard device, but the same down-only handling is still required. - // The suppressor-based "arm-on-KeyDown, clear-on-KeyUp" pattern leaks here - // because onBack typically calls Navigator.pop, swapping the focus tree - // before the matching KeyUp is dispatched — the orphaned KeyUp then never - // reaches a consumeIfSuppressed call, pinning the suppressor armed and - // silently swallowing the next press's KeyDown. + // No suppressor arming is needed here: onBack typically calls + // Navigator.pop, swapping the focus tree before the matching KeyUp is + // dispatched — but the orphaned KeyUp lands on the new chain, where this + // same branch consumes it silently. if (PlatformDetector.isAppleTV()) { if (event is KeyDownEvent) { BackKeyCoordinator.markHandled(); @@ -246,10 +245,10 @@ class BackKeySuppressorObserver extends NavigatorObserver { @override void didPop(Route route, Route? previousRoute) { // On AppleTV, handleBackKeyAction consumes the KeyUp silently regardless, - // so the suppressor isn't needed and arming it would pin state across the - // pop's focus-tree swap. (The atomic engine fix delivers KeyDown+KeyUp in - // a single recognizer Began callback, so didPop fires squarely inside the - // window where BackKeyPressTracker.isBackKeyDown is true.) + // so the suppressor isn't needed. (The atomic engine fix delivers + // KeyDown+KeyUp in a single recognizer Began callback, so didPop fires + // squarely inside the window where BackKeyPressTracker.isBackKeyDown is + // true.) if (PlatformDetector.isAppleTV()) return; if (BackKeyPressTracker.isBackKeyDown) { BackKeyUpSuppressor.suppressBackUntilKeyUp(); diff --git a/lib/screens/catalog_item_detail_screen.dart b/lib/screens/catalog_item_detail_screen.dart index 3aa3f351f..ee7e458f6 100644 --- a/lib/screens/catalog_item_detail_screen.dart +++ b/lib/screens/catalog_item_detail_screen.dart @@ -1282,6 +1282,10 @@ class _CatalogItemDetailScreenState extends State { actions: [ if (_watchlistSource != null) FocusableAction( + // Stable identities so the focused binding survives the + // list-shape changes of async enrichment (watchlist/request + // sources and trailer URL arrive at different times). + debugLabel: 'catalog_watchlist', icon: onWatchlist ?? false ? Symbols.bookmark_added_rounded : Symbols.bookmark_add_rounded, @@ -1292,6 +1296,7 @@ class _CatalogItemDetailScreenState extends State { ), if (_requestSource case final SeerrCatalogSource seerr when tmdbId != null) FocusableAction( + debugLabel: 'catalog_request', icon: Symbols.download_rounded, tooltip: t.seerr.request, onPressed: () => unawaited( @@ -1306,6 +1311,7 @@ class _CatalogItemDetailScreenState extends State { ), if (item.trailerUrl?.trim() case final trailer? when trailer.isNotEmpty) FocusableAction( + debugLabel: 'catalog_trailer', icon: Symbols.play_circle_rounded, tooltip: t.explore.detail.watchTrailer, onPressed: () => unawaited(_openExternalUrl(trailer)), diff --git a/lib/widgets/side_navigation_rail.dart b/lib/widgets/side_navigation_rail.dart index 1cb574f2c..3d766d5d1 100644 --- a/lib/widgets/side_navigation_rail.dart +++ b/lib/widgets/side_navigation_rail.dart @@ -658,6 +658,7 @@ class SideNavigationRailState extends State with MountedSetS required bool hasLiveTv, required bool hasNowPlaying, required bool hasExplore, + required bool isCollapsed, }) { return [ if (widget.isOfflineMode && widget.onReconnect != null) _kReconnect, @@ -665,7 +666,10 @@ class SideNavigationRailState extends State with MountedSetS _kHome, if (hasNowPlaying) _kNowPlaying, _kLibraries, - if (_librariesExpanded) ...[ + // Library rows render inside ExcludeFocus(excluding: !_librariesExpanded + // || isCollapsed); keep the D-pad order in lockstep with that render + // condition so a collapsed rail never targets a focus-excluded row. + if (_librariesExpanded && !isCollapsed) ...[ ..._focusKeysForLibraryRows(visibleRows), if (hasHiddenLibraries) ...[ _kHiddenLibraries, @@ -708,14 +712,19 @@ class SideNavigationRailState extends State with MountedSetS final currentIndex = focusOrder.indexOf(currentKey); if (currentIndex == -1) return KeyEventResult.ignored; - final nextIndex = isDown ? currentIndex + 1 : currentIndex - 1; - if (nextIndex < 0 || nextIndex >= focusOrder.length) return KeyEventResult.handled; - - final nextNode = _focusTracker.nodeFor(focusOrder[nextIndex]); - if (nextNode == null) return KeyEventResult.ignored; - - _requestFocusAndReveal(nextNode); - return KeyEventResult.handled; + // Scan onward past keys whose node is unmounted or focus-excluded (a row + // can outlive its focusability, e.g. under an ExcludeFocus ancestor). + // Returning handled without moving focus would swallow the key and also + // suppress the framework's own directional traversal, so yield ignored + // when no viable candidate remains in the requested direction. + final step = isDown ? 1 : -1; + for (var index = currentIndex + step; index >= 0 && index < focusOrder.length; index += step) { + final node = _focusTracker.nodeFor(focusOrder[index]); + if (node == null || node.context == null || !node.canRequestFocus) continue; + _requestFocusAndReveal(node); + return KeyEventResult.handled; + } + return KeyEventResult.ignored; } /// Collapse the sidebar (resets touch-expand state). @@ -827,6 +836,7 @@ class SideNavigationRailState extends State with MountedSetS hasLiveTv: hasLiveTv, hasNowPlaying: nowPlayingTrack != null, hasExplore: hasExplore, + isCollapsed: isCollapsed, ); _debugAssertUniqueFocusOrder(focusOrder); return TapRegion( diff --git a/test/focus/dpad_select_long_press_controller_test.dart b/test/focus/dpad_select_long_press_controller_test.dart index 384e49216..cd40ca9b3 100644 --- a/test/focus/dpad_select_long_press_controller_test.dart +++ b/test/focus/dpad_select_long_press_controller_test.dart @@ -27,6 +27,10 @@ const _up = KeyUpEvent( ); void main() { + // Long-press fire arms SelectKeyUpSuppressor, whose hardware observer + // registers on HardwareKeyboard.instance - that needs a live binding. + TestWidgetsFlutterBinding.ensureInitialized(); + tearDown(SelectKeyUpSuppressor.clearSuppression); test('initial down starts once and down/repeat events do not restart it', () { diff --git a/test/focus/focusable_action_bar_test.dart b/test/focus/focusable_action_bar_test.dart index 4b8249bbf..b08f3b32f 100644 --- a/test/focus/focusable_action_bar_test.dart +++ b/test/focus/focusable_action_bar_test.dart @@ -1,7 +1,9 @@ import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:material_symbols_icons/symbols.dart'; import 'package:plezy/focus/focusable_action_bar.dart'; +import 'package:plezy/focus/input_mode_tracker.dart'; void main() { Future pumpBar(WidgetTester tester, List actions, {double spacing = 12}) async { @@ -41,4 +43,127 @@ void main() { expect(version.left - play.right, 2); expect(shuffle.left - version.right, 12); }); + + Future pumpTrackedBar(WidgetTester tester, List actions, List rowFocusChanges) async { + await tester.pumpWidget( + InputModeTracker( + child: MaterialApp( + home: Scaffold( + body: Center( + child: FocusableActionBar(actions: actions, onFocusChange: rowFocusChanges.add), + ), + ), + ), + ), + ); + } + + FocusableActionBarState barState(WidgetTester tester) => + tester.state(find.byType(FocusableActionBar)); + + testWidgets('focused action keeps D-pad focus when an action is inserted before it', (tester) async { + // Watchlist-arrival shape: on media detail the action list grows + // asynchronously once tracker candidates resolve, inserting an action + // before the one the user is standing on. That count change used to + // dispose every node and silently drop focus. + final rowFocusChanges = []; + FocusableAction action(String label) => FocusableAction(debugLabel: label, onPressed: () {}); + + await pumpTrackedBar(tester, [action('play'), action('watched')], rowFocusChanges); + + barState(tester).getFocusNode(1)!.requestFocus(); + await tester.pump(); + // Enter keyboard mode; RIGHT is trapped at the row edge so focus stays put. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'watched'); + expect(rowFocusChanges, [true]); + + await pumpTrackedBar(tester, [action('play'), action('watchlist'), action('watched')], rowFocusChanges); + await tester.pump(); + + expect(FocusManager.instance.primaryFocus?.debugLabel, 'watched'); + expect(rowFocusChanges, [true], reason: 'a surviving focused action must not report a row-focus loss'); + + // Index wiring was rebuilt for the new slots: LEFT lands on the insertion. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'watchlist'); + }); + + testWidgets('removing the focused action notifies the host of the row-focus loss', (tester) async { + final rowFocusChanges = []; + FocusableAction action(String label) => FocusableAction(debugLabel: label, onPressed: () {}); + + await pumpTrackedBar(tester, [action('play'), action('watched')], rowFocusChanges); + + barState(tester).getFocusNode(1)!.requestFocus(); + await tester.pump(); + expect(rowFocusChanges, [true]); + + await pumpTrackedBar(tester, [action('play')], rowFocusChanges); + await tester.pump(); + + // The disposed node's listener is gone; the bar itself must report the + // genuine loss so hosts like the TV detail row can unlatch. + expect(rowFocusChanges, [true, false]); + expect(FocusManager.instance.primaryFocus?.debugLabel, isNot('watched')); + }); + + testWidgets('inserting an unlabeled action does not retarget the focused binding onto it', (tester) async { + // Catalog detail's enrichment shape: unlabeled Request appears before a + // focused unlabeled Trailer. Positional reuse across the insertion used + // to hand Trailer's focused binding to Request, so the next Select drove + // the wrong action. + final rowFocusChanges = []; + var requestPresses = 0; + var trailerPresses = 0; + FocusableAction unlabeled(VoidCallback onPressed) => + FocusableAction(icon: Symbols.circle_rounded, onPressed: onPressed); + + final watchlist = unlabeled(() {}); + final trailer = unlabeled(() => trailerPresses++); + await pumpTrackedBar(tester, [watchlist, trailer], rowFocusChanges); + + barState(tester).getFocusNode(1)!.requestFocus(); + await tester.pump(); + expect(rowFocusChanges, [true]); + final trailerNode = barState(tester).getFocusNode(1); + + final request = unlabeled(() => requestPresses++); + await pumpTrackedBar(tester, [watchlist, request, trailer], rowFocusChanges); + await tester.pump(); + + // The shape changed, so the focused binding must not be reused for the + // inserted action; losing focus outright is the correct outcome. + expect(barState(tester).getFocusNode(1), isNot(same(trailerNode))); + expect(barState(tester).getFocusNode(1)!.hasFocus, isFalse); + expect(rowFocusChanges, [true, false]); + + await tester.sendKeyEvent(LogicalKeyboardKey.select); + await tester.pump(); + expect(requestPresses, 0, reason: 'Select after the insertion must not drive the inserted action'); + expect(trailerPresses, 0); + }); + + testWidgets('equal-shape rebuild still preserves an unlabeled action focus positionally', (tester) async { + final rowFocusChanges = []; + FocusableAction unlabeled() => FocusableAction(icon: Symbols.circle_rounded, onPressed: () {}); + FocusableAction labeled(String label) => FocusableAction(debugLabel: label, onPressed: () {}); + + await pumpTrackedBar(tester, [labeled('play'), unlabeled()], rowFocusChanges); + + barState(tester).getFocusNode(1)!.requestFocus(); + await tester.pump(); + final focusedNode = barState(tester).getFocusNode(1); + + // A label change at another slot forces a rebind without changing the + // list shape: the unlabeled slot is provably stable and keeps its node. + await pumpTrackedBar(tester, [labeled('play_version'), unlabeled()], rowFocusChanges); + await tester.pump(); + + expect(barState(tester).getFocusNode(1), same(focusedNode)); + expect(focusedNode!.hasFocus, isTrue); + expect(rowFocusChanges, [true], reason: 'a positionally surviving action must not report a row-focus loss'); + }); } diff --git a/test/focus/focusable_text_field_test.dart b/test/focus/focusable_text_field_test.dart index 44136cbcc..3ab54e5b8 100644 --- a/test/focus/focusable_text_field_test.dart +++ b/test/focus/focusable_text_field_test.dart @@ -2,6 +2,8 @@ import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:plezy/focus/focusable_text_field.dart'; +import 'package:plezy/focus/input_mode_tracker.dart'; +import 'package:plezy/focus/key_event_utils.dart'; void main() { testWidgets('unwired single-line fields traverse with arrow keys', (tester) async { @@ -91,4 +93,49 @@ void main() { await tester.pump(); expect(FocusManager.instance.primaryFocus?.debugLabel, 'multiline'); }); + + testWidgets('back on a field with onBack fires once on key up and marks the frame handled', (tester) async { + // The field must route back through the shared handler like every other + // focusable: consume KeyDown, run onBack on KeyUp, and mark the frame so + // a parallel back dispatch (the layer focus lands on next) dedupes. + addTearDown(BackKeyCoordinator.clear); + final node = FocusNode(debugLabel: 'field'); + addTearDown(node.dispose); + final controller = TextEditingController(); + addTearDown(controller.dispose); + var backs = 0; + + await tester.pumpWidget( + InputModeTracker( + child: MaterialApp( + home: Scaffold( + body: FocusableTextField( + controller: controller, + focusNode: node, + onBack: () => backs++, + tvTextInputPresentation: TvTextInputPresentation.platform, + ), + ), + ), + ), + ); + await tester.pump(); + + node.requestFocus(); + await tester.pump(); + // Enter keyboard mode; the sole field has no neighbour so focus stays put. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); + await tester.pump(); + + await tester.sendKeyDownEvent(LogicalKeyboardKey.escape); + await tester.pump(); + expect(backs, 0, reason: 'KeyDown is consumed without firing so a pop cannot double-run on KeyUp'); + + await tester.sendKeyUpEvent(LogicalKeyboardKey.escape); + expect(backs, 1); + // Same-frame dedupe mark: assert before pumping, which runs the + // coordinator's post-frame clear. + expect(BackKeyCoordinator.consumeIfHandled(), isTrue); + await tester.pump(); + }); } diff --git a/test/screens/catalog_item_detail_screen_test.dart b/test/screens/catalog_item_detail_screen_test.dart index e0426a598..7e586af29 100644 --- a/test/screens/catalog_item_detail_screen_test.dart +++ b/test/screens/catalog_item_detail_screen_test.dart @@ -1056,7 +1056,7 @@ void main() { final source = _FakeCatalogSource(detail: const CatalogDetail(item: item)); await _pumpDetail(tester, source, item: item); - expect(FocusManager.instance.primaryFocus?.debugLabel, 'ActionBar[0]'); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'catalog_watchlist'); await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); await tester.pumpAndSettle(); @@ -1078,7 +1078,7 @@ void main() { addTearDown(tester.view.resetPhysicalSize); await _pumpDetail(tester, _FakeCatalogSource()); - expect(FocusManager.instance.primaryFocus?.debugLabel, 'ActionBar[0]'); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'catalog_watchlist'); await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); await tester.pumpAndSettle(); @@ -1098,7 +1098,7 @@ void main() { await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp); await tester.pumpAndSettle(); - expect(FocusManager.instance.primaryFocus?.debugLabel, 'ActionBar[0]'); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'catalog_watchlist'); expect(tester.widget(find.byKey(const Key('catalog_detail_scroll'))).controller!.offset, 0); }); @@ -1147,12 +1147,12 @@ void main() { final source = _FakeCatalogSource(watchlistLoading: true); await _pumpDetail(tester, source); - expect(FocusManager.instance.primaryFocus?.debugLabel, 'ActionBar[0]'); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'catalog_watchlist'); final actionNode = tester .widgetList(find.descendant(of: find.byType(FocusableActionBar), matching: find.byType(Focus))) .map((widget) => widget.focusNode) .whereType() - .singleWhere((node) => node.debugLabel == 'ActionBar[0]'); + .singleWhere((node) => node.debugLabel == 'catalog_watchlist'); expect(actionNode.canRequestFocus, isTrue); await tester.sendKeyEvent(LogicalKeyboardKey.select); diff --git a/test/widgets/focusable_text_field_test.dart b/test/widgets/focusable_text_field_test.dart index a8ff77c07..2dc955411 100644 --- a/test/widgets/focusable_text_field_test.dart +++ b/test/widgets/focusable_text_field_test.dart @@ -4,7 +4,9 @@ import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:material_symbols_icons/symbols.dart'; +import 'package:plezy/focus/dpad_navigator.dart'; import 'package:plezy/focus/focusable_text_field.dart'; +import 'package:plezy/focus/key_event_utils.dart'; import 'package:plezy/services/gamepad_service.dart'; import 'package:plezy/utils/platform_detector.dart'; @@ -13,6 +15,11 @@ void main() { TvDetectionService.debugSetAppleTVOverride(null); TvDetectionService.setForceTVSync(false); GamepadService.debugNativeTextInputFocusHandler = null; + // TV back tests arm the suppressor on KeyDown; a test ending mid-press + // would leak the armed state into the next test (flutter_test clears the + // hardware observer that would otherwise end it with the press). + BackKeyUpSuppressor.clearSuppression(); + BackKeyCoordinator.clear(); }); testWidgets('tab traversal focuses the text form field', (tester) async { @@ -1050,6 +1057,132 @@ void main() { expect(find.byType(Dialog), findsNothing); }); + testWidgets('Android TV back that moves focus off the field cannot double-fire on the orphaned KeyUp', ( + tester, + ) async { + TvDetectionService.debugSetAppleTVOverride(null); + await TvDetectionService.getInstance(forceTv: true); + TvDetectionService.setForceTVSync(true); + addTearDown(BackKeyCoordinator.clear); + addTearDown(BackKeyUpSuppressor.clearSuppression); + final controller = TextEditingController(); + final fieldFocusNode = FocusNode(debugLabel: 'search_field'); + final sidebarFocusNode = FocusNode(debugLabel: 'sidebar'); + var fieldBacks = 0; + var sidebarBacks = 0; + addTearDown(controller.dispose); + addTearDown(fieldFocusNode.dispose); + addTearDown(sidebarFocusNode.dispose); + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Column( + children: [ + FocusableTextFormField( + controller: controller, + focusNode: fieldFocusNode, + tvTextInputPresentation: TvTextInputPresentation.platform, + tvTextInputAutoOpenBehavior: TvTextInputAutoOpenBehavior.never, + onBack: () { + fieldBacks++; + sidebarFocusNode.requestFocus(); + }, + ), + // The destination focus chain runs the shared handler's KeyUp + // semantics — the SearchScreen sidebar shape. + Focus( + focusNode: sidebarFocusNode, + onKeyEvent: (node, event) => handleBackKeyAction(event, () => sidebarBacks++), + child: const SizedBox(width: 40, height: 40), + ), + ], + ), + ), + ), + ); + + fieldFocusNode.requestFocus(); + await tester.pump(); + + // TV back acts on KeyDown; onBack leaves the empty field for the sidebar. + await tester.sendKeyDownEvent(LogicalKeyboardKey.escape); + await tester.pump(); + expect(fieldBacks, 1); + expect(sidebarFocusNode.hasPrimaryFocus, isTrue); + + // The matching KeyUp is delivered to the NEW focus chain and must be + // swallowed there — one press, one action. + await tester.sendKeyUpEvent(LogicalKeyboardKey.escape); + await tester.pump(); + expect(sidebarBacks, 0, reason: 'the KeyUp of a press the field already handled must not run a second back'); + + // Anti-pinning: a full follow-up press works normally on the sidebar. + await tester.sendKeyDownEvent(LogicalKeyboardKey.escape); + await tester.sendKeyUpEvent(LogicalKeyboardKey.escape); + await tester.pump(); + expect(sidebarBacks, 1); + expect(fieldBacks, 1); + }); + + testWidgets('Android TV stale back suppression never swallows the next press', (tester) async { + TvDetectionService.debugSetAppleTVOverride(null); + await TvDetectionService.getInstance(forceTv: true); + TvDetectionService.setForceTVSync(true); + addTearDown(BackKeyCoordinator.clear); + addTearDown(BackKeyUpSuppressor.clearSuppression); + final controller = TextEditingController(); + final fieldFocusNode = FocusNode(debugLabel: 'search_field'); + final buttonFocusNode = FocusNode(debugLabel: 'plain_button'); + var fieldBacks = 0; + addTearDown(controller.dispose); + addTearDown(fieldFocusNode.dispose); + addTearDown(buttonFocusNode.dispose); + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Column( + children: [ + FocusableTextFormField( + controller: controller, + focusNode: fieldFocusNode, + tvTextInputPresentation: TvTextInputPresentation.platform, + tvTextInputAutoOpenBehavior: TvTextInputAutoOpenBehavior.never, + onBack: () { + fieldBacks++; + buttonFocusNode.requestFocus(); + }, + ), + // No suppressor-aware handler here: the armed state goes stale + // because the KeyUp is never consumed anywhere. + FilledButton(focusNode: buttonFocusNode, onPressed: () {}, child: const Text('Plain')), + ], + ), + ), + ), + ); + + fieldFocusNode.requestFocus(); + await tester.pump(); + + await tester.sendKeyDownEvent(LogicalKeyboardKey.escape); + await tester.pump(); + expect(fieldBacks, 1); + await tester.sendKeyUpEvent(LogicalKeyboardKey.escape); + await tester.pump(); + + // The stale armed suppressor must clear itself on the next KeyDown + // without consuming it — the second press still runs onBack. + fieldFocusNode.requestFocus(); + await tester.pump(); + await tester.sendKeyDownEvent(LogicalKeyboardKey.escape); + await tester.pump(); + expect(fieldBacks, 2, reason: 'a stray armed suppressor must never swallow a fresh press'); + await tester.sendKeyUpEvent(LogicalKeyboardKey.escape); + await tester.pump(); + }); + testWidgets('Android TV platform focus hint tracks activation, not focus', (tester) async { TvDetectionService.debugSetAppleTVOverride(null); await TvDetectionService.getInstance(forceTv: true); diff --git a/test/widgets/side_navigation_rail_test.dart b/test/widgets/side_navigation_rail_test.dart index f8816dabc..a5397bb83 100644 --- a/test/widgets/side_navigation_rail_test.dart +++ b/test/widgets/side_navigation_rail_test.dart @@ -437,6 +437,70 @@ void main() { expect(selectedTab, NavigationTabId.search); }); + testWidgets('collapsed rail with expanded Libraries skips focus-excluded library rows', (tester) async { + // The library rows render under ExcludeFocus while the rail is collapsed + // even though the Libraries section pref is expanded; targeting one used + // to swallow DOWN forever, cutting off everything below the header. + await SettingsService.getInstance(); + await SettingsService.instance.write(SettingsService.librariesSectionExpanded, true); + + final movies = _library(id: '1', title: 'Movies', serverId: ServerId('server-a'), serverName: 'Server A'); + + final librariesProvider = LibrariesProvider(); + await librariesProvider.updateLibraryOrder([movies]); + addTearDown(librariesProvider.dispose); + + final hiddenLibrariesProvider = HiddenLibrariesProvider(); + await hiddenLibrariesProvider.ensureInitialized(); + addTearDown(hiddenLibrariesProvider.dispose); + + final manager = MultiServerManager(); + final multiServerProvider = testMultiServerProvider(manager); + addTearDown(multiServerProvider.dispose); + + final sideNavKey = GlobalKey(); + NavigationTabId? selectedTab; + + await tester.pumpWidget( + TranslationProvider( + child: MultiProvider( + providers: [ + ChangeNotifierProvider.value(value: librariesProvider), + ChangeNotifierProvider.value(value: hiddenLibrariesProvider), + ChangeNotifierProvider.value(value: multiServerProvider), + ], + child: InputModeTracker( + child: MaterialApp( + theme: ThemeData(extensions: const [testMonoTokens]), + home: Scaffold( + body: SideNavigationRail( + key: sideNavKey, + selectedTab: NavigationTabId.discover, + isSidebarFocused: false, + alwaysExpanded: false, + onDestinationSelected: (tab) => selectedTab = tab, + onLibrarySelected: (_) {}, + ), + ), + ), + ), + ), + ), + ); + await tester.pumpAndSettle(); + + sideNavKey.currentState!.focusActiveItem(); + await tester.pumpAndSettle(); + + // Home -> Libraries -> Search. The Movies row stays out of D-pad order + // while the rail is collapsed, so DOWN lands on the next real row. + await _press(tester, LogicalKeyboardKey.arrowDown); + await _press(tester, LogicalKeyboardKey.arrowDown); + await _press(tester, LogicalKeyboardKey.enter); + + expect(selectedTab, NavigationTabId.search); + }); + testWidgets('hover expands the rail as an overlay and collapses on exit', (tester) async { await SettingsService.getInstance();