diff --git a/lib/focus/covered_route_focus_boundary.dart b/lib/focus/covered_route_focus_boundary.dart new file mode 100644 index 000000000..d95ce3230 --- /dev/null +++ b/lib/focus/covered_route_focus_boundary.dart @@ -0,0 +1,61 @@ +import 'package:flutter/widgets.dart'; + +/// Keeps a subtree from taking focus while the [ModalRoute] it lives in is +/// covered by another route, and hands focus back when it is uncovered. +/// +/// Flutter only marks a covered route's scope `skipTraversal`; any descendant +/// may still call `requestFocus()` and win. That is harmless on a single +/// navigator, where every screen's `ModalRoute.of(context).isCurrent` guard +/// tells the truth. It breaks with a nested navigator: a route pushed on the +/// *root* navigator (the profile picker, the PIN dialog) covers the whole +/// nested stack, yet each nested route still reports `isCurrent == true` and +/// its focus self-heals — sidebar reveal, library grid load, TV browse rail — +/// yank the remote off the visible route, which on tvOS reads as a dead +/// remote (#2034, #2239). +/// +/// The boundary restores the invariant once, at the navigator boundary, +/// instead of at every reclaim site: while the enclosing route is not current +/// the subtree is [ExcludeFocus]ed, so every `requestFocus()` below it is a +/// no-op. On uncover it re-requests the subtree's own [FocusScope], whose +/// focus history still leads back to the leaf that had focus before the +/// cover; Flutter's own restoration cannot, because the covering route's pop +/// culls the excluded scope from the route scope's history before the +/// exclusion lifts. +class CoveredRouteFocusBoundary extends StatefulWidget { + const CoveredRouteFocusBoundary({super.key, required this.child}); + + final Widget child; + + @override + State createState() => _CoveredRouteFocusBoundaryState(); +} + +class _CoveredRouteFocusBoundaryState extends State { + final _scope = FocusScopeNode(debugLabel: 'CoveredRouteFocusBoundary'); + bool _covered = false; + + @override + void dispose() { + _scope.dispose(); + super.dispose(); + } + + @override + Widget build(BuildContext context) { + // ModalRoute.of subscribes to the route's status, so this rebuilds on + // every isCurrent flip. Outside any route the subtree is never covered. + final covered = !(ModalRoute.of(context)?.isCurrent ?? true); + if (_covered && !covered) { + // The exclusion lifts in this build's didUpdateWidget, after the pop + // already parked focus on the route scope; restore once it has. + WidgetsBinding.instance.addPostFrameCallback((_) { + if (mounted && !_scope.hasFocus) _scope.requestFocus(); + }); + } + _covered = covered; + return ExcludeFocus( + excluding: covered, + child: FocusScope(node: _scope, child: widget.child), + ); + } +} diff --git a/lib/navigation/profile_session_screen.dart b/lib/navigation/profile_session_screen.dart index e2b52d1e0..3b9500e3d 100644 --- a/lib/navigation/profile_session_screen.dart +++ b/lib/navigation/profile_session_screen.dart @@ -5,6 +5,7 @@ import 'package:http/http.dart' as http; import 'package:provider/provider.dart'; import '../connection/connection_registry.dart'; +import '../focus/covered_route_focus_boundary.dart'; import '../focus/key_event_utils.dart'; import '../media/ids.dart'; import '../media/media_server_client.dart'; @@ -58,7 +59,10 @@ CatalogSourcesProvider _createCatalogSourcesProvider(BuildContext context) { /// /// Keep profile-owned routes, dialogs, sheets, and virtual keyboards on the /// nearest navigator from this subtree. Keep setup/auth/PIN/profile-picker flows -/// on the root navigator so they survive this subtree being replaced. +/// on the root navigator so they survive this subtree being replaced. While one +/// of those covers this route, [CoveredRouteFocusBoundary] keeps the subtree +/// from taking focus: nested routes still read as current, so their focus +/// self-heals would otherwise pull the remote behind the covering route. class ProfileSessionScreen extends StatefulWidget { const ProfileSessionScreen({super.key, this.isOfflineMode = false, this.initialPromptHandled = false}) : profileShellBuilder = null, @@ -273,10 +277,12 @@ class _ProfileSessionScreenState extends State { }, ), ], - child: _ProfileSessionNavigator( - isOfflineMode: widget.isOfflineMode, - initialPromptHandled: initialPromptHandled, - profileShellBuilder: widget.profileShellBuilder, + child: CoveredRouteFocusBoundary( + child: _ProfileSessionNavigator( + isOfflineMode: widget.isOfflineMode, + initialPromptHandled: initialPromptHandled, + profileShellBuilder: widget.profileShellBuilder, + ), ), ), ); diff --git a/lib/screens/video_player_screen.dart b/lib/screens/video_player_screen.dart index 4246a1acb..ebb0b4b9f 100644 --- a/lib/screens/video_player_screen.dart +++ b/lib/screens/video_player_screen.dart @@ -81,7 +81,6 @@ import '../utils/platform_detector.dart'; import '../utils/provider_extensions.dart'; import '../utils/snackbar_helper.dart'; import '../utils/stream_buffer_sizing.dart'; -import '../utils/route_visibility.dart'; import '../utils/video_player_navigation.dart'; import '../utils/android_exit_diagnostics.dart'; import 'video_player/completion_latch.dart'; @@ -1940,19 +1939,17 @@ class VideoPlayerScreenState extends State with WidgetsBindin /// descendant has focus, so internal movement between child controls /// does NOT trigger this. /// - /// Reclaim only while this screen is the app's top visible route. The - /// build's `canRequestFocus` already tracks routes pushed above the player - /// on its own (profile-session) navigator, but a route on an ancestor - /// navigator — the root-navigator profile picker on resume — leaves it - /// true, and reclaiming then yanks the remote off the visible route, - /// wedging D-pad devices (#2034). + /// Reclaim only while this screen is the top route of its navigator. A + /// route on an ancestor navigator — the root-navigator profile picker on + /// resume (#2034) — leaves `isCurrent` true, but CoveredRouteFocusBoundary + /// then excludes the whole session from focus, so the request is a no-op. void _onScreenFocusChanged() { if (_reclaimingFocus) return; if (!_screenFocusNode.hasFocus && mounted && !_isExiting.value) { _reclaimingFocus = true; WidgetsBinding.instance.addPostFrameCallback((_) { _reclaimingFocus = false; - if (mounted && !_isExiting.value && !_screenFocusNode.hasFocus && isRouteChainCurrent(context)) { + if (mounted && !_isExiting.value && !_screenFocusNode.hasFocus && ModalRoute.of(context)?.isCurrent == true) { _screenFocusNode.requestFocus(); } }); @@ -1968,7 +1965,7 @@ class VideoPlayerScreenState extends State with WidgetsBindin event, focusNode: _screenFocusNode, playerReady: _isPlayerInitialized && player != null && _firstFrame.uiReady.value, - isCurrentRoute: isRouteChainCurrent(context), + isCurrentRoute: ModalRoute.of(context)?.isCurrent ?? true, isAppleTV: PlatformDetector.isAppleTV(), ); return false; diff --git a/lib/utils/route_visibility.dart b/lib/utils/route_visibility.dart deleted file mode 100644 index 5ce0d138a..000000000 --- a/lib/utils/route_visibility.dart +++ /dev/null @@ -1,21 +0,0 @@ -import 'package:flutter/widgets.dart'; - -/// Whether [context]'s enclosing route is the app's top visible route. -/// -/// `ModalRoute.of(context).isCurrent` only answers for the route's own -/// navigator. With nested navigators that is not "nothing is on top": a route -/// pushed on an ancestor navigator — e.g. the root-navigator profile picker -/// over the nested profile-session navigator (#2034) — covers this route -/// while its own `isCurrent` stays true. Walks the hosting route of each -/// enclosing navigator up to the root, so any route stacked above [context]'s -/// route on any navigator makes this false. -bool isRouteChainCurrent(BuildContext context) { - ModalRoute? route = ModalRoute.of(context); - while (route != null) { - if (!route.isCurrent) return false; - final navigator = route.navigator; - if (navigator == null) break; - route = ModalRoute.of(navigator.context); - } - return true; -} diff --git a/lib/widgets/video_controls/video_controls.dart b/lib/widgets/video_controls/video_controls.dart index ac524ec36..cb69ba62b 100644 --- a/lib/widgets/video_controls/video_controls.dart +++ b/lib/widgets/video_controls/video_controls.dart @@ -64,7 +64,6 @@ import '../../utils/codec_utils.dart'; import '../../utils/formatters.dart'; import '../../utils/platform_detector.dart'; import '../../utils/player_utils.dart'; -import '../../utils/route_visibility.dart'; import '../../theme/mono_tokens.dart'; import '../../utils/provider_extensions.dart'; import '../../utils/snackbar_helper.dart'; @@ -1188,10 +1187,11 @@ class _PlexVideoControlsState extends State // 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 — - // on this navigator or an ancestor one (root-navigator dialogs/picker); - // re-activating the window must not pull the remote off the top route. - if (!isRouteChainCurrent(context)) return; + // A route pushed above the player on this navigator still leaves these + // controls mounted; re-activating the window must not pull the remote off + // the top route. (A root-navigator cover is handled for the whole session + // by CoveredRouteFocusBoundary, which makes the claim a no-op.) + if (ModalRoute.of(context)?.isCurrent != true) return; _claimPlayerSurfaceFocus(); } diff --git a/test/focus/covered_route_focus_boundary_test.dart b/test/focus/covered_route_focus_boundary_test.dart new file mode 100644 index 000000000..1abd11e7b --- /dev/null +++ b/test/focus/covered_route_focus_boundary_test.dart @@ -0,0 +1,156 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/focus/covered_route_focus_boundary.dart'; + +/// The shape from #2034/#2239: a nested navigator whose content sits under a +/// route pushed on the ROOT navigator. Nested-route `isCurrent` stays true +/// there, so a focus self-heal below must be neutralised by the boundary. +class _Harness { + _Harness(this.tester); + + final WidgetTester tester; + final rootNavigator = GlobalKey(); + final hidden = FocusNode(debugLabel: 'HiddenSidebarItem'); + final tile = FocusNode(debugLabel: 'ContentTile'); + final content = FocusScopeNode(debugLabel: 'ContentScope'); + final cover = FocusNode(debugLabel: 'PickerTile'); + late BuildContext nestedContext; + bool showTile = true; + StateSetter? _rebuildNested; + + Future pump({bool boundary = true}) async { + Widget session = Navigator( + onGenerateRoute: (_) => MaterialPageRoute( + builder: (context) => StatefulBuilder( + builder: (context, setState) { + nestedContext = context; + _rebuildNested = setState; + return Column( + children: [ + Focus(focusNode: hidden, child: const SizedBox(height: 10, width: 10)), + FocusScope( + node: content, + child: showTile + ? Focus(focusNode: tile, autofocus: true, child: const SizedBox(height: 10, width: 10)) + : const SizedBox(height: 10, width: 10), + ), + ], + ); + }, + ), + ), + ); + if (boundary) session = CoveredRouteFocusBoundary(child: session); + await tester.pumpWidget(MaterialApp(navigatorKey: rootNavigator, home: session)); + await tester.pump(); + expect(tile.hasPrimaryFocus, isTrue, reason: 'precondition: content tile autofocused'); + } + + Future pushCover() async { + rootNavigator.currentState!.push( + MaterialPageRoute( + builder: (_) => Focus(focusNode: cover, autofocus: true, child: const SizedBox.expand()), + ), + ); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + expect(cover.hasPrimaryFocus, isTrue, reason: 'precondition: covering route took focus'); + } + + Future popCover() async { + rootNavigator.currentState!.pop(); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + await tester.pump(); + } + + void unmountTile() => _rebuildNested!(() => showTile = false); + + void dispose() { + hidden.dispose(); + tile.dispose(); + content.dispose(); + cover.dispose(); + } +} + +void main() { + testWidgets('without the boundary a nested self-heal steals focus from the root cover', (tester) async { + final h = _Harness(tester); + addTearDown(h.dispose); + await h.pump(boundary: false); + await h.pushCover(); + + expect(ModalRoute.of(h.nestedContext)!.isCurrent, isTrue, reason: 'nested currency lies under a root cover'); + h.hidden.requestFocus(); + await tester.pump(); + + expect(h.hidden.hasPrimaryFocus, isTrue); + expect(h.cover.hasPrimaryFocus, isFalse); + }); + + testWidgets('requestFocus below a covered route is a no-op', (tester) async { + final h = _Harness(tester); + addTearDown(h.dispose); + await h.pump(); + await h.pushCover(); + + h.hidden.requestFocus(); + await tester.pump(); + + expect(h.hidden.hasPrimaryFocus, isFalse); + expect(h.cover.hasPrimaryFocus, isTrue); + }); + + testWidgets('focus returns to the leaf that had it once the cover pops', (tester) async { + final h = _Harness(tester); + addTearDown(h.dispose); + await h.pump(); + await h.pushCover(); + h.hidden.requestFocus(); + await tester.pump(); + + await h.popCover(); + + expect(h.tile.hasPrimaryFocus, isTrue); + }); + + testWidgets('a leaf unmounted while covered leaves focus on its scope, not the root scope', (tester) async { + final h = _Harness(tester); + addTearDown(h.dispose); + await h.pump(); + await h.pushCover(); + + h.unmountTile(); + await tester.pump(); + expect(h.cover.hasPrimaryFocus, isTrue, reason: 'unmounting below the cover must not disturb it'); + + await h.popCover(); + + expect(primaryFocus, isNot(same(FocusManager.instance.rootScope))); + expect(h.content.hasFocus, isTrue); + }); + + testWidgets('a route pushed on the nested navigator is not treated as a cover', (tester) async { + final h = _Harness(tester); + addTearDown(h.dispose); + await h.pump(); + final above = FocusNode(debugLabel: 'NestedAbove'); + addTearDown(above.dispose); + + Navigator.of(h.nestedContext).push( + MaterialPageRoute( + builder: (_) => Focus(focusNode: above, autofocus: true, child: const SizedBox.expand()), + ), + ); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + + expect(above.hasPrimaryFocus, isTrue); + // Flutter's own restoration still owns the nested pop. + Navigator.of(h.nestedContext).pop(); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + expect(h.tile.hasPrimaryFocus, isTrue); + }); +} diff --git a/test/navigation/profile_session_focus_boundary_test.dart b/test/navigation/profile_session_focus_boundary_test.dart new file mode 100644 index 000000000..15c88d1af --- /dev/null +++ b/test/navigation/profile_session_focus_boundary_test.dart @@ -0,0 +1,157 @@ +import 'package:drift/native.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/connection/connection_registry.dart'; +import 'package:plezy/database/app_database.dart'; +import 'package:plezy/models/plex/plex_home_user.dart'; +import 'package:plezy/navigation/profile_session_screen.dart'; +import 'package:plezy/profiles/active_profile_provider.dart'; +import 'package:plezy/profiles/plex_home_service.dart'; +import 'package:plezy/profiles/profile.dart'; +import 'package:plezy/profiles/profile_connection_registry.dart'; +import 'package:plezy/profiles/profile_registry.dart'; +import 'package:plezy/providers/multi_server_provider.dart'; +import 'package:plezy/services/multi_server_manager.dart'; +import 'package:plezy/services/offline_watch_sync_service.dart'; +import 'package:plezy/services/storage_service.dart'; +import 'package:plezy/services/system_shelf_service.dart'; +import 'package:provider/provider.dart'; + +import '../test_helpers/io_fakes.dart'; +import '../test_helpers/multi_server_fixtures.dart'; +import '../test_helpers/prefs.dart'; + +// Own file: SystemShelfService.debugReset awaits the mutation tail queued by +// the previous test's profile switch, which never settles outside that test's +// zone, so a second testWidgets in profile_session_screen_test.dart hangs. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + setUp(() async { + resetSharedPreferencesForTest(); + await SystemShelfService().debugReset(); + }); + + testWidgets( + 'a root-navigator route over the session blocks focus steals below it and hands focus back on pop (#2239)', + (tester) async { + final db = AppDatabase.forTesting(NativeDatabase.memory()); + final profileRegistry = ProfileRegistry(db); + final connectionRegistry = ConnectionRegistry(db); + final profileConnectionRegistry = ProfileConnectionRegistry(db); + final storage = await StorageService.getInstance(); + final plexHome = _FakePlexHomeService( + connections: connectionRegistry, + profileConnections: profileConnectionRegistry, + storage: storage, + ); + final activeProfile = ActiveProfileProvider( + registry: profileRegistry, + plexHome: plexHome, + connections: connectionRegistry, + profileConnections: profileConnectionRegistry, + storage: storage, + ); + final serverManager = MultiServerManager(); + final multiServer = testMultiServerProvider(serverManager); + final offlineWatch = OfflineWatchSyncService(database: db, serverManager: serverManager); + final rootNavigator = GlobalKey(); + final content = FocusNode(debugLabel: 'SessionContent'); + final sidebar = FocusNode(debugLabel: 'SessionSidebar'); + final picker = FocusNode(debugLabel: 'RootPicker'); + + addTearDown(() async { + await tester.pumpWidget(const SizedBox.shrink()); + await tester.pumpAndSettle(); + content.dispose(); + sidebar.dispose(); + picker.dispose(); + await activeProfile.resetForTesting(); + activeProfile.dispose(); + multiServer.dispose(); + serverManager.dispose(); + await plexHome.dispose(); + offlineWatch.dispose(); + await db.close(); + }); + + final owner = Profile.local(id: 'local-owner', displayName: 'Owner', createdAt: DateTime(2026, 1, 1)); + await profileRegistry.upsert(owner); + await storage.setActiveProfileId(owner.id); + await activeProfile.initialize(); + + await tester.pumpWidget( + MultiProvider( + providers: [ + Provider.value(value: storage), + Provider.value(value: db), + Provider.value(value: connectionRegistry), + Provider.value(value: profileConnectionRegistry), + Provider.value(value: plexHome), + ChangeNotifierProvider.value(value: activeProfile), + ChangeNotifierProvider.value(value: multiServer), + ChangeNotifierProvider.value(value: offlineWatch), + ], + child: MaterialApp( + navigatorKey: rootNavigator, + home: ProfileSessionScreen.forTesting( + initialPromptHandled: true, + httpClientFactory: () => FakeHttpClient(200, const []), + profileShellBuilder: (context) => Column( + children: [ + Focus(focusNode: sidebar, child: const SizedBox(height: 10, width: 10)), + Focus(focusNode: content, autofocus: true, child: const SizedBox(height: 10, width: 10)), + ], + ), + ), + ), + ), + ); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + expect(content.hasPrimaryFocus, isTrue); + + // The picker/PIN shape: pushed on the root navigator, above the whole + // nested profile-session navigator. + rootNavigator.currentState!.push( + MaterialPageRoute( + builder: (_) => Focus(focusNode: picker, autofocus: true, child: const SizedBox.expand()), + ), + ); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + expect(picker.hasPrimaryFocus, isTrue); + + // A resume-time self-heal under MainScreen (sidebar reveal, grid load). + sidebar.requestFocus(); + await tester.pump(); + expect(sidebar.hasPrimaryFocus, isFalse, reason: 'covered session must not take focus'); + expect(picker.hasPrimaryFocus, isTrue); + + rootNavigator.currentState!.pop(); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + expect(content.hasPrimaryFocus, isTrue, reason: 'focus returns to the session leaf that had it'); + }, + ); +} + +class _FakePlexHomeService extends PlexHomeService { + _FakePlexHomeService({required super.connections, required super.profileConnections, required StorageService storage}) + : super(storage: storage, plexHomeUserFetcher: (_) async => const []); + + @override + Map> get current => const {}; + + @override + Stream>> get stream => Stream.value(const {}); + + @override + Future start() async {} + + @override + Future reloadFromStorage() async {} + + @override + Future dispose() async {} +} diff --git a/test/utils/route_visibility_test.dart b/test/utils/route_visibility_test.dart deleted file mode 100644 index bbb1bcfc9..000000000 --- a/test/utils/route_visibility_test.dart +++ /dev/null @@ -1,58 +0,0 @@ -import 'dart:async'; - -import 'package:flutter/material.dart'; -import 'package:flutter_test/flutter_test.dart'; -import 'package:plezy/utils/route_visibility.dart'; - -void main() { - testWidgets('isRouteChainCurrent sees routes pushed on the own and ancestor navigators (#2034)', (tester) async { - final nestedNavigatorKey = GlobalKey(); - const contentKey = Key('nested-content'); - const nestedAboveKey = Key('nested-above'); - const rootAboveKey = Key('root-above'); - - await tester.pumpWidget( - MaterialApp( - home: Navigator( - key: nestedNavigatorKey, - onGenerateRoute: (settings) => MaterialPageRoute(builder: (_) => const SizedBox(key: contentKey)), - ), - ), - ); - - // The covered content stays mounted (maintainState) but goes offstage. - BuildContext content() => tester.element(find.byKey(contentKey, skipOffstage: false)); - - expect(isRouteChainCurrent(content()), isTrue, reason: 'sole route is current'); - - // A route above on the SAME (nested) navigator. - unawaited( - nestedNavigatorKey.currentState!.push( - MaterialPageRoute(builder: (_) => const SizedBox(key: nestedAboveKey)), - ), - ); - await tester.pumpAndSettle(); - expect(isRouteChainCurrent(content()), isFalse, reason: 'covered on own navigator'); - expect(isRouteChainCurrent(tester.element(find.byKey(nestedAboveKey))), isTrue); - - nestedNavigatorKey.currentState!.pop(); - await tester.pumpAndSettle(); - expect(isRouteChainCurrent(content()), isTrue, reason: 'uncovered after nested pop'); - - // A route above on the ROOT navigator — the profile-picker-over-player - // shape from #2034: the nested route's own isCurrent stays true. - unawaited( - Navigator.of( - content(), - rootNavigator: true, - ).push(MaterialPageRoute(builder: (_) => const SizedBox(key: rootAboveKey))), - ); - await tester.pumpAndSettle(); - expect(isRouteChainCurrent(content()), isFalse, reason: 'covered on ancestor navigator'); - expect(isRouteChainCurrent(tester.element(find.byKey(rootAboveKey))), isTrue); - - Navigator.of(content(), rootNavigator: true).pop(); - await tester.pumpAndSettle(); - expect(isRouteChainCurrent(content()), isTrue, reason: 'uncovered after root pop'); - }); -}