lightbox: Prevent hero animation between message lists #1348

lakshya1goel · 2025-02-12T13:35:55Z

Fixes: #930

Videos

Before

WhatsApp.Video.2025-02-12.at.5.18.39.PM.mp4

After

WhatsApp.Video.2025-02-12.at.5.18.35.PM.mp4

gnprice · 2025-02-15T22:57:23Z

Thanks! Chat thread here:
https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/Hero.20animation/near/2092791
with comments particularly on how to write a good test for this fix.

lakshya1goel · 2025-02-22T14:03:07Z

Hi @PIG208, PR is ready for a review now, PTAL, Thanks!
Related CZO Discussion

PIG208

Thanks for fixing this! The approach looks fine to me. Left a comment on the implementation and some more on the tests.

PIG208 · 2025-02-25T20:47:01Z

lib/widgets/lightbox.dart

@@ -21,20 +21,22 @@ import 'store.dart';
 //   fly to an image preview with a different URL, following a message edit
 //   while the lightbox was open.
 class _LightboxHeroTag {
-  _LightboxHeroTag({required this.messageId, required this.src});
+  _LightboxHeroTag({required this.messageId, required this.src, required this.pageContext});


nit: Wrap this because the parameter list gets a bit too wide.

PIG208 · 2025-02-25T20:55:10Z

lib/widgets/lightbox.dart


  @override
  Widget build(BuildContext context) {
    return Hero(
-      tag: _LightboxHeroTag(messageId: message.id, src: src),
+      tag: _LightboxHeroTag(messageId: message.id, src: src, pageContext: pageContext),


Do things go wrong if we get the page context from PageRoot.contextOf(context) here? If that works we don't need to pass it around.

Things would go wrong since using PageRoot.contextOf(context) here would break the hero animation because the widget exists in two different page contexts simultaneously in the MessageListPage and the lightbox. By explicitly passing the source page's context (pageContext), we ensure both the source and destination heroes use the same context value in their tags, allowing Flutter to properly match them for the animation.

Indeed. Thanks for the explanation.

PIG208 · 2025-02-25T20:55:50Z

test/widgets/lightbox_test.dart

+    Narrow narrow = const CombinedFeedNarrow(),
+    List<Message>? messages,
+    List<ZulipStream>? streams,
+


nit: extra empty line

PIG208 · 2025-02-25T20:57:43Z

test/widgets/lightbox_test.dart

+      // ZulipApp instead of TestZulipApp because we need:
+      // 1. The navigator to push the lightbox route. The lightbox page works
+      //    together with the route; it takes the route's entrance animation.
+      // 2. The PageRoot widget to provide context for Hero animations between
+      //    the message list and lightbox.
+      await tester.pumpWidget(PageRoot(
+        child: const ZulipApp()
+      ));
      await tester.pump();
      final navigator = await ZulipApp.navigator;
      unawaited(navigator.push(getImageLightboxRoute(
        accountId: eg.selfAccount.id,
+        pageContext: PageRoot.contextOf(navigator.context),


We should be able to skip these changes if not having pageContext as a parameter works.

I think we can't skip passing pageContext because during the hero animation, the transitioning widget lives in Flutter's overlay layer outside our PageRoot hierarchy. The hero animation needs the same tag value at both ends (source and destination) to know which widgets to animate between, and since we can't get the PageRoot context during the transition, we must pass it explicitly.

PIG208 · 2025-02-25T20:59:32Z

test/widgets/lightbox_test.dart

+
+  group('LightboxHero', () {
+    testWidgets('no hero animation occurs between different message list pages for same image', (tester) async {
+      final channel = eg.stream(streamId: eg.defaultStreamMessageStreamId);


Since the value of streamId is not used later, we can skip specifying. Using the default value should help keep the setup boring and bring focus to what's more interesting.

PIG208 · 2025-02-25T21:04:29Z

test/widgets/lightbox_test.dart

+      await tester.pumpAndSettle();
+    });
+  });
+}


Need a newline at the end of the file.

PIG208 · 2025-02-25T21:13:12Z

test/widgets/lightbox_test.dart

+      await tester.pump(const Duration(milliseconds: 150));
+
+      final imageInTransition = tester.getRect(imageFinder);
+      check(imageInTransition.top).equals(initialImageRect.top);
+      check(imageInTransition.left).equals(initialImageRect.left);
+      await tester.pumpAndSettle();


Is this supposed to be mid transition time? While this works, I feel that it can miss bugs if the duration somehow changes.

We can have a while-loop that checks tester.hasRunningAnimations, which pumps repeatedly with a fixed duration, and repeats the check:

Suggested change

await tester.pump(const Duration(milliseconds: 150));

final imageInTransition = tester.getRect(imageFinder);

check(imageInTransition.top).equals(initialImageRect.top);

check(imageInTransition.left).equals(initialImageRect.left);

await tester.pumpAndSettle();

int timeElapsed = 0;

const interval = 50;

while (timeElapsed < interval || tester.hasRunningAnimations) {

final imageInTransition = tester.getRect(imageFinder);

check(imageInTransition.top).equals(initialImageRect.top);

check(imageInTransition.left).equals(initialImageRect.left);

await tester.pump(const Duration(milliseconds: interval));

timeElapsed += interval;

}

check(timeElapsed).isGreaterOrEqual(interval);

Just saw the CZO comment:

In order to have that benefit, it's important to keep the two test cases similar to each other as far as possible. It should be easy for the reader to convince themself that the two tests are checking the same thing, and just expecting opposite outcomes.

I agree that having these two tests similar to each other does mitigate the concern of one of them not working properly. So it's probably also fine to leave them as-is. Either way, we should comment that the duration is specifically chosen such that we are in the middle of a hero animation (if there is one).

PIG208 · 2025-02-25T21:13:57Z

test/widgets/lightbox_test.dart

+  late PerAccountStore store;
+  late FakeApiConnection connection;
+
+  Future<void> setupMessageListPage(WidgetTester tester, {


Let's keep this helper close to where it is used in the 'LightBoxHero' group.

PIG208 · 2025-02-25T21:15:46Z

test/widgets/lightbox_test.dart

+    List<Message>? messages,
+    List<ZulipStream>? streams,


Because none of the two callers actually rely on having different messages/channels, it looks like we can remove these parameters. An advantage of keeping the helper close to where it's used is that we can more easily spot what properties the tests rely on.

For things that are constant to the tests (like the topic name, finders), we can have a shared local variable within the group.

PIG208 · 2025-02-25T21:16:36Z

test/widgets/lightbox_test.dart

+      connection.prepare(json:
+        eg.newestGetMessagesResult(foundOldest: true, messages: [message]).toJson());


nit: move this to the line just before tester.tap, because it prepares an API response for that

PIG208 · 2025-02-27T18:22:46Z

test/widgets/lightbox_test.dart

+      await tester.pump(const Duration(milliseconds: 150));
+
+      final imageInTransition = tester.getRect(imageFinder);
+      check(imageInTransition.top).not((it) => it.equals(initialImageRect.top));


I missed this in the previous review — we can further change this to

check(imageInTransition).top.not((it) => it.equals(initialImageRect.top));

lakshya1goel · 2025-02-28T09:57:55Z

Pushed the revision, PTAL @PIG208. Thanks!

PIG208

Thanks for the update! I found a place where we can potentially shake off the dependency on PageRoot. Let me know what you think!

PIG208 · 2025-02-28T21:13:23Z

test/widgets/lightbox_test.dart

+    final message = eg.streamMessage(stream: channel,
+        contentMarkdown: ContentExample.imageSingle.html, topic: 'test topic');


nit: indentation

Suggested change

final message = eg.streamMessage(stream: channel,

contentMarkdown: ContentExample.imageSingle.html, topic: 'test topic');

final message = eg.streamMessage(stream: channel,

contentMarkdown: ContentExample.imageSingle.html, topic: 'test topic');

PIG208 · 2025-02-28T21:14:07Z

test/widgets/lightbox_test.dart

+
+      await store.addUser(eg.selfUser);


nit:

Suggested change

await store.addUser(eg.selfUser);

await store.addUser(eg.selfUser);

because store.addUser is a part of the setup code stanza

PIG208 · 2025-02-28T21:30:19Z

lib/widgets/content.dart

@@ -663,12 +665,14 @@ class MessageImage extends StatelessWidget {
          src: resolvedSrcUrl,
          thumbnailUrl: resolvedThumbnailUrl,
          originalWidth: node.originalWidth,
-          originalHeight: node.originalHeight));
+          originalHeight: node.originalHeight,
+          pageContext: pageContext));


While testing this, I tried passing context to pageContext (and below), and it appears to work. Let's try this so that we don't need to rely on having a PageRoot.

lakshya1goel · 2025-03-01T11:51:25Z

Pushed the revision @PIG208, please have a look. Thanks!

PIG208 · 2025-03-03T18:34:38Z

Thanks! I think because we are not using the context of PageRoot, there is probably a better name than pageConext. (Maybe messageImageContext?) Marking this for Greg's review.

gnprice

Thanks @lakshya1goel for taking this on, and @PIG208 for the previous reviews! Comments below.

gnprice · 2025-03-04T00:46:43Z

lib/widgets/lightbox.dart

  required Uri? thumbnailUrl,
  required double? originalWidth,
  required double? originalHeight,
+  required BuildContext pageContext,


When a function has a bunch of parameters like this (or a class has a bunch of fields, etc.), it's important to keep them organized logically — that makes a real difference both for people trying to understand and make changes to the implementation of the function (or class etc.) itself, and for people trying to use it.

So that means new parameters should go in the position that makes the structure of the list make sense, not necessarily at the end of the list.

What are existing parameters on this function that this new one does a similar job to?

gnprice · 2025-03-04T00:49:29Z

lib/widgets/lightbox.dart


  final int messageId;
  final Uri src;
+  final BuildContext pageContext;


In addition to Zixuan's point above at #1348 (comment) about the name of this field, let's also give it a bit of dartdoc explaining what it's expected to be. I think that will help with thinking through the behavior.

gnprice · 2025-03-04T00:51:18Z

lib/widgets/lightbox.dart

 //   fly to an image preview with a different URL, following a message edit
 //   while the lightbox was open.


This comment points out one of the scenarios that can make it complicated to get these tags right — a message can be edited while the user has one of its images open in the lightbox. How does this version behave if that happens?

I think the current implementation handles this scenario well. When a message is edited while its image is open in the lightbox, the behavior depends on whether the image URL changes:

If the edit changes the image URL:
The Hero animation won't find a matching tag (because src is part of the tag and has changed)
It will gracefully fall back to a fade transition instead of attempting to animate to the wrong image

If the edit doesn't change the image URL:
All three tag components (messageId, src, and messageImageContext) still match
The Hero animation will work normally

gnprice · 2025-03-04T00:55:00Z

test/widgets/lightbox_test.dart

@@ -558,4 +566,76 @@ void main() {
      check(platform.position).equals(position);
    });
  });
+
+  group('LightboxHero', () {


nit: keep tests in an order matching the order of the code they're testing

This is really another example of the same point as the comment above about function parameters — when adding something new, take a moment to think about the most logical place for it to appear, rather than just putting it at the end.

gnprice · 2025-03-04T01:01:38Z

test/widgets/lightbox_test.dart

+      final imageInTransition = tester.getRect(imageFinder);
+      check(imageInTransition).top.equals(initialImageRect.top);
+      check(imageInTransition).left.equals(initialImageRect.left);


Given imageFinder = find.byType(RealmContentNetworkImage).first, this doesn't make an entirely convincing check that there isn't a hero animation going on. This effectively says that the original image is still the first RealmContentNetworkImage in the tree — but if there were a hero animation going on, there's no reason that necessarily has to be the first image in the tree. Maybe it comes later in the tree than the original image.

gnprice · 2025-03-04T01:06:37Z

test/widgets/lightbox_test.dart

+      final imageInTransition = tester.getRect(imageFinder);
+      check(imageInTransition).top.not((it) => it.equals(initialImageRect.top));
+      check(imageInTransition).left.not((it) => it.equals(initialImageRect.left));


Similarly, this says that the first RealmContentNetworkImage in the tree isn't at the place the original image was. But

there's the new route, the lightbox, which has a version of the image — maybe that's the first one in the tree now

there's the old route, the message list, which may have a version of the image — and although right now that won't have moved, maybe in the future we'll use a different navigation transition here so that the old route moves away while the new one is moving in. Again maybe that's the first one in the tree now.

So this should get more specific about our expectations in order to be convincing that it wouldn't end up passing even in some future where we've broken the hero animation entirely.

gnprice

Thanks for the revision — comments below.

For detailed how-to discussion, let's use #mobile-dev-help > Hero animation, since Zulip will be better for discussion than GitHub is.

gnprice · 2025-03-10T21:39:04Z

lib/widgets/lightbox.dart

+  /// The [BuildContext] of the image in the message list that's being expanded
+  /// into the lightbox. Used to coordinate the Hero animation between this specific
+  /// image and the lightbox view.
+  ///
+  /// This helps ensure the animation only happens between the correct image instances,
+  /// preventing unwanted animations between different message lists or between
+  /// different images that happen to have the same URL.
+  required BuildContext messageImageContext,


This doesn't function as dartdoc — try going to a call site of this function and hovering on either the function name, or this parameter, and you'll see that this text doesn't appear.

For documenting a function parameter, use dartdoc on the function.

For this particular data, I think the best home for the details of what it means is on the field on _LightboxHeroTag, as I suggested at #1348 (comment) . Then on this function one can point there for details.

gnprice · 2025-03-10T21:46:54Z

test/widgets/lightbox_test.dart

@@ -300,6 +308,100 @@ void main() {
    //   https://github.com/zulip/zulip-flutter/pull/833#issuecomment-2251782337
  });

+  group('LightboxHero', () {


Per #1348 (comment) , if the idea of these tests is to test LightboxHero, then they should go before the tests for _ImageLightboxPage, to match the fact that LightboxHero's definition comes before the definition of _ImageLightboxPage.

gnprice · 2025-03-10T21:48:19Z

lib/widgets/lightbox.dart

  final int messageId;
  final Uri src;
+  final BuildContext messageImageContext;


nit: have these three fields appear in the same order on this class as they do in the other places they appear together

gnprice · 2025-03-10T21:54:16Z

test/widgets/lightbox_test.dart

+      final imageRects = tester.widgetList(allImages).map((widget) {
+        final finder = find.byWidget(widget);
+        return tester.getRect(finder);
+      }).toList();
+
+      check(imageRects).isNotEmpty();
+      check(imageRects.any((rect) =>
+        rect.top != initialImageRect.top ||
+        rect.left != initialImageRect.left
+      )).isTrue();


Can you describe what condition this verifies is true, in terms that reflect what the user sees on the screen?

I believe this is less specific than the previous revision discussed at #1348 (comment), not more. That is, I believe this will pass in all the same situations where the previous revision would have passed, plus some additional situations.

gnprice · 2025-03-10T22:07:59Z

test/widgets/lightbox_test.dart

+      final imageInTransition = tester.getRect(imageFinder);
+      check(imageInTransition).top.equals(initialImageRect.top);
+      check(imageInTransition).left.equals(initialImageRect.left);


This test is also less specific than the previous revision. In fact this one no longer tests anything about the hero animation at all: even if you edit the code so that the tags are completely defeated and the hero animation happens all the time:

bool operator ==(Object other) { - return other is _LightboxHeroTag && - other.messageId == messageId && - other.src == src && - other.messageImageContext == messageImageContext; + return true; } @override - int get hashCode => Object.hash('_LightboxHeroTag', messageId, src, messageImageContext); + int get hashCode => '_LightboxHeroTag'.hashCode;

this test still passes.

Like with the other test case, can you describe what condition this verifies is true, in terms that reflect what the user sees on the screen?

lakshya1goel · 2025-03-21T14:25:19Z

Hi @gnprice, I have pushed the revision, please take a look. Thanks!

gnprice · 2025-03-21T21:57:17Z

Thanks! Chat thread here about the tests: #mobile-dev-help > Hero animation @ 💬

gnprice

Thanks for the revision. This is closer, but there are still significant things that need to be changed before we can merge it.

In particular, this version still doesn't have a test that successfully exercises the issue this PR is meant to fix. See the last pair of comments below.

gnprice · 2025-03-24T20:31:33Z

lib/widgets/lightbox.dart

    return other is _LightboxHeroTag &&
      other.messageId == messageId &&
+      other.messageImageContext == messageImageContext &&
      other.src == src;


Is it possible to have two of these tags that have the same messageImageContext, but different messageId?

I think it isn't. Note that MessageItem, which encloses a message in the list, has the message ID for its key — so an element in the tree won't get reused between one message and another.

Given that, there's no need to keep messageId around in these tags when we're adding messageImageContext.

gnprice · 2025-03-24T20:33:55Z

lib/widgets/lightbox.dart

+  /// - [messageId]: The unique identifier for the message.
+  /// - [messageImageContext]: The [BuildContext] of the image being expanded.
+  /// - [src]: The image source URI.
+  _LightboxHeroTag({


The place for documenting these is on the fields themselves, rather than on the constructor. See other classes around our codebase.

gnprice · 2025-03-24T20:34:03Z

lib/widgets/lightbox.dart

+  ///
+  /// - [messageId]: The unique identifier for the message.
+  /// - [messageImageContext]: The [BuildContext] of the image being expanded.
+  /// - [src]: The image source URI.


https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#url-not-uri

gnprice · 2025-03-24T20:36:18Z

lib/widgets/lightbox.dart

+  /// - [messageId]: The unique identifier for the message.
+  /// - [messageImageContext]: The [BuildContext] of the image being expanded.
+  /// - [src]: The image source URI.


These three look a lot like what you might get if you asked ChatGPT or another LLM to write docs for this constructor, so that it just looked at the definitions of the fields below and turned those into these doc lines.

That sort of documentation isn't useful. In general anything the reader can already see from the name and type isn't useful documentation. See:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-useless-documentation
https://dart.dev/effective-dart/documentation#avoid-redundancy-with-the-surrounding-context

It's fine for a lot of things to not get documented, but we should never check in useless documentation — that just adds noise.

gnprice · 2025-03-24T20:43:43Z

lib/widgets/lightbox.dart

              src: widget.src,
-              child: RealmContentNetworkImage(widget.src,
+              child: RealmContentNetworkImage(key: Key(widget.message.id.toString()),


This key is added purely for the sake of the tests, right? It doesn't have any useful effect in the actual app, because this widget is the only child of its parent.

Adding a key just for tests like this shouldn't be necessary — we should be able to write the test in a way that more faithfully reflects how the user experiences the app.

gnprice · 2025-03-24T20:44:41Z

test/widgets/lightbox_test.dart

+      final destinationHeroRenderBox = destinationHeroElement.renderObject as RenderBox;
+      final destinationHeroPosition = destinationHeroRenderBox.localToGlobal(Offset.zero);
+
+      final totalTopDistance = (initialImagePosition.top - destinationHeroPosition.dy).abs();
+      final totalLeftDistance = (initialImagePosition.left - destinationHeroPosition.dx).abs();


nit: lots of lines here too long — key information should all be kept within 80 columns

gnprice · 2025-03-24T20:49:50Z

test/widgets/lightbox_test.dart

+
+      await tester.pumpAndSettle();
+
+      final lightBoxImageFinder = find.byKey(Key(message.id.toString()));


This is meant to find the RealmContentNetworkImage widget, right?

How about identifying it by its src URL? That seems a lot more on-point for describing the UX the user actually sees.

gnprice · 2025-03-24T20:55:06Z

test/widgets/lightbox_test.dart

+      final destinationHeroElement = tester.elementList(find.byType(Hero))
+      .where((element) => (element.widget as Hero).tag == heroWidgetTag)
+      .where((element) =>
+        element.findAncestorWidgetOfExactType<Overlay>() != null &&
+        element.findAncestorWidgetOfExactType<InteractiveViewer>() != null)
+        .single;


As I've said several times, I'd much rather avoid having these tests look for Hero widgets — that's a way of prying into the details of the implementation. The user doesn't see a Hero widget — they just see an image moving.

And these checks about having an Overlay ancestor and an InteractiveViewer ancestor are quite mysterious to the reader.

It looks like the one way this logic ultimately gets used is to compute destinationHeroPosition. But the test gets the same information anyway lower down, under the name lightBoxImagePosition. So it can just wait until it gets the information that way.

gnprice · 2025-03-24T20:56:15Z

test/widgets/lightbox_test.dart

+        check(positionChangeTop.abs()).isLessThan(0.1*totalTopDistance);
+        check(positionChangeLeft.abs()).isLessThan(0.1*totalLeftDistance);


This is the one spot where the information from destinationHeroPosition gets used before we find the actual image at that location when the animation is complete.

These checks can move to a separate loop at the end. Then this loop can just accumulate a list of the observed data, rather than trying to check it at each step.

gnprice · 2025-03-24T20:57:52Z

test/widgets/lightbox_test.dart

+      for (int i = 1; i <= steps; i++) {
+        await tester.pump(stepDuration);
+
+        final sourceHeroElement = tester.elementList(find.byType(Hero))
+        .where((element) => (element.widget as Hero).tag == heroWidgetTag)
+        .where((element) => element.findAncestorWidgetOfExactType<InteractiveViewer>() == null);
+
+        final destinationHeroElement = tester.elementList(find.byType(Hero))
+        .where((element) => (element.widget as Hero).tag == heroWidgetTag)
+        .where((element) => element.findAncestorWidgetOfExactType<InteractiveViewer>() != null);
+
+        check(sourceHeroElement).isNotEmpty();
+        //without hero animation, destination hero element should not exist
+        check(destinationHeroElement).isEmpty();


Again, this should be written in terms of what the user actually sees: an image (or two images), at particular locations, not anything about whether Hero widgets exist or have particular tags. From the user's perspective, what is a "Hero widget" anyway? And what is a "tag" on such a thing?

The other test is a lot closer now to working that way. This one needs to work that way too.

And more fundamentally: this test does not succeed at doing its job as a test.

The purpose of a test is so that we don't accidentally revert your change. If we do revert your change, the test should fail.

Here:

Revert your change, but keep the new tests, with the command git checkout @~ lib/.

A different test below needs to be tweaked so it still compiles: comment out the messageImageContext: navigator.context, in a call to getImageLightboxRoute.

Run this test file: flutter test test/widgets/lightbox_test.dart.

This test "no hero animation occurs between different message list pages for same image" still passes, even though in reality a hero animation now does occur between such pages.

(And the other test "Hero animation occurs smoothly when opening lightbox from message list" also gets the wrong answer: that one fails, even though in reality the thing it's testing for still works. That's because of that test's use of the key that was artificially added for the test — which is a good illustration of why that's not a good way for that test to work.)

lakshya1goel · 2025-03-25T17:10:14Z

Hi @gnprice, thanks for the detailed reviews so far.
I have revised the implementation as:

Removed the hero related implementation and focused purely on the image now. (Used that earlier to check that the animation is actually a hero animation, nevertheless).
Removed the key used for the identification of the image rather used the image src now.
Moreover, sorry for the 2nd test, now I have checked and verified that it is failing on reverting my changes and passing after the fix.
Resolved the other comments mentioned above.

PTAL and let me know if anything else is required.
Thanks!

gnprice

Thanks for the revision. This is significantly closer; some comments below.

gnprice · 2025-03-28T23:21:49Z

lib/widgets/lightbox.dart

-      tag: _LightboxHeroTag(messageId: message.id, src: src),
+      tag: _LightboxHeroTag(messageImageContext: messageImageContext, src: src),


Is the message field on this class used for anything after this change?

yeah, its not required now.

gnprice · 2025-03-28T23:27:52Z

test/widgets/lightbox_test.dart

+        final imageFinder = find.byWidgetPredicate((widget) =>
+          widget is RealmContentNetworkImage && widget.src == imageSrcUrl
+        ).first;


Why .first?

The purpose of this test is to check that there isn't an image animating around. What if there's one copy that's animating around, and another that's staying in the initial position? The one that stays in the initial position could very well happen to be the first one found in the tree, and then this test would wrongly pass.

Here I am getting 2 exactly same images, I have checked the position of both the images inside the loop for 150 steps using the logs and the position is same for both the images for a particular step.
This is probably due to the navigation, on the press of back button the top most route is being popped and we are moving to the older route so at a particular instance we are having the image due to both of these routes.

Yeah, that sounds like the behavior we expect to see here.

The job of a test is to convince a skeptical reader that the behavior we expect to see is the behavior that's actually happening (given that the test passes). So in particular this test should be written so that it rules out the possibility I described above, and other alternative possibilities of that flavor.

gnprice · 2025-03-28T23:35:58Z

test/widgets/lightbox_test.dart

+
+        previousPosition = position;
+      }
+      await tester.pumpAndSettle();


If this is needed, that suggests that the animation isn't actually over when this test stops checking.

gnprice · 2025-03-28T23:44:48Z

test/widgets/lightbox_test.dart

+        check(positionChangeTop.abs()).isLessThan(0.1 * totalTopDistance);
+        check(positionChangeLeft.abs()).isLessThan(0.1 * totalLeftDistance);


10% of the distance traveled in the whole animation is quite a lot for a duration of 1/150th of the total duration.

We can measure this better by using the Pythagorean theorem to compute the overall distance moved in the two-dimensional plane, rather than looking at motion in each axis separately. That way if the start and end points happen to be close in one axis, this doesn't expect unreasonably tight bounds on how much the image might move in that axis during the animation.

gnprice · 2025-03-28T23:46:56Z

test/widgets/lightbox_test.dart

+      for (final position in animatedPositions) {
+        final positionChangeTop = position.top - previousPosition.top;
+        final positionChangeLeft = position.left - previousPosition.left;
+        check(positionChangeTop.abs()).isLessThan(0.1 * totalTopDistance);
+        check(positionChangeLeft.abs()).isLessThan(0.1 * totalLeftDistance);


Instead of checking each of these separately in a loop against the same upper bound, it's better to compute the max across all the steps and then check the comparison of just that max against the upper bound.

That will have the same behavior as to whether the test passes or fails. But if it fails, it will give more informative output: it'll print the number from the one step where the image moved the most, instead of just the one that happened to come first out of the steps that were over the bound. That's especially helpful when picking what the bound should be.

gnprice · 2025-03-28T23:55:19Z

test/widgets/lightbox_test.dart

+        final animatedFlightImagePosition = tester.getRect(animatedFlightImageFinder);
+        animatedPositions.add(animatedFlightImagePosition);


This can be simpler and shorter by inlining.

gnprice · 2025-03-31T23:50:25Z

Thanks for the revision. This looks good now, except that the test needs to be more specific as described in this subthread above: #1348 (comment)

Fixes: zulip#930

lakshya1goel · 2025-04-02T16:02:48Z

Pushed the revision, PTAL @gnprice, thanks!

gnprice · 2025-04-03T20:12:27Z

test/widgets/lightbox_test.dart

+          imagePositionsOverTime.putIfAbsent(element, () => []);
+          imagePositionsOverTime[element]!.add(position);
+        }
+      }
+
+      for (final element in imagePositionsOverTime.keys) {
+        final positions = imagePositionsOverTime[element]!;
+        check(positions).isNotEmpty();
+
+        final initialPos = positions[0];
+        for (final position in positions) {
+          check(position).equals(initialPos);


This still isn't very specific, because it doesn't check that the set of elements at one step has any relationship to the set of elements at another step.

For example, one plausible buggy behavior would be that at each step, there's a new image element and the old image element is gone. From the user's perspective this could produce an animation, because the new element could be at a different position along an animation path. But that would pass this test, because each list would have just one entry in it.

Instead, try this:

Before navigating to the second page, find the one image element and remember its location.

While on the second page, find the one image element that's now visible and remember its location.

Once the animation starts, at each step, check that the set of image elements is exactly those two image elements.

Also at each step, check that each image element is in the same place as it was at the start.

Does that pass? If so, great — that's very specific and makes clear what the behavior is.

If not, then loosen the conditions a little in order to make it pass. But experiment to find just how and why the specific version fails, so that you can loosen it in only the minimal way that makes it pass, keeping it as specific as possible.

lakshya1goel marked this pull request as draft February 12, 2025 13:36

lakshya1goel force-pushed the issue930 branch from 1fad781 to bf1d607 Compare February 15, 2025 15:49

lakshya1goel marked this pull request as ready for review February 15, 2025 16:01

lakshya1goel changed the title ~~msglist: Prevent hero animation between message lists~~ lightbox: Prevent hero animation between message lists Feb 15, 2025

lakshya1goel mentioned this pull request Feb 15, 2025

Hero animation for images should run only to/from lightbox, not between message lists #930

Open

lakshya1goel force-pushed the issue930 branch from bf1d607 to decb997 Compare February 18, 2025 18:03

PIG208 self-assigned this Feb 18, 2025

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Feb 18, 2025

PIG208 self-requested a review February 21, 2025 21:55

lakshya1goel force-pushed the issue930 branch from decb997 to 78e44cd Compare February 22, 2025 13:52

PIG208 reviewed Feb 25, 2025

View reviewed changes

lakshya1goel force-pushed the issue930 branch from 78e44cd to 992c875 Compare February 27, 2025 16:56

PIG208 reviewed Feb 28, 2025

View reviewed changes

lakshya1goel force-pushed the issue930 branch from 992c875 to df965ae Compare February 28, 2025 09:52

lakshya1goel requested a review from PIG208 February 28, 2025 09:57

PIG208 reviewed Feb 28, 2025

View reviewed changes

lakshya1goel force-pushed the issue930 branch from df965ae to b9c7a63 Compare March 1, 2025 11:46

lakshya1goel requested a review from PIG208 March 1, 2025 11:50

PIG208 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 Mar 3, 2025

PIG208 assigned gnprice and unassigned PIG208 Mar 3, 2025

PIG208 requested review from gnprice and removed request for PIG208 March 3, 2025 18:34

gnprice reviewed Mar 4, 2025

View reviewed changes

lakshya1goel force-pushed the issue930 branch 2 times, most recently from 96d038a to 7640e48 Compare March 9, 2025 15:56

lakshya1goel requested a review from gnprice March 9, 2025 15:59

gnprice reviewed Mar 10, 2025

View reviewed changes

lakshya1goel force-pushed the issue930 branch from 7640e48 to 97e328e Compare March 21, 2025 13:54

lakshya1goel requested a review from gnprice March 21, 2025 14:24

lakshya1goel force-pushed the issue930 branch from 97e328e to 83e393b Compare March 23, 2025 11:36

gnprice reviewed Mar 24, 2025

View reviewed changes

lakshya1goel force-pushed the issue930 branch 2 times, most recently from b2029e9 to 6228dc6 Compare March 25, 2025 15:12

lakshya1goel requested a review from gnprice March 25, 2025 17:10

gnprice reviewed Mar 28, 2025

View reviewed changes

lakshya1goel force-pushed the issue930 branch from 6228dc6 to 5576851 Compare March 29, 2025 15:37

lightbox: Prevent hero animation between message lists

cd89faf

Fixes: zulip#930

lakshya1goel force-pushed the issue930 branch from 5576851 to cd89faf Compare April 2, 2025 11:06

lakshya1goel requested a review from gnprice April 2, 2025 16:02

gnprice reviewed Apr 3, 2025

View reviewed changes

		connection.prepare(json:
		eg.newestGetMessagesResult(foundOldest: true, messages: [message]).toJson());

		final message = eg.streamMessage(stream: channel,
		contentMarkdown: ContentExample.imageSingle.html, topic: 'test topic');


	await store.addUser(eg.selfUser);
	await store.addUser(eg.selfUser);

		// fly to an image preview with a different URL, following a message edit
		// while the lightbox was open.


		await tester.pumpAndSettle();

		final lightBoxImageFinder = find.byKey(Key(message.id.toString()));

		check(positionChangeTop.abs()).isLessThan(0.1*totalTopDistance);
		check(positionChangeLeft.abs()).isLessThan(0.1*totalLeftDistance);

		tag: _LightboxHeroTag(messageId: message.id, src: src),
		tag: _LightboxHeroTag(messageImageContext: messageImageContext, src: src),

		final animatedFlightImagePosition = tester.getRect(animatedFlightImageFinder);
		animatedPositions.add(animatedFlightImagePosition);

lightbox: Prevent hero animation between message lists #1348

Are you sure you want to change the base?

lightbox: Prevent hero animation between message lists #1348

Conversation

lakshya1goel commented Feb 12, 2025

Videos

Before

After

gnprice commented Feb 15, 2025

lakshya1goel commented Feb 22, 2025 • edited Loading

PIG208 left a 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

lakshya1goel Feb 27, 2025 • 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

PIG208 Feb 25, 2025 • 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

lakshya1goel commented Feb 28, 2025

PIG208 left a 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

lakshya1goel commented Mar 1, 2025

PIG208 commented Mar 3, 2025

gnprice left a 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

gnprice left a 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

lakshya1goel commented Mar 21, 2025

gnprice commented Mar 21, 2025

gnprice left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakshya1goel commented Mar 25, 2025

gnprice left a 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

lakshya1goel commented Feb 22, 2025 •

edited

Loading

lakshya1goel Feb 27, 2025 •

edited

Loading

PIG208 Feb 25, 2025 •

edited

Loading