Skip to content

emoji: Fix bottom padding of emoji picker #1315

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.

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

Merged
merged 3 commits into from
Mar 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 26 additions & 19 deletions lib/widgets/emoji_reaction.dart
Original file line number Diff line number Diff line change
Expand Up @@ -417,20 +417,21 @@ void showEmojiPickerSheet({
// on my iPhone 13 Pro but is marked as "much slower":
// https://api.flutter.dev/flutter/dart-ui/Clip.html
clipBehavior: Clip.antiAlias,
// The bottom inset is left for [builder] to handle;
// see [EmojiPicker] and its [CustomScrollView] for how we do that.
useSafeArea: true,
isScrollControlled: true,
builder: (BuildContext context) {
return SafeArea(
child: Padding(
// By default, when software keyboard is opened, the ListView
// expands behind the software keyboard — resulting in some
// list entries being covered by the keyboard. Add explicit
// bottom padding the size of the keyboard, which fixes this.
padding: EdgeInsets.only(bottom: MediaQuery.viewInsetsOf(context).bottom),
// For _EmojiPickerItem, and RealmContentNetworkImage used in ImageEmojiWidget.
child: PerAccountStoreWidget(
accountId: store.accountId,
child: EmojiPicker(pageContext: pageContext, message: message))));
return Padding(
// By default, when software keyboard is opened, the ListView
// expands behind the software keyboard — resulting in some
// list entries being covered by the keyboard. Add explicit
// bottom padding the size of the keyboard, which fixes this.
padding: EdgeInsets.only(bottom: MediaQuery.viewInsetsOf(context).bottom),
// For _EmojiPickerItem, and RealmContentNetworkImage used in ImageEmojiWidget.
child: PerAccountStoreWidget(
accountId: store.accountId,
child: EmojiPicker(pageContext: pageContext, message: message)));
});
}

Expand Down Expand Up @@ -529,15 +530,21 @@ class _EmojiPickerState extends State<EmojiPicker> with PerAccountStoreAwareStat
style: const TextStyle(fontSize: 20, height: 30 / 20))),
])),
Expanded(child: InsetShadowBox(
top: 8, bottom: 8,
top: 8,
color: designVariables.bgContextMenu,
child: ListView.builder(
padding: const EdgeInsets.symmetric(vertical: 8),
itemCount: _resultsToDisplay.length,
itemBuilder: (context, i) => EmojiPickerListEntry(
pageContext: widget.pageContext,
emoji: _resultsToDisplay[i].candidate,
message: widget.message)))),
child: CustomScrollView(
slivers: [
SliverPadding(
padding: EdgeInsets.only(top: 8),
sliver: SliverSafeArea(
minimum: EdgeInsets.only(bottom: 8),
sliver: SliverList.builder(
itemCount: _resultsToDisplay.length,
itemBuilder: (context, i) => EmojiPickerListEntry(
pageContext: widget.pageContext,
emoji: _resultsToDisplay[i].candidate,
message: widget.message)))),
]))),
]);
}
}
Expand Down
5 changes: 5 additions & 0 deletions test/flutter_checks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ extension PaintChecks on Subject<Paint> {
Subject<Shader?> get shader => has((x) => x.shader, 'shader');
}

extension OffsetChecks on Subject<Offset> {
Subject<double> get dx => has((x) => x.dx, 'dx');
Subject<double> get dy => has((x) => x.dy, 'dy');
}

