lightbox: Prevent hero animation between message lists

Fixes: #930

Author: lakshya1goel

lib/widgets/content.dart

@@ -22,6 +22,7 @@ import 'icons.dart';
 import 'inset_shadow.dart';
 import 'lightbox.dart';
 import 'message_list.dart';
+import 'page.dart';
 import 'poll.dart';
 import 'store.dart';
 import 'text.dart';
@@ -648,6 +649,7 @@ class MessageImage extends StatelessWidget {
     // TODO image hover animation
     final srcUrl = node.srcUrl;
     final thumbnailUrl = node.thumbnailUrl;
+    final pageContext = PageRoot.contextOf(context);
     final store = PerAccountStoreWidget.of(context);
     final resolvedSrcUrl = store.tryResolveUrl(srcUrl);
     final resolvedThumbnailUrl = thumbnailUrl == null
@@ -663,12 +665,14 @@ class MessageImage extends StatelessWidget {
           src: resolvedSrcUrl,
           thumbnailUrl: resolvedThumbnailUrl,
           originalWidth: node.originalWidth,
-          originalHeight: node.originalHeight));
+          originalHeight: node.originalHeight,
+          pageContext: pageContext));
       },
       child: node.loading
         ? const CupertinoActivityIndicator()
         : resolvedSrcUrl == null ? null : LightboxHero(
             message: message,
+            pageContext: pageContext,
             src: resolvedSrcUrl,
             child: RealmContentNetworkImage(
               resolvedThumbnailUrl ?? resolvedSrcUrl,

lib/widgets/lightbox.dart

@@ -21,20 +21,26 @@ 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,
+  });
 
   final int messageId;
   final Uri src;
+  final BuildContext pageContext;
 
   @override
   bool operator ==(Object other) {
     return other is _LightboxHeroTag &&
       other.messageId == messageId &&
-      other.src == src;
+      other.src == src &&
+      other.pageContext == pageContext;
   }
 
   @override
-  int get hashCode => Object.hash('_LightboxHeroTag', messageId, src);
+  int get hashCode => Object.hash('_LightboxHeroTag', messageId, src, pageContext);
 }
 
 /// Builds a [Hero] from an image in the message list to the lightbox page.
@@ -44,16 +50,18 @@ class LightboxHero extends StatelessWidget {
     required this.message,
     required this.src,
     required this.child,
+    required this.pageContext,
   });
 
   final Message message;
   final Uri src;
   final Widget child;
