action_sheet: Add header param; use it to show message content

chrisbobbe · chrisbobbe · commit bf3fe42900e1 · 2025-07-08T22:09:32.000-07:00
See Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3483-29966&m=dev See also a different header variant in Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3481-26993&m=dev At first I'd thought this wouldn't be possible without more work toward issue #488: - #488 "content: Support Zulip content outside messages (even outside per-account contexts)". But it turns out that our content widgets don't assume a MessageList ancestor; it's sufficient that we provide a per-account context, via PerAccountStoreWidget, and a per-message context, via InheritedMessage (created by MessageContent). Fixes: #217
diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart
@@ -13,12 +13,14 @@ import '../api/route/channels.dart';
 import '../api/route/messages.dart';
 import '../generated/l10n/zulip_localizations.dart';
 import '../model/binding.dart';
+import '../model/content.dart';
 import '../model/emoji.dart';
 import '../model/internal_link.dart';
 import '../model/narrow.dart';
 import 'actions.dart';
 import 'color.dart';
 import 'compose_box.dart';
+import 'content.dart';
 import 'dialog.dart';
 import 'emoji.dart';
 import 'emoji_reaction.dart';
@@ -33,6 +35,7 @@ import 'topic_list.dart';
 
 void _showActionSheet(
   BuildContext pageContext, {
+  Widget? header,
   required List<Widget> optionButtons,
 }) {
   // Could omit this if we need _showActionSheet outside a per-account context.
@@ -57,15 +60,31 @@ void _showActionSheet(
             child: Column(
               mainAxisSize: MainAxisSize.min,
               children: [
-                SizedBox(height: 8),
+                if (header != null)
+                  Flexible(
+                    // TODO(upstream) Enforce a flex ratio (e.g. 1:3)
+                    //   only when the header height plus the buttons' height
+                    //   exceeds available space. Otherwise let one or the other
+                    //   grow to fill available space even if it breaks the ratio.
+                    //   Needs support for separate properties like `flex-grow`
+                    //   and `flex-shrink`.
+                    flex: 1,
+                    child: InsetShadowBox(
+                      top: 8, bottom: 8,
+                      color: designVariables.bgContextMenu,
+                      child: SingleChildScrollView(
+                        padding: EdgeInsets.symmetric(vertical: 8),
+                        child: header)))
+                else
+                  SizedBox(height: 8),
                 Flexible(
+                  flex: 3,
                   child: Padding(
                     padding: const EdgeInsets.fromLTRB(16, 0, 16, 0),
                     child: Column(
                       crossAxisAlignment: CrossAxisAlignment.stretch,
                       mainAxisSize: MainAxisSize.min,
                       children: [
-                        // TODO(#217): show message text
                         Flexible(child: InsetShadowBox(
                           top: 8, bottom: 8,
                           color: designVariables.bgContextMenu,
@@ -621,7 +640,42 @@ void showMessageActionSheet({required BuildContext context, required Message mes
       EditButton(message: message, pageContext: pageContext),
   ];
 
-  _showActionSheet(pageContext, optionButtons: optionButtons);
+  _showActionSheet(pageContext,
+    optionButtons: optionButtons,
+    header: _MessageActionSheetHeader(message: message));
+}
+
+class _MessageActionSheetHeader extends StatelessWidget {
+  const _MessageActionSheetHeader({required this.message});
+
+  final Message message;
+
+  @override
+  Widget build(BuildContext context) {
+    final designVariables = DesignVariables.of(context);
+
+    // TODO this seems to lose the hero animation when opening an image;
+    //   investigate.
+    // TODO should we close the sheet before opening a narrow link?
+    //   On popping the pushed narrow route, the sheet is still open.
+
+    return Container(
+      // TODO(#647) use different color for highlighted messages
+      // TODO(#681) use different color for DM messages
+      color: designVariables.bgMessageRegular,
+      padding: EdgeInsets.symmetric(vertical: 4),
+      child: Column(
+        spacing: 4,
+        children: [
+          SenderRow(message: message,
+            timestampStyle: MessageTimestampStyle.full),
+          Padding(
+            padding: const EdgeInsets.symmetric(horizontal: 16),
+            // TODO(#10) offer text selection; the Figma asks for it here:
+            //   https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3483-30210&m=dev
+            child: MessageContent(message: message, content: parseMessageContent(message))),
+        ]));
+  }
 }
 
 abstract class MessageActionSheetMenuItemButton extends ActionSheetMenuItemButton {
diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart
@@ -1893,7 +1893,8 @@ class SenderRow extends StatelessWidget {
     final revealedMutedMessagesState =
       MessageListPage.maybeRevealedMutedMessagesOf(context);
     // The "unrevealed" state only exists in the message list,
-    // and we're about to start showing a sender row outside the message list.
+    // and we show a sender row in at least one place outside the message list
+    // (the message action sheet).
     if (revealedMutedMessagesState == null) return false;
     return !revealedMutedMessagesState.isMutedMessageRevealed(message.id);
   }
@@ -1969,9 +1970,23 @@ class SenderRow extends StatelessWidget {
 enum MessageTimestampStyle {
   none,
   timeOnly,
+
+  /// The longest format, with full date and time as numbers, not "Today"/etc.
+  ///
+  /// For UI contexts focused just on the one message,
+  /// or as a tooltip on a shorter-formatted timestamp.
+  ///
+  /// The detail won't always be needed, but this format makes mental timezone
+  /// conversions easier, which is helpful when the user is thinking about
+  /// business hours on a different continent,
+  /// or traveling and they know their device timezone setting is wrong, etc.
+  // TODO(design) show "Today"/etc. after all? Discussion:
+  //   https://github.com/zulip/zulip-flutter/pull/1624#issuecomment-3050296488
+  full,
   ;
 
   static final _timeOnlyFormat = DateFormat('h:mm aa', 'en_US');
+  static final _fullFormat = DateFormat.yMMMd().add_jm();
 
   /// Format a [Message.timestamp] for this mode.
   // TODO(i18n): locale-specific formatting (see #45 for a plan with ffi)
@@ -1982,6 +1997,7 @@ enum MessageTimestampStyle {
     switch (this) {
       case none:     return null;
       case timeOnly: return _timeOnlyFormat.format(asDateTime);
+      case full: return _fullFormat.format(asDateTime);
     }
   }
 }
diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart
@@ -38,6 +38,7 @@ import '../api/fake_api.dart';
 import '../example_data.dart' as eg;
 import '../flutter_checks.dart';
 import '../model/binding.dart';
+import '../model/content_test.dart';
 import '../model/test_store.dart';
 import '../stdlib_checks.dart';
 import '../test_clipboard.dart';
@@ -860,6 +861,81 @@ void main() {
   });
 
   group('message action sheet', () {
+    group('header', () {
+      void checkSenderAndTimestampShown(WidgetTester tester, {required int senderId}) {
+        check(find.descendant(
+          of: find.byType(BottomSheet),
+          matching: find.byWidgetPredicate(
+            (widget) => widget is Avatar && widget.userId == senderId))
+        ).findsOne();
+        final expectedTimestampColor = MessageListTheme.of(
+          tester.element(find.byType(BottomSheet))).labelTime;
+        // TODO check the timestamp text itself, when it's convenient to do so:
+        //   https://github.com/zulip/zulip-flutter/pull/1624#discussion_r2181383754
+        check(find.descendant(
+          of: find.byType(BottomSheet),
+          matching: find.byWidgetPredicate((widget) =>
+            widget is Text
+            && widget.style?.color == expectedTimestampColor
+            && (widget.style?.fontFeatures?.contains(FontFeature.enable('c2sc')) ?? false)))
+        ).findsOne();
+      }
+
+      testWidgets('message sender and content shown', (tester) async {
+        final message = eg.streamMessage(
+          timestamp: 1671409088,
+          content: ContentExample.userMentionPlain.html);
+        await setupToMessageActionSheet(tester,
+          message: message,
+          narrow: TopicNarrow.ofMessage(message));
+        checkSenderAndTimestampShown(tester, senderId: message.senderId);
+        check(find.descendant(
+          of: find.byType(BottomSheet),
+          matching: find.byType(UserMention))
+        ).findsOne();
+      });
+
+      testWidgets('muted sender also shown', (tester) async {
+        final message = eg.streamMessage(
+          timestamp: 1671409088,
+          content: ContentExample.userMentionPlain.html);
+        await setupToMessageActionSheet(tester,
+          message: message,
+          narrow: TopicNarrow.ofMessage(message),
+          mutedUserIds: [message.senderId],
+          beforeLongPress: () async {
+            check(find.byType(MessageContent)).findsNothing();
+            await tester.tap(
+              find.widgetWithText(ZulipWebUiKitButton, 'Reveal message'));
+            await tester.pump();
+            check(find.byType(MessageContent)).findsOne();
+          },
+        );
+        checkSenderAndTimestampShown(tester, senderId: message.senderId);
+        check(find.descendant(
+          of: find.byType(BottomSheet),
+          matching: find.byType(UserMention))
+        ).findsOne();
+      });
+
+      testWidgets('poll is rendered', (tester) async {
+        final submessageContent = eg.pollWidgetData(
+          question: 'poll', options: ['First option', 'Second option']);
+        final message = eg.streamMessage(
+          timestamp: 1671409088,
+          sender: eg.selfUser,
+          submessages: [eg.submessage(content: submessageContent)]);
+        await setupToMessageActionSheet(tester,
+          message: message,
+          narrow: TopicNarrow.ofMessage(message));
+        checkSenderAndTimestampShown(tester, senderId: message.senderId);
+        check(find.descendant(
+          of: find.byType(BottomSheet),
+          matching: find.text('First option'))
+        ).findsOne();
+      });
+    });
+
     group('ReactionButtons', () {
       testWidgets('absent if ServerEmojiData not loaded', (tester) async {
         final message = eg.streamMessage();
