content: Refactor emojis to render with EmojiWidget in message content

[MritunjayTiwari14]

Fixes #966.
Unblocks #1995 (PR: #2185).

Before and After Changes

Before	After

Before	After

Overview

Previously, we rendered Unicode emoji with TextSpan and ImageEmoji with RealmContentNetworkImage (directly). When rendering emoji as emoji reactions, we have a couple of nuances However, we don't show for emojis appearing in message content. We should follow the same by rendering UnicodeEmoji and ImageEmoji using EmojiWidget.

Many Bugs were solved for emojis in message content, some of them which were not included by Greg in the issue's description:

• ImageEmoji now respects heading size.
• ImageEmoji now renders at the correct size of the paragraph, visually inline with text and Unicode emojis.
  etc.

Note:

• Unfortunately, due to Uncode, emoji rendering uses WidgetSpan. Strike-through line now appears behind the Unicode emoji — similar to what image emoji used to. For more details on this, see #mobile > ImageEmoji does not strike through as well as not scale.

[Copilot]

Pull request overview

Refactors emoji rendering in message content to use EmojiWidget (aligning behavior with emoji reactions) and adjusts the content model/tests to support that rendering.

Changes:

• Switch message-content Unicode and image emoji rendering from TextSpan/direct RealmContentNetworkImage to EmojiWidget via WidgetSpan.
• Extend UnicodeEmojiNode to include emojiCode, and plumb it through parsing/tests.
• Update widget tests to provide ServerEmojiData so emoji-name lookups can succeed.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
lib/widgets/content.dart	Renders Unicode and image emoji in message content using EmojiWidget via new helper widgets.
lib/widgets/emoji.dart	Adjusts Unicode emoji sizing logic based on whether it’s under a WidgetSpan.
lib/model/content.dart	Adds emojiCode to UnicodeEmojiNode and sets it during HTML parsing.
lib/model/emoji.dart	Extends UnicodeEmojiDisplay with underWidgetSpan metadata.
test/model/content_test.dart	Updates expected parse output to include emojiCode.
test/widgets/content_test.dart	Adds ServerEmojiData setup and updates emoji smoke tests to run with a per-account store.
test/widgets/compose_box_test.dart	Adds ServerEmojiData setup needed by emoji-name lookup.

Comments suppressed due to low confidence (1)

lib/model/content.dart:1160

• UnicodeEmojiNode now has an emojiCode field, but == and hashCode still only consider emojiUnicode. That can make nodes with different codes compare equal and also means tests won’t fail if the parser sets an incorrect emojiCode. Include emojiCode in equality and hashCode calculations.

  @override
  bool operator ==(Object other) {
    return other is UnicodeEmojiNode && other.emojiUnicode == emojiUnicode;
  }

  @override
  int get hashCode => Object.hash('UnicodeEmojiNode', emojiUnicode);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[MritunjayTiwari14]

Ready for Review @chrisbobbe / @gnprice .

[chrisbobbe]

Thanks for building this!

This comment from Copilot looks like it still applies: #2268 (comment)

ImageEmojiNode.alt values in Zulip-rendered HTML include surrounding colons (e.g. :flutter:), but EmojiDisplay.emojiName is expected to be the bare emoji name (e.g. flutter). Passing node.alt through will produce incorrect plain-text emoji output (textEmojiForEmojiName will wrap it in colons again). Consider using the title attribute (if available) or stripping leading/trailing : before passing as emojiName.

The "Message formatting" API doc seems to suggest the title attribute will indeed be present: https://zulip.com/api/message-formatting#emoji . Could you add a field for that in ImageEmojiNode? Maybe something like:

  /// The emoji's name, as in [Reaction.emojiName].
  final String emojiName;

[MritunjayTiwari14]

I have pushed new changes, PTAL. @chrisbobbe

For addressing the use of emojiName. I have added a field to ImageEmojiNode and am capturing it during parsing by using the title attribute from the docs you've provided (Thanks!).

In the Consecutive commit, I changed EmojiWidget to accept node.emojiName as a value instead of node.alt because the format was incorrect for rendering image emoji.

Notes:

• As I have confirmed, there is no visual difference due to this change, so my Before and After screenshot for changes in the PR description still holds good.

[chrisbobbe]

Thanks for building this! I'm eager for this to be fixed.

Comments below, from reading the first four commits:

