Animate lightbox bars

[onli]

Wraps the AppBar and the BottomAppBar used on the lightbox screen with AnimatedContainers. Now, when toggling their visibility, they should not be removed from the widget tree completely, but instead their height be changed. This achieves a slide-in animation.

Note the handling of the AppBar being a PreferredSize widget, which forces the additional wrapper.

Fixes: #38

(chris: edited to add "Fixes:" line)

[onli]

Preview:

appBarAnimation.mp4

[chrisbobbe]

Hi @onli, welcome and thanks for the PR and screen recording!

I've edited the PR description to add the Fixes: line, so this PR is now linked with its associated issue.

Please also format the commit message according to our style. (Which reminds me, we should probably move parts of that style guide over to this newer repo soon. 🙂)

The substantive comment I have so far is that this needs to work on devices where the OS intrudes on the app's space at the top and bottom of the screen. To see what I mean, here's a recording on an iPhone 14 Pro simulator:

I particularly notice:

• The "in" position of the top bar has changed (it shouldn't change)
• The content of the top bar (text; close button) isn't showing (it should show)

(The video shows lots of frames being dropped in the animation, but I think we can ignore that; I think it doesn't represent what would happen in a real device in production.)

I don't yet know how best to handle this, which is why I left it as a followup to the initial lightbox implementation in #29. The other thought that comes to mind when using absolute heights (like 80) is that the layout should look reasonable even when the user changes their preferred text size in their device's accessibility settings. But I haven't tested that with this revision yet.

[chrisbobbe]

nit: keep newline before .yMMMd( instead of after

[chrisbobbe]

nit: keep on one line

[chrisbobbe]

nit: two-space indent here (and elsewhere), like surrounding code

[chrisbobbe]

nit: This might fit well on one line after applying two-space indent instead of four. (Also on the "Make smaller, like a subtitle" style below.)

[onli]

@chrisbobbe

(The video shows lots of frames being dropped in the animation, but I think we can ignore that; I think it doesn't represent what would happen in a real device in production.)

If you haven't already, you could try to to run the app in profile mode. There the performance should be closer to the performance on the real device, even in the simulator.

The substantive comment I have so far is that this needs to work on devices where the OS intrudes on the app's space at the top and bottom of the screen.

I see the problem. Guess I should have tested it in the emulator as well :) I'll check whether I can solve this.

nit: keep on one line ...

VSCode/dart styles did not like this project code style and autoformatted the file differently, which I then tried to combat by copying just my changes from a simple editor, which did not work perfectly. Just to explain where the style divergence is coming from. Maybe this could be solved in general with some configuration of analysis_options.yaml (also possible that my setup was not optimal there, I usually write Flutter code on a different PC). I can fix those manually, if I can solve the main issue.

[gnprice]

VSCode/dart styles did not like this project code style and autoformatted the file differently, which I then tried to combat by copying just my changes from a simple editor,

Yeah, we should fix this — thanks for the reminder. I just filed #229 for that and will look into it. Sorry for the frustrating editing experience.

[onli]

@chrisbobbe If we wrap the Scaffold with a Safearea the AppBar gets painted below the OS toolbar - that might be acceptable I think, maybe even the best solution in general, but it would be different than before and probably best to be tackled as an app wide discussion.

So I'd like to try something else first - using AppBar.preferredHeightFor. I pushed that to this PR. It fixes the issue for me on Android (with the biggest possible font size set in the accessibility setting):

This also worked on Linux.

But I would not be too surprised if it does not work on iOS ;) But maybe you could give this a try.

[gnprice]

But I would not be too surprised if it does not work on iOS ;) But maybe you could give this a try.

One fun and helpful feature in Flutter is that you can always cause it to arrange the UI as if it were on some other platform that's not the one it's actually running on:
https://api.flutter.dev/flutter/foundation/debugDefaultTargetPlatformOverride.html
This looks like:

  debugDefaultTargetPlatformOverride = true;

which you can put in the main function, or for hot-reload convenience you can put in some build method (it's harmless to set it repeatedly to the same value).

Then to simulate the notch that some iPhones have, you can use a MediaQuery widget:
https://api.flutter.dev/flutter/widgets/MediaQuery-class.html
up near the root of the tree.

Between those two tools, it should be possible to reproduce the issue and so to verify that that fix resolves it.

We'll also want a unit test for this issue, and ideally for the animation itself. (This file doesn't currently have any tests because it was written when we were still in an experimental phase, but we're generally aiming for a higher standard of testing now that we're committed to building the app and also now that we've built out more test infrastructure to support it.) Those same tools should make it possible to write that test, too.

(In the test you'll want TargetPlatformVariant and the variant argument to testWidgets, which just make a convenient way to control debugDefaultTargetPlatformOverride. See test/widgets/content_test.dart for an example in our tree, or search the Flutter tree for numerous other examples.)

[gnprice]

on Android (with the biggest possible font size set in the accessibility setting):

For controlling this in a test, I believe you again want MediaQuery, setting the textScaleFactor property:
https://api.flutter.dev/flutter/widgets/MediaQueryData/textScaleFactor.html

To see examples of that, use a search like git grep textScaleFactor packages/flutter/test in the Flutter repo.

[onli]

@gnprice

One fun and helpful feature in Flutter is that you can always cause it to arrange the UI as if it were on some other platform that's not the one it's actually running on:

When testing this PR with debugDefaultTargetPlatformOverride = TargetPlatform.iOS; added to the build function of this widget (and to main, to be sure), as well as platform: TargetPlatform.iOS; set in the theme widget, this solution still seems to work. Though I noticed no rendering difference on this screen.

I am not aware of how I could use a MediaQuery to simulate a notch (I know it only to query things, not set things), but I can do so with Androids dev settings:

I would still advice to test it on iOS proper if available. We ran into a bunch of details that were only detectable like that, some were even only noticeable when running on a device. SafeArea + AppBar handling is such an area where things felt off.

We'll also want a unit test for this issue, and ideally for the animation itself.

No problem :) Could you describe a bit more what exactly you would want tested?

I would recommend against testing that the widget tree has the animation widget, that'd be an example for a useless test that will just produce work when this widget gets reworked. It would be nice to test for the pathological rendering shown in the initial review, but I'd need an idea how to test for that (also, hard to write that test when the current code does not produce that situation on my systems).

Maybe just a test that the AppBar is not too small, given the unusual way that it is wrapped? That its title visible could also make for a sound one.

[gnprice]

When testing this PR with debugDefaultTargetPlatformOverride = TargetPlatform.iOS; added to the build function of this widget (and to main, to be sure), as well as platform: TargetPlatform.iOS; set in the theme widget, this solution still seems to work.

Cool.

Though I noticed no rendering difference on this screen.

Yeah, this particular aspect is something I would expect not to vary by platform; I think the MediaQuery data will be the more relevant thing for exercising it.

I am not aware of how I could use a MediaQuery to simulate a notch (I know it only to query things, not set things),

Ah, OK. Well, how do you query it? You use the static method MediaQuery.of and its friends.

What those do:
https://api.flutter.dev/flutter/widgets/MediaQuery/of.html
is they look for the closest enclosing MediaQuery widget, and get their data from there.

So to change the data that all the widgets in a whole subtree will get when they call MediaQuery static methods, you insert a MediaQuery widget above them. Probably most convenient is to put it just above the MaterialApp widget.

Specifically if you want to make everything behave as if a notch is present, the property you want to change is the main one that SafeArea consults, which is MediaQueryData.padding:
https://api.flutter.dev/flutter/widgets/MediaQueryData/padding.html

So that looks like, in ZulipApp.build:

    return GlobalStoreWidget(
      child: MediaQuery(
        data: MediaQuery.of(context).copyWith(padding: EdgeInsets.only(top: 160)),
        child: MaterialApp(
          title: 'Zulip',
          theme: theme,
          home: const ChooseAccountPage())));

where 160 is an arbitrary value that just makes an unreasonably tall notch, to make its effects extra conspicuous.

That won't cause an actual black notch-shaped area to appear, the way the one in Android's dev settings will do. But it will cause Scaffold and everything else that consults SafeArea to avoid the area exactly as if there were a notch.

---

The other fun Flutter feature that is helpful for development here is timeDilation:
https://api.flutter.dev/flutter/scheduler/timeDilation.html
If you add a line like timeDilation = 50; to a build method, then all the animations will slow way down — 100ms becomes 5s — so that you can easily see what's happening in the middle of the animation.

---

We'll also want a unit test for this issue, and ideally for the animation itself.

No problem :) Could you describe a bit more what exactly you would want tested?

For testing the notch / safe-area issue, what I'd want is something like:

• Render a lightbox, with things set up to behave as if there's a notch (by using MediaQuery as described above).
• Check that the UI in the app bar — for example by finding the text widget with the sender's name — is located appropriately low on the screen, avoiding the notch, rather than at the top.

The key criterion for such a test would be that it fails on an implementation, like the one in the original version of the PR, where the issue was present 🙂. I often find it useful to check out an old version of the lib/ tree from Git, like git checkout @~3 -- lib/, to test a newly-written test against an old implementation and confirm it fails as desired.

For testing the animation, I agree it wouldn't be helpful to check that the widget tree has the animation widget. One way to check that the animation is present would be: do the tester.tap call (or whatever) to trigger the animation; then go a simulated 50ms (if it's a 100ms animation), and check that the app bars have moved partway out but aren't yet gone; then go past the time the animation should take, and check that they're completely gone.

