Skip to content
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

emoji: Fix bottom padding of emoji picker #1315

Merged
merged 3 commits into from
Mar 4, 2025
Merged

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Jan 30, 2025

Previously, the body of the bottom sheet was wrapped in SafeArea. This pads the bottom unconditionally, shifting the bottom of the list view above the device bottom inset.

This is undesirable, because the area beneath the bottom inset is still visible, and the list view should extend to the bottom regardless of the bottom inset.

By just removing the SafeArea, the list view extends to the bottom. However, if the bottom inset is more than 8px, we can't scroll past the last item in the list view. Essentially, we want the behavior of SafeArea.minimum with a ListView — the bottom of the list should always be padded by at least 8px, so that we can scroll past the shadow; and if the bottom inset is more than 8px, use that as the padding instead — which is achieved through the use ListView.padding and MediaQuery.

@PIG208
Copy link
Member Author

PIG208 commented Jan 30, 2025

Comparisons (older revision)

The bottom bar represents the device padding.

before (SafeArea + 8px bottom padding) after max(8px, bottom inset) bottom padding
partially scrolled to the bottom, with shadow visible
Screenshot_20250130_172009 Screenshot_20250130_171918
fully scrolled to the bottom
Screenshot_20250130_172003 Screenshot_20250130_171906
with keyboard open, unscrolled
Screenshot_20250130_172020 Screenshot_20250130_172056
Comparisons (w/ `CustomScrollView` and `SilverSafePadding`)
before (SafeArea + 8px bottom padding) after (SilverSafePadding)
partially scrolled to the bottom, with shadow visible
before-partial-bottom after-partial-bottom
fully scrolled to the bottom
before-full-bottom after-full-bottom
with keyboard open, unscrolled
before-keyboard after-keyboard

(This set of screenshots appear consistent with the older implementation; both should fix this issue)

Comparisons (w/ `CustomScrollView` and `SilverSafePadding` and no shadow)
before (SafeArea + 8px bottom padding) after (SilverSafePadding)
partially scrolled to the bottom
before-partial-bottom new-scroll-partial-w-padding
fully scrolled to the bottom
before-full-bottom new-scroll-full-w-padding
with keyboard open, unscrolled
before-keyboard after-keyboard

The following patch was used to make the builds in the screenshots:

diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart
index 2ad35e3e5..28a96cf68 100644
--- a/lib/widgets/app.dart
+++ b/lib/widgets/app.dart
@@ -201,6 +201,7 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
   @override
   Widget build(BuildContext context) {
     final themeData = zulipThemeData(context);
+    final mediaQuery = MediaQuery.of(context);
     return GlobalStoreWidget(
       child: MaterialApp(
         onGenerateTitle: (BuildContext context) {
@@ -218,7 +219,21 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
               (_) => widget._declareReady());
           }
           GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context);
-          return child!;
+          return Center(child: Stack(
+            children: [
+              MediaQuery(
+                data: mediaQuery.copyWith(
+                  viewPadding: mediaQuery.viewPadding.copyWith(bottom: 30),
+                  padding: mediaQuery.padding.copyWith(bottom: 30),
+                ),
+                child: child!),
+              Positioned(
+                left: 0,
+                right: 0,
+                bottom: 0,
+                child: Container(height: 30, decoration: BoxDecoration(color: Colors.black.withValues(alpha: 0.5)))),
+            ],
+          ));
         },
 
         // We use onGenerateInitialRoutes for the real work of specifying the

@PIG208 PIG208 added the maintainer review PR ready for review by Zulip maintainers label Jan 30, 2025
@PIG208 PIG208 force-pushed the pr-inset branch 2 times, most recently from ebfa257 to 584240c Compare January 30, 2025 23:06
@PIG208 PIG208 requested a review from chrisbobbe January 30, 2025 23:11
@PIG208 PIG208 changed the title emoji: Fix bottom inset padding of emoji picker emoji: Fix bottom padding of emoji picker Jan 30, 2025
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Comments below.

