Make read-receipts and view-reactions draggable-scrollable #1802

chrisbobbe · 2025-08-14T19:51:45Z

For manual testing, ideally use a real device, to see the fling and drag responses faithfully to what most users will experience :)

Screenshots coming soon.

Selected commit messages:

ead_receipts: Improve UX by making the sheet draggable-scrollable

The Figma asks that the sheet be expandable to fill the screen:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=11367-21131&m=dev
and that's implemented here.

Compare f8ddff2, where we removed an earlier implementation. I
hadn't tried to bring that back yet because I wanted to support
triggering resize from a drag handle at the top, and I couldn't
figure out how to do that. IIRC I could only get the drag handle to
respond to drag-down gestures (via enableDrag: true) by shifting
the sheet's position downward. That worked as a way to dismiss the
sheet, but it was frustratingly different from the gesture handling
on the scrollable list:

The slide-to-dismiss looked different from the shrink-and-dismiss,
an awkward inconsistency
The list would respond to upward drags, too (by growing and
showing more content)

I've managed it here, modulo with a header instead of a drag handle,
by making sure the scrollable area extends through the top of the
sheet. (Done with a CustomScrollView, pinning the header to the
viewport top.)

emoji_reaction: Use DraggableScrollableModalBottomSheet for view-reactions

Like we did for read-receipts in a recent commit. I think this
behavior is implied in the Figma, but it's not as explicit:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5878-19207&m=dev
...anyway, helpful to be consistent with read-receipts, I think,
which is similarly a read-only view that might have a long list to
show.

I thought this would be more complicated than it turned out to be --
thanks to SliverSemantics, I can actually wrap the list of voters in
a labeled container node, because
DraggableScrollableModalBottomSheet's API takes a sliver for the
content.

chrisbobbe · 2025-08-14T20:03:19Z

Finger down on header, move up and down

Before:

ScreenRecording_08-14-2025.12-55-18_1.mov

After:

ScreenRecording_08-14-2025.12-57-41_1.mov

Finger down in list, move up and down

Before:

ScreenRecording_08-14-2025.12-56-03_1.mov

After:

ScreenRecording_08-14-2025.12-58-02_1.mov

chrisbobbe · 2025-08-14T20:04:11Z

I can record the view-reactions behavior too, but it's similar, and anyway the best way to see the behavior is to try it yourself :)

rajveermalviya

Thanks for working on this @chrisbobbe! This works great on my device, and LGTM.

gnprice

Neat, thanks for working this out.

Generally this all looks good; just small comments.

gnprice · 2025-08-18T23:25:24Z

lib/widgets/action_sheet.dart