extension RectChecks on Subject<Rect> {
Subject<double> get left => has((d) => d.left, 'left');
Subject<double> get top => has((d) => d.top, 'top');
Expand Down
98 changes: 92 additions & 6 deletions test/widgets/emoji_reaction_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,9 @@ void main() {

final searchFieldFinder = find.widgetWithText(TextField, 'Search emoji');

Finder findInPicker(Finder finder) =>
find.descendant(of: find.byType(EmojiPicker), matching: finder);

Condition<Object?> conditionEmojiListEntry({
required ReactionType emojiType,
required String emojiCode,
Expand Down Expand Up @@ -429,9 +432,7 @@ void main() {
await setupEmojiPicker(tester, message: message, narrow: TopicNarrow.ofMessage(message));

connection.prepare(json: {});
await tester.tap(find.descendant(
of: find.byType(BottomSheet),
matching: find.text('\u{1f4a4}'))); // 'zzz' emoji
await tester.tap(findInPicker(find.text('\u{1f4a4}'))); // 'zzz' emoji
await tester.pump(Duration.zero);

check(connection.lastRequest).isA<http.Request>()
Expand All @@ -453,9 +454,8 @@ void main() {
connection.prepare(
delay: const Duration(seconds: 2),
apiException: eg.apiBadRequest(message: 'Invalid message(s)'));
await tester.tap(find.descendant(
of: find.byType(BottomSheet),
matching: find.text('\u{1f4a4}'))); // 'zzz' emoji

await tester.tap(findInPicker(find.text('\u{1f4a4}'))); // 'zzz' emoji
await tester.pump(); // register tap
await tester.pump(const Duration(seconds: 1)); // emoji picker animates away
await tester.pump(const Duration(seconds: 1)); // error arrives; error dialog shows
Expand All @@ -466,6 +466,92 @@ void main() {

debugNetworkImageHttpClientProvider = null;
});

group('handle view paddings', () {
const screenHeight = 400.0;

late Rect scrollViewRect;
final scrollViewFinder = findInPicker(find.bySubtype<ScrollView>());

Rect getListEntriesRect(WidgetTester tester) =>
tester.getRect(find.byType(EmojiPickerListEntry).first)
.expandToInclude(tester.getRect(find.byType(EmojiPickerListEntry).last));

Future<void> prepare(WidgetTester tester, {
required FakeViewPadding viewPadding,
}) async {
addTearDown(tester.view.reset);
tester.view.physicalSize = Size(640, screenHeight);
// This makes it easier to convert between device pixels used for
// [FakeViewPadding] and logical pixels used in tests.
// If needed, there is a clearer way to implement this generally.
// See comment: https://github.com/zulip/zulip-flutter/pull/1315/files#r1962703436
tester.view.devicePixelRatio = 1.0;

tester.view.viewPadding = viewPadding;
tester.view.padding = viewPadding;

final message = eg.streamMessage();
await setupEmojiPicker(tester,
message: message, narrow: TopicNarrow.ofMessage(message));

scrollViewRect = tester.getRect(scrollViewFinder);
// The scroll view should expand all the way to the bottom of the
// screen, even if there is device bottom padding.
check(scrollViewRect)
..bottom.equals(screenHeight)
// There should always be enough entries to overflow the scroll view.
..height.isLessThan(getListEntriesRect(tester).height);
}

testWidgets('no view padding', (tester) async {
await prepare(tester, viewPadding: FakeViewPadding.zero);

// The top edge of the list entries is padded by 8px from the top edge
// of the scroll view; the bottom edge is out of view.
Rect listEntriesRect = getListEntriesRect(tester);
check(scrollViewRect)
..top.equals(listEntriesRect.top - 8)
..bottom.isLessThan(listEntriesRect.bottom);

// Scroll to the very bottom of the list with a large offset.
await tester.drag(scrollViewFinder, Offset(0, -500));
await tester.pump();
// The top edge of the list entries is out of view;
// the bottom is padded by 8px, the minimum padding, from the bottom
// edge of the scroll view.
listEntriesRect = getListEntriesRect(tester);
check(scrollViewRect)
..top.isGreaterThan(listEntriesRect.top)
..bottom.equals(listEntriesRect.bottom + 8);

debugNetworkImageHttpClientProvider = null;
});

testWidgets('with bottom view padding', (tester) async {
await prepare(tester, viewPadding: FakeViewPadding(bottom: 10));

// The top edge of the list entries is padded by 8px from the top edge
// of the scroll view; the bottom edge is out of view.
Rect listEntriesRect = getListEntriesRect(tester);
check(scrollViewRect)
..top.equals(listEntriesRect.top - 8)
..bottom.isLessThan(listEntriesRect.bottom);

// Scroll to the very bottom of the list with a large offset.
await tester.drag(scrollViewFinder, Offset(0, -500));
await tester.pump();
// The top edge of the list entries is out of view;
// the bottom edge is padded by 10px from the bottom edge of the scroll
// view, because the view bottom padding is larger than the minimum 8px.
listEntriesRect = getListEntriesRect(tester);
check(scrollViewRect)
..top.isGreaterThan(listEntriesRect.top)
..bottom.equals(listEntriesRect.bottom + 10);

debugNetworkImageHttpClientProvider = null;
});
});
});
}

Expand Down