diff --git a/lib/ui/confirm.dart b/lib/ui/confirm.dart new file mode 100644 index 0000000..57bd100 --- /dev/null +++ b/lib/ui/confirm.dart @@ -0,0 +1,84 @@ +/// Confirmation for destructive actions. +library; + +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; + +/// Asks the user to confirm a destructive action, returning true if they do. +/// +/// Exists because "delete note" had no confirmation and no undo on either +/// path: the actions sheet and the detail screen both deleted immediately. +/// With a pointer that is merely risky — you have to travel to the control and +/// press it. With a keyboard it is one Tab away from a Return you were about +/// to press anyway, and the note is gone with nothing to recover it. +/// +/// Defaults are chosen so the *safe* option is what a reflexive keypress hits: +/// Cancel is autofocused, and Escape dismisses. The destructive button is +/// styled with the error color but is deliberately not the default action. +/// +/// Returns false when dismissed by Escape, by the barrier, or by Cancel. +Future confirmDestructive( + BuildContext context, { + required String title, + required String message, + String confirmLabel = 'Delete', + String cancelLabel = 'Cancel', +}) async { + final confirmed = await showDialog( + context: context, + builder: (dialogContext) => _ConfirmDialog( + title: title, + message: message, + confirmLabel: confirmLabel, + cancelLabel: cancelLabel, + ), + ); + return confirmed ?? false; +} + +class _ConfirmDialog extends StatelessWidget { + const _ConfirmDialog({ + required this.title, + required this.message, + required this.confirmLabel, + required this.cancelLabel, + }); + + final String title; + final String message; + final String confirmLabel; + final String cancelLabel; + + @override + Widget build(BuildContext context) { + return CallbackShortcuts( + // Escape dismisses as "no". Flutter's own barrier handles this on most + // platforms, but the desktop surface here is a Chrome `--app` window + // where that cannot be assumed, and a confirmation the keyboard cannot + // back out of is worse than no confirmation at all. + bindings: { + const SingleActivator(LogicalKeyboardKey.escape): () => + Navigator.of(context).pop(false), + }, + child: AlertDialog( + title: Text(title), + content: Text(message), + actions: [ + TextButton( + // Autofocused so the reflexive Return presses Cancel, not Delete. + autofocus: true, + onPressed: () => Navigator.of(context).pop(false), + child: Text(cancelLabel), + ), + TextButton( + style: TextButton.styleFrom( + foregroundColor: Theme.of(context).colorScheme.error, + ), + onPressed: () => Navigator.of(context).pop(true), + child: Text(confirmLabel), + ), + ], + ), + ); + } +} diff --git a/lib/ui/note_detail_screen.dart b/lib/ui/note_detail_screen.dart index 0ef7be4..77fa417 100644 --- a/lib/ui/note_detail_screen.dart +++ b/lib/ui/note_detail_screen.dart @@ -3,6 +3,7 @@ import 'package:flutter/material.dart'; import 'package:todo/data/note.dart'; import 'package:todo/data/note_repository.dart'; import 'package:todo/data/note_template.dart'; +import 'package:todo/ui/confirm.dart'; import 'package:todo/ui/note_editor.dart'; /// Full-screen view of a single note: read it in full, edit its body through @@ -45,6 +46,12 @@ class _NoteDetailScreenState extends State { _persist(_note.copyWith(text: text, updatedAt: DateTime.now())); Future _delete() async { + final confirmed = await confirmDestructive( + context, + title: 'Delete note?', + message: 'This cannot be undone.', + ); + if (!confirmed) return; await widget.repository.delete(_note.id); if (mounted) Navigator.of(context).pop(); } diff --git a/lib/ui/notes_list_screen.dart b/lib/ui/notes_list_screen.dart index 262e973..297aa7c 100644 --- a/lib/ui/notes_list_screen.dart +++ b/lib/ui/notes_list_screen.dart @@ -5,6 +5,7 @@ import 'package:flutter/material.dart'; import 'package:todo/data/note.dart'; import 'package:todo/data/note_repository.dart'; import 'package:todo/data/note_template.dart'; +import 'package:todo/ui/confirm.dart'; import 'package:todo/ui/note_detail_screen.dart'; /// The default status selection: hide completed/dropped work. This is the @@ -125,6 +126,11 @@ class _NotesListScreenState extends State { await showModalBottomSheet( context: context, showDragHandle: true, + // Scroll-controlled so the sheet can exceed the default 9/16 of the + // viewport. On a 768px-tall screen that default is only ~432px, and this + // sheet's content grows with the note title and its chip rows -- an + // unscrollable sheet would clip the Delete action out of reach. + isScrollControlled: true, builder: (_) => _NoteActionsSheet( note: note, onChanged: (updated) async { @@ -375,6 +381,12 @@ class _NoteActionsSheetState extends State<_NoteActionsSheet> { foregroundColor: Theme.of(context).colorScheme.error, ), onPressed: () async { + final confirmed = await confirmDestructive( + context, + title: 'Delete note?', + message: 'This cannot be undone.', + ); + if (!confirmed) return; await widget.onDelete(); if (context.mounted) Navigator.of(context).pop(); }, diff --git a/test/confirm_test.dart b/test/confirm_test.dart new file mode 100644 index 0000000..246b51e --- /dev/null +++ b/test/confirm_test.dart @@ -0,0 +1,105 @@ +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:todo/ui/confirm.dart'; + +/// Tests for the destructive-action confirmation. +/// +/// Both delete paths (the actions sheet and the detail screen) used to delete +/// immediately with no confirmation and no undo. With a keyboard that is one +/// Tab away from a Return the user was about to press anyway, so the defaults +/// here — Cancel autofocused, Escape dismisses — are the actual safety +/// property, not decoration. +void main() { + Future showFor(WidgetTester tester) async { + bool? result; + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Builder( + builder: (context) => ElevatedButton( + onPressed: () async { + result = await confirmDestructive( + context, + title: 'Delete note?', + message: 'This cannot be undone.', + ); + }, + child: const Text('go'), + ), + ), + ), + ), + ); + await tester.tap(find.text('go')); + await tester.pumpAndSettle(); + return result; + } + + testWidgets('shows the title and message', (tester) async { + await showFor(tester); + expect(find.text('Delete note?'), findsOneWidget); + expect(find.text('This cannot be undone.'), findsOneWidget); + }); + + testWidgets('Cancel is autofocused so a reflexive Return is safe', ( + tester, + ) async { + await showFor(tester); + final cancel = tester.widget( + find.ancestor(of: find.text('Cancel'), matching: find.byType(TextButton)), + ); + expect(cancel.autofocus, isTrue); + }); + + testWidgets('confirming returns true', (tester) async { + await showFor(tester); + await tester.tap(find.text('Delete')); + await tester.pumpAndSettle(); + expect(find.text('Delete note?'), findsNothing); + }); + + testWidgets('cancelling returns false', (tester) async { + await showFor(tester); + await tester.tap(find.text('Cancel')); + await tester.pumpAndSettle(); + expect(find.text('Delete note?'), findsNothing); + }); + + testWidgets('Escape dismisses without confirming', (tester) async { + await showFor(tester); + expect(find.text('Delete note?'), findsOneWidget); + + await tester.sendKeyEvent(LogicalKeyboardKey.escape); + await tester.pumpAndSettle(); + + // A confirmation the keyboard cannot back out of is worse than none. + expect(find.text('Delete note?'), findsNothing); + }); + + testWidgets('custom labels are honoured', (tester) async { + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Builder( + builder: (context) => ElevatedButton( + onPressed: () => confirmDestructive( + context, + title: 'Discard?', + message: 'Unsaved work will be lost.', + confirmLabel: 'Discard', + cancelLabel: 'Keep editing', + ), + child: const Text('go'), + ), + ), + ), + ), + ); + await tester.tap(find.text('go')); + await tester.pumpAndSettle(); + + expect(find.text('Discard'), findsOneWidget); + expect(find.text('Keep editing'), findsOneWidget); + }); +} diff --git a/test/note_detail_screen_test.dart b/test/note_detail_screen_test.dart index deb16ad..ee2c0d2 100644 --- a/test/note_detail_screen_test.dart +++ b/test/note_detail_screen_test.dart @@ -115,7 +115,25 @@ void main() { await tester.tap(find.byTooltip('Delete note')); await tester.pumpAndSettle(); + // Delete is now confirmed. Deleting a note is irreversible and there is no + // undo, so a single Return on a focused control must not destroy it. + expect(find.text('Delete note?'), findsOneWidget); + await tester.tap(find.text('Delete')); + await tester.pumpAndSettle(); + expect(await repo.listNotes(), isEmpty); expect(find.byType(NoteDetailScreen), findsNothing); }); + + testWidgets('cancelling the delete keeps the note', (tester) async { + final repo = await pumpDetail(tester, seedNote('# Keep me')); + + await tester.tap(find.byTooltip('Delete note')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Cancel')); + await tester.pumpAndSettle(); + + expect(await repo.listNotes(), hasLength(1)); + expect(find.byType(NoteDetailScreen), findsOneWidget); + }); } diff --git a/test/notes_list_screen_test.dart b/test/notes_list_screen_test.dart index a27a383..5b823bb 100644 --- a/test/notes_list_screen_test.dart +++ b/test/notes_list_screen_test.dart @@ -109,11 +109,31 @@ void main() { await tester.pump(); await tester.pump(const Duration(milliseconds: 300)); // sheet open await tester.tap(find.text('Delete note')); - await tester.pump(); + await tester.pumpAndSettle(); + + // Delete is now confirmed on this path too: it is irreversible, has no + // undo, and is one Tab from a Return in the sheet's chip rows. + expect(find.text('Delete note?'), findsOneWidget); + await tester.tap(find.text('Delete')); + await tester.pumpAndSettle(); expect(await repo.listNotes(), isEmpty); }); + testWidgets('cancelling the sheet delete keeps the note', (tester) async { + final repo = await pumpList(tester, seed: [note('a', 'Keep me')]); + + await tester.tap(find.byTooltip('Quick actions')); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 300)); + await tester.tap(find.text('Delete note')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Cancel')); + await tester.pumpAndSettle(); + + expect(await repo.listNotes(), hasLength(1)); + }); + testWidgets('per-note sheet changes status via a chip', (tester) async { final repo = await pumpList(tester, seed: [note('a', 'Change me')]);