Skip to content

dialog: Use Cupertino-flavored alert dialogs on iOS #1017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

dialog: Use Cupertino-flavored alert dialogs on iOS #1017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
40165ef
dialog [nfc]: Rename checkNoErrorDialog to checkNoDialog
u7088495 Feb 11, 2025
049a972
dialog: Use Cupertino-flavored alert dialogs on iOS
u7088495 Nov 18, 2024
39a6ffe
dialog [nfc]: Inline _materialDialogActionText in _adaptiveAction
u7088495 Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 35 additions & 22 deletions lib/widgets/dialog.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,32 @@
import 'package:flutter/cupertino.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';

import '../generated/l10n/zulip_localizations.dart';
import 'actions.dart';

Widget _dialogActionText(String text) {
return Text(
text,

// As suggested by
// https://api.flutter.dev/flutter/material/AlertDialog/actions.html :
// > It is recommended to set the Text.textAlign to TextAlign.end
// > for the Text within the TextButton, so that buttons whose
// > labels wrap to an extra line align with the overall
// > OverflowBar's alignment within the dialog.
textAlign: TextAlign.end,
);
/// A platform-appropriate action for [AlertDialog.adaptive]'s [actions] param.
Widget _adaptiveAction({required VoidCallback onPressed, required String text}) {
switch (defaultTargetPlatform) {
case TargetPlatform.android:
case TargetPlatform.fuchsia:
case TargetPlatform.linux:
case TargetPlatform.windows:
return TextButton(
onPressed: onPressed,
child: Text(
text,
// As suggested by
// https://api.flutter.dev/flutter/material/AlertDialog/actions.html :
// > It is recommended to set the Text.textAlign to TextAlign.end
// > for the Text within the TextButton, so that buttons whose
// > labels wrap to an extra line align with the overall
// > OverflowBar's alignment within the dialog.
textAlign: TextAlign.end));
case TargetPlatform.iOS:
Comment on lines +25 to +26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: a blank line to separate these cases helps make the structure easier to see:

Suggested change
textAlign: TextAlign.end));
case TargetPlatform.iOS:
textAlign: TextAlign.end));
case TargetPlatform.iOS:

case TargetPlatform.macOS:
return CupertinoDialogAction(onPressed: onPressed, child: Text(text));
}
}

/// Tracks the status of a dialog, in being still open or already closed.
Expand Down Expand Up @@ -46,17 +58,18 @@ DialogStatus showErrorDialog({
final zulipLocalizations = ZulipLocalizations.of(context);
final future = showDialog<void>(
context: context,
builder: (BuildContext context) => AlertDialog(
builder: (BuildContext context) => AlertDialog.adaptive(
title: Text(title),
content: message != null ? SingleChildScrollView(child: Text(message)) : null,
actions: [
if (learnMoreButtonUrl != null)
TextButton(
_adaptiveAction(
onPressed: () => PlatformActions.launchUrl(context, learnMoreButtonUrl),
child: _dialogActionText(zulipLocalizations.errorDialogLearnMore)),
TextButton(
text: zulipLocalizations.errorDialogLearnMore,
),
Comment on lines +68 to +69
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: preserve formatting

Suggested change
text: zulipLocalizations.errorDialogLearnMore,
),
text: zulipLocalizations.errorDialogLearnMore),

_adaptiveAction(
onPressed: () => Navigator.pop(context),
child: _dialogActionText(zulipLocalizations.errorDialogContinue)),
text: zulipLocalizations.errorDialogContinue),
]));
return DialogStatus(future);
}
Expand All @@ -71,18 +84,18 @@ void showSuggestedActionDialog({
final zulipLocalizations = ZulipLocalizations.of(context);
showDialog<void>(
context: context,
builder: (BuildContext context) => AlertDialog(
builder: (BuildContext context) => AlertDialog.adaptive(
title: Text(title),
content: SingleChildScrollView(child: Text(message)),
actions: [
TextButton(
_adaptiveAction(
onPressed: () => Navigator.pop(context),
child: _dialogActionText(zulipLocalizations.dialogCancel)),
TextButton(
text: zulipLocalizations.dialogCancel),
_adaptiveAction(
onPressed: () {
onActionButtonPress();
Navigator.pop(context);
},
child: _dialogActionText(actionButtonText ?? zulipLocalizations.dialogContinue)),
text: actionButtonText ?? zulipLocalizations.dialogContinue),
]));
}
10 changes: 5 additions & 5 deletions test/widgets/action_sheet_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ void main() {
await tester.tap(findButtonForLabel('Mark channel as read'));
await tester.pumpAndSettle();
checkRequest(someChannel.streamId);
checkNoErrorDialog(tester);
checkNoDialog(tester);
});

testWidgets('request fails', (tester) async {
Expand Down Expand Up @@ -688,7 +688,7 @@ void main() {
await tester.tap(findButtonForLabel('Mark as resolved'));
await tester.pumpAndSettle();

checkNoErrorDialog(tester);
checkNoDialog(tester);
checkRequest(message.id, '✔ zulip');
});

Expand All @@ -703,7 +703,7 @@ void main() {
await tester.tap(findButtonForLabel('Mark as resolved'));
await tester.pumpAndSettle();

checkNoErrorDialog(tester);
checkNoDialog(tester);
checkRequest(message.id, '✔ zulip');
});

Expand All @@ -717,7 +717,7 @@ void main() {
await tester.tap(findButtonForLabel('Mark as unresolved'));
await tester.pumpAndSettle();

checkNoErrorDialog(tester);
checkNoDialog(tester);
checkRequest(message.id, 'zulip');
});