[onli]

I added a first round of tests. Note the visibility change of LightboxPage. This took longer than I wanted - debugNetworkImageHttpClientProvider = null; was missing, which I did not realize for a while since the stacktrace pointed at debugDisableShadows being the problem. Thus the split.

I can add a second round of test for the notch next and then change the minor formatting comments above.

[gnprice]

This took longer than I wanted - debugNetworkImageHttpClientProvider = null; was missing, which I did not realize for a while since the stacktrace pointed at debugDisableShadows being the problem.

Hmm, interesting.

This is probably a place where Flutter could provide a better error message. What I see when I try removing one of those … = null; lines is:

The following assertion was thrown running a test:
The value of a painting debug variable was changed by the test.

When the exception was thrown, this was the stack:
#0      debugAssertAllPaintingVarsUnset.<anonymous closure> (package:flutter/src/painting/debug.dart:190:7)
#1      debugAssertAllPaintingVarsUnset (package:flutter/src/painting/debug.dart:193:4)
#2      TestWidgetsFlutterBinding._verifyInvariants (package:flutter_test/src/binding.dart:1051:12)
#3      AutomatedTestWidgetsFlutterBinding._verifyInvariants (package:flutter_test/src/binding.dart:1496:11)
#4      TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1025:7)

Ideally it would mention the variable by name in the error message, and you wouldn't have to look through the stack trace to work out what it means.

