fix(emoji): Adjust picker layout for plain-text-emojis setting #1587 #1759

loveucifer · 2025-07-28T14:42:42Z

Before	After

Before	After

chrisbobbe

Hi @loveucifer, thanks for the PR.

It needs a test, in the "EmojiPicker" group in test/widgets/emoji_reaction_test.dart.
- The test should have a comment like "Regression test for #1587". It should pass with the change in this commit, and it should fail without the change.
- In the test, set the "emojiset" to Emojiset.text, show the emoji picker, find a list item in there, and check that its height is correct. To check its height, try getting the render object with tester.renderObject and looking at its .size.height.
EmojiPickerListEntry.build has this comment: "// TODO deduplicate this logic with [_EmojiAutocompleteItem]
- That can be out of scope for this PR, but the comment is a hint that some other code (in _EmojiAutocompleteItem) might have the same problem, with a similar fix. That seems to be the case; could you please make that fix and post screenshots of it too, and write a test for it?
Please revise the commit message according to the project style (follow links in the README)

loveucifer · 2025-07-29T01:19:01Z

Hi @loveucifer, thanks for the PR.

It needs a test, in the "EmojiPicker" group in test/widgets/emoji_reaction_test.dart.

The test should have a comment like "Regression test for Emoji-reaction picker looks odd with plain-text-emojis setting #1587". It should pass with the change in this commit, and it should fail without the change.

In the test, set the "emojiset" to Emojiset.text, show the emoji picker, find a list item in there, and check that its height is correct. To check its height, try getting the render object with tester.renderObject and looking at its .size.height.

EmojiPickerListEntry.build has this comment: "// TODO deduplicate this logic with [_EmojiAutocompleteItem]

That can be out of scope for this PR, but the comment is a hint that some other code (in _EmojiAutocompleteItem) might have the same problem, with a similar fix. That seems to be the case; could you please make that fix and post screenshots of it too, and write a test for it?

Please revise the commit message according to the project style (follow links in the README)

oh okay will work on this

loveucifer · 2025-07-29T14:45:56Z

wrote the test for emojI-reaction but having a hard time with autocomplete test dont know why will try to send one asap

chrisbobbe · 2025-07-29T17:52:04Z

lib/widgets/emoji_reaction.dart

-      TextEmojiDisplay() => null, // The text is already shown separately.
+      TextEmojiDisplay() => Text(' ', style: TextStyle(fontSize: _emojiSize)),


When using the text-only emojis the emoji picker list items did not scale correctly with the system's text scale factor in the earlier pr . i think this was because the placeholder for the emoji glyph had a fixed, unscalable height, preventing the row from growing as we wanted.

Image and Unicode emoji don't scale with the system text-size setting (try it), so we shouldn't scale the empty-square placeholder either.

loveucifer · 2025-07-30T19:36:33Z

wrote the test for emojI-reaction but having a hard time with autocomplete test dont know why will try to send one asap

I had some help from llm to write the test for autocomplete but it did pass for now , couldn't figure out why the one I wrote kept falling even though I applied almost the same logic

loveucifer · 2025-07-30T19:43:00Z

wrote the test for emojI-reaction but having a hard time with autocomplete test dont know why will try to send one asap

I had some help from llm to write the test for autocomplete but it did pass for now , couldn't figure out why the one I wrote kept falling even though I applied almost the same logic

actually circling back and seeing why it failed I think it was a syntax error I think I used check(finder).findsOneWidget() instead of check(finder).findsOne()

chrisbobbe · 2025-07-30T19:49:30Z

OK, please comment when this is ready for another review. If you need help, see our README, in particular the "asking great questions" article linked from there: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#asking-questions-getting-help

loveucifer · 2025-07-30T19:51:00Z

OK, please comment when this is ready for another review. If you need help, see our README, in particular the "asking great questions" article linked from there: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#asking-questions-getting-help

i have done it , it is running through the checks rn , please do check and give a review when possible

chrisbobbe

Comments below. Please squash the two commits together and do another pass on the commit message to make sure it matches our style.

chrisbobbe · 2025-07-30T19:56:03Z

lib/widgets/autocomplete.dart

-    // TODO deduplicate this logic with [EmojiPickerListEntry]
+    // This logic is now corrected to match EmojiPickerListEntry as per todo