Expand All @@ -731,7 +731,7 @@ void main() {
await tester.tap(findButtonForLabel('Mark as unresolved'));
await tester.pumpAndSettle();

checkNoErrorDialog(tester);
checkNoDialog(tester);
checkRequest(message.id, 'zulip');
});

Expand Down
6 changes: 3 additions & 3 deletions test/widgets/app_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,14 +401,14 @@ void main() {
check(ZulipApp.ready).value.isFalse();
await tester.pump();
check(findSnackBarByText(message).evaluate()).isEmpty();
checkNoErrorDialog(tester);
checkNoDialog(tester);

check(ZulipApp.ready).value.isTrue();
// After app startup, reportErrorToUserBriefly displays a SnackBar.
reportErrorToUserBriefly(message, details: details);
await tester.pumpAndSettle();
check(findSnackBarByText(message).evaluate()).single;
checkNoErrorDialog(tester);
checkNoDialog(tester);

// Open the error details dialog.
await tester.tap(find.text('Details'));
Expand Down Expand Up @@ -493,7 +493,7 @@ void main() {
reportErrorToUserModally(title, message: message);
check(ZulipApp.ready).value.isFalse();
await tester.pump();
checkNoErrorDialog(tester);
checkNoDialog(tester);

check(ZulipApp.ready).value.isTrue();
// After app startup, reportErrorToUserModally displays an [AlertDialog].
Expand Down
10 changes: 5 additions & 5 deletions test/widgets/compose_box_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ void main() {
await prepareWithContent(tester,
makeStringWithCodePoints(kMaxMessageLengthCodePoints));
await tapSendButton(tester);
checkNoErrorDialog(tester);
checkNoDialog(tester);
});

testWidgets('code points not counted unnecessarily', (tester) async {
Expand Down Expand Up @@ -313,7 +313,7 @@ void main() {
await prepareWithTopic(tester,
makeStringWithCodePoints(kMaxTopicLengthCodePoints));
await tapSendButton(tester);
checkNoErrorDialog(tester);
checkNoDialog(tester);
});

testWidgets('code points not counted unnecessarily', (tester) async {
Expand Down Expand Up @@ -739,7 +739,7 @@ void main() {
await setupAndTapSend(tester, prepareResponse: (int messageId) {
connection.prepare(json: SendMessageResult(id: messageId).toJson());
});
checkNoErrorDialog(tester);
checkNoDialog(tester);
});

testWidgets('ZulipApiException', (tester) async {
Expand Down Expand Up @@ -877,7 +877,7 @@ void main() {
check(call.allowMultiple).equals(true);
check(call.type).equals(FileType.media);

checkNoErrorDialog(tester);
checkNoDialog(tester);

check(controller!.content.text)
.equals('see image: [Uploading image.jpg…]()\n\n');
Expand Down Expand Up @@ -936,7 +936,7 @@ void main() {
check(call.source).equals(ImageSource.camera);
check(call.requestFullMetadata).equals(false);

checkNoErrorDialog(tester);
checkNoDialog(tester);

check(controller!.content.text)
.equals('see image: [Uploading image.jpg…]()\n\n');
Expand Down
105 changes: 78 additions & 27 deletions test/widgets/dialog_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import 'package:checks/checks.dart';
import 'package:flutter/cupertino.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter_checks/flutter_checks.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:url_launcher/url_launcher.dart';
import 'package:zulip/widgets/dialog.dart';

/// In a widget test, check that showErrorDialog was called with the right text.
import '../model/binding.dart';

/// In a widget test, check that [showErrorDialog] was called with the right text.
///
/// Checks for an error dialog matching an expected title
/// and, optionally, matching an expected message. Fails if none is found.
Expand All @@ -14,25 +19,55 @@ import 'package:zulip/widgets/dialog.dart';
Widget checkErrorDialog(WidgetTester tester, {
required String expectedTitle,
String? expectedMessage,
Uri? expectedLearnMoreButtonUrl,
}) {
final dialog = tester.widget<AlertDialog>(find.byType(AlertDialog));
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.title!), matching: find.text(expectedTitle)));
if (expectedMessage != null) {
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.content!), matching: find.text(expectedMessage)));
}
switch (defaultTargetPlatform) {
case TargetPlatform.android:
case TargetPlatform.fuchsia:
case TargetPlatform.linux:
case TargetPlatform.windows:
final dialog = tester.widget<AlertDialog>(find.bySubtype<AlertDialog>());
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.title!), matching: find.text(expectedTitle)));
if (expectedMessage != null) {
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.content!), matching: find.text(expectedMessage)));
}
if (expectedLearnMoreButtonUrl != null) {
check(testBinding.takeLaunchUrlCalls()).single.equals((
url: expectedLearnMoreButtonUrl,
mode: LaunchMode.inAppBrowserView));
}
Comment on lines +36 to +40
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for this button is useful, but this check doesn't work in this context. The check will work only if the caller has already gone and tapped on the button. There's nothing in this function's dartdoc that says the caller should do that, and it's not something a reader would naturally assume they need to do.