fe8fe54 model [nfc]: Store corresponding emojiName in ImageEmojiNode during parsing
61e3e2e content: Render ImageEmoji with EmojiWidget, instead of 'RealmContentNetworkImage', directly
b3edc14 model [nfc]: Store corresponding emojiCode in UnicodeEmojiNode during parsing
52ec989 emoji [nfc]: Allow UnicodeEmojiDisplay to render at default size when ancestor is WidgetSpan on Android

and part of the fifth:

e24630b content: Render unicode emojis with EmojiWidget, instead of 'TextSpan'

(I'll finish reviewing that in a later round.)

Note in particular a few bugs that you could have caught with manual testing. 🙂

[chrisbobbe]

model [nfc]: Store corresponding emojiCode in `UnicodeEmojiNode` during parsing

Emojicode should not be extracted from emojiUnicode
during rendering; it would cause a performance issue.
During parsing, we already encounter emojiCode,
so we would rather store it at that moment.

Store method `getUnicodeEmojiNameByCode()` require
emojiCode, which will provide the corresponding emojiName,
a value required by `UnicodeEmojiDisplay`.

This would allow us to render UnicodeEmoji within
message content with `EmojiWidget` using `WidgetSpan`
instead of `TextSpan`.

Commit-message nits:

• Let's use content in the prefix instead of model, like other commits that touch this file (use Greg's "secret" to reading Git history)
• This isn't NFC, because we start rejecting image-emoji content that doesn't have this data in the form we're expecting. That's fine—it shouldn't happen in practice, as long as the doc is correct and servers behave as described—but it does mean this isn't a pure refactor.
• The commit-message body uses more words than needed to justify the change 🙂. We're just writing client code according to documented server behavior, for a feature that's obviously helpful.

Could say something like this:

content: Add ImageEmojiNode.emojiName

We'll pass this to EmojiWidget, when we start using that for #966.

API doc: https://zulip.com/api/message-formatting#emoji

[MritunjayTiwari14]

That commit message seems too short—straight to the point of why we are making the change. I would try to make my subsequent non-feature commits as concise as possible. Thanks!

[chrisbobbe]

How about giving EmojiWidget a static asWidgetSpan method, and cutting out MessageImageEmoji? For an example of this pattern, see UserStatusEmoji.asWidgetSpan.

Some differences from UserStatusEmoji.asWidgetSpan that I think exist:

• We don't need a position param
• We don't need an animationMode param, I think; we'll just accept the default behavior ImageAnimationMode.animateConditionally.
• We do want fontSize and textScaler params. And actually I think asWidgetSpan should choose size to be a bit larger than the scaled fontSize. Let's use the same ratio that applied before this commit, for plain-paragraph text with 1x text scaling. I think that would look like: textScaler.scale(fontSize) * (20 / 17).

[MritunjayTiwari14]

Thanks for the suggestion. I've implemented this for ImageEmoji locally. It looks clean. Can we replace UnicodeEmoji with the same static asWidgetSpan method?

[chrisbobbe]

Ah hmm. So this part of your commit message—

- Picking a still image instead of an animated one, depending on the setting.

—isn't done, right, because we're passing null unconditionally for resolvedStillUrl. We should pass a real value there if one exists.

Looks like a still URL isn't contained in the HTML (doc). It is contained in our data structures, though—(maybe with exceptions for ancient messages, I'm not sure?)—see EmojiStoreImpl.allRealmEmoji.

EmojiStoreImpl.allRealmEmoji is keyed by emoji-code, though, and that data isn't in the information either…

What if we used node.src to find the still URL in our data store? Would you try applying this diff in lib/model/emoji.dart, and change your widget code so that it computes the emojiDisplay to pass to EmojiWidget by calling store.imageEmojiDisplayForContent?

Details