I stopped reading the tests when I realized they might not work with an alternative implementation I thought of. (Actually would it be possible to write a set of tests that would work with either/any implementation? That's generally preferable.)

Comment on lines 421 to 422
// This does remove the bottom inset.
// Leave it to the descendent [ListView].
useSafeArea: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this yet—what do you mean by "remove the bottom inset"?

Here's the doc:

/// The [useSafeArea] parameter specifies whether the sheet will avoid system
/// intrusions on the top, left, and right. If false, no [SafeArea] is added;
/// and [MediaQuery.removePadding] is applied to the top,
/// so that system intrusions at the top will not be avoided by a [SafeArea]
/// inside the bottom sheet either.
/// Defaults to false.

It doesn't say anything about controlling what's done about the bottom inset.

context: context,
// The bottom is padded below with `padding` within the [ListView].
removeBottom: true,
child: ListView.builder(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using CustomScrollView, with SliverPadding for the top padding and SliverSafeArea for the bottom padding? My feeling is that the code would be simpler; I'm curious if that turns out to be true. We use SliverSafeArea in a few places already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! This works for me:

      Expanded(child: InsetShadowBox(
        top: 8, bottom: 8,
        color: designVariables.bgContextMenu,
        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)))),
          ]))),

@PIG208 PIG208 force-pushed the pr-inset branch 2 times, most recently from da4794b to 0c1d767 Compare February 10, 2025 20:58
@PIG208
Copy link
Member Author

PIG208 commented Feb 10, 2025

Updated the PR, with new screenshots here: #1315 (comment).

The new implementation does require reworking the tests. I have updated them with an approach that should work just fine with the emoji picker implementation on main.

@chrisbobbe
Copy link
Collaborator

Thanks! Reviewing now.

One nit: I think this is the wrong column label in the table of screenshots for the current revision:

after max(8px, bottom inset) bottom padding

because the max approach isn't used in this revision. I think just a copy-paste error from the other table of screenshots.

Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One design-level comment I noticed: now that the content can scroll all the way to the bottom edge of the screen (which I also think is an improvement), how about removing the InsetShadowBox shadow effect at the bottom? We don't have that effect on similar scrollables, such as the "Combined feed" message list.

