msglist: Support viewing who reacted to a message #1700

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

Jump to bottom

Open

chrisbobbe wants to merge 7 commits into zulip:main from chrisbobbe:pr-see-who-reacted

Collaborator

chrisbobbe commented Jul 10, 2025 •

edited

Loading

Fixes #740.

Followup:

View-reactions sheet doesn't update immediately when reactions change #1747

Notable changes from previous revision:

"See who reacted" button omitted in action sheet when no reactions
Emojis centered horizontally when few
Emojis scroll into view when tapped
Reasonable UI with VoiceOver (at least in my testing)
Wrote some basic tests

Screenshots

I've included some screenshots where there are

many emoji
many users

and in those, I set the scroll position such that you can see the shadow effect.

Light	Dark

Scroll-on-select animation:

chrisbobbe requested a review from gnprice

July 10, 2025 00:50

chrisbobbe assigned gnprice

chrisbobbe added the integration review label

chrisbobbe mentioned this pull request

Add a "(you)" indicator in direct messages list #1320

Open

Member

gnprice commented Jul 10, 2025

Thanks! I skimmed through this and I'm pretty confident it won't break any existing functionality. I also saw you demonstrate this running on your device, and it seemed to work well. So I plan to include this in today's upcoming release, without yet merging to main.

gnprice mentioned this pull request

See who left an emoji reaction #740

Open

sm-sayedi mentioned this pull request

Show read receipts #1706

Draft

chrisbobbe force-pushed the pr-see-who-reacted branch from 27dd265 to d0f7d15 Compare

July 25, 2025 21:44

chrisbobbe marked this pull request as ready for review

July 25, 2025 21:46

Collaborator Author

chrisbobbe commented Jul 25, 2025

Thanks! This is now ready for review, and I've updated the issue description with changes since the draft.

chrisbobbe added 6 commits

July 25, 2025 14:47


          action_sheet: Choose "Close" and "Cancel" consistently for bottom sheets

6d078e3

This fixes the two inconsistencies flagged in discussion:
  https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/bottom.20sheet.20.22Cancel.22.2F.22Close.22.20button/near/2216116

> I think it's reasonable to have both labels, but I think we should
> choose them differently than now:
>
> - "Cancel" when the sheet is about doing an action: [etc.]
>
> - "Close" when the sheet just presents information or nav options:
>   [etc.]


          icons: Add see_who_reacted, from Vlad on CZO

203a6a5

See Vlad's SVG on CZO:
  https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/View.20Reactions.20ModalBottomSheet/near/2025350


          action_sheet: Implement BottomSheetHeaderPlainText, following Figma

a29eac8


          inset_shadow test [nfc]: Pull PaintPatternPredicate helper outside it…

bcb5fc5

…s test

So we can add another test that uses it.


          inset_shadow: Implement start/end shadows, not just top/bottom

86710ba


          emoji_reaction test [nfc]: Pull out some data for other tests to use

e1f4bdd

chrisbobbe force-pushed the pr-see-who-reacted branch from d0f7d15 to 399cd6a Compare

July 25, 2025 21:47

Member

gnprice commented Jul 25, 2025

Thanks! Would you also post a few screenshots?


          msglist: Support viewing who reacted to a message

e653144

I experimented with using Semantics to help write human-centered
tests, and I ended up adding some configuration that actually seemed
to make a reasonable experience in the UI, at least in my testing
with VoiceOver.

Fixes zulip#740.

chrisbobbe force-pushed the pr-see-who-reacted branch from 399cd6a to e653144 Compare

July 25, 2025 22:04

Collaborator Author

chrisbobbe commented Jul 25, 2025

Sure! Done.

gnprice reviewed

View reviewed changes

Member

gnprice left a comment

Thanks!

Generally this looks good. Here's a full review except the tests.

lib/widgets/emoji_reaction.dart

+                        //    to the underlying Scrollable to remove an unwanted node
+                        //    in accessibility focus traversal.
+                        scrollDirection: Axis.horizontal,
+                        physics: ClampingScrollPhysics(),

Member

gnprice Jul 25, 2025

Interesting — why this instead of the default?

(I believe the default would be this on Android, but BouncingScrollPhysics on iOS.)

lib/widgets/emoji_reaction.dart

+                          messageId: widget.messageId,
+                          reactionType: reactionType,
+                          emojiCode: emojiCode,
+                          onRequestSelect: (r) => _setSelection(r),

Member

gnprice Jul 25, 2025

nit: tearoff

