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();