From bac2a0d2010123ad54d631607647a9960c0fbc8d Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Sun, 2 Aug 2026 07:35:43 +0200 Subject: [PATCH] fix(player): keep Delete and Home editing text in player sheets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bare Backspace and Home are player navigation keys, but they are also caret editing keys. The player screen's Focus wraps its OverlaySheetHost, so it saw them before the subtitle-search field could act: the press was consumed on key-down, DefaultTextEditingShortcuts never turned it into a deletion, and the back pipeline hid the chrome and then left the player. A focused text editor now takes both keys back, but only for physical keyboard presses — a synthesized dpad/gamepad press has no caret, and browserHome has no editing role at all. The screen also resolved its overlay-sheet controller from the State's own context, which sits above the host it was querying, so the lookup always returned null and Back skipped the sheet stage entirely. Resolve it from a context below the host instead, matching NowPlayingScreen. close #1741 --- lib/focus/key_event_utils.dart | 17 + lib/screens/video_player_screen.dart | 11 +- .../video_controls/video_controls.dart | 23 +- .../player_text_input_navigation_test.dart | 323 ++++++++++++++++++ test/widgets/video_controls_test.dart | 50 +++ 5 files changed, 421 insertions(+), 3 deletions(-) create mode 100644 test/screens/video_player/player_text_input_navigation_test.dart diff --git a/lib/focus/key_event_utils.dart b/lib/focus/key_event_utils.dart index 5262b43c..e9e0bd47 100644 --- a/lib/focus/key_event_utils.dart +++ b/lib/focus/key_event_utils.dart @@ -123,6 +123,23 @@ KeyEventResult handleOneShotSelect(KeyEvent event, VoidCallback onActivate) { return KeyEventResult.handled; } +/// Whether the primary focus currently belongs to an active text editor. +/// +/// Ancestor key handlers use this to stay off keys a focused field owns. +/// Flutter dispatches a key event from the focused node upwards, and the +/// editing shortcuts that turn Backspace into a deletion live in +/// [DefaultTextEditingShortcuts] at the very top of the app — *above* any +/// screen. An ancestor that claims Backspace as "back" therefore both steals +/// the navigation and stops the character from ever being deleted (#1741). +/// +/// [EditableText] builds its [Focus] internally, so the focused node's context +/// resolves to the owning [EditableTextState]. +bool isTextEditingFocused() { + final context = FocusManager.instance.primaryFocus?.context; + if (context == null) return false; + return context.findAncestorStateOfType() != null; +} + /// Expands a UTF-16 [range] to whole extended grapheme clusters in [text]. /// /// Flutter selections use UTF-16 code-unit offsets. Custom editors must pass a diff --git a/lib/screens/video_player_screen.dart b/lib/screens/video_player_screen.dart index a6d796fe..b2701a91 100644 --- a/lib/screens/video_player_screen.dart +++ b/lib/screens/video_player_screen.dart @@ -466,6 +466,14 @@ class VideoPlayerScreenState extends State with WidgetsBindin // key events never escape the video player route. late final FocusNode _screenFocusNode; + /// Key for a context below this screen's own [OverlaySheetHost]. The State's + /// context sits ABOVE the host, so resolving the controller with `context` + /// always misses it and the screen would walk its back pipeline (hide chrome, + /// then exit the player) while a sheet is still open (#1741). + final GlobalKey _overlayChildKey = GlobalKey(); + + BuildContext get _sheetContext => _overlayChildKey.currentContext ?? context; + // VLC-style in-player toast controller (rate changes, backend switch, etc.). final PlayerToastController _toastController = PlayerToastController(); bool _reclaimingFocus = false; @@ -1454,7 +1462,7 @@ class VideoPlayerScreenState extends State with WidgetsBindin void _handleScreenPlayerNavigation(PlayerNavigationKey navigationKey) { if (navigationKey != PlayerNavigationKey.home) { - final sheetController = OverlaySheetController.maybeOf(context); + final sheetController = OverlaySheetController.maybeOf(_sheetContext); if (sheetController?.isOpen ?? false) { sheetController!.pop(); return; @@ -1998,6 +2006,7 @@ class VideoPlayerScreenState extends State with WidgetsBindin _handleScreenPlayerNavigation(PlayerNavigationKey.back); }, child: Builder( + key: _overlayChildKey, builder: (sheetContext) => _isPlayerInitialized && player != null ? _buildVideoPlayer(sheetContext) : (_playerInitializationError != null diff --git a/lib/widgets/video_controls/video_controls.dart b/lib/widgets/video_controls/video_controls.dart index c39de772..4bff1baf 100644 --- a/lib/widgets/video_controls/video_controls.dart +++ b/lib/widgets/video_controls/video_controls.dart @@ -325,7 +325,22 @@ PlayerBackDisposition resolvePlayerBackDisposition({ return controlsVisible ? PlayerBackDisposition.hideControls : PlayerBackDisposition.exitPlayer; } -PlayerNavigationKey classifyPlayerNavigationKey(KeyEvent event, {required bool isAppleTV, bool? hasModifiers}) { +/// Maps a key event to the player-level navigation stage it should drive. +/// +/// [textEditingActive] defaults to [isTextEditingFocused]; inject it in tests. +/// Bare Backspace and Home double as player navigation *and* as caret editing +/// keys, so a focused text editor takes them back — otherwise typing in a +/// player sheet (subtitle search) walks the back pipeline out of the player +/// instead of correcting a character (#1741). Only physical-keyboard presses +/// are surrendered: a synthesized dpad/gamepad/companion press has no caret, +/// and [LogicalKeyboardKey.browserHome] is a dedicated navigation key with no +/// editing role at all. +PlayerNavigationKey classifyPlayerNavigationKey( + KeyEvent event, { + required bool isAppleTV, + bool? hasModifiers, + bool? textEditingActive, +}) { final key = event.logicalKey; if (key == LogicalKeyboardKey.escape) { return event.isPhysicalKeyboardEvent && !isAppleTV ? PlayerNavigationKey.physicalEscape : PlayerNavigationKey.back; @@ -338,10 +353,14 @@ PlayerNavigationKey classifyPlayerNavigationKey(KeyEvent event, {required bool i HardwareKeyboard.instance.isControlPressed || HardwareKeyboard.instance.isAltPressed || HardwareKeyboard.instance.isMetaPressed); + // Resolved lazily: only the two editing keys below pay for the focus lookup. + bool textEditorOwnsKey() => event.isPhysicalKeyboardEvent && (textEditingActive ?? isTextEditingFocused()); + if (key == LogicalKeyboardKey.backspace && event.isPhysicalKeyboardEvent && !modifiersPressed) { - return PlayerNavigationKey.back; + return textEditorOwnsKey() ? PlayerNavigationKey.none : PlayerNavigationKey.back; } if ((key == LogicalKeyboardKey.home || key == LogicalKeyboardKey.browserHome) && !modifiersPressed) { + if (key == LogicalKeyboardKey.home && textEditorOwnsKey()) return PlayerNavigationKey.none; return PlayerNavigationKey.home; } return PlayerNavigationKey.none; diff --git a/test/screens/video_player/player_text_input_navigation_test.dart b/test/screens/video_player/player_text_input_navigation_test.dart new file mode 100644 index 00000000..baff34c9 --- /dev/null +++ b/test/screens/video_player/player_text_input_navigation_test.dart @@ -0,0 +1,323 @@ +import 'dart:async'; + +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/focus/dpad_navigator.dart'; +import 'package:plezy/focus/focusable_text_field.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/widgets/overlay_sheet.dart'; +import 'package:plezy/widgets/video_controls/video_controls.dart'; +import 'package:provider/provider.dart'; + +import '../../test_helpers/media_items.dart'; +import '../../test_helpers/mock_player_channels.dart'; +import '../../test_helpers/prefs.dart'; + +/// Mirrors `VideoPlayerScreen.build`: a screen-level [Focus] that owns player +/// navigation, wrapping the screen's own [OverlaySheetHost]. Sheets therefore +/// sit *below* the navigation handler in the focus chain, which is what let a +/// Delete press inside the subtitle-search field walk the back pipeline out of +/// the player instead of correcting a character (#1741). +/// +/// Booting the real screen needs a live player, so this shell reproduces the +/// layering instead. That makes the Backspace/Home cases genuine regression +/// guards — they exercise the production [classifyPlayerNavigationKey] — while +/// the sheet-dismissal cases pin the wiring contract the screen must keep: +/// resolve [OverlaySheetController] from a context *below* the host +/// (`_overlayChildKey`/`_sheetContext`), never from the State's own context. +class _PlayerShell extends StatefulWidget { + final TextEditingController controller; + final FocusNode fieldFocusNode; + final FocusNode sheetButtonFocusNode; + final void Function(PlayerNavigationKey) onPlayerNavigation; + + const _PlayerShell({ + required this.controller, + required this.fieldFocusNode, + required this.sheetButtonFocusNode, + required this.onPlayerNavigation, + }); + + @override + State<_PlayerShell> createState() => _PlayerShellState(); +} + +class _PlayerShellState extends State<_PlayerShell> { + final FocusNode _screenFocusNode = FocusNode(debugLabel: 'VideoPlayerScreen'); + final GlobalKey _overlayChildKey = GlobalKey(); + + BuildContext get _sheetContext => _overlayChildKey.currentContext ?? context; + + @override + void dispose() { + _screenFocusNode.dispose(); + super.dispose(); + } + + void _handleScreenPlayerNavigation(PlayerNavigationKey navigationKey) { + if (navigationKey != PlayerNavigationKey.home) { + final sheetController = OverlaySheetController.maybeOf(_sheetContext); + if (sheetController?.isOpen ?? false) { + sheetController!.pop(); + return; + } + } + widget.onPlayerNavigation(navigationKey); + } + + @override + Widget build(BuildContext context) { + return Focus( + focusNode: _screenFocusNode, + autofocus: true, + onKeyEvent: (node, event) { + final navigationKey = classifyPlayerNavigationKey(event, isAppleTV: false); + if (navigationKey != PlayerNavigationKey.none) { + return handlePlayerNavigationKeyAction( + event, + navigationKey, + () => _handleScreenPlayerNavigation(navigationKey), + ); + } + if (node.hasPrimaryFocus) { + return event.logicalKey.isNavigationKey ? KeyEventResult.handled : KeyEventResult.ignored; + } + return KeyEventResult.ignored; + }, + child: OverlaySheetHost( + canPop: false, + onSystemBack: () => _handleScreenPlayerNavigation(PlayerNavigationKey.back), + child: Builder( + key: _overlayChildKey, + builder: (sheetContext) => Scaffold( + body: Center( + child: ElevatedButton( + onPressed: () => OverlaySheetController.of(sheetContext).show( + builder: (_) => Column( + mainAxisSize: MainAxisSize.min, + children: [ + FocusableTextField( + controller: widget.controller, + focusNode: widget.fieldFocusNode, + tvTextInputPresentation: TvTextInputPresentation.platform, + ), + TextButton( + focusNode: widget.sheetButtonFocusNode, + onPressed: () {}, + child: const Text('Download'), + ), + ], + ), + ), + child: const Text('Search subtitles'), + ), + ), + ), + ), + ), + ); + } +} + +class _Harness { + final TextEditingController controller; + final FocusNode fieldFocusNode; + final FocusNode sheetButtonFocusNode; + final List navigations; + + _Harness({ + required this.controller, + required this.fieldFocusNode, + required this.sheetButtonFocusNode, + required this.navigations, + }); + + bool get sheetIsOpen => find.byType(FocusableTextField).evaluate().isNotEmpty; +} + +Future<_Harness> _pumpPlayerWithOpenSearchSheet(WidgetTester tester) async { + final controller = TextEditingController(); + addTearDown(controller.dispose); + final fieldFocusNode = FocusNode(debugLabel: 'subtitleSearchField'); + addTearDown(fieldFocusNode.dispose); + final sheetButtonFocusNode = FocusNode(debugLabel: 'sheetButton'); + addTearDown(sheetButtonFocusNode.dispose); + final navigations = []; + + await tester.pumpWidget( + MaterialApp( + home: _PlayerShell( + controller: controller, + fieldFocusNode: fieldFocusNode, + sheetButtonFocusNode: sheetButtonFocusNode, + onPlayerNavigation: navigations.add, + ), + ), + ); + + await tester.tap(find.text('Search subtitles')); + await tester.pumpAndSettle(); + + return _Harness( + controller: controller, + fieldFocusNode: fieldFocusNode, + sheetButtonFocusNode: sheetButtonFocusNode, + navigations: navigations, + ); +} + +Future _focusFieldWithText(WidgetTester tester, _Harness harness, String text, {required int caret}) async { + await tester.tap(find.byType(TextField)); + await tester.pumpAndSettle(); + await tester.enterText(find.byType(TextField), text); + await tester.pumpAndSettle(); + harness.controller.selection = TextSelection.collapsed(offset: caret); + await tester.pump(); + expect(harness.fieldFocusNode.hasPrimaryFocus, isTrue); +} + +Future _pressKey(WidgetTester tester, LogicalKeyboardKey key) async { + await tester.sendKeyDownEvent(key); + await tester.sendKeyUpEvent(key); + await tester.pumpAndSettle(); +} + +void main() { + testWidgets('Backspace in the subtitle search field edits text instead of leaving the player', (tester) async { + final harness = await _pumpPlayerWithOpenSearchSheet(tester); + await _focusFieldWithText(tester, harness, 'star wars', caret: 5); + + await _pressKey(tester, LogicalKeyboardKey.backspace); + + expect(harness.controller.text, 'starwars'); + expect(harness.navigations, isEmpty); + expect(harness.sheetIsOpen, isTrue); + }); + + testWidgets('Home in the subtitle search field moves the caret instead of leaving the player', (tester) async { + final harness = await _pumpPlayerWithOpenSearchSheet(tester); + await _focusFieldWithText(tester, harness, 'star wars', caret: 5); + + await _pressKey(tester, LogicalKeyboardKey.home); + + // Proves the unhandled event reached DefaultTextEditingShortcuts rather + // than being consumed by the screen's navigation handler. + expect(harness.controller.selection, const TextSelection.collapsed(offset: 0)); + expect(harness.controller.text, 'star wars'); + expect(harness.navigations, isEmpty); + expect(harness.sheetIsOpen, isTrue); + }); + + testWidgets('Backspace on a non-text sheet control closes the sheet without leaving the player', (tester) async { + final harness = await _pumpPlayerWithOpenSearchSheet(tester); + harness.sheetButtonFocusNode.requestFocus(); + await tester.pumpAndSettle(); + + await _pressKey(tester, LogicalKeyboardKey.backspace); + + expect(harness.sheetIsOpen, isFalse); + expect(harness.navigations, isEmpty); + }); + + testWidgets('Backspace with no sheet open still drives player Back', (tester) async { + final harness = await _pumpPlayerWithOpenSearchSheet(tester); + harness.sheetButtonFocusNode.requestFocus(); + await tester.pumpAndSettle(); + await _pressKey(tester, LogicalKeyboardKey.backspace); + expect(harness.sheetIsOpen, isFalse); + + await _pressKey(tester, LogicalKeyboardKey.backspace); + + expect(harness.navigations, [PlayerNavigationKey.back]); + }); + + testWidgets('Escape still closes an open sheet', (tester) async { + final harness = await _pumpPlayerWithOpenSearchSheet(tester); + await _focusFieldWithText(tester, harness, 'star wars', caret: 5); + + await _pressKey(tester, LogicalKeyboardKey.escape); + + expect(harness.sheetIsOpen, isFalse); + expect(harness.navigations, isEmpty); + }); + + // Binds the fix to the real screen: the shell tests above cannot catch a + // regression of `_overlayChildKey`/`_sheetContext`, because they model the + // corrected wiring themselves. Resolving the host from the State's own + // context (which sits ABOVE it) silently returns null, so Back skips the + // sheet stage and runs the exit pipeline instead. + testWidgets('the player screen resolves its own sheet host before exiting', (tester) async { + resetSharedPreferencesForTest(); + SettingsService.resetForTesting(); + await SettingsService.getInstance(); + + final nativeInitialize = Completer(); + final navigatorKey = GlobalKey(); + final sheetButtonFocusNode = FocusNode(debugLabel: 'playerSheetButton'); + addTearDown(sheetButtonFocusNode.dispose); + + await withMockPlayerChannels( + methodChannelName: 'com.plezy/mpv_player', + eventChannelName: 'com.plezy/mpv_player/events', + // Never completes: the screen stays on its initialization surface, so no + // real player is needed and the chrome is not presented — the state in + // which a leaked Back goes straight to exitPlayer. + methodHandler: (call) => call.method == 'initialize' ? nativeInitialize.future : Future.value(), + eventHandler: (_) async => null, + testBody: () async { + await tester.pumpWidget( + ChangeNotifierProvider( + create: (_) => PlaybackStateProvider(), + child: MaterialApp( + navigatorKey: navigatorKey, + home: const Scaffold(body: Center(child: Text('behind the player'))), + ), + ), + ); + + unawaited( + navigatorKey.currentState!.push( + MaterialPageRoute( + builder: (_) => VideoPlayerScreen(metadata: testMediaItem(title: 'Sheet host test'), isOffline: true), + ), + ), + ); + // The loading spinner animates forever, so pumpAndSettle would hang. + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + expect(find.byType(VideoPlayerScreen), findsOneWidget); + + // A context below the screen's own OverlaySheetHost. + final sheetContext = tester.element(find.byType(CircularProgressIndicator)); + unawaited( + OverlaySheetController.of(sheetContext).show( + builder: (_) => + TextButton(focusNode: sheetButtonFocusNode, onPressed: () {}, child: const Text('Sheet action')), + ), + ); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 300)); + expect(find.text('Sheet action'), findsOneWidget); + + sheetButtonFocusNode.requestFocus(); + await tester.pump(); + + await tester.sendKeyDownEvent(LogicalKeyboardKey.backspace); + await tester.sendKeyUpEvent(LogicalKeyboardKey.backspace); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + await tester.pump(const Duration(seconds: 1)); + + expect(find.text('Sheet action'), findsNothing, reason: 'Back must close the sheet'); + expect(find.byType(VideoPlayerScreen), findsOneWidget, reason: 'Back must not also leave the player'); + + await tester.pumpWidget(const SizedBox.shrink()); + nativeInitialize.complete(true); + await tester.pump(); + }, + ); + }); +} diff --git a/test/widgets/video_controls_test.dart b/test/widgets/video_controls_test.dart index bc9817d7..231b8a25 100644 --- a/test/widgets/video_controls_test.dart +++ b/test/widgets/video_controls_test.dart @@ -362,6 +362,56 @@ void main() { ); } }); + + test('surrenders bare Backspace to a focused text editor', () { + final event = _navigationKeyDown(LogicalKeyboardKey.backspace, ui.KeyEventDeviceType.keyboard); + + expect( + classifyPlayerNavigationKey(event, isAppleTV: false, hasModifiers: false, textEditingActive: true), + PlayerNavigationKey.none, + ); + expect( + classifyPlayerNavigationKey(event, isAppleTV: false, hasModifiers: false, textEditingActive: false), + PlayerNavigationKey.back, + ); + }); + + test('surrenders bare Home to a focused text editor but never browser Home', () { + expect( + classifyPlayerNavigationKey( + _navigationKeyDown(LogicalKeyboardKey.home, ui.KeyEventDeviceType.keyboard), + isAppleTV: false, + hasModifiers: false, + textEditingActive: true, + ), + PlayerNavigationKey.none, + ); + // browserHome has no caret role, so an editor never takes it. + expect( + classifyPlayerNavigationKey( + _navigationKeyDown(LogicalKeyboardKey.browserHome, ui.KeyEventDeviceType.keyboard), + isAppleTV: false, + hasModifiers: false, + textEditingActive: true, + ), + PlayerNavigationKey.home, + ); + }); + + test('keeps simulated remote Home navigating while a text editor has focus', () { + for (final deviceType in [ui.KeyEventDeviceType.directionalPad, ui.KeyEventDeviceType.gamepad]) { + expect( + classifyPlayerNavigationKey( + _navigationKeyDown(LogicalKeyboardKey.home, deviceType), + isAppleTV: false, + hasModifiers: false, + textEditingActive: true, + ), + PlayerNavigationKey.home, + reason: 'a synthesized remote press has no caret to move', + ); + } + }); }); group('handlePlayerNavigationKeyAction', () {