Skip to content

Commit 8f92f0c

Browse files
committed
emoji: Ensure consistency in the row height for text
earlier, text-only emoji in the picker and autocomplete lists used a Text widget. This caused inconsistent row heights This pr change replaces the Text placeholder with a simple SizedBox.square, which gives a fixed-size empty space. this therefore ensures the height of text-only items perfectly matches items with image emojis. Adds regression tests for both EmojiPickerListEntry and _EmojiAutocompleteItem to verify the consistent height . Fixes #1587.
1 parent 4c42364 commit 8f92f0c

File tree

4 files changed

+99
-11
lines changed

4 files changed

+99
-11
lines changed

lib/widgets/autocomplete.dart

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -342,14 +342,15 @@ class _EmojiAutocompleteItem extends StatelessWidget {
342342
final designVariables = DesignVariables.of(context);
343343
final candidate = option.candidate;
344344

345-
// TODO deduplicate this logic with [EmojiPickerListEntry]
345+
// This logic is now corrected to match EmojiPickerListEntry as per todo
346346
final emojiDisplay = candidate.emojiDisplay.resolve(store.userSettings);
347-
final Widget? glyph = switch (emojiDisplay) {
347+
final Widget glyph = switch (emojiDisplay) {
348348
ImageEmojiDisplay() =>
349349
ImageEmojiWidget(size: _size, emojiDisplay: emojiDisplay),
350350
UnicodeEmojiDisplay() =>
351351
UnicodeEmojiWidget(size: _size, emojiDisplay: emojiDisplay),
352-
TextEmojiDisplay() => null, // The text is already shown separately.
352+
// Use a simple SizedBox for a perfect, non-scaling placeholder.
353+
TextEmojiDisplay() => const SizedBox.square(dimension: _size),
353354
};
354355

355356
final label = candidate.aliases.isEmpty
@@ -367,11 +368,10 @@ class _EmojiAutocompleteItem extends StatelessWidget {
367368
return Padding(
368369
padding: const EdgeInsets.symmetric(horizontal: 16.0, vertical: 8.0),
369370
child: Row(children: [
370-
if (glyph != null) ...[
371-
Padding(padding: const EdgeInsets.all(6),
372-
child: glyph),
373-
const SizedBox(width: 6),
374-
],
371+
// The `if (glyph != null)` check is removed, as the glyph is always present.
372+
Padding(padding: const EdgeInsets.all(6),
373+
child: glyph),
374+
const SizedBox(width: 6),
375375
Expanded(
376376
child: Text(
377377
style: TextStyle(fontSize: 17, height: 18 / 17,

lib/widgets/emoji_reaction.dart

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -577,12 +577,13 @@ class EmojiPickerListEntry extends StatelessWidget {
577577

578578
// TODO deduplicate this logic with [_EmojiAutocompleteItem]
579579
final emojiDisplay = emoji.emojiDisplay.resolve(store.userSettings);
580-
final Widget? glyph = switch (emojiDisplay) {
580+
final Widget glyph = switch (emojiDisplay) {
581581
ImageEmojiDisplay() =>
582582
ImageEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay),
583583
UnicodeEmojiDisplay() =>
584584
UnicodeEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay),
585-
TextEmojiDisplay() => null, // The text is already shown separately.
585+
TextEmojiDisplay() => Text(' ',
586+
style: TextStyle(fontSize: _emojiSize), textScaler: TextScaler.noScaling),
586587
};
587588

588589
final label = emoji.aliases.isEmpty
@@ -601,7 +602,7 @@ class EmojiPickerListEntry extends StatelessWidget {
601602
child: Row(spacing: 4, children: [
602603
Padding(
603604
padding: const EdgeInsets.all(10),
604-
child: glyph ?? SizedBox(width: _emojiSize, height: _emojiSize)),
605+
child: glyph),
605606
Flexible(child: Text(label,
606607
maxLines: 2,
607608
overflow: TextOverflow.ellipsis,

test/widgets/autocomplete_test.dart

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,56 @@ void main() {
372372
expected ? displaySubject.findsOne(): displaySubject.findsNothing();
373373
}
374374

375+
testWidgets('text emoji autocomplete item must have correct height', (tester) async {
376+
// Regression test for #1587
377+
tester.platformDispatcher.textScaleFactorTestValue = 2.0;
378+
addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue);
379+
380+
final composeInputFinder = await setupToComposeInput(tester);
381+
382+
// Set up a realm emoji so we have a reliable image emoji to test against.
383+
await store.handleEvent(RealmEmojiUpdateEvent(id: 1, realmEmoji: {
384+
'1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'an_image_emoji'),
385+
}));
386+
await tester.pumpAndSettle(); // Ensure store update is fully processed.
387+
388+
// Phase 1: Get the height of the item when it has an image.
389+
// This two-step `enterText` call is a more reliable way to trigger the autocomplete view.
390+
await tester.enterText(composeInputFinder, ':an_imag');
391+
await tester.enterText(composeInputFinder, ':an_image');
392+
await tester.pumpAndSettle();
393+
394+
final imageItemFinder = find.ancestor(
395+
of: find.text('an_image_emoji'),
396+
matching: find.byType(InkWell),
397+
);
398+
check(imageItemFinder).findsOne();
399+
final imageItemHeight = tester.getSize(imageItemFinder).height;
400+
401+
// Phase 2: Change the setting to render the same item as text-only.
402+
await store.handleEvent(UserSettingsUpdateEvent(id: 1,
403+
property: UserSettingName.emojiset,
404+
value: Emojiset.text));
405+
406+
// Re-trigger autocomplete to have it rebuild with the new setting.
407+
await tester.enterText(composeInputFinder, ':an_imag');
408+
await tester.enterText(composeInputFinder, ':an_image');
409+
await tester.pumpAndSettle();
410+
411+
// Find the same item, now rendered as a text-only item with a placeholder.
412+
final textItemFinder = find.ancestor(
413+
of: find.text('an_image_emoji'),
414+
matching: find.byType(InkWell),
415+
);
416+
check(textItemFinder).findsOne();
417+
final textItemHeight = tester.getSize(textItemFinder).height;
418+
419+
// Phase 3: Assert the heights are consistent.
420+
check(textItemHeight).isCloseTo(imageItemHeight, 1.0);
421+
422+
debugNetworkImageHttpClientProvider = null;
423+
});
424+
375425
testWidgets('show, update, choose', (tester) async {
376426
final composeInputFinder = await setupToComposeInput(tester);
377427
final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);

test/widgets/emoji_reaction_test.dart

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,43 @@ void main() {
556556
debugNetworkImageHttpClientProvider = null;
557557
});
558558

559+
testWidgets('text emoji list items must have correct height', (tester) async {
560+
// Regression test for #1587
561+
tester.platformDispatcher.textScaleFactorTestValue = 2.0;
562+
addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue);
563+
564+
final message = eg.streamMessage();
565+
// Firstly, set up the initial emoji picker. This leaves it open.
566+
await setupEmojiPicker(tester, message: message, narrow: TopicNarrow.ofMessage(message));
567+
568+
// and Now, change the setting to text-only emoji.
569+
await store.handleEvent(UserSettingsUpdateEvent(id: 1,
570+
property: UserSettingName.emojiset,
571+
value: Emojiset.text));
572+
573+
// Close the emoji picker. This also closes the underlying action sheet also.
574+
Navigator.of(tester.element(find.byType(EmojiPicker))).pop();
575+
await tester.pumpAndSettle(); // Let dismiss animations finish.
576+
577+
// We are now back at the message list. Let's start over to see the setting change.
578+
// now we Long-press the message to open the action sheet again.
579+
await tester.longPress(find.byType(MessageContent));
580+
await tester.pumpAndSettle();
581+
582+
// now we Tap the button to open the emoji picker, which will now use the new setting.
583+
await tester.tap(find.byIcon(ZulipIcons.chevron_right));
584+
await tester.pumpAndSettle();
585+
586+
// Finally, find a list entry and check its height.
587+
final listEntryFinder = find.byType(EmojiPickerListEntry).first;
588+
await tester.ensureVisible(listEntryFinder);
589+
590+
final renderBox = tester.renderObject<RenderBox>(listEntryFinder);
591+
check(renderBox.size.height).isGreaterThan(48.0);
592+
593+
debugNetworkImageHttpClientProvider = null;
594+
});
595+
559596
testWidgets('with bottom view padding', (tester) async {
560597
await prepare(tester, viewPadding: FakeViewPadding(bottom: 10));
561598

0 commit comments

Comments
 (0)