If a caller is going and tapping on the button themself, then the caller can also easily go and do this check — the check isn't using any specific knowledge about how the error dialogs work.

It looks like there's one call site that passes this option. So let's move this check to that call site.


return tester.widget(find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(TextButton, 'OK')));

// TODO check "Learn more" button?
case TargetPlatform.iOS:
case TargetPlatform.macOS:
final dialog = tester.widget<CupertinoAlertDialog>(find.byType(CupertinoAlertDialog));
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.title!), matching: find.text(expectedTitle)));
if (expectedMessage != null) {
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.content!), matching: find.text(expectedMessage)));
}
if (expectedLearnMoreButtonUrl != null) {
check(testBinding.takeLaunchUrlCalls()).single.equals((
url: expectedLearnMoreButtonUrl,
mode: LaunchMode.externalApplication));
}

return tester.widget(
find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(TextButton, 'OK')));
return tester.widget(find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(CupertinoDialogAction, 'OK')));
}
}

// TODO(#996) update this to check for per-platform flavors of alert dialog
void checkNoErrorDialog(WidgetTester tester) {
check(find.byType(AlertDialog)).findsNothing();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, in #1017 (comment) I meant those two checks were redundant with each other. If there's no widget that's of a subtype of AlertDialog, there's certainly none that's of the exact type AlertDialog.

/// Checks that there is no dialog.
/// Fails if one is found.
void checkNoDialog(WidgetTester tester) {
check(find.byType(Dialog)).findsNothing();
check(find.bySubtype<AlertDialog>()).findsNothing();
check(find.byType(CupertinoAlertDialog)).findsNothing();
}

/// In a widget test, check that [showSuggestedActionDialog] was called
Expand All @@ -49,19 +84,35 @@ void checkNoErrorDialog(WidgetTester tester) {
required String expectedMessage,
String? expectedActionButtonText,
}) {
final dialog = tester.widget<AlertDialog>(find.byType(AlertDialog));
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.title!), matching: find.text(expectedTitle)));
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.content!), matching: find.text(expectedMessage)));
switch (defaultTargetPlatform) {
case TargetPlatform.android:
case TargetPlatform.fuchsia:
case TargetPlatform.linux:
case TargetPlatform.windows:
final dialog = tester.widget<AlertDialog>(find.bySubtype<AlertDialog>());
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.title!), matching: find.text(expectedTitle)));
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.content!), matching: find.text(expectedMessage)));

final actionButton = tester.widget(
find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(TextButton, expectedActionButtonText ?? 'Continue')));
final actionButton = tester.widget(find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(TextButton, expectedActionButtonText ?? 'Continue')));
final cancelButton = tester.widget(find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(TextButton, 'Cancel')));
return (actionButton, cancelButton);

final cancelButton = tester.widget(
find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(TextButton, 'Cancel')));
case TargetPlatform.iOS:
case TargetPlatform.macOS:
final dialog = tester.widget<CupertinoAlertDialog>(find.byType(CupertinoAlertDialog));
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.title!), matching: find.text(expectedTitle)));
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.content!), matching: find.text(expectedMessage)));

return (actionButton, cancelButton);
final actionButton = tester.widget(find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(CupertinoDialogAction, expectedActionButtonText ?? 'Continue')));
final cancelButton = tester.widget(find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(CupertinoDialogAction, 'Cancel')));
return (actionButton, cancelButton);
}
}
Loading