When you do look at the stack, though, here's the inmost function:

bool debugAssertAllPaintingVarsUnset(String reason, { bool debugDisableShadowsOverride = false }) {
  assert(() {
    if (debugDisableShadows != debugDisableShadowsOverride ||
        debugNetworkImageHttpClientProvider != null ||
        debugOnPaintImage != null ||
        debugInvertOversizedImages ||
        debugImageOverheadAllowance != _imageOverheadAllowanceDefault) {
      throw FlutterError(reason);
// …

So that at least gives you a list of potentially-relevant variables. I guess you were perhaps looking at the next caller up:

    assert(debugAssertAllGesturesVarsUnset(
      'The value of a gestures debug variable was changed by the test.',
    ));
    assert(debugAssertAllPaintingVarsUnset(
      'The value of a painting debug variable was changed by the test.',
      debugDisableShadowsOverride: disableShadows,
    ));

[gnprice]

I can add a second round of test for the notch next

Sounds good. Do take a look also at the details of the animation, using timeDilation.

[onli]

Okay, two commits more:

1. A test for the AppBar behaviour. Testing for hitability of the RichText widget failed in the first commit of this PR, but works in the last -> it can detect issues.
2. I tried to address the formatting comments of the review by @chrisbobbe

Do take a look also at the details of the animation, using timeDilation.

Hm. That's a good catch. The animations of AppBar and BottomAppBar are not completely in sync, the AppBar behaves a bit strange (actually, the AppBar vanishing looks worse in the video than in the emulator, some frames seem to be skipped):

animated_transitions_slow.mp4

But I'd lean towards "That is good enough for now". Do you agree?

PS:

I guess you were perhaps looking at the next caller up:

Exactly :) I also saw the list of variables of course, but was convinced the shadow was the issue. Even debugged it, the variable was true, all seemed to fit. The easiest things are the biggest blocker sometimes.

[gnprice]

Yeah, the behavior of the top app bar is rather delayed (when on the way out), and then abrupt. The time dilation is very useful for pinning down what's happening; but it felt off to me even at normal speed, which is why I tried looking closer using time dilation.

Another oddity which becomes visible when using a large time dilation (like 50) is that the text in the app bar shrinks from both above and below, and then the app bar as a whole slides up out of the way. When I spotted that, I was also simulating a very tall notch as described above at #224 (comment) .

I think the issue where there's a delay before the app bar moves is actually fairly noticeable in the UX, so I'd want to fix that before merging something. The odd shrinking behavior might be tolerable on its own — I'm not sure whether it's noticeable in normal circumstances — but it seems like another symptom and may provide another clue.

---

In the code, I'd recommend taking a look specifically at this bit:

        height: _headerFooterVisible ? AppBar.preferredHeightFor(context, 
                                                                  MediaQuery.of(context).size) 
                                      : 0,

I don't think that's the sort of arguments that AppBar.preferredHeightOf is intended to take; take a look at its docs and/or implementation, and at the value it ends up returning here.

[onli]

You're right. This really used the complete height, which warped the animation. By using kToolbarHeight combined with MediaQuery.of(context).padding.top this looks as it should, and still saves us the SafeArea:

animated_transitions_slow_synced.mp4

Tests still pass.

[gnprice]

That video definitely looks much improved. But it still doesn't look quite right: when the top app bar is sliding out, the text should slide out with it, as if the app bar is a piece of physical material and the text is written on it. Instead the text gets cut off from both below and above:

---

I don't think that's the sort of arguments that AppBar.preferredHeightOf is intended to take; take a look at its docs and/or implementation, and at the value it ends up returning here.

In the new revision the code looks like this:

        height: _headerFooterVisible ? AppBar.preferredHeightFor(
                                        context, 
                                        Size(0, MediaQuery.of(context).padding.top + kToolbarHeight)
                                      ) 

But here's the definition of AppBar.preferredHeightFor:

  static double preferredHeightFor(BuildContext context, Size preferredSize) {
    if (preferredSize is _PreferredAppBarSize && preferredSize.toolbarHeight == null) {
      return (AppBarTheme.of(context).toolbarHeight ?? kToolbarHeight) + (preferredSize.bottomHeight ?? 0);
    }
    return preferredSize.height;
  }

So it never makes sense to call that function with a value you've just created with the Size constructor. An expression like AppBar.preferredHeightFor(context, Size(width, height)) will always just reduce down to height.

---

I realize that it's tricky to work out the logic to get this animation to work correctly. But I'm not going to want to merge a version that half-does it and produces behavior that feels glitchy. Until we have a version that gets it right, it's better to simply not have the animation.

The Flutter upstream style guide has a nice section about this principle:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#lazy-programming

[onli]

Hi :)

But it still doesn't look quite right: ...

I disagree. I did test this now again on Android and Linux. The animation looks fine, also when slowed down. Yes, the text does get cut not perfectly; This will not be noticed negatively (and can't be noticed at all on regular speed).

The AnimatedContainer is a good solution for this animation, not a workaround, not unused code put there for completeness. We shouldn't overcomplicate this.

Or do you have a better solution in mind?

But here's the definition of AppBar.preferredHeightFor: ... So it never makes sense to call that function with a value you've just created with the Size constructor.

You are completely right there. The function fits semantically, but not with the current code. Maybe this can be called in a more reasonable way, but probably the best solution for here would be to just not use it. I wouldn't mind to push that change, if we can go forward?

[gnprice]

@onli Are you interested in addressing the feedback above to finish this up into something we can merge?

If not, that's fine; I'm just going through our old PRs to make sure we take care of them one way or another.

[onli]

Hi @gnprice, thanks for asking. I think I should let it go, our approaches for this specific animation do not seem to be compatible - and I have no alternative solution to suggest currently than the AnimatedContainer.

[gnprice]

Sounds good. Thanks for the reply, and for the PR.