diff --git lib/model/emoji.dart lib/model/emoji.dart
index 3d9efd2e1..2ba71be77 100644
--- lib/model/emoji.dart
+++ lib/model/emoji.dart
@@ -123,6 +123,26 @@ final class EmojiCandidate {
 
 /// The portion of [PerAccountStore] describing what emoji exist.
 mixin EmojiStore {
+  /// An [EmojiDisplay] for an image-emoji node in Zulip message content.
+  ///
+  /// An image-emoji node in Zulip message content doesn't have an "emoji code",
+  /// which we'd normally use for lookups into [allRealmEmoji].
+  /// To work around -- in particular to get [RealmEmojiItem.stillUrl]
+  /// in order to support the device animation settings --
+  /// this function looks for [src]
+  /// in an index of [allRealmEmoji] by [RealmEmojiItem.sourceUrl].
+  ///
+  /// Returns an [ImageEmojiDisplay],
+  /// including [ImageEmojiDisplay.resolvedStillUrl] if one was found,
+  /// and falls back to [TextEmojiDisplay]
+  /// if [src] or the still URL fails parsing.
+  // TODO(server) maybe servers can start including the emoji-code in the HTML?
+  //   Discussion: https://chat.zulip.org/#narrow/channel/378-api-design/topic/Image-emoji.20IDs.20in.20message.20content/near/2431748
+  EmojiDisplay imageEmojiDisplayForContent({
+    required String src,
+    required String emojiName,
+  });
+
   /// An [EmojiDisplay] for the specified emoji.
   ///
   /// Use [EmojiDisplay.resolve] on the result to apply the user's [Emojiset]
@@ -157,6 +177,14 @@ mixin ProxyEmojiStore on EmojiStore {
   @protected
   EmojiStore get emojiStore;
 
+  @override
+  EmojiDisplay imageEmojiDisplayForContent({
+    required String src,
+    required String emojiName,
+  }) {
+    return emojiStore.imageEmojiDisplayForContent(src: src, emojiName: emojiName);
+  }
+
   @override
   EmojiDisplay emojiDisplayFor({
     required ReactionType emojiType,
@@ -190,7 +218,8 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore {
   EmojiStoreImpl({
     required super.core,
     required this.allRealmEmoji,
-  }) : _serverEmojiData = null; // TODO(#974) maybe start from a hard-coded baseline
+  }) : _serverEmojiData = null, // TODO(#974) maybe start from a hard-coded baseline
+       _imageEmojiCodesBySourceUrl = null;
 
   /// The realm's custom emoji, indexed by their [RealmEmojiItem.emojiCode],
   /// including deactivated emoji not available for new uses.
@@ -209,6 +238,25 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore {
   /// The realm-relative URL of the unique "Zulip extra emoji", :zulip:.
   static const kZulipEmojiUrl = '/static/generated/emoji/images/emoji/unicode/zulip.png';
 
+  /// A map from [RealmEmojiItem.sourceUrl] to [RealmEmojiItem.emojiCode].
+  ///
+  /// Computed lazily, and invalidated on [RealmEmojiUpdateEvent].
+  Map<String, String>? _imageEmojiCodesBySourceUrl;
+
+  @override
+  EmojiDisplay imageEmojiDisplayForContent({
+    required String src,
+    required String emojiName,
+  }) {
+    _imageEmojiCodesBySourceUrl
+      ??= allRealmEmoji.map((key, value) => MapEntry(value.sourceUrl, key));
+    final emojiCode = _imageEmojiCodesBySourceUrl![src];
+    final item = emojiCode == null ? null : allRealmEmoji[emojiCode];
+
+    return _tryImageEmojiDisplay(
+      sourceUrl: src, stillUrl: item?.stillUrl, emojiName: emojiName);
+  }
+
   @override
   EmojiDisplay emojiDisplayFor({
     required ReactionType emojiType,
@@ -442,6 +490,7 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore {
   void handleRealmEmojiUpdateEvent(RealmEmojiUpdateEvent event) {
     allRealmEmoji = event.realmEmoji;
     _allEmojiCandidates = null;
+    _imageEmojiCodesBySourceUrl = null;
   }
 }

[MritunjayTiwari14]

Yes, it does work on manual testing! 🎉

[MritunjayTiwari14]

Thanks for the review, @chrisbobbe. I have updated the commit history, addressed all feedback, and changed the emoji rendering size. PTAL.

This changes how emojis are rendered visually from the previous review.

Before and After Changes (Most Recent)

Before	After

Before	After

Changes

• Increased size of Unicode emoji to match the default size of image emoji (20px base).
• Refactored rendering of Unicode Emoji with EmojiWidget.asWidgetSpan.
• Manually, as well as did widget testing for animated emojis being rendered 'still' on Android, turn off the animation setting.

[chrisbobbe]

CI is failing; please fix that (see README: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#tests )

[MritunjayTiwari14]

Fixed the CI test: I missed adding a parse test for a newly added ContentExample in model/content_test.dart, which was weirdly not observed when I ran git rebase --exec 'tools/check --diff @~' main locally.

Anyways, @chrisbobbe, the changes I mentioned in my previous comment are ready for review.