+  final BuildContext pageContext;
 
   @override
   Widget build(BuildContext context) {
     return Hero(
-      tag: _LightboxHeroTag(messageId: message.id, src: src),
+      tag: _LightboxHeroTag(messageId: message.id, src: src, pageContext: pageContext),
       flightShuttleBuilder: (
         BuildContext flightContext,
         Animation<double> animation,
@@ -230,6 +238,7 @@ class _ImageLightboxPage extends StatefulWidget {
     required this.thumbnailUrl,
     required this.originalWidth,
     required this.originalHeight,
+    required this.pageContext,
   });
 
   final Animation<double> routeEntranceAnimation;
@@ -238,6 +247,7 @@ class _ImageLightboxPage extends StatefulWidget {
   final Uri? thumbnailUrl;
   final double? originalWidth;
   final double? originalHeight;
+  final BuildContext pageContext;
 
   @override
   State<_ImageLightboxPage> createState() => _ImageLightboxPageState();
@@ -319,6 +329,7 @@ class _ImageLightboxPageState extends State<_ImageLightboxPage> {
             child: LightboxHero(
               message: widget.message,
               src: widget.src,
+              pageContext: widget.pageContext,
               child: RealmContentNetworkImage(widget.src,
                 filterQuality: FilterQuality.medium,
                 frameBuilder: _frameBuilder,
@@ -603,6 +614,7 @@ Route<void> getImageLightboxRoute({
   required Uri? thumbnailUrl,
   required double? originalWidth,
   required double? originalHeight,
+  required BuildContext pageContext,
 }) {
   return _getLightboxRoute(
     accountId: accountId,
@@ -614,7 +626,8 @@ Route<void> getImageLightboxRoute({
         src: src,
         thumbnailUrl: thumbnailUrl,
         originalWidth: originalWidth,
-        originalHeight: originalHeight);
+        originalHeight: originalHeight,
+        pageContext: pageContext);
     });
 }
 

test/flutter_checks.dart

@@ -13,6 +13,8 @@ extension PaintChecks on Subject<Paint> {
 extension RectChecks on Subject<Rect> {
   Subject<double> get top => has((d) => d.top, 'top');
   Subject<double> get bottom => has((d) => d.bottom, 'bottom');
+  Subject<double> get left => has((d) => d.left, 'left');
+  Subject<double> get right => has((d) => d.right, 'right');
 
   // TODO others
 }

test/widgets/lightbox_test.dart

@@ -10,12 +10,20 @@ import 'package:video_player_platform_interface/video_player_platform_interface.
 import 'package:video_player/video_player.dart';
 import 'package:zulip/api/model/model.dart';
 import 'package:zulip/model/localizations.dart';
+import 'package:zulip/model/narrow.dart';
+import 'package:zulip/model/store.dart';
 import 'package:zulip/widgets/app.dart';
 import 'package:zulip/widgets/content.dart';
 import 'package:zulip/widgets/lightbox.dart';
+import 'package:zulip/widgets/message_list.dart';
+import 'package:zulip/widgets/page.dart';
 
+import '../api/fake_api.dart';
 import '../example_data.dart' as eg;
+import '../flutter_checks.dart';
 import '../model/binding.dart';
+import '../model/content_test.dart';
+import '../model/test_store.dart';
 import '../test_images.dart';
 import 'dialog_checks.dart';
 import 'test_app.dart';
@@ -207,14 +215,19 @@ void main() {
       addTearDown(testBinding.reset);
       await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
 
-      // ZulipApp instead of TestZulipApp because we need the navigator to push
-      // the lightbox route. The lightbox page works together with the route;
-      // it takes the route's entrance animation.
-      await tester.pumpWidget(const ZulipApp());
+      // 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),
         message: message ?? eg.streamMessage(),
         src: src,
         thumbnailUrl: thumbnailUrl,
@@ -558,4 +571,77 @@ void main() {
       check(platform.position).equals(position);
     });
   });
+
+  group('LightboxHero', () {
+    late PerAccountStore store;
+    late FakeApiConnection connection;
+
+    final channel = eg.stream();
+    final message = eg.streamMessage(stream: channel,
+        contentMarkdown: ContentExample.imageSingle.html, topic: 'test topic');
+
+    Future<void> setupMessageListPage(WidgetTester tester) async {
+      addTearDown(testBinding.reset);
+      final subscription = eg.subscription(channel);
+      await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot(
+        streams: [channel], subscriptions: [subscription]));
+      store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
+      connection = store.connection as FakeApiConnection;
+
+      await store.addUser(eg.selfUser);
+
+      connection.prepare(json:
+        eg.newestGetMessagesResult(foundOldest: true, messages: [message]).toJson());
+
+      await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
+        child: MessageListPage(initNarrow: const CombinedFeedNarrow())));
+
+      await tester.pumpAndSettle();
+    }
+
+    testWidgets('no hero animation occurs between different message list pages for same image', (tester) async {
+      prepareBoringImageHttpClient();
+
+      await setupMessageListPage(tester);
+
+      final imageFinder = find.byType(RealmContentNetworkImage).first;
+      final initialImageRect = tester.getRect(imageFinder);
+
+      connection.prepare(json:
+        eg.newestGetMessagesResult(foundOldest: true, messages: [message]).toJson());
+
+      await tester.tap(find.descendant(
+        of: find.byType(StreamMessageRecipientHeader),
+        matching: find.text('test topic')));
+      await tester.pump();
+      // Pump halfway through the hero animation duration (300ms)
+      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();
+      debugNetworkImageHttpClientProvider = null;
+    });
+
+    testWidgets('hero animation occurs when opening lightbox from message list', (tester) async {
+      prepareBoringImageHttpClient();
+
+      await setupMessageListPage(tester);
+
+      final imageFinder = find.byType(RealmContentNetworkImage).first;
+      final initialImageRect = tester.getRect(imageFinder);
+
+      await tester.tap(imageFinder);
+      await tester.pump();
+      // Pump halfway through the hero animation duration (300ms)
+      await tester.pump(const Duration(milliseconds: 150));
+
+      final imageInTransition = tester.getRect(imageFinder);
+      check(imageInTransition).top.not((it) => it.equals(initialImageRect.top));
+      check(imageInTransition).left.not((it) => it.equals(initialImageRect.left));
+      await tester.pumpAndSettle();
+      debugNetworkImageHttpClientProvider = null;
+    });
+  });
 }