From a23c7f215cf8f98154c8b23807eb002b4a115a62 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:08:28 +0200 Subject: [PATCH] fix(player): tighten staged back navigation --- lib/screens/main_screen.dart | 36 ++++ lib/screens/video_player_screen.dart | 24 ++- .../video_controls/video_controls.dart | 76 +++++--- test/screens/main_screen_layout_test.dart | 26 +++ test/widgets/video_controls_test.dart | 168 +++++++++++++++++- 5 files changed, 306 insertions(+), 24 deletions(-) diff --git a/lib/screens/main_screen.dart b/lib/screens/main_screen.dart index 72e062f3..fc181ce2 100644 --- a/lib/screens/main_screen.dart +++ b/lib/screens/main_screen.dart @@ -73,6 +73,17 @@ import '../watch_together/watch_together.dart'; // navigation/main_screen_scope.dart (re-exported above) so widgets like the // browse rail can import the scope without an import cycle through this file. +@visibleForTesting +bool shouldHandleMacOsRootEscape({ + required bool isMacOS, + required bool isPhysicalKeyboardEvent, + required LogicalKeyboardKey logicalKey, + required bool isCurrentRoute, + required bool isHomeTab, +}) { + return isMacOS && isPhysicalKeyboardEvent && logicalKey == LogicalKeyboardKey.escape && isCurrentRoute && isHomeTab; +} + @visibleForTesting ({double left, double width}) mainScreenSideNavigationContentLayout({ required double viewportWidth, @@ -1299,6 +1310,29 @@ class _MainScreenState extends State return KeyEventResult.handled; } + /// On macOS, native fullscreen is window state shared by every route. + /// Player Escape therefore leaves it alone; only root Home owns the + /// conventional Escape-to-leave-fullscreen behavior. + KeyEventResult _handleMacOsRootEscape(KeyEvent event) { + final tabs = _getVisibleTabs(_isOffline); + final shouldHandle = shouldHandleMacOsRootEscape( + isMacOS: Platform.isMacOS, + isPhysicalKeyboardEvent: event.isPhysicalKeyboardEvent, + logicalKey: event.logicalKey, + isCurrentRoute: ModalRoute.of(context)?.isCurrent == true, + isHomeTab: tabs.isNotEmpty && _currentTab == tabs.first.id, + ); + if (!shouldHandle) return KeyEventResult.ignored; + + if (event is KeyUpEvent) { + BackKeyCoordinator.markHandled(); + unawaited(FullscreenStateManager().exitFullscreenIfActive()); + } + return event is KeyDownEvent || event is KeyRepeatEvent || event is KeyUpEvent + ? KeyEventResult.handled + : KeyEventResult.ignored; + } + /// Handle Cmd+F (macOS) / Ctrl+F (Windows/Linux) to navigate to search. KeyEventResult _handleSearchShortcut(KeyEvent event) { if (event is! KeyDownEvent) return KeyEventResult.ignored; @@ -1693,6 +1727,8 @@ class _MainScreenState extends State canPop: false, child: Focus( onKeyEvent: (node, event) { + final rootEscapeResult = _handleMacOsRootEscape(event); + if (rootEscapeResult == KeyEventResult.handled) return rootEscapeResult; final fullscreenResult = _handleFullscreenShortcut(event); if (fullscreenResult == KeyEventResult.handled) return fullscreenResult; final searchResult = _handleSearchShortcut(event); diff --git a/lib/screens/video_player_screen.dart b/lib/screens/video_player_screen.dart index 82bbd392..aa29ffdf 100644 --- a/lib/screens/video_player_screen.dart +++ b/lib/screens/video_player_screen.dart @@ -582,8 +582,13 @@ class VideoPlayerScreenState extends State with WidgetsBindin chromeController: _chromeController, isPromptOpen: () => _showPlayNextDialog || _showStillWatchingPrompt, dismissPrompt: _dismissPlaybackPromptForBack, - isChromePresented: () => _isPlayerInitialized && player != null && _chromeController.controlsPresented, + isChromePresented: () => + _isPlayerInitialized && player != null && _hasFirstFrame.value && _chromeController.controlsPresented, exitFullscreenIfActive: FullscreenStateManager().exitFullscreenIfActive, + // macOS fullscreen belongs to the app window, not the player route. + // Escape stages through chrome/player Back and the root Home screen + // owns leaving native fullscreen. + physicalEscapeExitsFullscreen: !Platform.isMacOS, exitPlayer: () => unawaited(_handleBackButton()), navigateHome: _handleHomeButton, isActive: () => mounted, @@ -615,6 +620,7 @@ class VideoPlayerScreenState extends State with WidgetsBindin // Ensures a single stable focus target across loading → initialized phases. _screenFocusNode = FocusNode(debugLabel: 'VideoPlayerScreen'); _screenFocusNode.addListener(_onScreenFocusChanged); + HardwareKeyboard.instance.addHandler(_primeInitializationNavigationFocus); appLogger.d('VideoPlayerScreen initialized for: ${_currentMetadata.title}'); if (_preferredAudioTrack != null) { @@ -1270,6 +1276,7 @@ class VideoPlayerScreenState extends State with WidgetsBindin _stillWatchingContinueFocusNode.dispose(); _screenFocusNode.removeListener(_onScreenFocusChanged); + HardwareKeyboard.instance.removeHandler(_primeInitializationNavigationFocus); _screenFocusNode.dispose(); _mediaControlsManager?.clear(); @@ -1342,6 +1349,21 @@ class VideoPlayerScreenState extends State with WidgetsBindin } } + /// Loading and initialization-error phases can receive a Back key-down + /// before their autofocus request has settled. Claim focus immediately so + /// the matching key-up reaches the player route and exits exactly once. + bool _primeInitializationNavigationFocus(KeyEvent event) { + if (!mounted || _isExiting.value) return false; + primePlayerNavigationFocusForEvent( + event, + focusNode: _screenFocusNode, + playerReady: _isPlayerInitialized && player != null && _hasFirstFrame.value, + isCurrentRoute: ModalRoute.of(context)?.isCurrent == true, + isAppleTV: PlatformDetector.isAppleTV(), + ); + return false; + } + void _setupAppleTvRemotePlaybackActions() { if (!PlatformDetector.isAppleTV()) return; diff --git a/lib/widgets/video_controls/video_controls.dart b/lib/widgets/video_controls/video_controls.dart index 9854285e..41d3e6a7 100644 --- a/lib/widgets/video_controls/video_controls.dart +++ b/lib/widgets/video_controls/video_controls.dart @@ -193,6 +193,7 @@ class PlayerNavigationCoordinator { final VoidCallback dismissPrompt; final bool Function() isChromePresented; final Future Function() exitFullscreenIfActive; + final bool physicalEscapeExitsFullscreen; final VoidCallback exitPlayer; final VoidCallback navigateHome; final bool Function() isActive; @@ -205,6 +206,7 @@ class PlayerNavigationCoordinator { required this.dismissPrompt, required this.isChromePresented, required this.exitFullscreenIfActive, + this.physicalEscapeExitsFullscreen = true, required this.exitPlayer, required this.navigateHome, bool Function()? isActive, @@ -221,23 +223,30 @@ class PlayerNavigationCoordinator { dismissPrompt(); return; } - if (chromeController.contentStripVisible) { - chromeController.setContentStripVisible(false); - return; - } - if (navigationKey == PlayerNavigationKey.physicalEscape) { - unawaited(_handlePhysicalEscape()); - return; - } - _handleSemanticBack(); + final disposition = resolvePlayerBackDisposition( + navigationKey: navigationKey, + contentStripVisible: chromeController.contentStripVisible, + controlsVisible: isChromePresented(), + physicalEscapeExitsFullscreen: physicalEscapeExitsFullscreen, + ); + _applyDisposition(disposition); } - void _handleSemanticBack() { - if (isChromePresented()) { - chromeController.hide(); - return; + void _applyDisposition(PlayerBackDisposition disposition) { + switch (disposition) { + case PlayerBackDisposition.closeContentStrip: + chromeController.setContentStripVisible(false); + return; + case PlayerBackDisposition.exitFullscreenIfActive: + unawaited(_handlePhysicalEscape()); + return; + case PlayerBackDisposition.hideControls: + chromeController.hide(); + return; + case PlayerBackDisposition.exitPlayer: + exitPlayer(); + return; } - exitPlayer(); } Future _handlePhysicalEscape() async { @@ -247,13 +256,12 @@ class PlayerNavigationCoordinator { try { if (await exitFullscreenIfActive()) return; if (!isActive()) return; - if (chromeController.contentStripVisible) { - chromeController.setContentStripVisible(false); - } else if (chromeWasPresented || isChromePresented()) { - chromeController.hide(); - } else { - exitPlayer(); - } + final disposition = resolvePlayerBackDisposition( + navigationKey: PlayerNavigationKey.back, + contentStripVisible: chromeController.contentStripVisible, + controlsVisible: chromeWasPresented || isChromePresented(), + ); + _applyDisposition(disposition); } finally { _handlingPhysicalEscape = false; } @@ -264,10 +272,11 @@ PlayerBackDisposition resolvePlayerBackDisposition({ required PlayerNavigationKey navigationKey, required bool contentStripVisible, required bool controlsVisible, + bool physicalEscapeExitsFullscreen = true, }) { assert(navigationKey == PlayerNavigationKey.physicalEscape || navigationKey == PlayerNavigationKey.back); if (contentStripVisible) return PlayerBackDisposition.closeContentStrip; - if (navigationKey == PlayerNavigationKey.physicalEscape) { + if (navigationKey == PlayerNavigationKey.physicalEscape && physicalEscapeExitsFullscreen) { return PlayerBackDisposition.exitFullscreenIfActive; } return controlsVisible ? PlayerBackDisposition.hideControls : PlayerBackDisposition.exitPlayer; @@ -295,12 +304,35 @@ PlayerNavigationKey classifyPlayerNavigationKey(KeyEvent event, {required bool i return PlayerNavigationKey.none; } +/// Gives the player route ownership of navigation that arrives while its +/// loading/error body is still establishing focus. The matching key-up still +/// performs the action through the normal [Focus.onKeyEvent] path. +bool primePlayerNavigationFocusForEvent( + KeyEvent event, { + required FocusNode focusNode, + required bool playerReady, + required bool isCurrentRoute, + required bool isAppleTV, +}) { + if (!isCurrentRoute || playerReady || event is! KeyDownEvent) return false; + if (classifyPlayerNavigationKey(event, isAppleTV: isAppleTV) == PlayerNavigationKey.none) return false; + focusNode.requestFocus(); + return true; +} + KeyEventResult handlePlayerNavigationKeyAction( KeyEvent event, PlayerNavigationKey navigationKey, VoidCallback onAction, ) { if (navigationKey == PlayerNavigationKey.none) return KeyEventResult.ignored; + + // macOS may also translate Backspace / Escape / browser Back into a route + // pop on key-down. Suppress that parallel path before the player performs + // its single staged action on key-up. + if (navigationKey != PlayerNavigationKey.home && event is KeyDownEvent) { + BackKeyCoordinator.markHandled(); + } if (event.logicalKey.isBackKey) return handleBackKeyAction(event, onAction); if (event is KeyUpEvent) { diff --git a/test/screens/main_screen_layout_test.dart b/test/screens/main_screen_layout_test.dart index 812a4222..83564163 100644 --- a/test/screens/main_screen_layout_test.dart +++ b/test/screens/main_screen_layout_test.dart @@ -1,4 +1,5 @@ import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:plezy/screens/main_screen.dart'; import 'package:plezy/widgets/side_navigation_rail.dart'; @@ -70,6 +71,31 @@ void main() { expect(shouldPass(isAppleTV: false), isFalse); }); + test('macOS physical Escape is reserved for native fullscreen only at root Home', () { + bool shouldHandle({ + bool isMacOS = true, + bool isPhysicalKeyboardEvent = true, + LogicalKeyboardKey logicalKey = LogicalKeyboardKey.escape, + bool isCurrentRoute = true, + bool isHomeTab = true, + }) { + return shouldHandleMacOsRootEscape( + isMacOS: isMacOS, + isPhysicalKeyboardEvent: isPhysicalKeyboardEvent, + logicalKey: logicalKey, + isCurrentRoute: isCurrentRoute, + isHomeTab: isHomeTab, + ); + } + + expect(shouldHandle(), isTrue); + expect(shouldHandle(isHomeTab: false), isFalse); + expect(shouldHandle(isCurrentRoute: false), isFalse); + expect(shouldHandle(isPhysicalKeyboardEvent: false), isFalse); + expect(shouldHandle(isMacOS: false), isFalse); + expect(shouldHandle(logicalKey: LogicalKeyboardKey.gameButtonB), isFalse); + }); + test('profile switch invalidates nothing here — the keyed session remount owns it', () { expect( profileInvalidationAction( diff --git a/test/widgets/video_controls_test.dart b/test/widgets/video_controls_test.dart index e41adafb..eda90a0b 100644 --- a/test/widgets/video_controls_test.dart +++ b/test/widgets/video_controls_test.dart @@ -4,6 +4,7 @@ import 'dart:ui' as ui; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/focus/key_event_utils.dart'; import 'package:plezy/i18n/strings.g.dart'; import 'package:plezy/media/media_source_info.dart'; import 'package:plezy/media/media_version.dart'; @@ -313,6 +314,7 @@ void main() { PlayerNavigationKey.back, () => actions++, ); + expect(BackKeyCoordinator.consumeIfHandled(), isTrue, reason: 'parallel route pop is suppressed on key down'); handlePlayerNavigationKeyAction(_keyUp(LogicalKeyboardKey.backspace), PlayerNavigationKey.back, () => actions++); expect(actions, 1); @@ -320,12 +322,112 @@ void main() { }); }); + group('primePlayerNavigationFocusForEvent', () { + testWidgets('claims loading-route focus on navigation key down', (tester) async { + final playerFocus = FocusNode(); + final otherFocus = FocusNode(); + addTearDown(playerFocus.dispose); + addTearDown(otherFocus.dispose); + + await tester.pumpWidget( + MaterialApp( + home: Column( + children: [ + Focus(focusNode: playerFocus, child: const SizedBox()), + Focus(focusNode: otherFocus, child: const SizedBox()), + ], + ), + ), + ); + otherFocus.requestFocus(); + await tester.pump(); + + final primed = primePlayerNavigationFocusForEvent( + _keyDown(LogicalKeyboardKey.gameButtonB), + focusNode: playerFocus, + playerReady: false, + isCurrentRoute: true, + isAppleTV: false, + ); + await tester.pump(); + + expect(primed, isTrue); + expect(playerFocus.hasPrimaryFocus, isTrue); + }); + + testWidgets('does not steal focus after the player is ready', (tester) async { + final playerFocus = FocusNode(); + final otherFocus = FocusNode(); + addTearDown(playerFocus.dispose); + addTearDown(otherFocus.dispose); + + await tester.pumpWidget( + MaterialApp( + home: Column( + children: [ + Focus(focusNode: playerFocus, child: const SizedBox()), + Focus(focusNode: otherFocus, child: const SizedBox()), + ], + ), + ), + ); + otherFocus.requestFocus(); + await tester.pump(); + + final primed = primePlayerNavigationFocusForEvent( + _keyDown(LogicalKeyboardKey.gameButtonB), + focusNode: playerFocus, + playerReady: true, + isCurrentRoute: true, + isAppleTV: false, + ); + await tester.pump(); + + expect(primed, isFalse); + expect(otherFocus.hasPrimaryFocus, isTrue); + }); + + testWidgets('does not steal focus from a route above the player', (tester) async { + final playerFocus = FocusNode(); + final overlayFocus = FocusNode(); + addTearDown(playerFocus.dispose); + addTearDown(overlayFocus.dispose); + + await tester.pumpWidget( + MaterialApp( + home: Column( + children: [ + Focus(focusNode: playerFocus, child: const SizedBox()), + Focus(focusNode: overlayFocus, child: const SizedBox()), + ], + ), + ), + ); + overlayFocus.requestFocus(); + await tester.pump(); + + final primed = primePlayerNavigationFocusForEvent( + _keyDown(LogicalKeyboardKey.gameButtonB), + focusNode: playerFocus, + playerReady: false, + isCurrentRoute: false, + isAppleTV: false, + ); + await tester.pump(); + + expect(primed, isFalse); + expect(overlayFocus.hasPrimaryFocus, isTrue); + }); + }); + group('PlayerNavigationCoordinator focus dispatch', () { PlayerNavigationCoordinator coordinatorFor( PlayerChromeController chromeController, { bool Function()? isPromptOpen, VoidCallback? dismissPrompt, + bool Function()? isChromePresented, Future Function()? exitFullscreenIfActive, + bool physicalEscapeExitsFullscreen = true, VoidCallback? exitPlayer, VoidCallback? navigateHome, bool Function()? isActive, @@ -334,8 +436,9 @@ void main() { chromeController: chromeController, isPromptOpen: isPromptOpen ?? () => false, dismissPrompt: dismissPrompt ?? () {}, - isChromePresented: () => chromeController.controlsPresented, + isChromePresented: isChromePresented ?? () => chromeController.controlsPresented, exitFullscreenIfActive: exitFullscreenIfActive ?? () async => false, + physicalEscapeExitsFullscreen: physicalEscapeExitsFullscreen, exitPlayer: exitPlayer ?? () {}, navigateHome: navigateHome ?? () {}, isActive: isActive, @@ -377,6 +480,19 @@ void main() { expect(exits, 1); }); + testWidgets('Back exits during pre-first-frame loading even when controls default visible', (tester) async { + final chromeController = PlayerChromeController(); + addTearDown(chromeController.dispose); + var exits = 0; + final coordinator = coordinatorFor(chromeController, isChromePresented: () => false, exitPlayer: () => exits++); + await pumpNavigationFocus(tester, coordinator); + + await tester.sendKeyEvent(LogicalKeyboardKey.gameButtonB); + + expect(chromeController.controlsVisible, isTrue); + expect(exits, 1); + }); + testWidgets('physical Escape outside fullscreen hides presented chrome without exiting', (tester) async { final chromeController = PlayerChromeController(); addTearDown(chromeController.dispose); @@ -441,6 +557,35 @@ void main() { expect(exits, 0); }); + testWidgets('macOS physical Escape stages through chrome and player without leaving fullscreen', (tester) async { + final chromeController = PlayerChromeController(); + addTearDown(chromeController.dispose); + var fullscreenChecks = 0; + var exits = 0; + final coordinator = coordinatorFor( + chromeController, + exitFullscreenIfActive: () async { + fullscreenChecks++; + return true; + }, + physicalEscapeExitsFullscreen: false, + exitPlayer: () => exits++, + ); + await pumpNavigationFocus(tester, coordinator); + + await tester.sendKeyEvent(LogicalKeyboardKey.escape); + + expect(fullscreenChecks, 0); + expect(chromeController.controlsVisible, isFalse); + expect(exits, 0); + + chromeController.markControlsHidden(); + await tester.sendKeyEvent(LogicalKeyboardKey.escape); + + expect(fullscreenChecks, 0); + expect(exits, 1); + }); + testWidgets('physical Escape does nothing after its player route is disposed', (tester) async { final chromeController = PlayerChromeController(); addTearDown(chromeController.dispose); @@ -559,6 +704,27 @@ void main() { ); }); + test('macOS physical Escape uses the same staged disposition as semantic Back', () { + expect( + resolvePlayerBackDisposition( + navigationKey: PlayerNavigationKey.physicalEscape, + contentStripVisible: false, + controlsVisible: true, + physicalEscapeExitsFullscreen: false, + ), + PlayerBackDisposition.hideControls, + ); + expect( + resolvePlayerBackDisposition( + navigationKey: PlayerNavigationKey.physicalEscape, + contentStripVisible: false, + controlsVisible: false, + physicalEscapeExitsFullscreen: false, + ), + PlayerBackDisposition.exitPlayer, + ); + }); + test('semantic Back hides visible controls then exits when hidden', () { expect( resolvePlayerBackDisposition(