From 097e48d7059e1a427f63be4c42f007882ef810a7 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Fri, 29 May 2026 03:31:53 +0200 Subject: [PATCH] fix(tv): trap d-pad focus at button row edges close #1181 --- lib/focus/focusable_action_bar.dart | 3 + lib/focus/key_event_utils.dart | 41 ++++- lib/screens/media_detail_screen.dart | 12 +- test/focus/key_event_utils_test.dart | 263 ++++++++++++++++++++++++++- 4 files changed, 311 insertions(+), 8 deletions(-) diff --git a/lib/focus/focusable_action_bar.dart b/lib/focus/focusable_action_bar.dart index aa44ce4c..7316cfe0 100644 --- a/lib/focus/focusable_action_bar.dart +++ b/lib/focus/focusable_action_bar.dart @@ -140,6 +140,9 @@ class FocusableActionBarState extends State { : widget.onNavigateRight, onDown: widget.onNavigateDown, onUp: widget.onNavigateUp, + // Consume LEFT/RIGHT at the row's first/last button when no edge + // callback is wired, so focus can't fall off the row (#1181). + trapHorizontalEdges: true, )(node, event); }, child: ClickableCursor( diff --git a/lib/focus/key_event_utils.dart b/lib/focus/key_event_utils.dart index ef1dc5bb..45f2dcdb 100644 --- a/lib/focus/key_event_utils.dart +++ b/lib/focus/key_event_utils.dart @@ -120,6 +120,13 @@ KeyEventResult handleOneShotSelect(KeyEvent event, VoidCallback onActivate) { /// (passed through to the framework). Directions mapped to a callback /// automatically return [KeyEventResult.handled]. /// +/// When [trapHorizontalEdges] is true, LEFT/RIGHT with no callback return +/// [KeyEventResult.handled] (consumed) instead of being passed through. Use +/// this for a self-contained horizontal group (e.g. a button row) so D-pad +/// can't escape off the edge into an off-screen "black hole" (#1181); wire an +/// explicit [onLeft]/[onRight] only where edge-escape into another region is +/// intended. UP/DOWN are unaffected and always pass through when unmapped. +/// /// Directional keys repeat on [KeyRepeatEvent] (via [isActionable]). /// Select is one-shot: fires on [KeyDownEvent] only, consumes repeat and up. /// @@ -140,6 +147,7 @@ FocusOnKeyEventCallback dpadKeyHandler({ VoidCallback? onLeft, VoidCallback? onRight, VoidCallback? onSelect, + bool trapHorizontalEdges = false, }) { return (FocusNode _, KeyEvent event) { // Select: one-shot activation (no repeat), must run before isActionable @@ -160,19 +168,40 @@ FocusOnKeyEventCallback dpadKeyHandler({ onDown(); return KeyEventResult.handled; } - if (key.isLeftKey && onLeft != null) { - onLeft(); - return KeyEventResult.handled; + if (key.isLeftKey) { + if (onLeft != null) { + onLeft(); + return KeyEventResult.handled; + } + if (trapHorizontalEdges) return KeyEventResult.handled; } - if (key.isRightKey && onRight != null) { - onRight(); - return KeyEventResult.handled; + if (key.isRightKey) { + if (onRight != null) { + onRight(); + return KeyEventResult.handled; + } + if (trapHorizontalEdges) return KeyEventResult.handled; } return KeyEventResult.ignored; }; } +/// Whether [container] has another focusable descendant beyond the currently +/// focused one in [direction]. Lets a row's key handler move between interior +/// items (true) but trap at the row's edge (false) so focus can't escape into +/// an off-screen "black hole" (#1181). Horizontal only. +/// +/// Relies on [FocusNode.traversalDescendants] being in reading (left→right) +/// order, which holds for a flat [Row]. +bool hasHorizontalNeighbor(FocusNode container, TraversalDirection direction) { + assert(direction == TraversalDirection.left || direction == TraversalDirection.right); + final items = container.traversalDescendants.toList(); + final index = items.indexWhere((node) => node.hasPrimaryFocus); + if (index < 0) return false; + return direction == TraversalDirection.right ? index < items.length - 1 : index > 0; +} + /// Navigator observer that automatically suppresses stray back KeyUp events /// after any route pop caused by a back key press. /// diff --git a/lib/screens/media_detail_screen.dart b/lib/screens/media_detail_screen.dart index 54c4927a..20009ac5 100644 --- a/lib/screens/media_detail_screen.dart +++ b/lib/screens/media_detail_screen.dart @@ -1013,6 +1013,9 @@ class _MediaDetailScreenState extends State if (key.isUpKey) { return KeyEventResult.handled; // consume — nothing above } + if (key.isLeftKey || key.isRightKey) { + return KeyEventResult.handled; // consume — single chip, nothing beside it (#1181) + } return KeyEventResult.ignored; }, child: GestureDetector( @@ -2001,7 +2004,7 @@ class _MediaDetailScreenState extends State } /// Intercept DOWN from the play button row to focus the first available section - KeyEventResult _handlePlayButtonKeyEvent(FocusNode _, KeyEvent event) { + KeyEventResult _handlePlayButtonKeyEvent(FocusNode node, KeyEvent event) { final key = event.logicalKey; if (!event.isActionable) return KeyEventResult.ignored; final isTv = PlatformDetector.isTV(); @@ -2019,6 +2022,13 @@ class _MediaDetailScreenState extends State return KeyEventResult.handled; } + // LEFT/RIGHT: let the framework move between buttons in the row, but trap + // at the row's edges so focus can't fall off into a black hole (#1181). + if (key.isLeftKey || key.isRightKey) { + final dir = key.isRightKey ? TraversalDirection.right : TraversalDirection.left; + return hasHorizontalNeighbor(node, dir) ? KeyEventResult.ignored : KeyEventResult.handled; + } + if (!key.isDownKey) return KeyEventResult.ignored; final metadata = _fullMetadata ?? _metadata; diff --git a/test/focus/key_event_utils_test.dart b/test/focus/key_event_utils_test.dart index bab6a73a..2011f5fd 100644 --- a/test/focus/key_event_utils_test.dart +++ b/test/focus/key_event_utils_test.dart @@ -1,9 +1,10 @@ import 'dart:ui' as ui; +import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; -import 'package:flutter/widgets.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:plezy/focus/dpad_navigator.dart'; +import 'package:plezy/focus/focusable_action_bar.dart'; import 'package:plezy/focus/key_event_utils.dart'; import 'package:plezy/utils/platform_detector.dart'; @@ -41,4 +42,264 @@ void main() { expect(upResult, KeyEventResult.handled); expect(backs, 1); }); + + group('hasHorizontalNeighbor', () { + testWidgets('reports interior vs. edge neighbors for a flat button row', (tester) async { + final container = FocusNode(debugLabel: 'row', skipTraversal: true); + final play = FocusNode(debugLabel: 'play'); + final download = FocusNode(debugLabel: 'download'); + final more = FocusNode(debugLabel: 'more'); + for (final node in [container, play, download, more]) { + addTearDown(node.dispose); + } + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Focus( + focusNode: container, + skipTraversal: true, + child: Row( + mainAxisSize: MainAxisSize.min, + children: [ + FilledButton(focusNode: play, onPressed: () {}, child: const Text('Play')), + IconButton(focusNode: download, onPressed: () {}, icon: const Icon(Icons.download)), + IconButton(focusNode: more, onPressed: () {}, icon: const Icon(Icons.more_vert)), + ], + ), + ), + ), + ), + ); + await tester.pump(); + + // The ordinal helper assumes one traversal node per button, in reading + // order. Assert it so a framework change that breaks the assumption fails + // loudly here rather than silently re-opening the black hole. + expect(container.traversalDescendants.length, 3); + + play.requestFocus(); + await tester.pump(); + expect(hasHorizontalNeighbor(container, TraversalDirection.left), isFalse); + expect(hasHorizontalNeighbor(container, TraversalDirection.right), isTrue); + + download.requestFocus(); + await tester.pump(); + expect(hasHorizontalNeighbor(container, TraversalDirection.left), isTrue); + expect(hasHorizontalNeighbor(container, TraversalDirection.right), isTrue); + + more.requestFocus(); + await tester.pump(); + expect(hasHorizontalNeighbor(container, TraversalDirection.right), isFalse); + expect(hasHorizontalNeighbor(container, TraversalDirection.left), isTrue); + }); + + testWidgets('single-item row has no horizontal neighbor either way (rating-chip case)', (tester) async { + final container = FocusNode(debugLabel: 'row', skipTraversal: true); + final only = FocusNode(debugLabel: 'chip'); + addTearDown(container.dispose); + addTearDown(only.dispose); + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Focus( + focusNode: container, + skipTraversal: true, + child: Focus(focusNode: only, child: const SizedBox(width: 50, height: 50)), + ), + ), + ), + ); + await tester.pump(); + only.requestFocus(); + await tester.pump(); + + expect(hasHorizontalNeighbor(container, TraversalDirection.left), isFalse); + expect(hasHorizontalNeighbor(container, TraversalDirection.right), isFalse); + }); + + testWidgets('returns false when focus is outside the container', (tester) async { + final container = FocusNode(debugLabel: 'row'); + final inside = FocusNode(debugLabel: 'inside'); + final outside = FocusNode(debugLabel: 'outside'); + for (final node in [container, inside, outside]) { + addTearDown(node.dispose); + } + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Row( + children: [ + Focus( + focusNode: container, + child: Focus(focusNode: inside, child: const SizedBox(width: 40, height: 40)), + ), + Focus(focusNode: outside, child: const SizedBox(width: 40, height: 40)), + ], + ), + ), + ), + ); + await tester.pump(); + outside.requestFocus(); + await tester.pump(); + + expect(hasHorizontalNeighbor(container, TraversalDirection.left), isFalse); + expect(hasHorizontalNeighbor(container, TraversalDirection.right), isFalse); + }); + }); + + group('dpadKeyHandler trapHorizontalEdges', () { + testWidgets('consumes edge LEFT/RIGHT so focus cannot escape the group', (tester) async { + final trapped = FocusNode(debugLabel: 'trapped'); + final outside = FocusNode(debugLabel: 'outside'); + addTearDown(trapped.dispose); + addTearDown(outside.dispose); + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Row( + children: [ + Focus( + focusNode: trapped, + onKeyEvent: dpadKeyHandler(trapHorizontalEdges: true), + child: const SizedBox(width: 50, height: 50), + ), + Focus(focusNode: outside, child: const SizedBox(width: 50, height: 50)), + ], + ), + ), + ), + ); + await tester.pump(); + trapped.requestFocus(); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'trapped'); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'trapped'); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'trapped'); + }); + + testWidgets('default (no trap) still lets edge RIGHT pass through to the framework', (tester) async { + final node = FocusNode(debugLabel: 'node'); + final outside = FocusNode(debugLabel: 'outside'); + addTearDown(node.dispose); + addTearDown(outside.dispose); + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Row( + children: [ + Focus(focusNode: node, onKeyEvent: dpadKeyHandler(), child: const SizedBox(width: 50, height: 50)), + Focus(focusNode: outside, child: const SizedBox(width: 50, height: 50)), + ], + ), + ), + ), + ); + await tester.pump(); + node.requestFocus(); + await tester.pump(); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'outside'); + }); + }); + + group('FocusableActionBar edge trapping', () { + testWidgets('traps LEFT/RIGHT at row edges when no horizontal nav is wired', (tester) async { + final key = GlobalKey(); + final outside = FocusNode(debugLabel: 'outside'); + addTearDown(outside.dispose); + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Row( + children: [ + FocusableActionBar( + key: key, + actions: [ + FocusableAction(icon: Icons.add, onPressed: () {}), + FocusableAction(icon: Icons.remove, onPressed: () {}), + ], + ), + Focus(focusNode: outside, child: const SizedBox(width: 50, height: 50)), + ], + ), + ), + ), + ); + await tester.pump(); + + key.currentState!.requestFocusOnFirst(); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'ActionBar[0]'); + + // Interior RIGHT moves to the next button. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'ActionBar[1]'); + + // RIGHT at the last button is trapped — must NOT escape to 'outside'. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'ActionBar[1]'); + + // LEFT back to the first, then LEFT again is trapped. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'ActionBar[0]'); + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'ActionBar[0]'); + }); + + testWidgets('still invokes onNavigateLeft at the left edge when wired', (tester) async { + final key = GlobalKey(); + final leftTarget = FocusNode(debugLabel: 'left-target'); + addTearDown(leftTarget.dispose); + var navigatedLeft = false; + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Row( + children: [ + Focus(focusNode: leftTarget, child: const SizedBox(width: 50, height: 50)), + FocusableActionBar( + key: key, + onNavigateLeft: () { + navigatedLeft = true; + leftTarget.requestFocus(); + }, + actions: [FocusableAction(icon: Icons.add, onPressed: () {})], + ), + ], + ), + ), + ), + ); + await tester.pump(); + + key.currentState!.requestFocusOnFirst(); + await tester.pump(); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'ActionBar[0]'); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + expect(navigatedLeft, isTrue); + expect(FocusManager.instance.primaryFocus?.debugLabel, 'left-target'); + }); + }); }