The shadow effect makes the viewport edge more graceful when there's not already a clear boundary that coincides with it, but the screen edge is such a boundary. Similarly the top edge of the keyboard when that's open. (Clear definition of the top edge of the keyboard is the platform's job, not ours.)

I'd be happy to discuss more in #mobile-design if needed.

Comment on lines 509 to 511
final options = tester.widgetList(find.byType(EmojiPickerListEntry));
firstOption = options.first;
lastOption = options.last;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
final options = tester.widgetList(find.byType(EmojiPickerListEntry));
firstOption = options.first;
lastOption = options.last;
final listEntryFinder = find.byType(EmojiPickerListEntry);
firstOption = tester.widget(listEntryFinder.first);
lastOption = tester.widget(listEntryFinder.last);

with the goal of leaning on flutter_test just a little bit more, in case it gives richer / more helpful feedback when a test fails—I don't know if it actually would in this case though.

Actually, with these chained finders (foo.first and foo.last), would we still need to store firstOption and lastOption to pass to a find.byWidget, or could we just used the chained finders directly?

Comment on lines 482 to 485
double getScrollChildrenTop(WidgetTester tester) =>
tester.getTopLeft(find.byWidget(firstOption)).dy;
double getScrollChildrenBottom(WidgetTester tester) =>
tester.getBottomLeft(find.byWidget(lastOption)).dy;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name, return type, and implementation don't seem to fit with each other; I can say more if helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I kind of struggled naming these helpers. I meant to express that this fetches y-coord of the top-most/bottom-most edge of the scrollable content (scroll items/scroll list?).

Are double getScrollElementTopEdgeDy and double getScrollElementBottomEdgeDy better?

Comment on lines 517 to 520
// The top of the list of children is padded by 8px;
// the bottom is out of view.
check(scrollViewTop).equals(getScrollChildrenTop(tester) - 8);
check(scrollViewBottom).isLessThan(getScrollChildrenBottom(tester));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this assumes there's a very long list of children, but I'm not seeing that clearly in the setup code. Reading setupEmojiPicker, called by prepare, I think I see two items being set up?

      store.setServerEmojiData(ServerEmojiData(codeToNames: {
        '1f4a4': ['zzz', 'sleepy'], // (just 'zzz' in real data)
      }));
      await store.handleEvent(RealmEmojiUpdateEvent(id: 1, realmEmoji: {
        '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'buzzing'),
      }));

Copy link
Member Author

@PIG208 PIG208 Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of them come from EmojiStore.popularEmojiCandidates.

The test controls the screen height so that the scroll view can be filled. I think we can add some additional checks in prepare to verify that.

Comment on lines 420 to 421
// This removes insets from all sides except bottom.
// Leave it to the descendent [CustomScrollView].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what "removes insets" means. Taken literally, it doesn't make sense; the insets are features of the device (which might include hardware) that our app isn't able to remove.

How about:

    // The bottom inset is left for [builder] to handle;
    // see [EmojiPicker] and its [CustomScrollView] for how we do that.

It's also possible that we don't need a comment at all, and the useSafeArea dartdoc is sufficient if readers are curious.

@PIG208
Copy link
Member Author

PIG208 commented Feb 14, 2025

Thanks for the review! Just pushed a new revision addressing the comments, and updated the screenshots.

Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Small comments on the tests, which actually I'm curious for Greg's thoughts on (so, marking for his review): one is a naming question, the other is about adding a helper that might be helpful but maybe not necessary.

Comment on lines 491 to 489
// This makes it easier to convert between device pixels used for
// [FakeViewPadding] and logical pixels used in tests.
tester.view.devicePixelRatio = 1.0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a way to let callers use less code, but it doesn't really remove the caller's burden to think about the physical/logical pixel distinction. That's because callers are still asked to make a FakeViewPadding, and the dartdoc of that says it's in physical pixels, while the rest of the test code is naturally in logical pixels. Callers are asked to assume the pixel ratio is 1, but they're not asked explicitly, in prepare's interface (it has no dartdoc), so they have to look at this line in the implementation, to be sure.

It's helpful that we're not asking callers to do math with devicePixelRatio, but I think there's a more satisfying way to do that, and with something reusable. (Helpful because this is likely to come up again.) What if we did this:

  • New file, like test/test_window.dart
  • New private subclass of FakeViewPadding:
    • It implements a method FakeViewPadding resolve(double devicePixelRatio) that returns a plain FakeViewPadding, scaled from this by devicePixelRatio
    • Its left, top, right, and bottom getters throw errors saying that resolve must be called. (Or maybe call the superclass's getter just if the value is zero.) See for example the operator getter on ApiNarrowDm.
  • An extension on FakeViewPadding, adding a static method with return type FakeViewPadding, named something like fromLogicalPx, that creates and returns an instance of the private subclass

Then for these tests, callers would say:

FakeViewPadding.fromLogicalPx(bottom: 10)

and prepare would say

        tester.view.viewPadding = viewPadding.resolve(devicePixelRatio);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds useful but I think it's OK to skip it for these tests — this private prepare helper is used only by a couple of test cases right next to it, so it's OK that they depend on some details of it which aren't clearly described.

It'd be good to add a little comment here linking to the above PR comment to describe a cleaner way this could be done. That way if in the future we're doing something similar and consulting this code as an example, we'll see that and have the option to act on it.

Comment on lines 481 to 479
double getListEntriesTopEdgeOffset(WidgetTester tester) =>
tester.getTopLeft(listEntryFinder.first).dy;
double getListEntriesBottomEdgeOffset(WidgetTester tester) =>
tester.getBottomLeft(listEntryFinder.last).dy;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the finders restricted to list entries that are at least partially visible in the viewport, and do the tests depend on that? If so, maybe that should be reflected in these names. I agree naming these is hard 😅 #1315 (comment) —maybe Greg would have some thoughts, having recently worked with the sticky-header code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will find only list items that have actually been built and are present in the element tree.

The list will build items until either it runs out or it fills the viewport, plus 250px in either direction — the default value of [ScrollView.cacheExtent]. It looks like these tests never care about how far beyond the viewport the list items extend, as long as they extend beyond it at all — so the fact that cacheExtent is positive is enough to make things work.

@chrisbobbe chrisbobbe assigned gnprice and unassigned chrisbobbe Feb 20, 2025
@chrisbobbe chrisbobbe requested a review from gnprice February 20, 2025 01:56
@chrisbobbe chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Feb 20, 2025
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @PIG208 for fixing this, and @chrisbobbe for the previous reviews! Generally this looks good. Comments below.

Comment on lines 352 to 354
final searchFieldFinder = find.widgetWithText(TextField, 'Search emoji');
Finder findInPicker(Finder finder) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the name of this.

It should have a blank line to separate from the other declaration, because they're not both one-line declarations (nor closely related):

Suggested change
final searchFieldFinder = find.widgetWithText(TextField, 'Search emoji');
Finder findInPicker(Finder finder) =>
final searchFieldFinder = find.widgetWithText(TextField, 'Search emoji');
Finder findInPicker(Finder finder) =>

@@ -169,3 +169,8 @@ extension TableChecks on Subject<Table> {
extension IconButtonChecks on Subject<IconButton> {
Subject<bool?> get isSelected => has((x) => x.isSelected, 'isSelected');
}

extension OffsetChecks on Subject<Offset> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: keep the file organized more or less logically, rather than in the order we needed things; so put this near the top because it's from dart:ui, i.e. the engine; specifically just above Rect, because it's closely related to Rect but simpler

Comment on lines 497 to 499
scrollViewTopEdgeOffset = tester.getTopLeft(scrollViewFinder).dy;
scrollViewBottomEdgeOffset = tester.getBottomLeft(scrollViewFinder).dy;
final scrollViewHeight = tester.getSize(scrollViewFinder).height;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: These facts can all be bundled together as a Rect:

Suggested change
scrollViewTopEdgeOffset = tester.getTopLeft(scrollViewFinder).dy;
scrollViewBottomEdgeOffset = tester.getBottomLeft(scrollViewFinder).dy;
final scrollViewHeight = tester.getSize(scrollViewFinder).height;
scrollViewRect = tester.getRect(scrollViewFinder);

Then references below to these three variables can instead use .top, .bottom, .height on the Rect.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! I think we can make a helper for the list entries that returns a Rect too:

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


// 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.
check(scrollViewTopEdgeOffset).equals(getListEntriesTopEdgeOffset(tester) - 8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: line too long, key information past 80 columns

Suggested change
check(scrollViewTopEdgeOffset).equals(getListEntriesTopEdgeOffset(tester) - 8);
check(scrollViewTopEdgeOffset)
.equals(getListEntriesTopEdgeOffset(tester) - 8);

Comment on lines 481 to 479
double getListEntriesTopEdgeOffset(WidgetTester tester) =>
tester.getTopLeft(listEntryFinder.first).dy;
double getListEntriesBottomEdgeOffset(WidgetTester tester) =>
tester.getBottomLeft(listEntryFinder.last).dy;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will find only list items that have actually been built and are present in the element tree.

The list will build items until either it runs out or it fills the viewport, plus 250px in either direction — the default value of [ScrollView.cacheExtent]. It looks like these tests never care about how far beyond the viewport the list items extend, as long as they extend beyond it at all — so the fact that cacheExtent is positive is enough to make things work.

Comment on lines 491 to 489
// This makes it easier to convert between device pixels used for
// [FakeViewPadding] and logical pixels used in tests.
tester.view.devicePixelRatio = 1.0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds useful but I think it's OK to skip it for these tests — this private prepare helper is used only by a couple of test cases right next to it, so it's OK that they depend on some details of it which aren't clearly described.

It'd be good to add a little comment here linking to the above PR comment to describe a cleaner way this could be done. That way if in the future we're doing something similar and consulting this code as an example, we'll see that and have the option to act on it.

@PIG208
Copy link
Member Author

PIG208 commented Feb 26, 2025

Thanks! This is ready for review now.

@PIG208 PIG208 requested a review from gnprice February 26, 2025 04:37
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One nit — otherwise all now looks good.

// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
check(scrollViewRect.top).equals(listEntriesRect.top - 8);
check(scrollViewRect).top.equals(listEntriesRect.top - 8);

then combine with the next check as a cascade

@PIG208
Copy link
Member Author

PIG208 commented Feb 26, 2025

Thanks! Updated the PR.

@PIG208 PIG208 requested a review from gnprice March 3, 2025 19:55
PIG208 added 3 commits March 3, 2025 16:31
Previously, the body of the bottom sheet was wrapped in `SafeArea`. This
pads the bottom unconditionally, shifting the bottom of the list view
above the device bottom padding.

This is undesirable, because the area beneath the bottom padding is still
visible, and the list view should extend to the bottom regardless of the
bottom inset.

By just removing the `SafeArea`, the list view extends to the bottom.
However, if the bottom padding is more than 8px, we can't scroll past the
last entry in the list view.  Essentially, we want the behavior of
`SilverSafeArea.minimum` — the bottom edge of the list entries should
always be padded by at least 8px, so that we can scroll past the shadow;
and if the bottom padding is more than 8px, use that as the padding
instead.

Signed-off-by: Zixuan James Li <[email protected]>
We don't have other scrollables that are vertical, touching the bottom
viewport edge, and have shadow there.

This was brought up by Chris:

> The shadow effect makes the viewport edge more graceful when there's
  not already a clear boundary that coincides with it, but the screen
  edge is such a boundary. Similarly the top edge of the keyboard when
  that's open. (Clear definition of the top edge of the keyboard is
  the platform's job, not ours.)

CZO discussion:
  https://chat.zulip.org/#narrow/channel/530-mobile-design

Signed-off-by: Zixuan James Li <[email protected]>
@gnprice
Copy link
Member

gnprice commented Mar 4, 2025

Thanks! Looks good; merging.

@gnprice gnprice merged commit 78f58ac into zulip:main Mar 4, 2025
@PIG208 PIG208 deleted the pr-inset branch March 19, 2025 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants