diff --git a/lib/features/command_palette/command_palette_dialog.dart b/lib/features/command_palette/command_palette_dialog.dart index 3cd0a206..99ea11ab 100644 --- a/lib/features/command_palette/command_palette_dialog.dart +++ b/lib/features/command_palette/command_palette_dialog.dart @@ -5,6 +5,7 @@ import 'package:querya_desktop/core/actions/querya_command_host.dart'; import 'package:querya_desktop/core/actions/querya_command_registry.dart'; import 'package:querya_desktop/core/layout/window_layout.dart'; import 'package:querya_desktop/features/command_palette/command_match_highlight.dart'; +import 'package:querya_desktop/features/command_palette/list_reveal.dart'; import 'package:querya_desktop/shared/widgets/widgets.dart'; /// Opens the Command Palette (`Ctrl/Cmd+P`). Motion Off snaps via [showAppDialog]. @@ -36,16 +37,29 @@ class CommandPaletteDialog extends StatefulWidget { class _CommandPaletteDialogState extends State { final _controller = TextEditingController(); final _focusNode = FocusNode(); + final _scroll = material.ScrollController(); var _selected = 0; + /// Set by keyboard navigation: the list moves under a still pointer, and the + /// hover that causes must not take the selection back. Cleared as soon as + /// the mouse really moves. + var _ignoreHover = false; + + static const _itemExtent = 36.0; + static const _listPadding = EdgeInsets.fromLTRB(8, 4, 8, 8); + @override void initState() { super.initState(); - _controller.addListener(() => setState(() => _selected = 0)); + _controller.addListener(() { + setState(() => _selected = 0); + if (_scroll.hasClients) _scroll.jumpTo(0); + }); } @override void dispose() { + _scroll.dispose(); _controller.dispose(); _focusNode.dispose(); super.dispose(); @@ -87,7 +101,30 @@ class _CommandPaletteDialogState extends State { setState(() { _selected = (_selected + delta) % hits.length; if (_selected < 0) _selected += hits.length; + _ignoreHover = true; }); + material.WidgetsBinding.instance.addPostFrameCallback((_) => _reveal()); + } + + /// Scrolls the list so the selected row is visible (#1369). + void _reveal() { + if (!mounted || !_scroll.hasClients) return; + final hits = _hits; + if (hits.isEmpty) return; + final index = _selected.clamp(0, hits.length - 1); + final position = _scroll.position; + final target = revealOffsetForItem( + index: index, + itemExtent: _itemExtent, + offset: position.pixels, + viewportExtent: position.viewportDimension, + leadingPadding: _listPadding.top, + trailingPadding: _listPadding.bottom, + ); + if (target == null) return; + _scroll.jumpTo( + target.clamp(position.minScrollExtent, position.maxScrollExtent), + ); } @override @@ -148,17 +185,26 @@ class _CommandPaletteDialogState extends State { child: Text('No matching commands'), ), ) - : ListView.builder( - padding: const EdgeInsets.fromLTRB(8, 4, 8, 8), + : material.Listener( + onPointerHover: (_) => _ignoreHover = false, + child: ListView.builder( + controller: _scroll, + padding: _listPadding, itemCount: hits.length, - itemExtent: 36, + itemExtent: _itemExtent, itemBuilder: (context, index) { final command = hits[index]; final active = index == selectedIndex; - return material.InkWell( + return material.Semantics( + button: true, + selected: active, + child: material.InkWell( key: ValueKey(command.id), onTap: () => _run(command), - onHover: (_) => setState(() => _selected = index), + onHover: (_) { + if (_ignoreHover) return; + setState(() => _selected = index); + }, borderRadius: BorderRadius.circular(6), child: DecoratedBox( decoration: BoxDecoration( @@ -191,9 +237,11 @@ class _CommandPaletteDialogState extends State { ), ), ), + ), ); }, ), + ), ), ], ), diff --git a/lib/features/command_palette/list_reveal.dart b/lib/features/command_palette/list_reveal.dart new file mode 100644 index 00000000..3a113ca9 --- /dev/null +++ b/lib/features/command_palette/list_reveal.dart @@ -0,0 +1,24 @@ +/// The scroll offset that brings item [index] of a fixed-extent list into the +/// viewport, or null when it is already fully visible (#1369). +/// +/// [offset] is the current scroll position and [viewportExtent] the visible +/// height. [leadingPadding] / [trailingPadding] are the list's own padding, so +/// the first and last rows can be revealed with their margin. +double? revealOffsetForItem({ + required int index, + required double itemExtent, + required double offset, + required double viewportExtent, + double leadingPadding = 0, + double trailingPadding = 0, +}) { + final top = leadingPadding + index * itemExtent; + final bottom = top + itemExtent; + if (top < offset) { + return (top - leadingPadding).clamp(0.0, double.infinity); + } + if (bottom > offset + viewportExtent) { + return bottom + trailingPadding - viewportExtent; + } + return null; +} diff --git a/lib/features/command_palette/quick_switcher_dialog.dart b/lib/features/command_palette/quick_switcher_dialog.dart index 492cdf9a..6704d1a9 100644 --- a/lib/features/command_palette/quick_switcher_dialog.dart +++ b/lib/features/command_palette/quick_switcher_dialog.dart @@ -5,6 +5,7 @@ import 'package:querya_desktop/core/actions/querya_command_host.dart'; import 'package:querya_desktop/core/actions/querya_schema_object.dart'; import 'package:querya_desktop/core/layout/window_layout.dart'; import 'package:querya_desktop/features/command_palette/command_match_highlight.dart'; +import 'package:querya_desktop/features/command_palette/list_reveal.dart'; import 'package:querya_desktop/shared/widgets/widgets.dart'; /// Threshold where filter runs on a worker isolate (hundreds of tables). @@ -53,7 +54,16 @@ class QuickSwitcherDialog extends StatefulWidget { class _QuickSwitcherDialogState extends State { late final TextEditingController _controller; final _focusNode = FocusNode(); + final _scroll = material.ScrollController(); var _selected = 0; + + /// Set by keyboard navigation: the list moves under a still pointer, and the + /// hover that causes must not take the selection back. Cleared as soon as + /// the mouse really moves. + var _ignoreHover = false; + + static const _itemExtent = 40.0; + static const _listPadding = EdgeInsets.fromLTRB(8, 4, 8, 8); var _loading = false; String? _error; List _all = const []; @@ -78,6 +88,7 @@ class _QuickSwitcherDialogState extends State { @override void dispose() { _controller.removeListener(_onQueryChanged); + _scroll.dispose(); _controller.dispose(); _focusNode.dispose(); super.dispose(); @@ -105,6 +116,7 @@ class _QuickSwitcherDialogState extends State { void _onQueryChanged() { setState(() => _selected = 0); + if (_scroll.hasClients) _scroll.jumpTo(0); _applyFilter(_controller.text); } @@ -133,7 +145,28 @@ class _QuickSwitcherDialogState extends State { setState(() { _selected = (_selected + delta) % _hits.length; if (_selected < 0) _selected += _hits.length; + _ignoreHover = true; }); + material.WidgetsBinding.instance.addPostFrameCallback((_) => _reveal()); + } + + /// Scrolls the list so the selected row is visible (#1369). + void _reveal() { + if (!mounted || !_scroll.hasClients || _hits.isEmpty) return; + final index = _selected.clamp(0, _hits.length - 1); + final position = _scroll.position; + final target = revealOffsetForItem( + index: index, + itemExtent: _itemExtent, + offset: position.pixels, + viewportExtent: position.viewportDimension, + leadingPadding: _listPadding.top, + trailingPadding: _listPadding.bottom, + ); + if (target == null) return; + _scroll.jumpTo( + target.clamp(position.minScrollExtent, position.maxScrollExtent), + ); } @override @@ -217,17 +250,26 @@ class _QuickSwitcherDialogState extends State { ), ); } - return ListView.builder( - padding: const EdgeInsets.fromLTRB(8, 4, 8, 8), + return material.Listener( + onPointerHover: (_) => _ignoreHover = false, + child: ListView.builder( + controller: _scroll, + padding: _listPadding, itemCount: hits.length, - itemExtent: 40, + itemExtent: _itemExtent, itemBuilder: (context, index) { final object = hits[index]; final active = index == selectedIndex; - return material.InkWell( + return material.Semantics( + button: true, + selected: active, + child: material.InkWell( key: ValueKey(object.id), onTap: () => _open(object), - onHover: (_) => setState(() => _selected = index), + onHover: (_) { + if (_ignoreHover) return; + setState(() => _selected = index); + }, borderRadius: BorderRadius.circular(6), child: DecoratedBox( decoration: BoxDecoration( @@ -261,8 +303,10 @@ class _QuickSwitcherDialogState extends State { ), ), ), + ), ); }, + ), ); } } diff --git a/test/features/command_palette/list_reveal_test.dart b/test/features/command_palette/list_reveal_test.dart new file mode 100644 index 00000000..bf6a293d --- /dev/null +++ b/test/features/command_palette/list_reveal_test.dart @@ -0,0 +1,38 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:querya_desktop/features/command_palette/list_reveal.dart'; + +void main() { + // 10 rows of 40 px in a 100 px viewport, 4 px leading and 8 px trailing. + double? reveal(int index, double offset) => revealOffsetForItem( + index: index, + itemExtent: 40, + offset: offset, + viewportExtent: 100, + leadingPadding: 4, + trailingPadding: 8, + ); + + test('a fully visible row needs no scroll', () { + expect(reveal(0, 0), isNull); + expect(reveal(1, 0), isNull); + }); + + test('a row below the viewport scrolls it up to the bottom edge', () { + // Row 2 spans 84..124; the viewport shows 0..100. + expect(reveal(2, 0), 124 + 8 - 100); + }); + + test('a row above the viewport scrolls back to it', () { + // Row 3 spans 124..164; the viewport shows 150..250. + expect(reveal(3, 150), 120); + }); + + test('the first row scrolls to the very top', () { + expect(reveal(0, 60), 0); + }); + + test('a partly visible row is revealed', () { + // Row 2 spans 84..124; the viewport shows 20..120 → bottom 4 px hidden. + expect(reveal(2, 20), 124 + 8 - 100); + }); +} diff --git a/test/features/command_palette/palette_keyboard_scroll_test.dart b/test/features/command_palette/palette_keyboard_scroll_test.dart new file mode 100644 index 00000000..265b4c8e --- /dev/null +++ b/test/features/command_palette/palette_keyboard_scroll_test.dart @@ -0,0 +1,138 @@ +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:querya_desktop/core/actions/querya_command.dart'; +import 'package:querya_desktop/core/actions/querya_command_registry.dart'; +import 'package:querya_desktop/core/actions/querya_schema_object.dart'; +import 'package:querya_desktop/core/motion/querya_motion_scope.dart'; +import 'package:querya_desktop/features/command_palette/command_palette_dialog.dart'; +import 'package:querya_desktop/features/command_palette/quick_switcher_dialog.dart'; +import 'package:shadcn_flutter/shadcn_flutter.dart'; + +import '../../support/querya_theme_test_shell.dart'; + +/// Scroll position of the result list of the open dialog. +double _listOffset(WidgetTester tester) { + final scrollable = find + .descendant( + of: find.byType(ListView), + matching: find.byType(Scrollable), + ) + .first; + return tester.state(scrollable).position.pixels; +} + +Future _openDialog( + WidgetTester tester, + Future Function(BuildContext context) open, +) async { + await tester.pumpWidget( + queryaThemeTestShell( + child: QueryaMotionScope( + level: QueryaMotionLevel.full, + child: Builder( + builder: (context) { + return Center( + child: Button.primary( + onPressed: () => open(context), + child: const Text('Open dialog'), + ), + ); + }, + ), + ), + ), + ); + await tester.tap(find.text('Open dialog')); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); +} + +Future _press(WidgetTester tester, LogicalKeyboardKey key, int times) async { + for (var i = 0; i < times; i++) { + await tester.sendKeyEvent(key); + await tester.pump(); + } +} + +void main() { + group('Command palette keeps the selected row in view (#1369)', () { + final registry = QueryaCommandRegistry.instance; + + setUp(registry.resetForTest); + tearDown(registry.resetForTest); + + testWidgets('Arrow Down past the viewport scrolls, Arrow Up scrolls back', + (tester) async { + registry.ensureCoreDefaults(); + for (var i = 0; i < 40; i++) { + registry.register( + QueryaCommand( + id: 'test.command_$i', + title: 'Scroll test command $i', + execute: (_) {}, + ), + ); + } + await _openDialog(tester, showCommandPalette); + + expect(_listOffset(tester), 0); + + await _press(tester, LogicalKeyboardKey.arrowDown, 20); + expect(_listOffset(tester), greaterThan(0)); + + await _press(tester, LogicalKeyboardKey.arrowUp, 20); + expect(_listOffset(tester), 0); + }); + + testWidgets('typing resets the list to the top', (tester) async { + registry.ensureCoreDefaults(); + for (var i = 0; i < 40; i++) { + registry.register( + QueryaCommand( + id: 'test.command_$i', + title: 'Scroll test command $i', + execute: (_) {}, + ), + ); + } + await _openDialog(tester, showCommandPalette); + + await _press(tester, LogicalKeyboardKey.arrowDown, 20); + expect(_listOffset(tester), greaterThan(0)); + + await tester.enterText(find.byType(TextField), 'Scroll test'); + await tester.pump(); + await tester.pump(); + + expect(_listOffset(tester), 0); + }); + }); + + group('Quick switcher keeps the selected row in view (#1369)', () { + final seed = [ + for (var i = 0; i < 40; i++) + QueryaSchemaObject.postgres( + database: 'app', + schema: 'public', + name: 'table_${i.toString().padLeft(2, '0')}', + kind: QueryaSchemaObjectKind.table, + ), + ]; + + testWidgets('Arrow Down past the viewport scrolls, Arrow Up scrolls back', + (tester) async { + await _openDialog( + tester, + (context) => showQuickSwitcher(context, seed: seed), + ); + + expect(_listOffset(tester), 0); + + await _press(tester, LogicalKeyboardKey.arrowDown, 20); + expect(_listOffset(tester), greaterThan(0)); + + await _press(tester, LogicalKeyboardKey.arrowUp, 20); + expect(_listOffset(tester), 0); + }); + }); +}