compose_box: Wait for send message request to complete. #924

PIG208 · 2024-08-31T05:19:00Z

This supports the new UX of disabling the topic input, content input and the buttons on the compose box when sending a message, and only re-enable them and clearing the content field when the message has been successfully sent.

~~This partially implements #720, while mostly leaving the UI changes alone until we get to #915.~~

Fixes: #720

screenshots

	normal	banner	highlighted	progress bar	banner & progress bar
light
dark

screenshots (longer message)

normal	banner	progress bar	banner & progress bar

Note that this also changes the appearance of the error banner that replaces the compose box in narrows where you can't send messages:

screenshot

gnprice · 2024-11-25T21:28:51Z

Glad to see this ready for review!

Should this be marked as fixing #720, or is there something from that issue that this leaves open?

PIG208 · 2024-11-25T21:51:11Z

The second to last commit (fae4af1) has that "Fixes: #720" line, not sure why the backlink wasn't created (I thought GitHub just queued that job when I pushed 3 days ago).

gnprice · 2024-11-25T21:54:28Z

Ah. Yeah, the PR description needs it too:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#mention-fixed-issue

PIG208 · 2024-11-25T22:00:09Z

(resolved conflicts with the icon font)

chrisbobbe

Thanks! Comments below.

When reviewing this, I found / dug into some things in the compose-box code in main that I don't love, and I'll send a PR to propose treating those (edit: #1090). If that's merged, I'm hoping you'll find the code organized more helpfully for your work here. I acknowledge that this would be a tricky rebase, though, and I hope to help if you need it. 🙂

ComposeBoxController is implemented twice (by _StreamComposeBox and _FixedDestinationComposeBox). Some things, like tracking a new send-in-progress flag, should only need to be implemented once.
ComposeBox shouldn't have a role on pages like Combined Feed where we've decided not to show a compose box. Its "there-is-none" logic should be moved out.
_ComposeBoxLayout really isn't what it says on the tin. It's also confusing to have _ComposeBoxContainer too; I think neither widget is focused enough to make it really clear which one has what responsibilities.
Neater to arrange things so we can have just one code path for rendering the new UI (progress indicator and error banners), which is common to both kinds of compose boxes. Instead of one codepath through _StreamComposeBox and one through _FixedDestinationComposeBox.

chrisbobbe · 2024-11-21T02:13:00Z

lib/widgets/compose_box.dart

@@ -274,13 +274,15 @@ class ComposeContentController extends ComposeController<ContentValidationError>

 class _ContentInput extends StatefulWidget {
  const _ContentInput({
+    required this.enabled,


compose: Disable compose box widgets when sending message Also clear the content input only after the message is successfully sent. For the send button, unlike when there are validation errors, having an error dialog when its disabled for sending a message feels a bit redundant and doesn't help much. This can be annoying when the user misclicks and hit the send button multiple times. This implements some behavior not specified in Figma. When disabled: - Because the height of the progress indicator is 2px, to avoid shifting the content of the compose box, we expand it by 2px in height instead of 1px. - The bar containing only buttons other than the send button is faded, because the send button can be disabled separately through validation errors. If we need to disable the other compose buttons individually in the future, we can control the styling of the buttons instead. The progress indicator will be added in a later commit. Signed-off-by: Zixuan James Li <[email protected]>

I think this part isn't quite what you meant:

This implements some behavior not specified in Figma.

I read that as saying the Figma doesn't say anything about the thing; it has no answer. There are many things that the Figma doesn't answer because there isn't time or space to specify them, like how to handle left and right insets (e.g. for iPhone in landscape mode) or how to be careful with some accessibility features that people expect (like #1023).

For your first bullet point, I think what's happening is that the Figma does specify things about size and position, but this revision just doesn't follow those things. 🙂 Specifically, it looks like the Figma paints the loading bar as a layer on top of the compose box (above it in the z-direction), with its top edge aligned with the bottom of the compose box's 1px top border:

So we should either do that or explain why we aren't doing it. Your bullet point sounds kind of like that kind of explanation, except it's not convincing, because "shifting the content of the compose box" wouldn't be a consequence of following the Figma. 🙂

The second bullet point doesn't make sense to me because (if I'm reading correctly) all of the buttons are faded in the new disabled state, and that matches the Figma. There's an implementation detail about how that gets done (with an Opacity vs. something else), but I don't see an inconsistency with the Figma to mention here.

Yeah, I assumed the size/position change is a typo, as the baseline of the text does move in Figma:
We can still move the border up 1px and render the loading indicator below it, without also moving the text. Maybe that's something we can ask in chat for clarification.

The second bullet point doesn't make sense to me because (if I'm reading correctly) all of the buttons are faded in the new disabled state, and that matches the Figma. There's an implementation detail about how that gets done (with an Opacity vs. something else), but I don't see an inconsistency with the Figma to mention here.

Makes sense. I was referring to how we apply opacity to the entire compose button bar and etc. Let's leave this one out.

the baseline of the text does move in Figma

Oh, that is subtle! Thanks for pointing that out, and also that the border moves up 1px.

I agree it looks accidental. It looks like something is adding 1px of height to the compose box as empty space at the bottom.

Looks like the thing doing that is a "spacing bar" element. It looks like all these frames have such an element, and most of them use it to pad the bottom inset:

But in frames where the keyboard shows, it's meant to be a no-op because the keyboard is already padding the bottom inset; we don't want to double-pad it. In this particular frame (with the loading indicator) the element fails to be a no-op; its top edge sticks out from under the keyboard by 1px, unlike in the adjacent frame where the top of the element is aligned with the top of the keyboard.

Frame on the left Frame on the right

chrisbobbe · 2024-11-21T21:39:17Z

lib/widgets/compose_box.dart

-          child: Theme(
-            data: inputThemeData,


compose [nfc]: Apply redesigned iconButtonTheme more generally

This iconButtonThemeData

removes buttons' splash effect, and

sets the shape and color of buttons' on-pressed background.

(1) is something we want to do everywhere (#417), but I'm not sure we want (2) to include the "x" button on the error banner. Let's maybe leave that part of iconButtonThemeData here, even if we move (1) outward. (The no-splash part should be near the root of the app anyway.)

…Hmm, I guess we don't have a clear direction for this "x" button's pressed state, though. How about this, quoting Vlad on #417:

It is also ok to not bother for the smaller elements which user might not even see under the finger.

i.e., no touch feedback at all. Since the button's response is synchronous, there won't be a period where touch feedback can help reassure the user that the tap was registered. Either the banner will disappear immediately on touch (which is plenty of feedback already), or the app is being laggy because it's starved for resources. In the laggy case, the touch-feedback code will probably also be defeated by the lag, so won't help.

chrisbobbe · 2024-11-21T23:27:55Z

lib/widgets/compose_box.dart

  }
 }

 class _SendButton extends StatefulWidget {
  const _SendButton({
+    required this.enabled,


The name enabled doesn't feel like the right name for this new _SendButton param. The button will frequently seem disabled when this param is true. That's actually the initial state of any compose box, since an empty content input is a validation error.

How about either composingEnabled (or composingDisabled) or sendInProgress, as a new name? (Here and for all the added enabled/_enableds.)

chrisbobbe · 2024-11-21T23:55:21Z

lib/widgets/compose_box.dart

-        child: SafeArea(minimum: const EdgeInsets.symmetric(horizontal: 8),
-          child: child)));
+        child: Column(children: children)));


compose [nfc]: Make _ComposeBoxContainer accept children

I'd prefer to put the SafeArea change (making callers responsible for consuming the device insets) and the change to add a new Column into separate commits. That should make it easier to verify both changes as NFC.

Hmm, but also: handling the SafeArea logic around the compose box's inputs and buttons was a helpful job that _ComposeBoxContainer was doing, and it's not doing that anymore. Can we have it keep doing that for those elements, while still delegating the error banner's SafeArea handling to the caller? (Seems like the error banner will want to pad the horizontal insets.) To set different expectations for safe-area handling, we can replace _ComposeBoxContainer.children with two specific "slots" like body and errorBanner, and each one can give its expectations in its dartdoc. I have a draft PR to try this approach.

chrisbobbe · 2024-11-27T00:51:36Z

lib/widgets/theme.dart

      bgContextMenu: const Color(0xfff2f2f2),
      bgCounterUnread: const Color(0xff666699).withValues(alpha: 0.15),
      bgTopBar: const Color(0xfff5f5f5),
      borderBar: Colors.black.withValues(alpha: 0.2),
+      btnLabelAttLowIntDanger: const Color(0xffc0070a),
+      btnLabelAttMediumDanger: const Color(0xffac0508),


It's btn-label-att_medium-int_danger in the Figma (with "int", I guess for "intensity"?).

chrisbobbe · 2024-11-27T02:39:42Z

lib/widgets/compose_box.dart

@@ -1128,6 +1176,9 @@ class _StreamComposeBox extends StatefulWidget {
 }

 class _StreamComposeBoxState extends State<_StreamComposeBox> implements ComposeBoxController<_StreamComposeBox> {
+  @override bool get enabled => _enabled.value;
+  final _enabled = ValueNotifier<bool>(true);


Annoying that we need to implement this enabled flag separately for the two different kinds of compose-box controls (_StreamComposeBoxState and _FixedDestinationComposeBoxState). It shouldn't differ between them, and it would be better to have just a single implementation. Two implementations could diverge accidentally.

I have a local branch that makes this duplication unnecessary so we only have to implement this once; I plan to send that as a PR, which (if it looks good / gets merged) could make a nicer starting point for your work here.

chrisbobbe · 2024-11-27T02:44:07Z

assets/l10n/app_en.arb

@@ -169,6 +169,10 @@
  "@errorQuotationFailed": {
    "description": "Error message when quoting a message failed."
  },
+  "errorSendMessageTimeout": "Message isn’t sent. Check your connection.",


This is what it says in the Figma, but "isn't" doesn't sound like the right word. How about "Your message was not sent. Check your connection."

chrisbobbe · 2024-11-27T02:51:01Z

lib/widgets/compose_box.dart

@@ -1403,10 +1411,7 @@ class ComposeBox extends StatelessWidget {
  Widget build(BuildContext context) {
    final errorBanner = _errorBanner(context);
    if (errorBanner != null) {
-      return _ComposeBoxContainer(children: [
-        SafeArea(minimum: const EdgeInsets.symmetric(horizontal: 8),


This commit removes a SafeArea without adding one somewhere else, but doesn't make it clear that we don't need one.

Here are some screenshots at the tip of this branch; note the "x" button can be unusable and the text can be illegible or awkwardly close to the bottom:

In my prep-proposal branch, I have a version of the banner from the Figma that I think handles the insets correctly; this might be easier than investigating/explaining what's needed in prose.

chrisbobbe · 2024-11-27T03:02:47Z

lib/widgets/compose_box.dart

@@ -276,16 +276,33 @@ class ComposeContentController extends ComposeController<ContentValidationError>
 }

 class _TopBar extends StatelessWidget {
-  const _TopBar({required this.showProgressIndicator});
+  const _TopBar({required this.showProgressIndicator, required this.sendMessageError});


compose: Show send message error with dismissable error banners Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. […]

What's the rounded border you mention? In the Figma frame I'm looking at, the error banner does not have a rounded border:

The is a reference to: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3707-41711&node-type=frame&t=AeBFDmHie8U9C9EJ-0

We could just remove this as we resolve #924 (comment), though.

chrisbobbe · 2024-11-27T03:06:32Z

lib/widgets/compose_box.dart

+    final padding = (action == null)
+      // Ensure that the text is centered when it is the only element.
+      ? const EdgeInsets.symmetric(horizontal: 16, vertical: 9)
+      : const EdgeInsetsDirectional.fromSTEB(16, 9, 8, 9);


The comment is about vertical centering, right? I'm guessing that from a clue in the commit message (which I have in front of me because I'm reviewing it, but won't when I'm just reading code 🙂):

We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding.

There's a condition on action, but the two branches only differ in what horizontal padding they apply; that's confusing. I also need to load more things into my brain to prove to myself that the comment is correct. Does the device text-size setting make it incorrect? I don't know, I'd have to think more.

Ideally the comment wouldn't leave the reader with doubts about whether it's true, especially if they can't remove the doubts except by looking somewhere other than the code being commented on.

For this, I think I'd prefer to just center it in the standard way that makes sense, or else please say specifically what awkwardness happens when you try that. It's this revision that looks awkward to me, vs. if I simply remove the crossAxisAlignment: CrossAxisAlignment.start, line and accept the default (center):

This revision That line removed

This cherry-picks (with some adjustments) UI changes from Zixuan's current revision of PR zulip#924 (for issue zulip#720): compose: Support error banner redesign for replacing the compose box See the Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-15770&node-type=frame&t=vOSEWlXAhBa5EsAv-0 The new design is meant to be flush with the top of the compose-box surface, so zulip#1089 (where the old banner's top margin disappeared, looking buggy) is resolved. The surface (but not content) of the redesigned banner seems like it should extend through any left/right device insets to those edges of the screen. By taking the banner as param separate from `body`, _ComposeBoxContainer lets the banner do that -- without dropping its responsibility for keeping the body (inputs, compose buttons, and send button) out of the insets. The body doesn't need to consume the insets itself and it's already complicated enough without needing its own SafeAreas. It also seems like the banner should pad the bottom inset when it's meant to replace the body, so we do that and make the expectation clear in the `errorBanner` dartdoc. Co-authored-by: Zixuan James Li <[email protected]> Fixes: zulip#1089

This cherry-picks (with some adjustments) UI changes from Zixuan's current revision of PR zulip#924 (for issue zulip#720): compose: Support error banner redesign for replacing the compose box See the Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-15770&node-type=frame&t=vOSEWlXAhBa5EsAv-0 The new design is meant to put the banner at the very top of the compose-box surface, with no margin. So zulip#1089, where the old banner's top margin disappeared, looking buggy, is resolved. The surface (but not content) of the redesigned banner seems like it should extend through any left/right device insets to those edges of the screen. By taking the banner as param separate from `body`, _ComposeBoxContainer lets the banner do that -- without dropping its responsibility for keeping the body (inputs, compose buttons, and send button) out of the insets. The body doesn't need to consume the insets itself and it's already complicated enough without needing its own SafeAreas. It also seems like the banner should pad the bottom inset when it's meant to replace the body, so we do that and make the expectation clear in the `errorBanner` dartdoc. Co-authored-by: Zixuan James Li <[email protected]> Fixes: zulip#1089

chrisbobbe · 2024-12-04T00:03:43Z

#1090 has been merged, so this work is ripe to be redone on top of that. Please let me know if I can help.

Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2024-12-09T22:14:57Z

Happy to see this moving forward again!

I just tried the new revision, and one piece of UX that needs fixing: in a stream/channel narrow, after you send a message, focus moves to the topic input. That gets in the way of smoothly going on to type a follow-up message — and it also opens up the topic autocomplete, which is bothersome because presumably you want to continue in the same topic.

(I'm not sure if previous revisions had the same issue. I did go back and confirm the issue doesn't reproduce before this PR.)

PIG208 · 2024-12-09T22:15:39Z

Updated the PR resolving the major merge conflicts after the refactor. This isn't fully ready for merge because we haven't resolved the layout shift issue with the linear progress indicator. This update is mainly for the early beta release (cc @gnprice).

chrisbobbe · 2024-12-09T22:21:28Z

we haven't resolved the layout shift issue with the linear progress indicator

What's the layout shift issue? The indicator gets overlaid on top of the compose box so nothing needs to be resized or moved to accommodate it. The relevant Figma frame has an unrelated 1px shift which is just Vlad's small mistake related to safe-area handling: #924 (comment)

PIG208 · 2024-12-09T22:25:13Z

This revision mostly reproduces the previous draft and hasn't resolved the TODO comment on using a Stack to overlay the progress indicator yet:

      // TODO(#720) try a Stack for the overlaid linear progress indicator

I'm looking into the issue with the focus node right now, and will get back to that later.

Also clear the content input only after the message is successfully sent. For the send button, unlike when there are validation errors, having an error dialog when its disabled for sending a message feels a bit redundant and doesn't help much. This can be annoying when the user misclicks and hit the send button multiple times. Signed-off-by: Zixuan James Li <[email protected]>

This progress bar shifts the message list because it increases the height of the compose box. This is undesirable, but a fix can be implemented in the future. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

Exported from Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-32875&node-type=symbol&t=vOSEWlXAhBa5EsAv-0 Signed-off-by: Zixuan James Li <[email protected]>

Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding. Apart from that, when the action button is not available (e.g. _ErrorBanner that replaces the compose box in narrows without post permission), the padding around the label is adjusted to center it. Fixes: zulip#720 Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2024-12-09T22:55:57Z

Thanks for finding that @gnprice! After switching from using enabled to readOnly on both TextField's, I'm no longer reproducing the issue. Let me know if it's working for you. As a follow-up, it would probably be helpful to disable autocomplete if a field is read-only.

gnprice · 2024-12-09T23:13:25Z

Cool, that aspect is fixed.

With this revision there's still a behavior that I think people will find pretty annoying: when you hit send, the keyboard goes away, and then returns a moment later when the sent message arrives in the message list. Because of the keyboard's effect on the space available to the app, this means a big animation that moves everything on the screen, and then moves it back, over the course of something like half a second. I think we probably don't want to ship that to users.

It might be that this means that this strategy — the strategy described in #720 — just isn't going to work. On the one hand what the strategy calls for is to have a text field that (a) looks disabled and (b) really is disabled, not accepting input… but on the other hand, trying this UX out, I think we need (c) the keyboard to remain in place, just like it would if the text field still were accepting input. That's unavoidably going to be somewhat hacky — it's a keyboard that won't do anything, which risks a confusing UX — and it may be tricky to get TextField and friends to arrange such a thing for us.

gnprice · 2025-01-03T00:19:17Z

As discussed above, I think we probably won't go ahead with this approach. So closing this PR.

@PIG208 if there are parts of these changes that you think are useful on their own, please send a small new PR with those. In any case the changes are all still available in Git to refer back to if we end up wanting pieces of them later.

Thanks again @PIG208 and @chrisbobbe for your work on this!

PIG208 marked this pull request as draft August 31, 2024 05:19

PIG208 force-pushed the pr-await-send branch from 4214005 to 50842af Compare August 31, 2024 05:21

PIG208 force-pushed the pr-await-send branch 4 times, most recently from 96aff6a to a7774c0 Compare November 20, 2024 23:14

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Nov 20, 2024

PIG208 requested a review from chrisbobbe November 20, 2024 23:15

PIG208 marked this pull request as ready for review November 20, 2024 23:15

PIG208 assigned chrisbobbe Nov 20, 2024

PIG208 force-pushed the pr-await-send branch 2 times, most recently from c5b5a24 to 9051195 Compare November 22, 2024 20:30

PIG208 force-pushed the pr-await-send branch from 9051195 to 01c91ce Compare November 25, 2024 21:59

chrisbobbe mentioned this pull request Nov 27, 2024

compose: Error banner for deactivated DM recipient has lost its top margin #1089

Closed

chrisbobbe reviewed Nov 27, 2024

View reviewed changes

chrisbobbe mentioned this pull request Nov 27, 2024

compose: Refactor some logic; implement redesigned error banner #1090

Merged

PIG208 force-pushed the pr-await-send branch 2 times, most recently from ba0086d to ebddf7c Compare December 9, 2024 22:04

compose_box [nfc]: Name error banner helper more specifically

20c982d

Signed-off-by: Zixuan James Li <[email protected]>

PIG208 force-pushed the pr-await-send branch from ebddf7c to 66c944f Compare December 9, 2024 22:15

gnprice mentioned this pull request Dec 9, 2024

Implement bottom tabs and main menu #1076

Merged

PIG208 added 5 commits December 9, 2024 17:53

compose: Show a progress bar when sending message

c9a5d73

This progress bar shifts the message list because it increases the height of the compose box. This is undesirable, but a fix can be implemented in the future. Signed-off-by: Zixuan James Li <[email protected]>

compose: Add timeout to send message requests

b89a9aa

Signed-off-by: Zixuan James Li <[email protected]>

icons: Add remove icon

a46f8de

Exported from Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-32875&node-type=symbol&t=vOSEWlXAhBa5EsAv-0 Signed-off-by: Zixuan James Li <[email protected]>

PIG208 force-pushed the pr-await-send branch from 66c944f to 63494ab Compare December 9, 2024 22:54

PIG208 mentioned this pull request Dec 10, 2024

Await send request; handle failure #720

Closed

gnprice closed this Jan 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose_box: Wait for send message request to complete. #924

compose_box: Wait for send message request to complete. #924

PIG208 commented Aug 31, 2024 •

edited

Loading

gnprice commented Nov 25, 2024

PIG208 commented Nov 25, 2024 •

edited

Loading

gnprice commented Nov 25, 2024

PIG208 commented Nov 25, 2024

chrisbobbe left a comment •

edited

Loading

chrisbobbe Nov 21, 2024

PIG208 Nov 28, 2024

chrisbobbe Dec 2, 2024 •

edited

Loading

chrisbobbe Nov 21, 2024

chrisbobbe Nov 21, 2024

chrisbobbe Nov 21, 2024

chrisbobbe Nov 27, 2024

chrisbobbe Nov 27, 2024

chrisbobbe Nov 27, 2024

chrisbobbe Nov 27, 2024

chrisbobbe Nov 27, 2024

PIG208 Nov 28, 2024

chrisbobbe Nov 27, 2024

chrisbobbe commented Dec 4, 2024

gnprice commented Dec 9, 2024 •

edited

Loading

PIG208 commented Dec 9, 2024

chrisbobbe commented Dec 9, 2024

PIG208 commented Dec 9, 2024

PIG208 commented Dec 9, 2024

gnprice commented Dec 9, 2024

gnprice commented Jan 3, 2025

compose_box: Wait for send message request to complete. #924

compose_box: Wait for send message request to complete. #924

Conversation

PIG208 commented Aug 31, 2024 • edited Loading

gnprice commented Nov 25, 2024

PIG208 commented Nov 25, 2024 • edited Loading

gnprice commented Nov 25, 2024

PIG208 commented Nov 25, 2024

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Dec 4, 2024

gnprice commented Dec 9, 2024 • edited Loading

PIG208 commented Dec 9, 2024

chrisbobbe commented Dec 9, 2024

PIG208 commented Dec 9, 2024

PIG208 commented Dec 9, 2024

gnprice commented Dec 9, 2024

gnprice commented Jan 3, 2025

PIG208 commented Aug 31, 2024 •

edited

Loading

PIG208 commented Nov 25, 2024 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe Dec 2, 2024 •

edited

Loading

gnprice commented Dec 9, 2024 •

edited

Loading