diff --git a/lib/focus/dpad_navigator.dart b/lib/focus/dpad_navigator.dart index ddcdc589..64877fcf 100644 --- a/lib/focus/dpad_navigator.dart +++ b/lib/focus/dpad_navigator.dart @@ -53,7 +53,15 @@ extension DpadKeyExtension on LogicalKeyboardKey { bool get isBackKey => _backKeys.contains(this); bool get isContextMenuKey => _contextMenuKeys.contains(this); - bool get isNavigationKey => + /// Whether this key is a shell / remote control key rather than a text + /// character — D-pad direction, select, back, context menu, or Tab. + /// + /// Use it to decide "is this a printable character?" and "must this route + /// consume the key so it cannot leak to the route below?". It is NOT evidence + /// that the viewer wants to navigate by focus — `eventRequestsFocusNavigation` + /// in focus_navigation_intent.dart answers that, and conflating the two is what + /// made a plain Enter switch the whole app into keyboard mode. + bool get isReservedControlKey => isDpadDirection || isSelectKey || isBackKey || isContextMenuKey || this == LogicalKeyboardKey.tab; bool get isLeftKey => this == LogicalKeyboardKey.arrowLeft; diff --git a/lib/focus/focus_navigation_intent.dart b/lib/focus/focus_navigation_intent.dart new file mode 100644 index 00000000..fde05253 --- /dev/null +++ b/lib/focus/focus_navigation_intent.dart @@ -0,0 +1,85 @@ +import 'package:flutter/services.dart'; +import 'package:flutter/widgets.dart'; + +import '../utils/platform_detector.dart'; +import 'dpad_navigator.dart'; + +/// A [FocusNode] whose owner consumes arrow / D-pad keys itself instead of +/// letting them traverse focus — the video player seeks with them. +/// +/// Focus-mode detection needs this fact: an arrow pressed while such a node +/// holds focus is not evidence that the viewer wants to navigate by focus, so +/// it must not switch the app into keyboard mode and light up focus chrome +/// everywhere. +/// +/// The fact rides on the node rather than on a subtree because it depends on +/// *what has focus*, not on where a widget sits. Every ordinary control, sheet, +/// prompt and OSD button therefore stays a plain [FocusNode] and keeps working. +/// +/// Declare a node only when it can hold primary focus **without the viewer +/// having navigated to it** — autofocus, a focus reclaim, a self-heal. +/// Everything reached by traversal is already in keyboard mode, so the marker +/// would be redundant there: sliders, spinners, the TV keyboard and the OSD's +/// own buttons all consume arrows and all correctly stay plain nodes. +class DirectionalShortcutFocusNode extends FocusNode { + DirectionalShortcutFocusNode({required this.consumesDirectionalKeys, super.debugLabel, super.skipTraversal}); + + /// Evaluated per key press, so live settings and chrome state need no + /// syncing. Takes the key because a feature can own one axis and not the + /// other — the player seeks with Left/Right while Up/Down raises the chrome. + final bool Function(LogicalKeyboardKey key) consumesDirectionalKeys; + + static bool ownsDirectionalKeys(FocusNode? node, LogicalKeyboardKey key) => + node is DirectionalShortcutFocusNode && node.consumesDirectionalKeys(key); +} + +/// Whether [event] is evidence that the viewer wants to navigate by focus. +/// +/// This is the single answer to two questions that must never disagree: +/// whether [InputModeTracker] switches to keyboard mode, and whether a key may +/// hand focus to the video player's chrome. Deciding them separately is what +/// produced focus appearing on a control while the app still believed a pointer +/// was driving — focus chrome is mode-gated, so the viewer got an invisible +/// selection. +/// +/// Activation (Enter) and dismissal (Escape) from a physical keyboard are *not* +/// navigation: they act on whatever already has focus. Promoting on them arms +/// focus chrome — and hides the desktop cursor — for a viewer who only pressed +/// play. +/// +/// [focused] is the node that will receive the event; it defaults to the +/// primary focus. Handlers inside a `Focus.onKeyEvent` pass their own node +/// instead of re-reading the global. +bool eventRequestsFocusNavigation(KeyEvent event, {FocusNode? focused}) { + if (event is! KeyDownEvent) return false; + final key = event.logicalKey; + + // Unambiguous traversal on every platform. + if (key == LogicalKeyboardKey.tab) return true; + + // Opens a menu that takes focus into a new scope, so it does start a session. + if (key.isContextMenuKey) return true; + + // `select` / `gameButtonA` are remote-only whatever the engine claims about + // deviceType; `enter` counts from a non-keyboard device, or on TV where a + // remote's OK legitimately arrives as a keyboard `enter` (the same shape + // pin_entry_dialog.dart already compensates for). + if (key.isSelectKey) { + return event.isTvSelectEvent || (PlatformDetector.isTV() && event.isPhysicalKeyboardEnter); + } + + // Remote BACK proves a pointerless device. A physical keyboard's `escape` and + // a media keyboard's `browserBack` only dismiss, so they stay out — matching + // how classifyPlayerNavigationKey already reads a non-keyboard `escape`. + if (key.isBackKey) { + return key == LogicalKeyboardKey.goBack || key == LogicalKeyboardKey.gameButtonB || !event.isPhysicalKeyboardEvent; + } + + // Arrows are navigation only where they are not the focused feature's own + // shortcut. + if (key.isDpadDirection) { + return !DirectionalShortcutFocusNode.ownsDirectionalKeys(focused ?? FocusManager.instance.primaryFocus, key); + } + + return false; +} diff --git a/lib/focus/focusable_text_field.dart b/lib/focus/focusable_text_field.dart index 02ca5d01..1b4b951e 100644 --- a/lib/focus/focusable_text_field.dart +++ b/lib/focus/focusable_text_field.dart @@ -393,7 +393,7 @@ KeyEventResult _handleTvHardwareKeyboardKey({ } final character = event.character; - if (character != null && character.isNotEmpty && !key.isNavigationKey && !_isControlCharacter(character)) { + if (character != null && character.isNotEmpty && !key.isReservedControlKey && !_isControlCharacter(character)) { _insertText( controller: controller, text: character, diff --git a/lib/focus/input_mode_tracker.dart b/lib/focus/input_mode_tracker.dart index 85ab29cb..5a8beb5f 100644 --- a/lib/focus/input_mode_tracker.dart +++ b/lib/focus/input_mode_tracker.dart @@ -4,9 +4,8 @@ import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import '../utils/platform_detector.dart'; -import '../services/gamepad_service.dart'; import 'dpad_navigator.dart'; -import '../services/companion_remote/companion_remote_receiver.dart'; +import 'focus_navigation_intent.dart'; /// Tracks whether the user is navigating via keyboard/d-pad or pointer (mouse/touch). /// @@ -55,6 +54,21 @@ class InputModeTracker extends StatefulWidget { return Platform.isAndroid && isKeyboardMode(context); } + /// Report input from a device that cannot point — gamepad, companion remote, + /// Siri Remote touch. Those services synthesize their key events through + /// [KeyEventSimulatorController], which dispatches straight down the focus + /// chain and never reaches [HardwareKeyboard], so they cannot be observed by + /// [eventRequestsFocusNavigation] and must announce themselves here. + /// + /// Calls `setState`, so never call this during build. A no-op when no tracker + /// is mounted. + static void reportNonPointerInput() { + final state = _instance; + if (state != null && state.mounted) state._setMode(InputMode.keyboard); + } + + static _InputModeTrackerState? _instance; + @override State createState() => _InputModeTrackerState(); } @@ -66,23 +80,21 @@ class _InputModeTrackerState extends State { @override void initState() { super.initState(); + // Published before anything can report input. The outgoing tracker of a + // subtree swap disposes *after* the incoming one initialises, so teardown + // is identity-guarded — otherwise startup's bootstrap→app swap would leave + // the live registration cleared. + InputModeTracker._instance = this; // Initialize focus highlight strategy based on starting mode _updateFocusHighlightStrategy(_mode); // Listen to hardware keyboard events globally HardwareKeyboard.instance.addHandler(_handleKeyEvent); - - // Register callback for gamepad input to switch to keyboard mode - GamepadService.onGamepadInput = () => _setMode(InputMode.keyboard); - - // Register callback for companion remote input to switch to keyboard mode - CompanionRemoteReceiver.onRemoteInput = () => _setMode(InputMode.keyboard); } @override void dispose() { HardwareKeyboard.instance.removeHandler(_handleKeyEvent); - GamepadService.onGamepadInput = null; - CompanionRemoteReceiver.onRemoteInput = null; + if (identical(InputModeTracker._instance, this)) InputModeTracker._instance = null; super.dispose(); } @@ -91,9 +103,10 @@ class _InputModeTrackerState extends State { // events after route pops (see BackKeySuppressorObserver). BackKeyPressTracker.handleKeyEvent(event); - // Only switch to keyboard mode on navigation key down (not repeats, releases, - // or non-navigation keys like volume buttons or letter keys while typing) - if (event is KeyDownEvent && event.logicalKey.isNavigationKey) { + // Only a key that asks to navigate by focus starts a keyboard session. + // Activation and dismissal act on what is already focused, so promoting on + // them would arm focus chrome for a viewer who never asked to navigate. + if (eventRequestsFocusNavigation(event)) { _setMode(InputMode.keyboard); } // Return false to let the event continue propagating diff --git a/lib/screens/video_player/parts/playback_prompts.dart b/lib/screens/video_player/parts/playback_prompts.dart index 85a0b48c..068b2285 100644 --- a/lib/screens/video_player/parts/playback_prompts.dart +++ b/lib/screens/video_player/parts/playback_prompts.dart @@ -82,7 +82,7 @@ extension _VideoPlayerPlaybackPromptMethods on VideoPlayerScreenState { } // Capture keyboard mode before async gap - final isKeyboardMode = PlatformDetector.isTV() && InputModeTracker.isKeyboardMode(context); + final isKeyboardMode = PlatformDetector.isTV() && InputModeTracker.isKeyboardMode(context, listen: false); final settings = await SettingsService.getInstance(); if (!mounted) return; @@ -171,7 +171,7 @@ extension _VideoPlayerPlaybackPromptMethods on VideoPlayerScreenState { // Don't show if auto-play dialog is already visible if (_showPlayNextDialog) return; - final isKeyboardMode = PlatformDetector.isTV() && InputModeTracker.isKeyboardMode(context); + final isKeyboardMode = PlatformDetector.isTV() && InputModeTracker.isKeyboardMode(context, listen: false); _setPlayerState(() { _showStillWatchingPrompt = true; diff --git a/lib/screens/video_player_screen.dart b/lib/screens/video_player_screen.dart index 31cc9268..e5a94bff 100644 --- a/lib/screens/video_player_screen.dart +++ b/lib/screens/video_player_screen.dart @@ -102,6 +102,7 @@ import '../widgets/video_controls/widgets/player_toast_indicator.dart'; import '../focus/focusable_button.dart'; import '../focus/input_mode_tracker.dart'; import '../focus/dpad_navigator.dart'; +import '../focus/focus_navigation_intent.dart'; import '../focus/key_event_utils.dart'; import '../i18n/strings.g.dart'; import '../watch_together/providers/watch_together_provider.dart'; @@ -571,9 +572,6 @@ class VideoPlayerScreenState extends State with WidgetsBindin final PlayerToastController _toastController = PlayerToastController(); bool _reclaimingFocus = false; - // Cached setting: when false on Windows/Linux, ESC should not exit the player - bool _videoPlayerNavigationEnabled = false; - // App lifecycle state tracking bool _wasPlayingBeforeInactive = false; bool _hiddenForBackground = false; @@ -865,7 +863,7 @@ class VideoPlayerScreenState extends State with WidgetsBindin // Escape is plain Back (#1624). physicalEscapeExitsFullscreen: () => shouldPhysicalEscapeExitFullscreen( isMacOS: Platform.isMacOS, - videoPlayerNavigationEnabled: _videoPlayerNavigationEnabled, + videoPlayerNavigationEnabled: videoPlayerNavigationPreference(), playerEnteredFullscreen: FullscreenStateManager().scopeOwnsFullscreen, ), exitPlayer: () => unawaited(_handleBackButton()), @@ -906,7 +904,7 @@ class VideoPlayerScreenState extends State with WidgetsBindin // Screen-level focus node that wraps the entire build output. // Ensures a single stable focus target across loading → initialized phases. - _screenFocusNode = FocusNode(debugLabel: 'VideoPlayerScreen'); + _screenFocusNode = playerSurfaceFocusNode('VideoPlayerScreen'); _screenFocusNode.addListener(_onScreenFocusChanged); HardwareKeyboard.instance.addHandler(_primeInitializationNavigationFocus); @@ -1188,7 +1186,6 @@ class VideoPlayerScreenState extends State with WidgetsBindin initPhase = 'loading settings'; final settingsService = await SettingsService.getInstance(); if (!_isPlayerInitializationCurrent(generation)) return; - _videoPlayerNavigationEnabled = settingsService.read(SettingsService.videoPlayerNavigationEnabled); _autoPipEnabled = settingsService.read(SettingsService.autoPip); _exitFullscreenOnPlayerClose = settingsService.read(SettingsService.exitFullscreenOnPlayerClose); _rewindOnResume = settingsService.read(SettingsService.rewindOnResume); @@ -2196,25 +2193,33 @@ class VideoPlayerScreenState extends State with WidgetsBindin // The chrome deliberately stays down; _remoteTransport announces the // accepted command with a centred transient disc instead (#1676). final transportCommand = classifyTransportKey(event.logicalKey); - if (_videoPlayerNavigationEnabled && !PlatformDetector.isAppleTV() && transportCommand != null) { + if (videoPlayerNavigationPreference() && !PlatformDetector.isAppleTV() && transportCommand != null) { if (event is KeyDownEvent) { unawaited(_remoteTransport(transportCommand, source: 'Hardware media key')); } return KeyEventResult.handled; // consume down, repeat, and up } // Self-heal: if this node itself has primary focus (no descendant - // focused, e.g. after controls auto-hide), redirect to first descendant. - // Arrows stay playback shortcuts on desktop unless the viewer opted into - // player navigation; only Tab/select may deliberately pull focus into - // the OSD (#1797). Consuming navigation keys either way keeps them from - // leaking to the route below. + // focused, e.g. during loading or after a window re-activation), + // redirect to the first descendant. Arrows stay playback shortcuts on + // desktop unless the viewer opted into player navigation; only Tab and + // a remote's OK deliberately pull focus into the OSD (#1797). Consuming + // reserved control keys either way keeps them from leaking to the route + // below. if (node.hasPrimaryFocus) { - final claimsChrome = - !event.logicalKey.isDpadDirection || _videoPlayerNavigationEnabled || PlatformDetector.isTV(); - if (event.isActionable && claimsChrome) { - _chromeController.show(focusTarget: PlayerChromeFocusTarget.playPause); + if (event.isActionable) { + // One decision drives both halves: the key that hands the chrome + // focus is the same key that switches the app into keyboard mode, + // so focus can never land on a control while focus chrome is still + // suppressed. For an arrow this already answers "did the viewer opt + // into player navigation", because the screen node owns arrows + // exactly while that setting is off. + final navigating = eventRequestsFocusNavigation(event, focused: node); + if (!event.logicalKey.isDpadDirection || navigating) { + _chromeController.show(focusTarget: navigating ? PlayerChromeFocusTarget.playPause : null); + } } - return event.logicalKey.isNavigationKey ? KeyEventResult.handled : KeyEventResult.ignored; + return event.logicalKey.isReservedControlKey ? KeyEventResult.handled : KeyEventResult.ignored; } // A descendant has focus — let events pass through so // DirectionalFocusAction / ActivateAction can process them. diff --git a/lib/services/apple_tv_remote_touch_service.dart b/lib/services/apple_tv_remote_touch_service.dart index 955d7de8..4ee95ee0 100644 --- a/lib/services/apple_tv_remote_touch_service.dart +++ b/lib/services/apple_tv_remote_touch_service.dart @@ -1,8 +1,8 @@ import 'dart:async'; import 'package:flutter/services.dart'; -import 'package:flutter/widgets.dart'; +import '../focus/input_mode_tracker.dart'; import '../utils/app_logger.dart'; import '../utils/key_event_simulator.dart' as key_sim; import 'gamepad_service.dart'; @@ -31,6 +31,10 @@ class AppleTvRemoteTouchService { final VoidCallback _scheduleFrame; final DateTime Function() _now; final GamepadDuplicateInputGuard _duplicateInputGuard; + + /// Announces that a pointerless device produced input. Injected so tests can + /// observe it without a widget tree; defaults to the app-wide tracker. + final void Function() reportNonPointerInput; final StreamController _playPauseController = StreamController.broadcast(); final double swipeThreshold; @@ -53,6 +57,7 @@ class AppleTvRemoteTouchService { VoidCallback? scheduleFrame, DateTime Function()? now, GamepadDuplicateInputGuard? duplicateInputGuard, + this.reportNonPointerInput = InputModeTracker.reportNonPointerInput, Duration duplicateSuppressionWindow = GamepadDuplicateInputGuard.defaultSuppressionWindow, this.swipeThreshold = defaultSwipeThreshold, this.axisSwitchDominanceRatio = defaultAxisSwitchDominanceRatio, @@ -229,7 +234,7 @@ class AppleTvRemoteTouchService { return false; } - _setTraditionalFocusHighlight(); + reportNonPointerInput(); _scheduleFrame(); _log('emit key=${_keyName(logicalKey)} source=$source${detail == null ? '' : ' $detail'}'); _simulateKeyPress(logicalKey); @@ -256,12 +261,6 @@ class AppleTvRemoteTouchService { _nativeKeyHandlerRegistered = false; } - void _setTraditionalFocusHighlight() { - if (FocusManager.instance.highlightStrategy != FocusHighlightStrategy.alwaysTraditional) { - FocusManager.instance.highlightStrategy = FocusHighlightStrategy.alwaysTraditional; - } - } - void _logTouch(String type, Map arguments) { final x = _toDouble(arguments['x']); final y = _toDouble(arguments['y']); diff --git a/lib/services/companion_remote/companion_remote_receiver.dart b/lib/services/companion_remote/companion_remote_receiver.dart index 3006eef9..d7087ebb 100644 --- a/lib/services/companion_remote/companion_remote_receiver.dart +++ b/lib/services/companion_remote/companion_remote_receiver.dart @@ -1,6 +1,7 @@ import 'package:flutter/services.dart'; import 'package:flutter/widgets.dart'; +import '../../focus/input_mode_tracker.dart'; import '../../models/companion_remote/remote_command.dart'; import '../../utils/app_logger.dart'; import '../../utils/key_event_simulator.dart'; @@ -15,10 +16,6 @@ class CompanionRemoteReceiver { return _instance!; } - /// Called on any remote input so InputModeTracker can switch to keyboard mode. - /// Same pattern as [GamepadService.onGamepadInput]. - static VoidCallback? onRemoteInput; - /// Owners prevent a disposed screen from clearing callbacks installed by a /// replacement screen later in the same frame. Object? navigationOwner; @@ -49,10 +46,14 @@ class CompanionRemoteReceiver { void handleCommand(RemoteCommand command, BuildContext? _) { appLogger.d('CompanionRemoteReceiver: Handling command: ${command.type}'); - // Switch to keyboard mode so focus visuals render - onRemoteInput?.call(); - _setTraditionalFocusHighlight(); - scheduleFrameIfIdle(); + // A paired phone cannot point, so any viewer command is evidence of a + // pointerless device. Protocol frames are not viewer input: promoting on the + // periodic ping would flip an idle desktop host into keyboard mode — and hide + // its cursor — on every heartbeat. + if (_isViewerInput(command.type)) { + InputModeTracker.reportNonPointerInput(); + scheduleFrameIfIdle(); + } switch (command.type) { case RemoteCommandType.dpadUp: @@ -140,10 +141,49 @@ class CompanionRemoteReceiver { appLogger.w('CompanionRemoteReceiver: Unhandled command type: ${command.type}'); } } - - void _setTraditionalFocusHighlight() { - if (FocusManager.instance.highlightStrategy != FocusHighlightStrategy.alwaysTraditional) { - FocusManager.instance.highlightStrategy = FocusHighlightStrategy.alwaysTraditional; - } - } } + +/// Exhaustive by design: no default clause, so adding a [RemoteCommandType] +/// is a compile error until someone decides whether it counts as viewer input. +bool _isViewerInput(RemoteCommandType type) => switch (type) { + RemoteCommandType.ping || + RemoteCommandType.pong || + RemoteCommandType.ack || + RemoteCommandType.deviceInfo || + RemoteCommandType.disconnect || + RemoteCommandType.syncState => false, + RemoteCommandType.dpadUp || + RemoteCommandType.dpadDown || + RemoteCommandType.dpadLeft || + RemoteCommandType.dpadRight || + RemoteCommandType.select || + RemoteCommandType.back || + RemoteCommandType.contextMenu || + RemoteCommandType.play || + RemoteCommandType.pause || + RemoteCommandType.playPause || + RemoteCommandType.stop || + RemoteCommandType.seekForward || + RemoteCommandType.seekBackward || + RemoteCommandType.nextTrack || + RemoteCommandType.previousTrack || + RemoteCommandType.skipIntro || + RemoteCommandType.skipCredits || + RemoteCommandType.volumeUp || + RemoteCommandType.volumeDown || + RemoteCommandType.volumeMute || + RemoteCommandType.volumeSet || + RemoteCommandType.tabNext || + RemoteCommandType.tabPrevious || + RemoteCommandType.tabDiscover || + RemoteCommandType.tabLibraries || + RemoteCommandType.tabSearch || + RemoteCommandType.tabDownloads || + RemoteCommandType.tabSettings || + RemoteCommandType.home || + RemoteCommandType.search || + RemoteCommandType.subtitles || + RemoteCommandType.audioTracks || + RemoteCommandType.qualitySettings || + RemoteCommandType.fullscreen => true, +}; diff --git a/lib/services/gamepad_service.dart b/lib/services/gamepad_service.dart index 93f01fea..8ba1f1fe 100644 --- a/lib/services/gamepad_service.dart +++ b/lib/services/gamepad_service.dart @@ -7,6 +7,7 @@ import 'package:flutter/widgets.dart'; import 'package:universal_gamepad/universal_gamepad.dart'; import 'package:window_manager/window_manager.dart'; +import '../focus/input_mode_tracker.dart'; import '../utils/app_logger.dart'; import '../utils/key_event_simulator.dart' as key_sim; import '../utils/platform_detector.dart'; @@ -155,10 +156,6 @@ class GamepadService with WindowListener { StreamSubscription? _subscription; final GamepadDuplicateInputGuard _duplicateInputGuard; - /// Callback to switch InputModeTracker to keyboard mode. - /// Set by InputModeTracker when it initializes. - static VoidCallback? onGamepadInput; - static final Map _tabNavigationHandlers = {}; @@ -403,8 +400,7 @@ class GamepadService with WindowListener { // Switch to keyboard mode on any button press if (event.pressed) { - onGamepadInput?.call(); - _setTraditionalFocusHighlight(); + InputModeTracker.reportNonPointerInput(); } // Ensure a frame is scheduled so addPostFrameCallback-based key // simulation fires promptly. Without this, key-up events can be @@ -503,11 +499,10 @@ class GamepadService with WindowListener { return; } - // Switch to keyboard mode on significant axis input. Navigation itself - // schedules frames only when the stick crosses the real deadzone. - if (event.value.abs() > 0.3) { - onGamepadInput?.call(); - _setTraditionalFocusHighlight(); + // Promotion must fire on the same event that navigates: gating below the + // real deadzone would let analog-stick drift hide the desktop cursor. + if (event.value.abs() > _stickDeadzone) { + InputModeTracker.reportNonPointerInput(); } switch (event.axis) { @@ -599,13 +594,4 @@ class GamepadService with WindowListener { _leftStickRight = false; } } - - // Ensure Material uses traditional (keyboard) focus highlights when navigating - // via gamepad. Synthetic key events we dispatch below don't go through the - // platform key pipeline, so Flutter won't automatically flip highlight mode. - void _setTraditionalFocusHighlight() { - if (FocusManager.instance.highlightStrategy != FocusHighlightStrategy.alwaysTraditional) { - FocusManager.instance.highlightStrategy = FocusHighlightStrategy.alwaysTraditional; - } - } } diff --git a/lib/widgets/tv_virtual_keyboard.dart b/lib/widgets/tv_virtual_keyboard.dart index 893a0d60..f1597018 100644 --- a/lib/widgets/tv_virtual_keyboard.dart +++ b/lib/widgets/tv_virtual_keyboard.dart @@ -392,7 +392,7 @@ class _TvVirtualKeyboardDialogState extends State<_TvVirtualKeyboardDialog> { } final character = event.character; - if (character != null && character.isNotEmpty && !key.isNavigationKey) { + if (character != null && character.isNotEmpty && !key.isReservedControlKey) { _insert(character); return KeyEventResult.handled; } @@ -422,7 +422,7 @@ class _TvVirtualKeyboardDialogState extends State<_TvVirtualKeyboardDialog> { } final character = event.character; - if (character != null && character.isNotEmpty && !key.isNavigationKey && !_isControlCharacter(character)) { + if (character != null && character.isNotEmpty && !key.isReservedControlKey && !_isControlCharacter(character)) { _insert(character); _dismissForPhysicalKeyboardInput(); return true; diff --git a/lib/widgets/video_controls/desktop_video_controls.dart b/lib/widgets/video_controls/desktop_video_controls.dart index 6d8e6888..52e1d6ad 100644 --- a/lib/widgets/video_controls/desktop_video_controls.dart +++ b/lib/widgets/video_controls/desktop_video_controls.dart @@ -152,7 +152,7 @@ class DesktopVideoControls extends StatefulWidget { this.onLiveSeek, this.onLiveSeekBy, this.onJumpToLive, - this.useDpadNavigation = false, + required this.useDpadNavigation, this.serverId, this.showQueueTab = false, this.onQueueItemSelected, @@ -297,16 +297,17 @@ class DesktopVideoControlsState extends State { } } - /// Request focus on the play/pause button (called when controls shown via keyboard) + /// Move focus to the play/pause button. + /// + /// Raw mechanism: it does not decide whether focus *should* enter the chrome. + /// A key that raises the chrome makes that decision with + /// `eventRequestsFocusNavigation` before queueing a + /// [PlayerChromeFocusTarget]; internal hand-offs (the skip-marker button's + /// ArrowDown, an item swap) are already inside a focus session. void requestPlayPauseFocus() { _playPauseFocusNode.requestFocus(); } - /// Request focus on the timeline (called when controls shown via LEFT/RIGHT) - void requestTimelineFocus() { - _timelineFocusNode.requestFocus(); - } - /// Hide content strip (called by parent when controls hide) void hideContentStrip() { if (_contentStripVisible) { diff --git a/lib/widgets/video_controls/parts/key_events.dart b/lib/widgets/video_controls/parts/key_events.dart index e6ea5416..42e6557d 100644 --- a/lib/widgets/video_controls/parts/key_events.dart +++ b/lib/widgets/video_controls/parts/key_events.dart @@ -66,19 +66,27 @@ extension _PlexVideoControlsKeyEventMethods on _PlexVideoControlsState { return event is KeyDownEvent ? _transportCommandFor(event) : null; } - void _activateHiddenControlsPrimaryAction() { + /// The player surface's Select action. + /// + /// [requestFocus] is the caller's `eventRequestsFocusNavigation` answer, so a + /// remote OK lands on Play/Pause while a physical-keyboard Enter leaves focus + /// where it is — activation is not a request to start navigating. + void _activatePlayerSurfaceSelect({required bool requestFocus}) { if (!widget.canControl) { - _showControlsWithFocus(); + _showControlsWithFocus(requestFocus: requestFocus); return; } - if (_isSkipMarkerButtonVisible) { + // Skip-Intro is the primary action only while the chrome is down and the + // button is the sole affordance on screen; with the OSD up it is a real + // focusable control and Select must stay "toggle playback". + if (!_showControls && _isSkipMarkerButtonVisible) { _activateSkipMarker(); return; } // Raise the chrome *before* toggling: Select is the deliberate "show me the // controls" affordance, and the visible chrome suppresses the transient // transport disc that would otherwise flash underneath it. - _showControlsWithFocus(); + _showControlsWithFocus(requestFocus: requestFocus); unawaited(_playOrPause()); } @@ -161,7 +169,7 @@ extension _PlexVideoControlsKeyEventMethods on _PlexVideoControlsState { } // Only handle when video player navigation is disabled (desktop mode without D-pad nav) - if (_videoPlayerNavigationEnabled) return false; + if (videoPlayerNavigationPreference()) return false; // Skip on mobile (unless TV) final isMobile = PlatformDetector.isMobile(context) && !PlatformDetector.isTV(); @@ -214,7 +222,7 @@ extension _PlexVideoControlsKeyEventMethods on _PlexVideoControlsState { // Consume KeyUp events for navigation keys to prevent leaking to previous routes. // Let non-navigation keys (volume, etc.) pass through to the OS. if (!event.isActionable) { - if (!event.logicalKey.isNavigationKey) return KeyEventResult.ignored; + if (!event.logicalKey.isReservedControlKey) return KeyEventResult.ignored; return KeyEventResult.handled; } @@ -231,7 +239,7 @@ extension _PlexVideoControlsKeyEventMethods on _PlexVideoControlsState { // The chrome deliberately stays down — the screen announces the accepted // command with a centred transient disc instead (#1676). if (transportCommand != null) { - if ((_videoPlayerNavigationEnabled || isMobile) && event is KeyDownEvent) { + if ((videoPlayerNavigationPreference() || isMobile) && event is KeyDownEvent) { unawaited(_playOrPause(command: transportCommand)); } return KeyEventResult.handled; @@ -256,18 +264,35 @@ extension _PlexVideoControlsKeyEventMethods on _PlexVideoControlsState { return KeyEventResult.handled; } - // Handle Select/Enter when controls are hidden. - // Only intercept if this Focus node itself has primary focus (not a descendant). - // When the skip marker button is the only visible affordance, Select activates - // it; otherwise it falls back to play/pause + show controls. - if (_isSelectKey(key) && !_showControls && _focusNode.hasPrimaryFocus) { - return handleOneShotSelect(event, _activateHiddenControlsPrimaryAction); + // Select on the player surface. Only intercept when this Focus node itself + // holds primary focus — a focused OSD control owns its own activation. + // Whether the raised chrome also takes focus is the key's own answer, so + // mode and focus can never disagree: a remote OK starts a focus session, a + // physical-keyboard Enter just shows the controls and toggles playback. + if (_isSelectKey(key) && _focusNode.hasPrimaryFocus) { + return handleOneShotSelect( + event, + () => _activatePlayerSurfaceSelect(requestFocus: eventRequestsFocusNavigation(event, focused: _focusNode)), + ); + } + + // Tab is the deliberate way into the OSD (#1797). With the chrome down, + // raise it and hand it focus; with the chrome up, let Flutter's app-level + // Shortcuts run NextFocusAction and walk in, rather than consuming the key + // into a dead end below. Returning ignored cannot leak to the route below: + // key dispatch only walks the current focus chain, and covered routes are + // not on it. + if (key == LogicalKeyboardKey.tab && _focusNode.hasPrimaryFocus) { + if (event is! KeyDownEvent) return KeyEventResult.handled; + if (_showControls) return KeyEventResult.ignored; + _showControlsWithFocus(); + return KeyEventResult.handled; } // On desktop/TV, directional input drives the player without the chrome. // LEFT/RIGHT seeks in place with a transient badge; UP/DOWN is the // deliberate "show me the controls" gesture. - if (!isMobile && _isDirectionalKey(key) && (_videoPlayerNavigationEnabled || PlatformDetector.isTV())) { + if (!isMobile && _isDirectionalKey(key) && playerDirectionalNavigationEnabled()) { if (!_showControls) { if (_isHorizontalKey(key)) { if (shouldStartHiddenDirectionalSeek(event)) { @@ -279,19 +304,31 @@ extension _PlexVideoControlsKeyEventMethods on _PlexVideoControlsState { } return KeyEventResult.handled; } - // Children (DesktopVideoControls) handle navigation first via their own onKeyEvent. - // If we reach here, children already declined the event — consume it to prevent leaking. + // Children (DesktopVideoControls) handle navigation first via their own + // onKeyEvent. Reaching here with the surface still focused means nothing + // in the chrome owns focus yet — hand it over instead of consuming the + // key into nothing. This is the same key that just switched the app into + // keyboard mode, so focus has to become visible or the two diverge. + if (_focusNode.hasPrimaryFocus) { + _desktopControlsKey.currentState?.requestPlayPauseFocus(); + } return KeyEventResult.handled; } + // Reserved control keys are consumed rather than returned as ignored, so + // they cannot leak to the route below. Tab is the exception: app-level + // Shortcuts turn it into NextFocusAction, which is how focus traverses + // *inside* the chrome, and key dispatch only walks the current focus chain + // so it cannot reach a covered route anyway. + final consumeToPreventLeak = key.isReservedControlKey && key != LogicalKeyboardKey.tab; + // Pass other events to the keyboard shortcuts service. if (_keyboardService == null) { - return event.logicalKey.isNavigationKey ? KeyEventResult.handled : KeyEventResult.ignored; + return consumeToPreventLeak ? KeyEventResult.handled : KeyEventResult.ignored; } final result = _dispatchShortcut(event, onSkipMarker: _performAutoSkip); - if (!event.logicalKey.isNavigationKey) return result; - // Never return .ignored for navigation keys — prevent leaking to previous routes. + if (!consumeToPreventLeak) return result; return result == KeyEventResult.ignored ? KeyEventResult.handled : result; } } diff --git a/lib/widgets/video_controls/parts/markers.dart b/lib/widgets/video_controls/parts/markers.dart index d82a7fff..b16cb26d 100644 --- a/lib/widgets/video_controls/parts/markers.dart +++ b/lib/widgets/video_controls/parts/markers.dart @@ -76,7 +76,7 @@ extension _PlexVideoControlsMarkerMethods on _PlexVideoControlsState { } // Auto-focus skip button on TV when marker appears (only in keyboard/TV mode) - if (PlatformDetector.isTV() && InputModeTracker.isKeyboardMode(context)) { + if (PlatformDetector.isTV() && InputModeTracker.isKeyboardMode(context, listen: false)) { WidgetsBinding.instance.addPostFrameCallback((_) { if (mounted) { _skipMarkerFocusNode.requestFocus(); diff --git a/lib/widgets/video_controls/parts/navigation.dart b/lib/widgets/video_controls/parts/navigation.dart index 3db52554..64e85b6c 100644 --- a/lib/widgets/video_controls/parts/navigation.dart +++ b/lib/widgets/video_controls/parts/navigation.dart @@ -7,7 +7,7 @@ extension _PlexVideoControlsNavigationMethods on _PlexVideoControlsState { playbackState: playbackState, onToggleAlwaysOnTop: Platform.isMacOS ? null : _toggleAlwaysOnTop, ); - final useDpad = _videoPlayerNavigationEnabled || PlatformDetector.isTV(); + final useDpad = playerDirectionalNavigationEnabled(); return Listener( behavior: HitTestBehavior.translucent, diff --git a/lib/widgets/video_controls/parts/visibility.dart b/lib/widgets/video_controls/parts/visibility.dart index 06721f64..3c2150bb 100644 --- a/lib/widgets/video_controls/parts/visibility.dart +++ b/lib/widgets/video_controls/parts/visibility.dart @@ -16,14 +16,29 @@ extension _PlexVideoControlsVisibilityMethods on _PlexVideoControlsState { } } - /// Focus play/pause button if we're in keyboard navigation mode (desktop/TV only) - void _focusPlayPauseIfKeyboardMode() { - if (!mounted) return; - if (!_videoPlayerNavigationEnabled) return; + /// Focus Play/Pause when the viewer is already driving with keyboard/D-pad + /// and opted into player navigation. + /// + /// This is an *automatic* grab — no key caused it — so it additionally + /// requires an active keyboard session; a key-driven grab only needs + /// [eventRequestsFocusNavigation]. + /// + /// Returns whether it actually moved focus, because the caller uses that to + /// decide whether the player surface still needs to claim the remote. It must + /// therefore report `false` when the chrome is not mounted — a TV route opens + /// with the chrome down, and claiming that it focused something there would + /// leave the remote parked on the screen node (#1765). + bool _focusPlayPauseIfKeyboardMode() { + if (!mounted || !_showControls) return false; + // The raw preference, not the directional policy: a TV viewer who turned + // player navigation off must not get Play/Pause focused on open. + if (!videoPlayerNavigationPreference()) return false; final isMobile = PlatformDetector.isMobile(context) && !PlatformDetector.isTV(); - if (!isMobile && InputModeTracker.isKeyboardMode(context)) { - _desktopControlsKey.currentState?.requestPlayPauseFocus(); - } + if (isMobile || !InputModeTracker.isKeyboardMode(context, listen: false)) return false; + final controls = _desktopControlsKey.currentState; + if (controls == null) return false; + controls.requestPlayPauseFocus(); + return true; } /// Listen to playback state changes to manage auto-hide timer @@ -54,7 +69,7 @@ extension _PlexVideoControlsVisibilityMethods on _PlexVideoControlsState { return const Duration(seconds: 30); } final isMobile = (Platform.isIOS || Platform.isAndroid) && !PlatformDetector.isTV(); - if (isMobile || PlatformDetector.isTV() || _videoPlayerNavigationEnabled) { + if (isMobile || playerDirectionalNavigationEnabled()) { return const Duration(seconds: 5); } return const Duration(seconds: 3); @@ -285,16 +300,19 @@ extension _PlexVideoControlsVisibilityMethods on _PlexVideoControlsState { /// self-heal raises the whole chrome on the first actionable key, which is /// what the transient seek and transport indicators exist to avoid. void _claimPlayerSurfaceFocus() { - final sheetOpen = OverlaySheetController.maybeOf(context)?.isOpen ?? false; - if (sheetOpen) return; + if (_sheetIsOpen()) return; _focusNode.requestFocus(); WidgetsBinding.instance.addPostFrameCallback((_) { - if (mounted && !_focusNode.hasPrimaryFocus) { - _focusNode.requestFocus(); - } + // Re-check: a sheet or a route can open during the frame we deferred + // over, and the retry must not pull the remote back out of it. + if (!mounted || _focusNode.hasPrimaryFocus || _sheetIsOpen()) return; + if (ModalRoute.of(context)?.isCurrent != true) return; + _focusNode.requestFocus(); }); } + bool _sheetIsOpen() => OverlaySheetController.maybeOf(context)?.isOpen ?? false; + void _requestFocusTarget(PlayerChromeFocusTarget target) { WidgetsBinding.instance.addPostFrameCallback((_) { if (!mounted || !widget.chromeController.controlsVisible) return; @@ -304,8 +322,6 @@ extension _PlexVideoControlsVisibilityMethods on _PlexVideoControlsState { switch (target) { case PlayerChromeFocusTarget.playPause: _desktopControlsKey.currentState?.requestPlayPauseFocus(); - case PlayerChromeFocusTarget.timeline: - _desktopControlsKey.currentState?.requestTimelineFocus(); } }); } diff --git a/lib/widgets/video_controls/player_chrome_controller.dart b/lib/widgets/video_controls/player_chrome_controller.dart index 45eb0213..6becf66b 100644 --- a/lib/widgets/video_controls/player_chrome_controller.dart +++ b/lib/widgets/video_controls/player_chrome_controller.dart @@ -8,7 +8,10 @@ import 'package:flutter/material.dart' enum PlayerChromeHold { pip, contentStrip, promptInteraction, scrub } /// Focus target to request after chrome has rebuilt visible controls. -enum PlayerChromeFocusTarget { playPause, timeline } +/// +/// Named rather than a bool so the call sites that hand the chrome focus say +/// what they mean, and so adding a target forces every consumer to decide. +enum PlayerChromeFocusTarget { playPause } /// Owns video-player chrome visibility and auto-hide policy for one player route. class PlayerChromeController extends ChangeNotifier implements ValueListenable { diff --git a/lib/widgets/video_controls/video_controls.dart b/lib/widgets/video_controls/video_controls.dart index 89c856d9..077fcffa 100644 --- a/lib/widgets/video_controls/video_controls.dart +++ b/lib/widgets/video_controls/video_controls.dart @@ -40,6 +40,7 @@ import '../../mixins/mounted_set_state_mixin.dart'; import '../../mpv/mpv.dart'; import '../overlay_sheet.dart'; import '../../focus/dpad_navigator.dart'; +import '../../focus/focus_navigation_intent.dart'; import '../../database/app_database.dart'; import '../../media/media_backend.dart'; @@ -217,6 +218,52 @@ bool shouldShowSkipMarkerButton({ return hasFirstFrame && hasMarker && !hasPlayNextPrompt && (!skipButtonDismissed || controlsVisible); } +/// The viewer's raw "Video Player Navigation" preference, ignoring TV. +/// +/// Use this for genuine preferences — auto-hide delay, Escape semantics, +/// hardware transport interception. For "do arrow keys move focus in the +/// player?" use [playerDirectionalNavigationEnabled] instead: OR-ing `isTV()` +/// into a preference read changes behaviour for a TV viewer who turned the +/// preference off. +/// +/// Reads through [SettingsService.instanceOrNull] because focus policy is +/// consulted from a global key handler that can run before startup finishes. +/// The pre-init answer is `false`; the preference's real default is +/// `TvDetectionService.isTVSync`, so a pre-init read understates it on TV — +/// harmless only because no player route, and so no +/// [DirectionalShortcutFocusNode], can mount before settings are loaded. +bool videoPlayerNavigationPreference() => + SettingsService.instanceOrNull?.read(SettingsService.videoPlayerNavigationEnabled) ?? false; + +/// Whether the video player treats arrow / D-pad keys as focus navigation +/// rather than playback shortcuts. A television always navigates. +/// +/// This is the single derivation of that policy. Every site that used to spell +/// it `pref || PlatformDetector.isTV()` by hand reads it from here. +bool playerDirectionalNavigationEnabled() => videoPlayerNavigationPreference() || PlatformDetector.isTV(); + +/// Builds one of the player's catch-all surface nodes. +/// +/// Both the screen node and the player-surface node can hold primary focus +/// without the viewer ever having navigated — they autofocus and are actively +/// reclaimed — so an arrow pressed while they hold focus is a playback +/// shortcut, not traversal, and must not switch the app into keyboard mode. +/// Minting them here keeps that derivation in one place; [alsoOwns] adds the +/// per-key exceptions a surface still claims once navigation is enabled. +/// +/// `skipTraversal` keeps these full-screen invisible nodes out of the Tab ring: +/// they are entered by autofocus and explicit requests, never by traversal. +DirectionalShortcutFocusNode playerSurfaceFocusNode( + String debugLabel, { + bool Function(LogicalKeyboardKey key)? alsoOwns, +}) { + return DirectionalShortcutFocusNode( + debugLabel: debugLabel, + skipTraversal: true, + consumesDirectionalKeys: (key) => !playerDirectionalNavigationEnabled() || (alsoOwns?.call(key) ?? false), + ); +} + enum PlayerNavigationKey { none, physicalEscape, back, home } enum PlayerBackDisposition { closeContentStrip, exitFullscreenIfActive, hideControls, exitPlayer } @@ -743,8 +790,6 @@ class _PlexVideoControlsState extends State // Skip button dismiss state bool _skipButtonDismissed = false; Timer? _skipButtonDismissTimer; - // Video player navigation (use arrow keys to navigate controls) - bool get _videoPlayerNavigationEnabled => _settings.read(SettingsService.videoPlayerNavigationEnabled); // Performance overlay bool get _showPerformanceOverlay => _settings.read(SettingsService.showPerformanceOverlay); bool get _autoHidePerformanceOverlay => _settings.read(SettingsService.autoHidePerformanceOverlay); @@ -776,7 +821,14 @@ class _PlexVideoControlsState extends State _lastControlsVisible = widget.chromeController.controlsVisible; _controlsMounted = _lastControlsVisible; _controlsOpaque = _lastControlsVisible; - _focusNode = FocusNode(); + // Horizontal arrows stay the player's even with navigation enabled: with + // the chrome down they run a hidden seek (see parts/key_events.dart), so + // they are not evidence of a focus session. Up/Down raises the chrome and + // is traversal, so it promotes. + _focusNode = playerSurfaceFocusNode( + 'PlayerSurface', + alsoOwns: (key) => !_showControls && (key.isLeftKey || key.isRightKey), + ); _skipMarkerFocusNode = FocusNode(debugLabel: 'SkipMarkerButton'); _seekThrottle = throttle( (Duration pos) { @@ -861,11 +913,15 @@ class _PlexVideoControlsState extends State _lastReportedRate = widget.player.state.rate; _rateSubscription = widget.player.streams.rate.listen(_onRateChanged); _loadPlaybackExtras(); - _focusPlayPauseIfKeyboardMode(); - // A route that opened with no chrome never ran the hide transition that - // normally hands focus down here, and this Focus autofocuses too late to - // win it: the screen node claimed it during the loading phase. - if (!widget.chromeController.controlsVisible) _claimPlayerSurfaceFocus(); + // The player surface owns the remote whenever no chrome control was + // deliberately given focus. A route that opens with the chrome already up + // (every desktop route — see playerChromeStartsVisible) never runs the + // hide transition that normally hands focus down here, and this Focus + // autofocuses too late to win it back from the screen node claimed during + // the loading phase. Leaving focus parked up there is what turns the first + // actionable key into a chrome-raising self-heal instead of a playback + // shortcut. + if (!_focusPlayPauseIfKeyboardMode()) _claimPlayerSurfaceFocus(); if (PlatformDetector.isMobile(context) && !PlatformDetector.isTV()) { _refreshDeviceAdjustmentValues(); } diff --git a/test/focus/focus_navigation_intent_test.dart b/test/focus/focus_navigation_intent_test.dart new file mode 100644 index 00000000..c99b137c --- /dev/null +++ b/test/focus/focus_navigation_intent_test.dart @@ -0,0 +1,149 @@ +import 'dart:ui' show KeyEventDeviceType; + +import 'package:flutter/services.dart'; +import 'package:flutter/widgets.dart' show FocusNode; +import 'package:plezy/focus/dpad_navigator.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/focus/focus_navigation_intent.dart'; +import 'package:plezy/utils/platform_detector.dart'; + +/// Executable spec for the one predicate that decides both whether the app +/// switches to keyboard mode and whether a key may hand focus to the player +/// chrome. Every row is a key the app can actually receive; the table is the +/// contract, so widening it is a deliberate edit rather than an accident. +/// +/// `WidgetTester.sendKeyEvent` always reports `deviceType == keyboard`, so rows +/// that vary the device construct the event directly. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + KeyDownEvent down(LogicalKeyboardKey key, {KeyEventDeviceType deviceType = KeyEventDeviceType.keyboard}) { + return KeyDownEvent( + logicalKey: key, + physicalKey: PhysicalKeyboardKey.f13, + timeStamp: Duration.zero, + deviceType: deviceType, + ); + } + + tearDown(() => TvDetectionService.debugSetAppleTVOverride(null)); + + group('off TV', () { + setUp(() => TvDetectionService.debugSetAppleTVOverride(false)); + + final promotes = { + 'tab': down(LogicalKeyboardKey.tab), + 'contextMenu': down(LogicalKeyboardKey.contextMenu), + 'gameButtonX': down(LogicalKeyboardKey.gameButtonX), + 'select': down(LogicalKeyboardKey.select), + 'select reported as a keyboard device (tvOS engine shape)': down( + LogicalKeyboardKey.select, + deviceType: KeyEventDeviceType.keyboard, + ), + 'gameButtonA': down(LogicalKeyboardKey.gameButtonA), + 'gameButtonB': down(LogicalKeyboardKey.gameButtonB), + 'goBack': down(LogicalKeyboardKey.goBack), + 'enter from a gamepad': down(LogicalKeyboardKey.enter, deviceType: KeyEventDeviceType.gamepad), + 'escape from a d-pad': down(LogicalKeyboardKey.escape, deviceType: KeyEventDeviceType.directionalPad), + }; + + final ignores = { + 'enter from a physical keyboard': down(LogicalKeyboardKey.enter), + 'numpadEnter from a physical keyboard': down(LogicalKeyboardKey.numpadEnter), + 'escape from a physical keyboard': down(LogicalKeyboardKey.escape), + 'browserBack (media keyboards emit it)': down(LogicalKeyboardKey.browserBack), + 'space': down(LogicalKeyboardKey.space), + 'a letter': down(LogicalKeyboardKey.keyF), + 'mediaPlayPause': down(LogicalKeyboardKey.mediaPlayPause), + }; + + promotes.forEach((name, event) { + test('$name requests focus navigation', () { + expect(eventRequestsFocusNavigation(event), isTrue); + }); + }); + + ignores.forEach((name, event) { + test('$name does not request focus navigation', () { + expect(eventRequestsFocusNavigation(event), isFalse); + }); + }); + + test('only key-down counts', () { + const up = KeyUpEvent( + logicalKey: LogicalKeyboardKey.tab, + physicalKey: PhysicalKeyboardKey.tab, + timeStamp: Duration.zero, + ); + const repeat = KeyRepeatEvent( + logicalKey: LogicalKeyboardKey.arrowDown, + physicalKey: PhysicalKeyboardKey.arrowDown, + timeStamp: Duration.zero, + ); + + expect(eventRequestsFocusNavigation(up), isFalse); + expect(eventRequestsFocusNavigation(repeat), isFalse); + }); + }); + + group('on TV', () { + setUp(() => TvDetectionService.debugSetAppleTVOverride(true)); + + test('a remote OK that arrives as a keyboard enter still requests navigation', () { + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.enter)), isTrue); + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.numpadEnter)), isTrue); + }); + + test('escape from a physical keyboard still only dismisses', () { + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.escape)), isFalse); + }); + }); + + group('arrows ask the focused node', () { + setUp(() => TvDetectionService.debugSetAppleTVOverride(false)); + + test('an ordinary node means arrows traverse', () { + final node = FocusNode(debugLabel: 'plain'); + addTearDown(node.dispose); + + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.arrowDown), focused: node), isTrue); + }); + + test('a node that owns arrows suppresses promotion', () { + final node = DirectionalShortcutFocusNode(debugLabel: 'surface', consumesDirectionalKeys: (_) => true); + addTearDown(node.dispose); + + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.arrowDown), focused: node), isFalse); + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.arrowRight), focused: node), isFalse); + }); + + test('a node can own one axis and leave the other as traversal', () { + final node = DirectionalShortcutFocusNode( + debugLabel: 'surface', + consumesDirectionalKeys: (key) => key.isLeftKey || key.isRightKey, + ); + addTearDown(node.dispose); + + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.arrowLeft), focused: node), isFalse); + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.arrowUp), focused: node), isTrue); + }); + + test('a node that has stopped owning arrows lets them traverse again', () { + var owns = true; + final node = DirectionalShortcutFocusNode(debugLabel: 'surface', consumesDirectionalKeys: (_) => owns); + addTearDown(node.dispose); + + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.arrowDown), focused: node), isFalse); + owns = false; + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.arrowDown), focused: node), isTrue); + }); + + test('select is unaffected by who owns arrows', () { + final node = DirectionalShortcutFocusNode(debugLabel: 'surface', consumesDirectionalKeys: (_) => true); + addTearDown(node.dispose); + + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.select), focused: node), isTrue); + expect(eventRequestsFocusNavigation(down(LogicalKeyboardKey.enter), focused: node), isFalse); + }); + }); +} diff --git a/test/focus/input_mode_tracker_test.dart b/test/focus/input_mode_tracker_test.dart index 67ff213f..18348a19 100644 --- a/test/focus/input_mode_tracker_test.dart +++ b/test/focus/input_mode_tracker_test.dart @@ -1,7 +1,7 @@ import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:plezy/focus/input_mode_tracker.dart'; -import 'package:plezy/services/gamepad_service.dart'; import 'package:plezy/utils/platform_detector.dart'; void main() { @@ -44,7 +44,7 @@ void main() { expect(listeningBuilds, 1); expect(oneShotBuilds, 1); - GamepadService.onGamepadInput!.call(); + InputModeTracker.reportNonPointerInput(); await tester.pump(); expect(listeningMode, InputMode.keyboard); @@ -70,7 +70,7 @@ void main() { ), ); - GamepadService.onGamepadInput!.call(); + InputModeTracker.reportNonPointerInput(); await tester.pump(); expect(tester.widget(find.byType(MouseRegion)).cursor, SystemMouseCursors.none); @@ -105,7 +105,7 @@ void main() { ), ); - GamepadService.onGamepadInput!.call(); + InputModeTracker.reportNonPointerInput(); await tester.pump(); expect(find.byType(MouseRegion), findsNothing); @@ -113,4 +113,80 @@ void main() { await tester.pump(); expect(taps, 1); }); + testWidgets('a physical-keyboard Enter does not switch the app into keyboard mode', (tester) async { + expect(await _modeAfterKey(tester, LogicalKeyboardKey.enter), InputMode.pointer); + }); + + testWidgets('Escape dismisses without switching the app into keyboard mode', (tester) async { + expect(await _modeAfterKey(tester, LogicalKeyboardKey.escape), InputMode.pointer); + }); + + testWidgets('an arrow key still switches the app into keyboard mode', (tester) async { + expect(await _modeAfterKey(tester, LogicalKeyboardKey.arrowDown), InputMode.keyboard); + }); + + testWidgets('Tab still switches the app into keyboard mode', (tester) async { + expect(await _modeAfterKey(tester, LogicalKeyboardKey.tab), InputMode.keyboard); + }); + + // Startup replaces the bootstrap tree with the app tree: the incoming tracker + // initialises during the build pass and the outgoing one disposes afterwards. + // An unguarded teardown left no tracker registered, so gamepad and companion + // remote input silently stopped switching the app into keyboard mode. + testWidgets('a tracker swap keeps device input reporting', (tester) async { + late InputMode observed; + Widget tree(Key key) => InputModeTracker( + key: key, + child: Directionality( + textDirection: TextDirection.ltr, + child: Builder( + builder: (context) { + observed = InputModeTracker.of(context); + return const SizedBox.shrink(); + }, + ), + ), + ); + + await tester.pumpWidget(tree(const Key('bootstrap'))); + await tester.pumpWidget(tree(const Key('app'))); + + InputModeTracker.reportNonPointerInput(); + await tester.pump(); + + expect(observed, InputMode.keyboard); + }); + + testWidgets('reporting device input with no tracker mounted is a no-op', (tester) async { + await tester.pumpWidget(const SizedBox.shrink()); + + expect(InputModeTracker.reportNonPointerInput, returnsNormally); + }); +} + +/// Pumps a tracker, sends [key], and reports the mode the tree observes. +Future _modeAfterKey(WidgetTester tester, LogicalKeyboardKey key) async { + late InputMode observed; + + await tester.pumpWidget( + InputModeTracker( + child: Directionality( + textDirection: TextDirection.ltr, + child: Builder( + builder: (context) { + observed = InputModeTracker.of(context); + return const SizedBox.shrink(); + }, + ), + ), + ), + ); + expect(observed, InputMode.pointer, reason: 'precondition: the app starts pointer-driven off TV'); + + await tester.sendKeyDownEvent(key); + await tester.pump(); + await tester.sendKeyUpEvent(key); + await tester.pump(); + + return observed; } diff --git a/test/screens/video_player/player_text_input_navigation_test.dart b/test/screens/video_player/player_text_input_navigation_test.dart index baff34c9..71d83595 100644 --- a/test/screens/video_player/player_text_input_navigation_test.dart +++ b/test/screens/video_player/player_text_input_navigation_test.dart @@ -83,7 +83,7 @@ class _PlayerShellState extends State<_PlayerShell> { ); } if (node.hasPrimaryFocus) { - return event.logicalKey.isNavigationKey ? KeyEventResult.handled : KeyEventResult.ignored; + return event.logicalKey.isReservedControlKey ? KeyEventResult.handled : KeyEventResult.ignored; } return KeyEventResult.ignored; }, diff --git a/test/widgets/video_controls_select_key_test.dart b/test/widgets/video_controls_select_key_test.dart new file mode 100644 index 00000000..38562997 --- /dev/null +++ b/test/widgets/video_controls_select_key_test.dart @@ -0,0 +1,355 @@ +import 'package:drift/native.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:intl/date_symbol_data_local.dart'; +import 'package:provider/provider.dart'; + +import 'package:plezy/database/app_database.dart'; +import 'package:plezy/focus/input_mode_tracker.dart'; +import 'package:plezy/i18n/strings.g.dart'; +import 'package:plezy/mpv/mpv.dart'; +import 'package:plezy/providers/playback_state_provider.dart'; +import 'package:plezy/services/settings_service.dart'; +import 'package:plezy/services/video_volume_controller.dart'; +import 'package:plezy/utils/platform_detector.dart'; +import 'package:plezy/watch_together/providers/watch_together_provider.dart'; +import 'package:plezy/widgets/video_controls/desktop_video_controls.dart'; +import 'package:plezy/widgets/video_controls/player_chrome_controller.dart'; +import 'package:plezy/widgets/video_controls/video_controls.dart'; +import 'package:plezy/widgets/video_controls/widgets/player_toast_indicator.dart'; + +import '../test_helpers/media_items.dart'; +import '../test_helpers/prefs.dart'; +import '../test_helpers/theme.dart'; + +/// Pressing Enter on the player surface used to raise the chrome *and* drop +/// focus onto Play/Pause, while the global input-mode tracker separately +/// switched the whole app into keyboard mode — even with "Video Player +/// Navigation" off. Enter activates whatever already has focus; it is not a +/// request to start navigating, so it must leave focus where the viewer left it +/// while still doing its job. Tab and a remote's OK remain the deliberate ways +/// into the OSD. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + late _TogglePlayer player; + late PlayerChromeController chrome; + late PlayerToastController toast; + late VideoVolumeController volume; + late PlaybackStateProvider playbackState; + late WatchTogetherProvider watchTogether; + late AppDatabase database; + late ValueNotifier hasFirstFrame; + late SettingsService settings; + late FocusNode screenFocusNode; + var toggles = 0; + + Future setNavigationEnabled(bool value) => settings.write(SettingsService.videoPlayerNavigationEnabled, value); + + setUp(() async { + LocaleSettings.setLocaleSync(AppLocale.en); + await initializeDateFormatting('en'); + resetSharedPreferencesForTest(); + SettingsService.resetForTesting(); + settings = await SettingsService.getInstance(); + + // Desktop with a pointer: the configuration the report came from. + TvDetectionService.debugSetAppleTVOverride(false); + PlatformDetector.debugSetIsDesktopOSOverride(true); + + database = AppDatabase.forTesting(NativeDatabase.memory()); + player = _TogglePlayer(); + chrome = PlayerChromeController(initiallyVisible: false); + toast = PlayerToastController(); + volume = VideoVolumeController(player: player, settings: settings, initialVolume: 100); + playbackState = PlaybackStateProvider(); + watchTogether = WatchTogetherProvider(); + hasFirstFrame = ValueNotifier(true); + screenFocusNode = FocusNode(debugLabel: 'VideoPlayerScreen'); + toggles = 0; + }); + + tearDown(() async { + TvDetectionService.debugSetAppleTVOverride(null); + PlatformDetector.debugSetIsDesktopOSOverride(null); + hasFirstFrame.dispose(); + screenFocusNode.dispose(); + volume.dispose(); + playbackState.dispose(); + watchTogether.dispose(); + chrome.dispose(); + toast.dispose(); + await database.close(); + }); + + /// Mirrors the production tree: the screen-level node autofocuses during the + /// loading phase and already holds the remote by the time the controls mount, + /// so the controls' own `autofocus` cannot win it back. Without that, the + /// surface would take focus by default and the startup claim would look + /// correct even when it is not. + Widget shell(Widget child) => InputModeTracker( + child: MultiProvider( + providers: [ + Provider.value(value: database), + ChangeNotifierProvider.value(value: playbackState), + ChangeNotifierProvider.value(value: watchTogether), + ], + child: MaterialApp( + theme: ThemeData(platform: TargetPlatform.windows, extensions: const [testMonoTokens]), + home: Scaffold( + body: SizedBox( + width: 1280, + height: 720, + child: Focus(focusNode: screenFocusNode, autofocus: true, child: child), + ), + ), + ), + ), + ); + + Future pumpPlayer(WidgetTester tester) async { + await tester.pumpWidget(shell(const SizedBox.expand())); + await tester.pump(); + expect(screenFocusNode.hasPrimaryFocus, isTrue, reason: 'precondition: the screen node owns the remote'); + + await tester.pumpWidget( + shell( + PlexVideoControls( + player: player, + volumeController: volume, + metadata: testMediaItem(id: 'select-key'), + toastController: toast, + chromeController: chrome, + hasFirstFrame: hasFirstFrame, + canNavigateMediaItems: false, + onPlayPauseRequested: (_) async => toggles++, + ), + ), + ); + await tester.pumpAndSettle(); + } + + Future press(WidgetTester tester, LogicalKeyboardKey key) async { + await tester.sendKeyDownEvent(key); + await tester.pump(); + await tester.sendKeyUpEvent(key); + await tester.pumpAndSettle(); + } + + String? focusLabel() => FocusManager.instance.primaryFocus?.debugLabel; + + InputMode currentMode(WidgetTester tester) => + InputModeTracker.of(tester.element(find.byType(PlexVideoControls)), listen: false); + + /// Mounts the player, runs [body], then unmounts and disarms the auto-hide + /// timer so the harness's pending-timer check stays honest. + void playerTest(String description, Future Function(WidgetTester tester) body) { + testWidgets(description, (tester) async { + await pumpPlayer(tester); + await body(tester); + chrome.cancelAutoHide(); + await tester.pumpWidget(const SizedBox.shrink()); + }); + } + + group('player navigation disabled', () { + setUp(() => setNavigationEnabled(false)); + + playerTest('the player surface, not the screen node, owns the remote once mounted', (tester) async { + expect(focusLabel(), 'PlayerSurface'); + }); + + playerTest('Enter raises the chrome and toggles playback without taking focus', (tester) async { + await press(tester, LogicalKeyboardKey.enter); + + expect(chrome.controlsVisible, isTrue, reason: 'Select is the show-me-the-controls affordance'); + expect(toggles, 1); + expect(focusLabel(), 'PlayerSurface', reason: 'a keyboard Enter must not start a focus session'); + expect(currentMode(tester), InputMode.pointer, reason: 'and must not arm focus chrome app-wide'); + }); + + playerTest('Enter keeps toggling once the chrome is up', (tester) async { + await press(tester, LogicalKeyboardKey.enter); + await press(tester, LogicalKeyboardKey.enter); + + expect(toggles, 2, reason: 'Select must not become a one-shot key when the chrome is visible'); + expect(focusLabel(), 'PlayerSurface'); + }); + + playerTest('a remote OK does hand the chrome focus', (tester) async { + await press(tester, LogicalKeyboardKey.select); + + expect(chrome.controlsVisible, isTrue); + expect(focusLabel(), 'PlayPause', reason: 'a remote has no pointer, so OK is a navigation request'); + expect(currentMode(tester), InputMode.keyboard); + }); + + playerTest('Tab is the keyboard way into the OSD, and keeps traversing inside it', (tester) async { + await press(tester, LogicalKeyboardKey.tab); + expect(chrome.controlsVisible, isTrue); + expect(focusLabel(), 'PlayPause', reason: 'Tab with the chrome down raises it and hands it focus'); + + // The surface handler must not swallow Tab once focus is inside the OSD, + // or the viewer is stranded on the control Tab first landed on. + await press(tester, LogicalKeyboardKey.tab); + expect( + focusLabel(), + isNot(anyOf('PlayPause', 'PlayerSurface')), + reason: 'app-level NextFocusAction must reach the next OSD control', + ); + }); + + playerTest('an arrow seeks without switching the app into keyboard mode', (tester) async { + await press(tester, LogicalKeyboardKey.arrowRight); + + expect(currentMode(tester), InputMode.pointer, reason: 'arrows are playback shortcuts here'); + expect(focusLabel(), 'PlayerSurface'); + }); + }); + + group('player navigation enabled', () { + setUp(() => setNavigationEnabled(true)); + + playerTest('Enter shows the chrome and toggles, leaving focus put', (tester) async { + await press(tester, LogicalKeyboardKey.enter); + + expect(chrome.controlsVisible, isTrue); + expect(toggles, 1); + // Opting into player navigation buys arrow keys, not a focus session from + // a plain Enter: focus and input mode move together or not at all. + expect(focusLabel(), 'PlayerSurface'); + expect(currentMode(tester), InputMode.pointer); + }); + + playerTest('ArrowUp raises the chrome onto Play/Pause', (tester) async { + await press(tester, LogicalKeyboardKey.arrowUp); + + expect(chrome.controlsVisible, isTrue); + expect(focusLabel(), 'PlayPause'); + expect(currentMode(tester), InputMode.keyboard); + }); + + playerTest('ArrowUp hands focus over when the chrome is already up', (tester) async { + chrome.show(); + await tester.pumpAndSettle(); + expect(focusLabel(), 'PlayerSurface', reason: 'precondition: nothing in the chrome owns focus'); + + await press(tester, LogicalKeyboardKey.arrowUp); + + expect(focusLabel(), 'PlayPause', reason: 'the arrow must not be consumed into a dead end'); + }); + + playerTest('a horizontal arrow hands focus over when the chrome is already up', (tester) async { + chrome.show(); + await tester.pumpAndSettle(); + expect(focusLabel(), 'PlayerSurface', reason: 'precondition: nothing in the chrome owns focus'); + + await press(tester, LogicalKeyboardKey.arrowRight); + + // The surface only owns horizontals while the chrome is down, so this + // arrow promotes the app into keyboard mode; focus has to become visible + // with it or the two diverge. + expect(currentMode(tester), InputMode.keyboard); + expect(focusLabel(), 'PlayPause', reason: 'the arrow must not be consumed into a dead end'); + }); + + playerTest('a horizontal arrow seeks under the chrome without arming focus', (tester) async { + await press(tester, LogicalKeyboardKey.arrowRight); + + expect(currentMode(tester), InputMode.pointer, reason: 'a hidden-chrome seek is not navigation'); + expect(chrome.controlsVisible, isFalse); + }); + }); + + playerTest('the OSD focus entry point is raw mechanism, not policy', (tester) async { + await setNavigationEnabled(false); + chrome.show(); + await tester.pumpAndSettle(); + + // Internal hand-offs — the skip-marker button's ArrowDown, an item swap — + // must keep working even with navigation off and no keyboard session. + tester.state(find.byType(DesktopVideoControls)).requestPlayPauseFocus(); + await tester.pumpAndSettle(); + + expect(focusLabel(), 'PlayPause'); + }); + + // The player surface must own the remote from the moment it mounts, whatever + // the chrome is doing. When it does not, the screen node answers the first + // key with its chrome-raising self-heal instead of a playback shortcut. + group('startup hands the remote to the player surface', () { + setUp(() { + chrome.dispose(); + chrome = PlayerChromeController(initiallyVisible: true); + }); + + playerTest('when the route opens with the chrome already up (desktop)', (tester) async { + expect(focusLabel(), 'PlayerSurface'); + }); + + playerTest('and Enter there toggles playback instead of raising a self-heal', (tester) async { + await press(tester, LogicalKeyboardKey.enter); + + expect(toggles, 1); + expect(focusLabel(), 'PlayerSurface'); + expect(currentMode(tester), InputMode.pointer); + }); + }); + + group('startup on a television', () { + setUp(() async { + TvDetectionService.debugSetAppleTVOverride(true); + PlatformDetector.debugSetIsDesktopOSOverride(false); + await setNavigationEnabled(true); + }); + + // A TV route opens with the chrome down, navigation on and keyboard mode + // already active — the exact combination that used to skip the claim. + playerTest('the surface owns the remote even though the chrome starts down', (tester) async { + expect(chrome.controlsVisible, isFalse); + expect(focusLabel(), 'PlayerSurface'); + }); + }); +} + +/// Minimal [Player] reporting steady playback; transport is routed to the +/// widget's `onPlayPauseRequested` callback so the test can count toggles. +class _TogglePlayer implements Player { + Duration _position = const Duration(minutes: 5); + + @override + String get playerType => 'mpv'; + + @override + PlayerState get state => + PlayerState(playing: true, position: _position, duration: const Duration(minutes: 45), seekable: true); + + @override + Future seek(Duration position) async => _position = position; + + @override + PlayerStreams get streams => PlayerStreams( + playing: const Stream.empty(), + completed: const Stream.empty(), + buffering: const Stream.empty(), + position: const Stream.empty(), + duration: const Stream.empty(), + seekable: const Stream.empty(), + buffer: const Stream.empty(), + volume: const Stream.empty(), + rate: const Stream.empty(), + tracks: const Stream.empty(), + track: const Stream.empty(), + log: const Stream.empty(), + error: const Stream.empty(), + audioDevice: const Stream.empty(), + audioDevices: const Stream>.empty(), + bufferRanges: const Stream>.empty(), + playbackRestart: const Stream.empty(), + backendSwitched: const Stream.empty(), + ); + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} diff --git a/test/widgets/video_controls_test.dart b/test/widgets/video_controls_test.dart index 1239db7c..41a8518e 100644 --- a/test/widgets/video_controls_test.dart +++ b/test/widgets/video_controls_test.dart @@ -1049,6 +1049,7 @@ void main() { width: 1000, height: 700, child: DesktopVideoControls( + useDpadNavigation: false, player: player, volumeController: volume, metadata: testMediaItem(id: 'desktop'), diff --git a/test/widgets/video_controls_window_focus_test.dart b/test/widgets/video_controls_window_focus_test.dart index fad4d3eb..87c69c12 100644 --- a/test/widgets/video_controls_window_focus_test.dart +++ b/test/widgets/video_controls_window_focus_test.dart @@ -131,9 +131,11 @@ void main() { ); await tester.pumpAndSettle(); - // The controls' own `autofocus` cannot win the scope back, and a visible - // chrome never runs the hide transition that hands focus down — so this - // is exactly the state a window re-activation leaves behind. + // Mounting parks the remote on the player surface. A window blur then + // drops focus to the root scope and the screen node's reclaim takes it, + // which is the state this suite is about — stage it explicitly. + screenFocusNode.requestFocus(); + await tester.pumpAndSettle(); expect( screenFocusNode.hasPrimaryFocus, isTrue,