+/// Simply a [BottomSheetEmptyContentPlaceholder] wrapped in
+/// [SliverToBoxAdapter].
+class SliverBottomSheetEmptyContentPlaceholder extends StatelessWidget {


Seems like this might be clearer inlined, then — less than a line's worth of extra code, and more transparent.

gnprice · 2025-08-18T23:29:32Z

lib/widgets/action_sheet.dart

+        // The "inset shadow" effect in Figma is a bit awkwardly
+        // implemented and there might be a better factoring:
+        // 1. This effect leans on the abstraction that [contentSliver]


Suggested change

// The "inset shadow" effect in Figma is a bit awkwardly

// implemented and there might be a better factoring:

// 1. This effect leans on the abstraction that [contentSliver]

// The "inset shadow" effect in Figma is a bit awkwardly

// implemented here and there might be a better factoring:

// 1. This effect leans on the abstraction that [contentSliver]

Is that the intended meaning? My first thought was this line was about what's in Figma.

Eep, yeah, thanks for flagging.

gnprice · 2025-08-18T23:40:08Z

lib/widgets/action_sheet.dart

+                  physics: ClampingScrollPhysics(),
+                  controller: controller,
+                  slivers: [
+                    PinnedHeaderSliver(child: headerWithShadow),


I've managed it here, modulo with a header instead of a drag handle, by making sure the scrollable area extends through the top of the sheet. (Done with a CustomScrollView, pinning the header to the viewport top.)

Neat!

gnprice · 2025-08-18T23:47:03Z

lib/widgets/emoji_reaction.dart

+    final userIds = message?.reactions?.aggregated.firstWhereOrNull(
+      (x) => x.reactionType == reactionType && x.emojiCode == emojiCode
+    )?.userIds.toList();
+
+    Widget contentSliver;
+    if (reactionType != null && emojiCode != null && userIds != null) {
+      assert(emojiName != null);
+
+      // (No filtering of muted or deactivated users.
+      // Muted users will be shown as muted.)
+
+      contentSliver = SliverList.builder(
+        itemCount: userIds.length,
+        itemBuilder: (_, index) => ViewReactionsUserItem(userId: userIds[index]));
+
+      contentSliver = SliverSemantics(
+        identifier: ViewReactions.semanticsIdentifier, // See note on `controlsNodes` on the tab.
+        label: zulipLocalizations.seeWhoReactedSheetUserListLabel(emojiName!, userIds.length),
+        role: SemanticsRole.tabPanel,
+        container: true,
+        explicitChildNodes: true,
+        sliver: contentSliver);


This part feels like it would be useful to continue to have in its own widget, like ViewReactionsUserList.

Could put "sliver" in the name of the widget to make clear its render object will be a sliver rather than a box.

chrisbobbe · 2025-08-20T04:52:10Z

Thanks for the review, revision pushed!

gnprice

Thanks! Looks good; just a couple of nits.

gnprice · 2025-08-20T21:26:25Z

lib/widgets/action_sheet.dart

+        final backgroundColor = Theme.of(context).bottomSheetTheme.backgroundColor!;
+
+        // The "inset shadow" effect in Figma is a bit awkwardly
+        // implemented here and there might be a better factoring:


nit:

Suggested change

// implemented here and there might be a better factoring:

// implemented here, and there might be a better factoring:

I know I suggested this wording myself 😅 yesterday above, but I misparsed this on first reading today, with the phrase "here and there".

gnprice · 2025-08-20T21:31:34Z

lib/widgets/emoji_reaction.dart

    if (userIds == null) {
      // This reaction lost all its votes, or the message was deleted.
-      return SizedBox.shrink();
+      return SliverPadding(padding: EdgeInsets.zero);


I guess this is also now the condition that handles the case where reactionType and/or emojiCode is null.

It'd be good to make that explicit, at least by editing the comment. Might be clearer still to put an explicit condition for it at the top of the build method.

chrisbobbe · 2025-08-20T22:44:24Z

Thanks! Revision pushed.

There are other localization classes, like MaterialLocalizations, and in any case it seems right to have a consistent naming pattern.

The Figma asks that the sheet be expandable to fill the screen: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=11367-21131&m=dev and that's implemented here. Compare f8ddff2, where we removed an earlier implementation. I hadn't tried to bring that back yet because I wanted to support triggering resize from a drag handle at the top, and I couldn't figure out how to do that. IIRC I could only get the drag handle to respond to drag-down gestures (via `enableDrag: true`) by shifting the sheet's position downward. That worked as a way to dismiss the sheet, but it was frustratingly different from the gesture handling on the scrollable list: - The slide-to-dismiss looked different from the shrink-and-dismiss, an awkward inconsistency - The list would respond to upward drags, too (by growing and showing more content) I've managed it here, modulo with a header instead of a drag handle, by making sure the scrollable area extends through the top of the sheet. (Done with a CustomScrollView, pinning the header to the viewport top.)

This fixes a test failure when we make an upcoming refactor. (The container node's label would apparently have its children's labels appended to it...I haven't seen that behavior in manual testing on my iPhone with VoiceOver, before or after that refactor, but, shrug; this change does seem correct anyway.)

…tions Like we did for read-receipts in a recent commit. I *think* this behavior is implied in the Figma, but it's not as explicit: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5878-19207&m=dev ...anyway, helpful to be consistent with read-receipts, I think, which is similarly a read-only view that might have a long list to show. I thought this would be more complicated than it turned out to be -- thanks to SliverSemantics, I can actually wrap the list of voters in a labeled container node, because DraggableScrollableModalBottomSheet's API takes a sliver for the content.

gnprice · 2025-08-20T23:33:09Z

Thanks! Looks good; merging.

chrisbobbe requested a review from rajveermalviya August 14, 2025 19:51

chrisbobbe assigned rajveermalviya Aug 14, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Aug 14, 2025

rajveermalviya approved these changes Aug 15, 2025

View reviewed changes

rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Aug 15, 2025

rajveermalviya assigned gnprice and unassigned rajveermalviya Aug 15, 2025

rajveermalviya requested a review from gnprice August 15, 2025 18:30

gnprice reviewed Aug 18, 2025

View reviewed changes

chrisbobbe mentioned this pull request Aug 20, 2025

share: Support choosing an account, if there are multiple #1816

Open

chrisbobbe force-pushed the pr-draggable-scrollable-sheet-again branch from e32e34f to a9b5745 Compare August 20, 2025 04:52

gnprice reviewed Aug 20, 2025

View reviewed changes

chrisbobbe force-pushed the pr-draggable-scrollable-sheet-again branch from a9b5745 to a2774bb Compare August 20, 2025 22:44

chrisbobbe added 6 commits August 20, 2025 16:32

inset_shadow [nfc]: Bring out a public helper, to use soon

9884df7

code [nfc]: s/localizations/zulipLocalizations/g

4c05735

There are other localization classes, like MaterialLocalizations, and in any case it seems right to have a consistent naming pattern.

read_receipts [nfc]: Inline _ReadReceiptsUserList

ee303d0

gnprice force-pushed the pr-draggable-scrollable-sheet-again branch from a2774bb to d9ab075 Compare August 20, 2025 23:33

gnprice merged commit d9ab075 into zulip:main Aug 20, 2025
1 check passed

chrisbobbe deleted the pr-draggable-scrollable-sheet-again branch August 21, 2025 00:22

	// implemented here and there might be a better factoring:
	// implemented here, and there might be a better factoring:

Make read-receipts and view-reactions draggable-scrollable #1802

Make read-receipts and view-reactions draggable-scrollable #1802

Uh oh!

Conversation

chrisbobbe commented Aug 14, 2025

Uh oh!

chrisbobbe commented Aug 14, 2025

Finger down on header, move up and down

Finger down in list, move up and down

Uh oh!

chrisbobbe commented Aug 14, 2025

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 20, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 20, 2025

Uh oh!

gnprice commented Aug 20, 2025

Uh oh!

Uh oh!

Uh oh!