Suggested change

      
                        onRequestSelect: (r) => _setSelection(r),
          
                        onRequestSelect: _setSelection,

lib/widgets/emoji_reaction.dart

+                /// Check that the given reaction still has votes;
+                /// if not, select a different one if possible or clear the selection.
+                void _reconcile() {

Member

gnprice Jul 25, 2025

It looks like this always concludes with _setSelection. It might be a little clearer to read by making that explicit up front:

Suggested change

      
              void _reconcile() {
          
              void _reconcile() {
          
                _setSelection(_findMatchingReaction());
          
              }

Then _findMatchingReaction can directly return whenever it's found its answer.

lib/widgets/emoji_reaction.dart

Comment on lines +722 to +726

+                  if (reactionType == null && widget.initialReactionType != null) {
+                    assert(emojiCode == null);
+                    assert(widget.initialEmojiCode != null);
+                    reactionType = widget.initialReactionType!;
+                    emojiCode = widget.initialEmojiCode!;

Member

gnprice Jul 25, 2025

This doesn't depend on the store, does it? It looks like it doesn't use the context at all; so it could run as early as initState.

And conversely it doesn't sound like it'd be desirable for this to run repeatedly, or after a new store — these are the initial emoji type and code, after all.

lib/widgets/emoji_reaction.dart

Comment on lines +741 to +744

+                  return SizedBox(
+                    width: double.infinity,
+                    child: Column(
+                      mainAxisSize: MainAxisSize.min,

Member

gnprice Jul 25, 2025

Would this be equivalent?

Suggested change

      
                return SizedBox(
          
                  width: double.infinity,
          
                  child: Column(
          
                    mainAxisSize: MainAxisSize.min,
          
                return Column(
          
                  mainAxisSize: MainAxisSize.min,
          
                  crossAxisAlignment: CrossAxisAlignment.stretch,

lib/widgets/emoji_reaction.dart

+                    (x) => x.reactionType == reactionType && x.emojiCode == emojiCode
+                  )?.userIds.toList();
+                  // (No filtering of muted or deactivated users.)

Member

gnprice Jul 25, 2025

Suggested change

      
                // (No filtering of muted or deactivated users.)
          
                // (No filtering of muted or deactivated users.
          
                //  Muted users will be shown as muted.)

My first reaction to this comment was "is that right? seems like we shouldn't show muted users here." Then I remembered that that's probably handled at a different layer, and indeed it looks like it is.

lib/widgets/emoji_reaction.dart

Comment on lines +967 to +973

+                  Widget result = InsetShadowBox(
+                    top: 8, bottom: 8,
+                    color: designVariables.bgContextMenu,
+                    child: SizedBox(
+                      height: 400, // TODO(design) tune
+                      child: ListView.builder(
+                        padding: EdgeInsets.symmetric(vertical: 8),

Member

gnprice Jul 25, 2025

nit: move SizedBox outward, so the InsetShadowBox is right next to (in the source code) the padding that needs to match it

lib/widgets/emoji_reaction.dart

Comment on lines +975 to +976

		itemBuilder: (context, index) =>
		ViewReactionsUserItem(context, userId: userIds[index]))));

Member

gnprice Jul 25, 2025

Same comment as I told Sayed last week 🙂 : #1706 (comment)

(on code that I guess was modeled in part on this PR, so no coincidence)

lib/widgets/emoji_reaction.dart

Comment on lines +999 to +1002

+                  Navigator.pop(pageContext);
+                  Navigator.push(pageContext,
+                    ProfilePage.buildRoute(context: pageContext, userId: userId));

Member

gnprice Jul 26, 2025

I think the reason this works even though this pageContext isn't really a page context is that there's no await here.

The reason our action-sheet buttons often need a real page context is that they'll dismiss the action sheet, then do something that takes time like a network request, and then after that completes they need to use a context to do something with the result, or to show an error. So if the request takes longer than the few hundred ms of the dismiss animation, the action sheet's own context may be unmounted by that point.

For just synchronously acting on the navigation like this, the widget's own context is fine.

lib/widgets/emoji_reaction.dart

+                    onTap: _onPressed,
+                    splashFactory: NoSplash.splashFactory,
+                    overlayColor: WidgetStateColor.resolveWith((states) =>
+                      states.any((e) => e == WidgetState.pressed)

Member

gnprice Jul 26, 2025

Suggested change

      
                    states.any((e) => e == WidgetState.pressed)
          
                    states.contains(WidgetState.pressed)

Looks like it's a Set, so that should be equivalent (and potentially faster)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration review