From 058422a0986466b9dd20bd0f4c24fa5c72e4166a Mon Sep 17 00:00:00 2001 From: loveucifer Date: Tue, 5 Aug 2025 20:10:32 +0530 Subject: [PATCH] emoji: Ensure consistent height for text-only list items 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 #1587. --- lib/widgets/autocomplete.dart | 11 ++-- lib/widgets/emoji_reaction.dart | 92 ++++++++++++++------------- test/widgets/autocomplete_test.dart | 36 +++++++++++ test/widgets/emoji_reaction_test.dart | 26 ++++++++ 4 files changed, 116 insertions(+), 49 deletions(-) diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 5c853d2da0..41ae51fbf2 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -377,7 +377,7 @@ class _EmojiAutocompleteItem extends StatelessWidget { ImageEmojiWidget(size: _size, emojiDisplay: emojiDisplay), UnicodeEmojiDisplay() => UnicodeEmojiWidget(size: _size, emojiDisplay: emojiDisplay), - TextEmojiDisplay() => null, // The text is already shown separately. + TextEmojiDisplay() => null, }; final label = candidate.aliases.isEmpty @@ -395,11 +395,12 @@ class _EmojiAutocompleteItem extends StatelessWidget { return Padding( padding: const EdgeInsets.symmetric(horizontal: 16.0, vertical: 8.0), child: Row(children: [ - if (glyph != null) ...[ + if (glyph != null) Padding(padding: const EdgeInsets.all(6), - child: glyph), - const SizedBox(width: 6), - ], + child: glyph) + else + const SizedBox(height: 36.0, width: 0), + const SizedBox(width: 6), Expanded( child: Text( style: TextStyle(fontSize: 17, height: 18 / 17, diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart index 1f7d9c1872..6e4fc656f6 100644 --- a/lib/widgets/emoji_reaction.dart +++ b/lib/widgets/emoji_reaction.dart @@ -597,49 +597,53 @@ class EmojiPickerListEntry extends StatelessWidget { Navigator.pop(pageContext, emoji); } - @override - Widget build(BuildContext context) { - final store = PerAccountStoreWidget.of(context); - final designVariables = DesignVariables.of(context); - - // TODO deduplicate this logic with [_EmojiAutocompleteItem] - final emojiDisplay = emoji.emojiDisplay.resolve(store.userSettings); - final Widget? glyph = switch (emojiDisplay) { - ImageEmojiDisplay() => - ImageEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay), - UnicodeEmojiDisplay() => - UnicodeEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay), - TextEmojiDisplay() => null, // The text is already shown separately. - }; - - final label = emoji.aliases.isEmpty - ? emoji.emojiName - : [emoji.emojiName, ...emoji.aliases].join(", "); // TODO(#1080) - - return InkWell( - onTap: _onPressed, - splashFactory: NoSplash.splashFactory, - overlayColor: WidgetStateColor.resolveWith((states) => - states.any((e) => e == WidgetState.pressed) - ? designVariables.contextMenuItemBg.withFadedAlpha(0.20) - : Colors.transparent), - child: Padding( - padding: const EdgeInsets.symmetric(horizontal: 8), - child: Row(spacing: 4, children: [ - if (glyph != null) - Padding( - padding: const EdgeInsets.all(10), - child: glyph), - Flexible(child: Text(label, - maxLines: 2, - overflow: TextOverflow.ellipsis, - style: TextStyle( - fontSize: 17, - height: 18 / 17, - color: designVariables.textMessage))) - ]), - )); - } +@override +Widget build(BuildContext context) { + final store = PerAccountStoreWidget.of(context); + final designVariables = DesignVariables.of(context); + + // TODO deduplicate this logic with [_EmojiAutocompleteItem] + final emojiDisplay = emoji.emojiDisplay.resolve(store.userSettings); + final Widget? glyph = switch (emojiDisplay) { + ImageEmojiDisplay() => + ImageEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay), + UnicodeEmojiDisplay() => + UnicodeEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay), + TextEmojiDisplay() => null,// The text is already shown separately. + }; + + final label = emoji.aliases.isEmpty + ? emoji.emojiName + : [emoji.emojiName, ...emoji.aliases].join(", ");// TODO(#1080) + + return InkWell( + onTap: _onPressed, + splashFactory: NoSplash.splashFactory, + overlayColor: WidgetStateColor.resolveWith((states) => + states.any((e) => e == WidgetState.pressed) + ? designVariables.contextMenuItemBg.withFadedAlpha(0.20) + : Colors.transparent), + child: Padding( + padding: const EdgeInsets.symmetric(horizontal: 8), + child: Row(spacing: 4, children: [ + if (glyph != null) + Padding( + padding: const EdgeInsets.symmetric(vertical: 10.0), + child: glyph) + else + SizedBox( + height: _emojiSize + 8.0 + 8.0, + ), + Flexible(child: Text(label, + maxLines: 2, + overflow: TextOverflow.ellipsis, + style: TextStyle( + fontSize: 17, + height: 18 / 17, + color: designVariables.textMessage))) + ]), + )); +} } /// Opens a bottom sheet showing who reacted to the message. @@ -1084,4 +1088,4 @@ class ViewReactionsUserItem extends StatelessWidget { store.userDisplayName(userId))), ]))); } -} +} \ No newline at end of file diff --git a/test/widgets/autocomplete_test.dart b/test/widgets/autocomplete_test.dart index ddcfc26036..59efb5d7ee 100644 --- a/test/widgets/autocomplete_test.dart +++ b/test/widgets/autocomplete_test.dart @@ -372,6 +372,42 @@ void main() { expected ? displaySubject.findsOne(): displaySubject.findsNothing(); } + testWidgets('text emoji autocomplete item must have correct height', (tester) async { + // Regression test for #1587 + final composeInputFinder = await setupToComposeInput(tester); + + await store.handleEvent(RealmEmojiUpdateEvent(id: 1, realmEmoji: { + '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'an_image_emoji'), + })); + await tester.pump(); + + // 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(); + + final imageItemFinder = find.widgetWithText(InkWell, 'an_image_emoji'); + check(imageItemFinder).findsOne(); + final imageItemHeight = tester.getSize(imageItemFinder).height; + + // TODO(#226): Remove this extra edit when this bug is fixed. + await store.handleEvent(UserSettingsUpdateEvent(id: 1, + property: UserSettingName.emojiset, + value: Emojiset.text)); + + await tester.enterText(composeInputFinder, ':an_imag'); + await tester.enterText(composeInputFinder, ':an_image'); + await tester.pumpAndSettle(); + + final textItemFinder = find.widgetWithText(InkWell, 'an_image_emoji'); + check(textItemFinder).findsOne(); + final textItemHeight = tester.getSize(textItemFinder).height; + + check(textItemHeight).isCloseTo(imageItemHeight, 1.0); + + debugNetworkImageHttpClientProvider = null; + }); + testWidgets('show, update, choose', (tester) async { final composeInputFinder = await setupToComposeInput(tester); final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); diff --git a/test/widgets/emoji_reaction_test.dart b/test/widgets/emoji_reaction_test.dart index 027caf998b..89f37b397d 100644 --- a/test/widgets/emoji_reaction_test.dart +++ b/test/widgets/emoji_reaction_test.dart @@ -625,6 +625,32 @@ void main() { debugNetworkImageHttpClientProvider = null; }); + testWidgets('text emoji list items must have correct height', (tester) async { + // Regression test for #1587. + final message = eg.streamMessage(); + await setupEmojiPicker(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + + // Find a list entry and get its height with the default image-based emoji. + final listEntryFinder = find.byWidgetPredicate( + (w) => w is EmojiPickerListEntry && w.emoji.emojiName == 'zzz'); + await tester.ensureVisible(listEntryFinder); + final imageRenderBox = tester.renderObject(listEntryFinder); + final imageHeight = imageRenderBox.size.height; + + // Change the setting to text-only emoji. + await store.handleEvent(UserSettingsUpdateEvent(id: 1, + property: UserSettingName.emojiset, + value: Emojiset.text)); + await tester.pumpAndSettle(); + + // Find the same list entry again and check that its height has not changed. + await tester.ensureVisible(listEntryFinder); + final textRenderBox = tester.renderObject(listEntryFinder); + check(textRenderBox.size.height).isCloseTo(imageHeight, 1.0); + + debugNetworkImageHttpClientProvider = null; + }); + testWidgets('with bottom view padding', (tester) async { await prepare(tester, viewPadding: FakeViewPadding(bottom: 10));