This TODO still applies; please don't delete it. The comment doesn't need to be changed. ("Deduplicate" means make a helper function or something that can be invoked in both places. As I mentioned earlier, you don't have to write that helper function as part of this PR.)

chrisbobbe · 2025-07-30T19:58:58Z

lib/widgets/autocomplete.dart

+      // Use a simple SizedBox for a perfect, non-scaling placeholder.
+      TextEmojiDisplay() => const SizedBox.square(dimension: _size),


Suggested change

// Use a simple SizedBox for a perfect, non-scaling placeholder.

TextEmojiDisplay() => const SizedBox.square(dimension: _size),

// The text is already shown; just put a placeholder here to set the row height.

TextEmojiDisplay() => const SizedBox.square(dimension: _size),

chrisbobbe · 2025-07-30T19:59:55Z

lib/widgets/autocomplete.dart

-            child: glyph),
-          const SizedBox(width: 6),
-        ],
+        // The `if (glyph != null)` check is removed, as the glyph is always present.


This comment isn't needed; it refers to something that isn't visible in the code.

chrisbobbe · 2025-07-30T20:00:18Z

lib/widgets/emoji_reaction.dart

+      TextEmojiDisplay() => Text(' ',
+        style: TextStyle(fontSize: _emojiSize), textScaler: TextScaler.noScaling),


We should use an empty square placeholder here too.

chrisbobbe · 2025-07-30T20:03:34Z

test/widgets/autocomplete_test.dart

+      await tester.pumpAndSettle(); // Ensure store update is fully processed.
+
+      // Phase 1: Get the height of the item when it has an image.
+      // This two-step `enterText` call is a more reliable way to trigger the autocomplete view.


Suggested change

// This two-step `enterText` call is a more reliable way to trigger the autocomplete view.

// TODO(#226): Remove this extra edit when this bug is fixed.

(here and the other place(s) where it applies)

chrisbobbe · 2025-07-30T20:04:51Z

test/widgets/emoji_reaction_test.dart

+      // Close the emoji picker. This also closes the underlying action sheet also.
+      Navigator.of(tester.element(find.byType(EmojiPicker))).pop();
+      await tester.pumpAndSettle(); // Let dismiss animations finish.
+
+      // We are now back at the message list. Let's start over to see the setting change.
+      // now we Long-press the message to open the action sheet again.
+      await tester.longPress(find.byType(MessageContent));
+      await tester.pumpAndSettle();
+
+      // now we Tap the button to open the emoji picker, which will now use the new setting.
+      await tester.tap(find.byIcon(ZulipIcons.chevron_right));
+      await tester.pumpAndSettle();


Is it necessary to close and re-open the emoji picker? What happens if the test doesn't do that?

I thought that byy completely closing the emoji picker and re-opening it, we could be be sure that it was a brand-new widget built with the updated "Text only" setting, is that bad?

loveucifer · 2025-07-30T20:31:54Z

Comments below. Please squash the two commits together and do another pass on the commit message to make sure it matches our style.

as mentioned in my other pr im not sure if im doing the rebase thing correctly I havent used it yet ,will try another go again, also trying another pass on the commit style and as an update the test still works without reopening the emoji-picker

chrisbobbe

Thanks, comments below.

Please find the place in our commit-discipline doc where it says how to wrap the commit-message body text. If that's difficult for you, please explain where you looked and how we can make it more obvious for future contributors.

Looking back at the issue #1587 and your screenshots (thanks for posting those!), I've also realized that we don't actually want to add any horizontal whitespace, only vertical whitespace. The empty space on the left looks odd. So in the text-emoji case, instead of creating a padded SizedBox.square, we should put a zero-width SizedBox(height: n) where n is whatever would make the rows the same height as in the non-text-emoji case.

chrisbobbe · 2025-07-31T19:17:19Z

test/widgets/emoji_reaction_test.dart

+        final renderBox = tester.renderObject<RenderBox>(listEntryFinder);
+        check(renderBox.size.height).isLessThan(45.0);


Why check that the height is less than a number? The point of this test is to make sure the height is large enough. In other words (as you've written in a comment at the top), it's a regression test for #1587.

chrisbobbe · 2025-07-31T19:19:28Z

test/widgets/emoji_reaction_test.dart

+        tester.platformDispatcher.textScaleFactorTestValue = 2.0;
+        addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue);


The reproduction recipe for #1587 doesn't involve setting the system text-size setting, so I don't think these lines are needed in a regression test for #1587. What happens if we remove them?

chrisbobbe · 2025-07-31T19:20:01Z

test/widgets/emoji_reaction_test.dart

+        // Set up the emoji picker, which leaves it open.
+        await setupEmojiPicker(tester, message: message, narrow: TopicNarrow.ofMessage(message));


nit: Remove the comment; it describes something that's already obvious from reading the code

chrisbobbe · 2025-07-31T19:20:13Z

test/widgets/emoji_reaction_test.dart

+        // Set up the emoji picker, which leaves it open.
+        await setupEmojiPicker(tester, message: message, narrow: TopicNarrow.ofMessage(message));
+
+        // Now, change the setting to text-only emoji.


nit: also remove this comment for the same reason

chrisbobbe · 2025-07-31T19:20:23Z

test/widgets/emoji_reaction_test.dart

+        await store.handleEvent(UserSettingsUpdateEvent(id: 1,
+          property: UserSettingName.emojiset,
+          value: Emojiset.text));
+        // The picker is already open and will rebuild with the new setting.


nit: and this one

chrisbobbe · 2025-07-31T19:21:01Z

test/widgets/autocomplete_test.dart

+
+      final composeInputFinder = await setupToComposeInput(tester);
+
+      // Set up a realm emoji so we have a reliable image emoji to test against.


nit: Remove this comment; it explains something that's already obvious from reading the code

chrisbobbe · 2025-07-31T19:21:29Z

test/widgets/autocomplete_test.dart

+      await store.handleEvent(RealmEmojiUpdateEvent(id: 1, realmEmoji: {
+        '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'an_image_emoji'),
+      }));
+      await tester.pumpAndSettle(); // Ensure store update is fully processed.


Can this just be an await tester.pump()? Also the comment doesn't seem necessary.

chrisbobbe · 2025-07-31T19:21:42Z

test/widgets/autocomplete_test.dart

+      check(imageItemFinder).findsOne();
+      final imageItemHeight = tester.getSize(imageItemFinder).height;
+
+      //TODO(#226): Remove this extra edit when this bug is fixed.


nit: space after //

chrisbobbe · 2025-07-31T19:22:09Z

test/widgets/autocomplete_test.dart

+      // TODO(#226): Remove this extra edit when this bug is fixed.
+      await tester.enterText(composeInputFinder, ':an_imag');
+      await tester.enterText(composeInputFinder, ':an_image');
+      await tester.pumpAndSettle();


nit: Can this just be an await tester.pump()? What do the other tests do?

chrisbobbe · 2025-07-31T19:23:01Z

test/widgets/autocomplete_test.dart

+      final imageItemFinder = find.ancestor(
+        of: find.text('an_image_emoji'),
+        matching: find.byType(InkWell),
+      );


Can this be find.widgetWithText(InkWell, 'an_image_emoji')? (and similarly below)

loveucifer · 2025-08-01T07:01:58Z

So in the text-emoji case, instead of creating a padded SizedBox.square, we should put a zero-width SizedBox(height: n) where n is whatever would make the rows the same height as in the non-text-emoji case.

I gave this code initially -- final imageHeight = imageRenderBox.size.height; but the test failed and gave output he following TestFailure was thrown running a test: Expected: a double that: is within <1.0> of <40.0> Actual: <44.0> Which: differs by <4.0> so im changing it to this for now to see how it will go
40.0 (Target Height) - 10.0 (Top Padding) - 10.0 (Bottom Padding) = 20.0

loveucifer · 2025-08-01T07:31:09Z

Thanks, comments below.

Please find the place in our commit-discipline doc where it says how to wrap the commit-message body text. If that's difficult for you, please explain where you looked and how we can make it more obvious for future contributors.

Looking back at the issue #1587 and your screenshots (thanks for posting those!), I've also realized that we don't actually want to add any horizontal whitespace, only vertical whitespace. The empty space on the left looks odd. So in the text-emoji case, instead of creating a padded SizedBox.square, we should put a zero-width SizedBox(height: n) where n is whatever would make the rows the same height as in the non-text-emoji case.

also updating the image addressing this new change for autocomplete

pubspec.lock

lib/widgets/autocomplete.dart

chrisbobbe · 2025-08-01T18:05:14Z

lib/widgets/emoji_reaction.dart

-          if (glyph != null)
-            Padding(
+          Padding(
              padding: const EdgeInsets.all(10),
              child: glyph),


This adds horizontal padding (const EdgeInsets.all(10)). We don't want to do that, as I mentioned in #1759 (review).

I suggest keeping null for glyph in the text-emoji case, and adding an else here that makes a SizedBox(height: _emojiSize + 10 + 10). Similarly in the other case.

okay will work on that

loveucifer · 2025-08-05T15:05:45Z

@chrisbobbe could you help me out in this thread--https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/merge.20conflicts.20help

Previously, text-only emoji rows in the emoji picker and autocomplete lists were shorter than rows with image emoji, causing layout inconsistency. This commit fixes the layout by conditionally adding a `SizedBox` spacer for text-only items. The spacer's height is set to match the total height of a padded image glyph, ensuring all rows have a consistent height without adding unwanted horizontal padding. This change is applied to both `EmojiPickerListEntry` and `_EmojiAutocompleteItem`. The corresponding regression tests are updated to verify that the height of a text-only item is identical to an image-based item. Fixes zulip#1587.

chrisbobbe requested changes Jul 28, 2025

View reviewed changes

chrisbobbe reviewed Jul 29, 2025

View reviewed changes

loveucifer force-pushed the fix/emoji-picker-plain-text-1587 branch from adeca3b to 8f92f0c Compare July 30, 2025 19:31

loveucifer requested a review from chrisbobbe July 30, 2025 19:59

chrisbobbe requested changes Jul 30, 2025

View reviewed changes

loveucifer force-pushed the fix/emoji-picker-plain-text-1587 branch from 8f92f0c to f301858 Compare July 30, 2025 20:46

loveucifer requested a review from chrisbobbe July 31, 2025 03:29

chrisbobbe requested changes Jul 31, 2025

View reviewed changes

loveucifer force-pushed the fix/emoji-picker-plain-text-1587 branch from f301858 to 24826ef Compare August 1, 2025 07:22

loveucifer force-pushed the fix/emoji-picker-plain-text-1587 branch from 24826ef to 2603c0d Compare August 1, 2025 07:40

chrisbobbe requested changes Aug 1, 2025

View reviewed changes

loveucifer force-pushed the fix/emoji-picker-plain-text-1587 branch from 2603c0d to ffe4843 Compare August 1, 2025 20:03

loveucifer marked this pull request as draft August 5, 2025 13:02

loveucifer force-pushed the fix/emoji-picker-plain-text-1587 branch 2 times, most recently from 2e74f56 to bfc715b Compare August 5, 2025 14:52

loveucifer marked this pull request as ready for review August 5, 2025 15:12

loveucifer force-pushed the fix/emoji-picker-plain-text-1587 branch from bfc715b to 058422a Compare August 5, 2025 15:17

loveucifer requested a review from chrisbobbe August 5, 2025 16:54

		TextEmojiDisplay() => null, // The text is already shown separately.
		TextEmojiDisplay() => Text(' ', style: TextStyle(fontSize: _emojiSize)),

		// TODO deduplicate this logic with [EmojiPickerListEntry]
		// This logic is now corrected to match EmojiPickerListEntry as per todo

		// Use a simple SizedBox for a perfect, non-scaling placeholder.
		TextEmojiDisplay() => const SizedBox.square(dimension: _size),

		TextEmojiDisplay() => Text(' ',
		style: TextStyle(fontSize: _emojiSize), textScaler: TextScaler.noScaling),

	// This two-step `enterText` call is a more reliable way to trigger the autocomplete view.
	// TODO(#226): Remove this extra edit when this bug is fixed.

		final renderBox = tester.renderObject<RenderBox>(listEntryFinder);
		check(renderBox.size.height).isLessThan(45.0);

		tester.platformDispatcher.textScaleFactorTestValue = 2.0;
		addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue);

		// Set up the emoji picker, which leaves it open.
		await setupEmojiPicker(tester, message: message, narrow: TopicNarrow.ofMessage(message));


		final composeInputFinder = await setupToComposeInput(tester);

		// Set up a realm emoji so we have a reliable image emoji to test against.

fix(emoji): Adjust picker layout for plain-text-emojis setting #1587 #1759

Are you sure you want to change the base?

fix(emoji): Adjust picker layout for plain-text-emojis setting #1587 #1759

Conversation

loveucifer commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

loveucifer commented Jul 29, 2025

Uh oh!

loveucifer commented Jul 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loveucifer commented Jul 30, 2025

Uh oh!

loveucifer commented Jul 30, 2025

Uh oh!

chrisbobbe commented Jul 30, 2025

Uh oh!

loveucifer commented Jul 30, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loveucifer commented Jul 30, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loveucifer commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loveucifer commented Aug 1, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loveucifer commented Aug 5, 2025

Uh oh!

Uh oh!

loveucifer commented Jul 28, 2025 •

edited

Loading

loveucifer commented Aug 1, 2025 •

edited

Loading