diff --git a/lib/screens/video_player_screen.dart b/lib/screens/video_player_screen.dart index f39c178e..ae958c07 100644 --- a/lib/screens/video_player_screen.dart +++ b/lib/screens/video_player_screen.dart @@ -2127,8 +2127,14 @@ class VideoPlayerScreenState extends State with WidgetsBindin } // 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. if (node.hasPrimaryFocus) { - if (event.isActionable) { + final claimsChrome = + !event.logicalKey.isDpadDirection || _videoPlayerNavigationEnabled || PlatformDetector.isTV(); + if (event.isActionable && claimsChrome) { _chromeController.show(focusTarget: PlayerChromeFocusTarget.playPause); } return event.logicalKey.isNavigationKey ? KeyEventResult.handled : KeyEventResult.ignored; diff --git a/lib/widgets/video_controls/parts/visibility.dart b/lib/widgets/video_controls/parts/visibility.dart index 3e24775f..06721f64 100644 --- a/lib/widgets/video_controls/parts/visibility.dart +++ b/lib/widgets/video_controls/parts/visibility.dart @@ -250,7 +250,7 @@ extension _PlexVideoControlsVisibilityMethods on _PlexVideoControlsState { _controlsOpaque = false; if (_currentMarker != null) _skipButtonDismissed = true; }); - _claimHiddenChromeFocus(); + _claimPlayerSurfaceFocus(); } else if (visibilityChanged) { // The timeline is about to take over held-key seeking; commit whatever // the hidden-chrome burst accumulated so it can't rebase from a stale @@ -280,11 +280,11 @@ extension _PlexVideoControlsVisibilityMethods on _PlexVideoControlsState { } } - /// Park focus on the player surface so the hidden-chrome key layer owns the + /// Park focus on the player surface so this widget's key layer owns the /// remote. Without this the screen node keeps primary focus and its /// self-heal raises the whole chrome on the first actionable key, which is /// what the transient seek and transport indicators exist to avoid. - void _claimHiddenChromeFocus() { + void _claimPlayerSurfaceFocus() { final sheetOpen = OverlaySheetController.maybeOf(context)?.isOpen ?? false; if (sheetOpen) return; _focusNode.requestFocus(); @@ -299,7 +299,7 @@ extension _PlexVideoControlsVisibilityMethods on _PlexVideoControlsState { WidgetsBinding.instance.addPostFrameCallback((_) { if (!mounted || !widget.chromeController.controlsVisible) return; // Never steal focus from an open sheet (same rule as - // _claimHiddenChromeFocus). + // _claimPlayerSurfaceFocus). if (OverlaySheetController.maybeOf(context)?.isOpen ?? false) return; switch (target) { case PlayerChromeFocusTarget.playPause: diff --git a/lib/widgets/video_controls/video_controls.dart b/lib/widgets/video_controls/video_controls.dart index 5f5dc36a..97536b43 100644 --- a/lib/widgets/video_controls/video_controls.dart +++ b/lib/widgets/video_controls/video_controls.dart @@ -858,7 +858,7 @@ class _PlexVideoControlsState extends State // 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) _claimHiddenChromeFocus(); + if (!widget.chromeController.controlsVisible) _claimPlayerSurfaceFocus(); if (PlatformDetector.isMobile(context) && !PlatformDetector.isTV()) { _refreshDeviceAdjustmentValues(); } @@ -1010,6 +1010,25 @@ class _PlexVideoControlsState extends State } } + /// Re-activating the window drops Flutter's primary focus to the root scope, + /// and the enclosing player screen reclaims it onto its own node. Nothing + /// hands it back down while the chrome stays visible — the hide transition is + /// the only other handoff — so the next arrow key reaches the screen's + /// self-heal and jumps focus into the OSD (#1797). Take the surface back + /// unless a control below already owns it. + @override + void onWindowFocus() { + // Claim now rather than post-frame: this arrives on a platform callback, + // which is not guaranteed to be followed by a frame. The screen's own + // reclaim re-tests `hasFocus` when it runs, so once the surface holds the + // remote the two no longer compete. + if (!mounted || _focusNode.hasFocus) return; + // A route pushed above the player still leaves these controls mounted; + // re-activating the window must not pull the remote off the top route. + if (ModalRoute.of(context)?.isCurrent != true) return; + _claimPlayerSurfaceFocus(); + } + @override // ignore: no-empty-block - required by WindowListener interface void onWindowResize() {} diff --git a/test/screens/video_player/player_self_heal_keys_test.dart b/test/screens/video_player/player_self_heal_keys_test.dart new file mode 100644 index 00000000..2af5fa05 --- /dev/null +++ b/test/screens/video_player/player_self_heal_keys_test.dart @@ -0,0 +1,92 @@ +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/providers/playback_state_provider.dart'; +import 'package:plezy/screens/video_player_screen.dart'; +import 'package:plezy/services/settings_service.dart'; +import 'package:plezy/utils/platform_detector.dart'; +import 'package:plezy/widgets/video_controls/player_chrome_controller.dart'; +import 'package:provider/provider.dart'; + +import '../../test_helpers/media_items.dart'; +import '../../test_helpers/mock_player_channels.dart'; +import '../../test_helpers/prefs.dart'; + +/// Regression coverage for #1797: while the screen node holds primary focus, +/// its self-heal answers an actionable key by raising the chrome onto the +/// Play/Pause button. With "Video Player Navigation" off, arrows are playback +/// shortcuts and must not be turned into a focus jump — but Tab is the +/// deliberate way into the OSD and must keep working. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + setUp(() async { + resetSharedPreferencesForTest(); + SettingsService.resetForTesting(); + await SettingsService.getInstance(); + TvDetectionService.debugSetAppleTVOverride(false); + }); + + tearDown(() { + TvDetectionService.debugSetAppleTVOverride(null); + }); + + testWidgets('an arrow is left to the playback shortcuts when player navigation is off', (tester) async { + final target = await _selfHealTargetFor(tester, LogicalKeyboardKey.arrowLeft); + + expect(target, isNull, reason: 'an arrow must seek, not pull focus onto Play/Pause'); + }); + + testWidgets('Tab still walks into the player controls when player navigation is off', (tester) async { + final target = await _selfHealTargetFor(tester, LogicalKeyboardKey.tab); + + expect( + target, + PlayerChromeFocusTarget.playPause, + reason: 'Tab is the deliberate way into the OSD and must keep reaching it', + ); + }); +} + +/// Sends [key] to a freshly opened player route — whose screen node still owns +/// primary focus, exactly as after a window re-activation — and reports the +/// focus target its self-heal queued on the chrome, if any. +Future _selfHealTargetFor(WidgetTester tester, LogicalKeyboardKey key) async { + final screenKey = GlobalKey(); + PlayerChromeFocusTarget? target; + + await withMockPlayerChannels( + methodChannelName: 'com.plezy/mpv_player', + eventChannelName: 'com.plezy/mpv_player/events', + testBody: () async { + await tester.pumpWidget( + ChangeNotifierProvider( + create: (_) => PlaybackStateProvider(), + child: MaterialApp( + home: VideoPlayerScreen( + key: screenKey, + metadata: testMediaItem(title: 'Self-heal keys'), + isOffline: true, + ), + ), + ), + ); + await tester.pump(); + + final chrome = screenKey.currentState!.chromeController; + // Drain anything the route queued while opening, so the assertion can + // only see what this key press produced. + chrome.takeFocusTarget(); + + await tester.sendKeyDownEvent(key); + await tester.pump(); + await tester.sendKeyUpEvent(key); + await tester.pump(); + + target = chrome.takeFocusTarget(); + await tester.pumpWidget(const SizedBox.shrink()); + }, + ); + + return target; +} diff --git a/test/widgets/video_controls_window_focus_test.dart b/test/widgets/video_controls_window_focus_test.dart new file mode 100644 index 00000000..fad4d3eb --- /dev/null +++ b/test/widgets/video_controls_window_focus_test.dart @@ -0,0 +1,242 @@ +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:window_manager/window_manager.dart'; + +import 'package:plezy/database/app_database.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'; + +/// Regression coverage for #1797: returning to the desktop window while the +/// chrome is up left primary focus parked on the enclosing screen node. The +/// chrome-hide transition is the only other handoff back to the player surface, +/// so with the OSD visible nothing reclaimed it, and the screen's self-heal +/// turned the next arrow key into a jump onto the Play/Pause button — from +/// there every further arrow navigated the OSD instead of seeking. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + group('window refocus', () { + late _PlayingPlayer player; + late PlayerChromeController chrome; + late PlayerToastController toast; + late VideoVolumeController volume; + late PlaybackStateProvider playbackState; + late WatchTogetherProvider watchTogether; + late AppDatabase database; + late ValueNotifier hasFirstFrame; + late FocusNode screenFocusNode; + late List keysReachingScreen; + + setUp(() async { + LocaleSettings.setLocaleSync(AppLocale.en); + await initializeDateFormatting('en'); + resetSharedPreferencesForTest(); + SettingsService.resetForTesting(); + final settings = await SettingsService.getInstance(); + + TvDetectionService.debugSetAppleTVOverride(false); + PlatformDetector.debugSetIsDesktopOSOverride(false); + + database = AppDatabase.forTesting(NativeDatabase.memory()); + player = _PlayingPlayer(); + // The reported state: the viewer came back with the OSD still up. + chrome = PlayerChromeController(initiallyVisible: true); + toast = PlayerToastController(); + volume = VideoVolumeController(player: player, settings: settings, initialVolume: 100); + playbackState = PlaybackStateProvider(); + watchTogether = WatchTogetherProvider(); + hasFirstFrame = ValueNotifier(true); + screenFocusNode = FocusNode(debugLabel: 'VideoPlayerScreen'); + keysReachingScreen = []; + }); + + 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(); + }); + + Widget shell(Widget child) { + return 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, + onKeyEvent: (node, event) { + if (event is KeyDownEvent) keysReachingScreen.add(event.logicalKey); + return KeyEventResult.ignored; + }, + child: child, + ), + ), + ), + ), + ); + } + + /// Reproduces the post-blur layout: the screen node owns primary focus (the + /// player screen reclaims it whenever focus leaves the subtree) while the + /// controls are mounted with their chrome up. + Future pumpControlsUnderScreenFocus(WidgetTester tester) async { + await tester.pumpWidget(shell(const SizedBox.expand())); + await tester.pump(); + expect(screenFocusNode.hasPrimaryFocus, isTrue); + + await tester.pumpWidget( + shell( + PlexVideoControls( + player: player, + volumeController: volume, + metadata: testMediaItem(id: 'window-refocus'), + toastController: toast, + chromeController: chrome, + hasFirstFrame: hasFirstFrame, + canNavigateMediaItems: false, + ), + ), + ); + 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. + expect( + screenFocusNode.hasPrimaryFocus, + isTrue, + reason: 'precondition: focus is stranded on the screen node with the OSD up', + ); + } + + testWidgets('re-activating the window hands the remote back to the player surface', (tester) async { + await pumpControlsUnderScreenFocus(tester); + + (tester.state(find.byType(PlexVideoControls)) as WindowListener).onWindowFocus(); + await tester.pumpAndSettle(); + + expect( + screenFocusNode.hasPrimaryFocus, + isFalse, + reason: 'the player surface owns the remote again, not the screen node', + ); + + // The reported symptom lands on the second press: the first is answered + // while the very same event also steals focus into the OSD. + for (var press = 0; press < 2; press++) { + await tester.sendKeyDownEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + await tester.sendKeyUpEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + } + + expect( + keysReachingScreen, + isEmpty, + reason: 'no arrow may reach the screen self-heal, or it jumps focus onto Play/Pause', + ); + + chrome.cancelAutoHide(); + await tester.pumpWidget(const SizedBox.shrink()); + }); + + testWidgets('a control the viewer focused on purpose keeps the remote', (tester) async { + await pumpControlsUnderScreenFocus(tester); + + // Deliberate focus inside the OSD, as traversal or a TV remote leaves it. + tester.state(find.byType(DesktopVideoControls)).requestPlayPauseFocus(); + await tester.pumpAndSettle(); + final focusedControl = FocusManager.instance.primaryFocus; + expect(focusedControl?.debugLabel, 'PlayPause', reason: 'a control took the remote'); + + (tester.state(find.byType(PlexVideoControls)) as WindowListener).onWindowFocus(); + await tester.pumpAndSettle(); + + expect( + FocusManager.instance.primaryFocus, + same(focusedControl), + reason: 'the surface claim must not yank focus off a control the viewer chose', + ); + + chrome.cancelAutoHide(); + await tester.pumpWidget(const SizedBox.shrink()); + }); + }); +} + +/// Minimal [Player] reporting steady playback, the state the player settles +/// into once the media is open. +class _PlayingPlayer implements Player { + final List seeks = []; + 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 { + seeks.add(position); + _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); +}