Compose box redesign

[PIG208]

Fixes: #915

[PIG208]

(getting the screenshots ready)

[PIG208]

The design specifies a 8px high padding before the message content:

The content can be scrolled past the padding, then clipped at the top border of the compose box:

This turned out to be tricky to implement, because InputDecorator's contentPadding also pads the scrollable view, and the content cannot be scrolled past the padding, leaving a gap:

with padding	without padding

A previous workaround was to fully vertically expand TextField by wrapping it within a SingleChildScrollView (implemented in e16251f), which does not handle the case that the cursor gets scrolled out of view when editing:

Example

The latest workaround disables clipping on the TextField and wraps it with a ClipRect, whose size is determined by a ConstrainedBox under our control.

[PIG208]

I think I have found a true simple solution to this: set TextField.clipBehavior to Clip.none, set InputDecoration.contentPadding and wrap the TextField with a ClipRect under our control. This seems to work perfectly without the cursor issue.

[PIG208]

Preview

light	dark

[PIG208]

Updated the PR with the screenshots. Ready for @chrisbobbe and @alya to review!

[alya]

I haven't compared carefully vs. the designs in Figma, but the screenshots look nice!

[gnprice]

Thanks @PIG208 for building this, and @chrisbobbe for the previous rounds of review!

Various comments below. I've read most of the branch, but not the last commit:
ea7d8b3 compose_box: Support redesigned button feedback

or the tests of the next-to-last, main commit:
1f49f51 compose_box: Implement most of compose box redesign

[gnprice]

I believe this relies on an assumption that child is going to take care of the left and right insets.

Looking at where this _ComposeBoxContainer widget gets used, that's true at one call site, in _ComposeBoxLayout. But the other call site puts an _ErrorBanner there, and that doesn't appear to take care of these.

That could be handled by having _ErrorBanner use its own SafeArea widget. But probably the right answer is for the compose-box container to continue handling SafeArea with a minimum of 8px on both sides, like it does in main, to keep that job centralized.

[gnprice]

This version where the two children of the column use SafeArea with different minimum paddings doesn't seem quite right. In particular in the screenshots in #928 (comment) , it loses horizontal alignment between the send button and the end of the topic/content divider line.

Instead I think we want the same minimum horizontal padding on each of these SafeArea widgets (which is another reason it's helpful to centralize them as mentioned above), and then add 8px padding here on top of that. That way this always gets 8px more padding than its sibling.

[gnprice]

nit: a bit easier to read when stated in the positive:

Suggested change

	/// `factor` must not be less than 0 or greater than 1.
	/// `factor` must be between 0 and 1, inclusive.

[gnprice]

nit: should be a separate test case (or ideally two) — these are logically independent of the main check this case is making

[gnprice]

theme: Update borderBar to match Figma design

See also:
#928 (comment)

I guess this is also a reminder that it'd be good for us to take care of #831 and then the follow-up of having a script do the same with the main variables.

[gnprice]

Also a nit: the commit message reads to me like it's saying the previous value just wasn't written to match Figma. But from the linked comment it sounds like it was, and it just got edited in Figma since then.

That's interesting information in particular because it tells us Vlad does in practice edit existing variables that we've already used. Which motivates #831 and that follow-up task after it.

Can fix by s/match/follow change in/.

[gnprice]

I see the Figma design calls for 44x42 buttons here.

But the standard minimum for accessibility is 44x44 logical pixels — and that's already the more permissive standard from Apple, where Google recommends a minimum of 48x48.

I don't see the point in cutting corners on accessibility to shave 2px of vertical space in the layout, and I don't see a rationale from Vlad about that. So let's move forward with 44x44 instead.

After this is merged, I can start a thread mentioning we made this adjustment and we'll see if Vlad has any tweaks he'd want to make elsewhere in the layout to account for that (or wants to give a rationale for why it's essential that these be only 42px tall instead of 44px). That's a follow-up because I don't want it to distract from or slow down getting this merged — in general, we want following accessibility guidelines to be the easy path of least resistance, not vice versa.

[gnprice]

I don't follow this comment. This color property is about the foreground color, not the background, right?

[PIG208]

We do want to set the foreground color to iconColor; either IconButton.color or Icon.color will work.

We do want to avoid the default background colors (i.e. the color of the button: IconButton.hoverColor, IconButton.highlightColor); these are derived from IconButton.color if it is set (it is the fallback when the ambient ButtonStyle.overlayColor is not set, see IconButton.styleFrom, which is called by IconButton.build).

This exact behavior of falling back to the foreground color (IconButton.color) doesn't seem to be mentioned on the colors' dartdoc. When we set IconButton.highlightColor through an IconButtonTheme, it gets overridden by the color derived from IconButton.color.

we can clarify this bit by mentioning the specific button styles involved.

[gnprice]

nit: these are all internal, right? Let's make that clear by making them private

[gnprice]

That's at default text scaling, right?

On my phone at my usual settings, I see 54px for this. If I switch my system text-size setting to default, I reproduce 60px.

[gnprice]

Ah I see, rereading, there's this reference at the end of the sentence. Clearer if it's moved next to the mention of 60x vertical distance, I think; it wasn't clear to me the first time what this parenthetical was about.

[PIG208]

Thanks for the review! I have updated the PR.

[gnprice]

Thanks! The changes in the revision all look great.

I've now read the whole thing; comments below on the remaining parts.

One commit-message nit: use "compose" for the prefix rather than "compose_box", to save a bit of space in our often-crowded summary lines. The shorter version is what we originally used and have used most; see #886 (comment) where I looked at previous commits.

[gnprice]

Great, thanks — this comment explains it.

[gnprice]

Can this step just go in the test cases that need it? That way we avoid making this shared helper more complex with a growing number of optional features, plus it makes it transparent in the test cases exactly what's happening.

If this step were a bit more code than this (or if we were writing lots of very short tests that used it), another tactic would be to make a helper function just for this, which those tests could call.

[PIG208]

Inlined these without a helper, because they are short enough.

[gnprice]

The reason this step is needed is the enterText calls inside checkContentInputMaxHeight, right?

In that case, a good home for this is inside that function. That makes clearer the relationship to why it's needed, and as a bonus makes each test case simpler.

[gnprice]

nit: just inline finder; the name "finder" isn't very informative about what this line is doing

Or use contentInputFinder? That'd match the getRect call just below, and its name is clear.

[gnprice]

This loop is kind of confusing to follow. A very local fix would be to make i represent the number of lines in content:

Suggested change

	// On top of the existing two lines, add exactly the number of lines
	// needed such that the last line will not visible in the content input
	// after reaching the max height.
	for (int i = 0; i < maxVisibleLines - 2 + 1; i++) {
	// On top of the existing two lines, add exactly the number of lines
	// needed such that the last line will not visible in the content input
	// after reaching the max height.
	for (int i = 2; i < maxVisibleLines + 1; i++) {

I think a further clarification is then to have just one loop variable, the number of lines, instead of effectively two in parallel (i and content). That can look like this:

      // Add one line at a time, until the content input reaches its max height.
      int numLines;
      double? height;
      for (numLines = 2; numLines <= 1000; numLines++) {
        final content = List.generate(numLines, (_) => 'foo').join('\n');
        await tester.enterText(finder, content);
        await tester.pump();
        final newHeight = tester.getRect(contentInputFinder).height;
        if (newHeight == height) {
          break;
        }
        height = newHeight;
      }
      check(height).isNotNull().isCloseTo(maxHeight, 0.5);
      // The last line added did not stretch the content input,
      // so only the lines before it are at least partially visible.
      check(numLines - 1).equals(maxVisibleLines);

[gnprice]

One commit-message nit: use "compose" for the prefix rather than "compose_box"

Ah, I see we raced and you've already made that change 🙂

[gnprice]

More commit-message nits:

  `ButtonStyle` for the send button that was added in # 399, which is

should complete the link

---

See also:

  - https://github.com/zulip/zulip-flutter/pull/928#discussion_r1815661095
  - https://github.com/zulip/zulip-flutter/pull/928#issuecomment-2335042432,
    which elaborates on the issue we intend to solve with the
    `ClipRect` setup.
  - https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3954-13395
  - https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3862-14350
  - https://github.com/zulip/zulip-flutter/pull/928#discussion_r1842988359

These should each get a few words about what they're about, like the second one has. In particular that's needed so that the reader can tell whether they're relevant for whatever the reader is currently trying to understand. (Otherwise they have to go open all of them one by one… which is extra slow because both Figma, and GitHub in long threads, take a long time to load.)

The second link sounds (from the description here) like it's about the same thing as the second paragraph above. So probably it should just move into that paragraph, like:

  The shadow is always there, but it is only visible when the
  `TextField` is long enough to be scrollable. Discussion here:
    https://github.com/zulip/zulip-flutter/pull/928#issuecomment-2335042432

and then (a) its context is clear and (b) someone reading the paragraph sees there's a relevant link.

See also:
  https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3707-41711&n

ode-type=frame&t=sSYomsJzGCt34D8N-0

---

Similarly this should describe how the link relates to what the commit message is saying. If I understand right, that can be:

Design spec here:
  https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3707-41711&n

ode-type=frame&t=sSYomsJzGCt34D8N-0

[gnprice]

Ah, rereading the commit message I see this:

- `ClipRect`'s size is determined by the `ConstrainedBox`.  This is
  mainly for showing the content through the `contentPadding` of the
  `TextField`, so that our `InsetShadowBox` can fade it smoothly there.

and it explains the presence of this widget. I think that will be useful information for readers of the resulting code, not only of this commit's changes. So let's put it in a code comment:

Suggested change

	child: ClipRect(
	// This [ClipRect] replaces the [TextField] clipping we disable below.
	child: ClipRect(

(See https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#commit-messages-vs-comments .)

[gnprice]

commit-message nit:

- For `InputDecorationTheme` on `_ComposeBoxLayout`, we eliminate
  `contentPadding`, while keeping `isDense` as `true` to explicitly
  remove paddings on the input widgets.

I reread this paragraph just now and thought "wait, didn't we set contentPadding to zero? I sure thought this code still set both isDense and contentPadding". (Then I went back to the code to check.) IOW, I read this as saying we stopped passing that argument.

s/eliminate/zero out/ would fix it.

[gnprice]

Also move the commas: "we eliminate contentPadding while keeping isDense true, to explicitly …".

As is, it looks like it's saying that the way we remove padding is by keeping isDense true, all at the same time as we eliminate contentPadding. In reality (IIUC) both parameters are part of removing padding.

[PIG208]

Moved the links to the paragraph they each correspond to, and added the comment on ClipRect (the related paragraph is still kept in the commit, in case the reader is looking back at this for a more verbose explanation on this design's origin).

[gnprice]

Looks good — merging. Thanks again for all your work on this!