Skip to content

Commit decb997

Browse files
lakshya1goellakshya1goel
committed
lightbox: Prevent hero animation between message lists
Fixes: #930
1 parent 44df81f commit decb997

File tree

3 files changed

+162
-11
lines changed

3 files changed

+162
-11
lines changed

Diff for: ‎lib/widgets/content.dart

+5-1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import 'dialog.dart';
2020
import 'icons.dart';
2121
import 'lightbox.dart';
2222
import 'message_list.dart';
23+
import 'page.dart';
2324
import 'poll.dart';
2425
import 'store.dart';
2526
import 'text.dart';
@@ -638,6 +639,7 @@ class MessageImage extends StatelessWidget {
638639
// TODO image hover animation
639640
final srcUrl = node.srcUrl;
640641
final thumbnailUrl = node.thumbnailUrl;
642+
final pageContext = PageRoot.contextOf(context);
641643
final store = PerAccountStoreWidget.of(context);
642644
final resolvedSrcUrl = store.tryResolveUrl(srcUrl);
643645
final resolvedThumbnailUrl = thumbnailUrl == null
@@ -653,12 +655,14 @@ class MessageImage extends StatelessWidget {
653655
src: resolvedSrcUrl,
654656
thumbnailUrl: resolvedThumbnailUrl,
655657
originalWidth: node.originalWidth,
656-
originalHeight: node.originalHeight));
658+
originalHeight: node.originalHeight,
659+
pageContext: pageContext));
657660
},
658661
child: node.loading
659662
? const CupertinoActivityIndicator()
660663
: resolvedSrcUrl == null ? null : LightboxHero(
661664
message: message,
665+
pageContext: pageContext,
662666
src: resolvedSrcUrl,
663667
child: RealmContentNetworkImage(
664668
resolvedThumbnailUrl ?? resolvedSrcUrl,

Diff for: ‎lib/widgets/lightbox.dart

+14-5
Original file line numberDiff line numberDiff line change
@@ -21,20 +21,22 @@ import 'store.dart';
2121
// fly to an image preview with a different URL, following a message edit
2222
// while the lightbox was open.
2323
class _LightboxHeroTag {
24-
_LightboxHeroTag({required this.messageId, required this.src});
24+
_LightboxHeroTag({required this.messageId, required this.src, required this.pageContext});
2525

2626
final int messageId;
2727
final Uri src;
28+
final BuildContext pageContext;
2829

2930
@override
3031
bool operator ==(Object other) {
3132
return other is _LightboxHeroTag &&
3233
other.messageId == messageId &&
33-
other.src == src;
34+
other.src == src &&
35+
other.pageContext == pageContext;
3436
}
3537

3638
@override
37-
int get hashCode => Object.hash('_LightboxHeroTag', messageId, src);
39+
int get hashCode => Object.hash('_LightboxHeroTag', messageId, src, pageContext);
3840
}
3941

4042
/// Builds a [Hero] from an image in the message list to the lightbox page.
@@ -44,16 +46,18 @@ class LightboxHero extends StatelessWidget {
4446
required this.message,
4547
required this.src,
4648
required this.child,
49+
required this.pageContext,
4750
});
4851

4952
final Message message;
5053
final Uri src;
5154
final Widget child;
55+
final BuildContext pageContext;
5256

5357
@override
5458
Widget build(BuildContext context) {
5559
return Hero(
56-
tag: _LightboxHeroTag(messageId: message.id, src: src),
60+
tag: _LightboxHeroTag(messageId: message.id, src: src, pageContext: pageContext),
5761
flightShuttleBuilder: (
5862
BuildContext flightContext,
5963
Animation<double> animation,
@@ -230,6 +234,7 @@ class _ImageLightboxPage extends StatefulWidget {
230234
required this.thumbnailUrl,
231235
required this.originalWidth,
232236
required this.originalHeight,
237+
required this.pageContext,
233238
});
234239

235240
final Animation<double> routeEntranceAnimation;
@@ -238,6 +243,7 @@ class _ImageLightboxPage extends StatefulWidget {
238243
final Uri? thumbnailUrl;
239244
final double? originalWidth;
240245
final double? originalHeight;
246+
final BuildContext pageContext;
241247

242248
@override
243249
State<_ImageLightboxPage> createState() => _ImageLightboxPageState();
@@ -319,6 +325,7 @@ class _ImageLightboxPageState extends State<_ImageLightboxPage> {
319325
child: LightboxHero(
320326
message: widget.message,
321327
src: widget.src,
328+
pageContext: widget.pageContext,
322329
child: RealmContentNetworkImage(widget.src,
323330
filterQuality: FilterQuality.medium,
324331
frameBuilder: _frameBuilder,
@@ -603,6 +610,7 @@ Route<void> getImageLightboxRoute({
603610
required Uri? thumbnailUrl,
604611
required double? originalWidth,
605612
required double? originalHeight,
613+
required BuildContext pageContext,
606614
}) {
607615
return _getLightboxRoute(
608616
accountId: accountId,
@@ -614,7 +622,8 @@ Route<void> getImageLightboxRoute({
614622
src: src,
615623
thumbnailUrl: thumbnailUrl,
616624
originalWidth: originalWidth,
617-
originalHeight: originalHeight);
625+
originalHeight: originalHeight,
626+
pageContext: pageContext);
618627
});
619628
}
620629

Diff for: ‎test/widgets/lightbox_test.dart

+143-5
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,22 @@ import 'package:video_player_platform_interface/video_player_platform_interface.
1010
import 'package:video_player/video_player.dart';
1111
import 'package:zulip/api/model/model.dart';
1212
import 'package:zulip/model/localizations.dart';
13+
import 'package:zulip/model/narrow.dart';
14+
import 'package:zulip/model/store.dart';
1315
import 'package:zulip/widgets/app.dart';
1416
import 'package:zulip/widgets/content.dart';
1517
import 'package:zulip/widgets/lightbox.dart';
18+
import 'package:zulip/widgets/message_list.dart';
19+
import 'package:zulip/widgets/page.dart';
20+
import 'package:zulip/widgets/store.dart';
1621

22+
import '../api/fake_api.dart';
1723
import '../example_data.dart' as eg;
1824
import '../model/binding.dart';
25+
import '../model/content_test.dart';
26+
import '../model/test_store.dart';
1927
import '../test_images.dart';
28+
import '../test_navigation.dart';
2029
import 'dialog_checks.dart';
2130
import 'test_app.dart';
2231

@@ -197,6 +206,39 @@ class FakeVideoPlayerPlatform extends Fake
197206
void main() {
198207
TestZulipBinding.ensureInitialized();
199208

209+
late PerAccountStore store;
210+
late FakeApiConnection connection;
211+
212+
Future<void> setupMessageListPage(WidgetTester tester, {
213+
Narrow narrow = const CombinedFeedNarrow(),
214+
List<Message>? messages,
215+
List<ZulipStream>? streams,
216+
List<NavigatorObserver> navObservers = const [],
217+
218+
}) async {
219+
addTearDown(testBinding.reset);
220+
final stream = streams?.first ?? eg.stream(streamId: eg.defaultStreamMessageStreamId);
221+
final subscription = eg.subscription(stream);
222+
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot(
223+
streams: streams ?? [stream], subscriptions: [subscription]));
224+
store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
225+
connection = store.connection as FakeApiConnection;
226+
227+
await store.addUser(eg.selfUser);
228+
229+
prepareBoringImageHttpClient();
230+
231+
connection.prepare(json:
232+
eg.newestGetMessagesResult(foundOldest: true, messages: messages ?? []).toJson());
233+
234+
await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
235+
navigatorObservers: navObservers,
236+
child: MessageListPage(initNarrow: narrow)));
237+
238+
await tester.pumpAndSettle();
239+
debugNetworkImageHttpClientProvider = null;
240+
}
241+
200242
group('_ImageLightboxPage', () {
201243
final src = Uri.parse('https://chat.example/lightbox-image.png');
202244

@@ -207,14 +249,19 @@ void main() {
207249
addTearDown(testBinding.reset);
208250
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
209251

210-
// ZulipApp instead of TestZulipApp because we need the navigator to push
211-
// the lightbox route. The lightbox page works together with the route;
212-
// it takes the route's entrance animation.
213-
await tester.pumpWidget(const ZulipApp());
252+
// ZulipApp instead of TestZulipApp because we need:
253+
// 1. The navigator to push the lightbox route. The lightbox page works
254+
// together with the route; it takes the route's entrance animation.
255+
// 2. The PageRoot widget to provide context for Hero animations between
256+
// the message list and lightbox.
257+
await tester.pumpWidget(PageRoot(
258+
child: const ZulipApp()
259+
));
214260
await tester.pump();
215261
final navigator = await ZulipApp.navigator;
216262
unawaited(navigator.push(getImageLightboxRoute(
217263
accountId: eg.selfAccount.id,
264+
pageContext: PageRoot.contextOf(navigator.context),
218265
message: message ?? eg.streamMessage(),
219266
src: src,
220267
thumbnailUrl: thumbnailUrl,
@@ -558,4 +605,95 @@ void main() {
558605
check(platform.position).equals(position);
559606
});
560607
});
561-
}
608+
609+
group('LightboxHero', () {
610+
testWidgets('no hero animation occurs between different message list pages for same image', (tester) async {
611+
final pushedRoutes = <Route<dynamic>>[];
612+
final testNavObserver = TestNavigatorObserver()
613+
..onPushed = (route, prevRoute) => pushedRoutes.add(route);
614+
final channel = eg.stream(streamId: eg.defaultStreamMessageStreamId);
615+
final message = eg.streamMessage(stream: channel,
616+
contentMarkdown: ContentExample.imageSingle.html, topic: 'test topic');
617+
await setupMessageListPage(tester,
618+
narrow: const CombinedFeedNarrow(),
619+
streams: [channel],
620+
messages: [message],
621+
navObservers: [testNavObserver]);
622+
623+
assert(pushedRoutes.length == 1);
624+
pushedRoutes.clear();
625+
626+
final heroTag1 = tester.widget<Hero>(
627+
find.descendant(
628+
of: find.byType(LightboxHero),
629+
matching: find.byType(Hero),
630+
)
631+
);
632+
633+
connection.prepare(json:
634+
eg.newestGetMessagesResult(foundOldest: true, messages: [message]).toJson());
635+
636+
await tester.tap(find.descendant(
637+
of: find.byType(StreamMessageRecipientHeader),
638+
matching: find.text('test topic')));
639+
await tester.pump();
640+
641+
final heroFinder = find.descendant(
642+
of: find.byType(Overlay),
643+
matching: find.byType(Hero),
644+
);
645+
expect(heroFinder, findsOneWidget);
646+
await tester.pumpAndSettle();
647+
648+
final heroTag2 = tester.widget<Hero>(
649+
find.descendant(
650+
of: find.byType(LightboxHero),
651+
matching: find.byType(Hero),
652+
)
653+
);
654+
expect(heroTag1.tag, isNot(equals(heroTag2.tag)));
655+
});
656+
657+
testWidgets('hero animation occurs when opening lightbox from message list', (tester) async {
658+
final pushedRoutes = <Route<dynamic>>[];
659+
final testNavObserver = TestNavigatorObserver()
660+
..onPushed = (route, prevRoute) => pushedRoutes.add(route);
661+
final channel = eg.stream(streamId: eg.defaultStreamMessageStreamId);
662+
final message = eg.streamMessage(stream: channel,
663+
contentMarkdown: ContentExample.spoilerHeaderHasImage.html);
664+
await setupMessageListPage(tester,
665+
narrow: const CombinedFeedNarrow(),
666+
streams: [channel],
667+
messages: [message],
668+
navObservers: [testNavObserver]);
669+
670+
connection.prepare(json:
671+
eg.newestGetMessagesResult(foundOldest: true, messages: [message]).toJson());
672+
673+
pushedRoutes.clear();
674+
675+
final initialHero = tester.widget<Hero>(
676+
find.descendant(
677+
of: find.byType(LightboxHero),
678+
matching: find.byType(Hero),
679+
)
680+
);
681+
expect(initialHero.flightShuttleBuilder, isNotNull);
682+
await tester.tap(find.byType(RealmContentNetworkImage));
683+
await tester.pump();
684+
await tester.pump(const Duration(milliseconds: 150));
685+
expect(find.byType(PerAccountStoreWidget), findsWidgets);
686+
await tester.pumpAndSettle();
687+
688+
final lightboxHero = tester.widget<Hero>(
689+
find.descendant(
690+
of: find.byType(Overlay),
691+
matching: find.byType(Hero),
692+
)
693+
);
694+
check(lightboxHero.tag).equals(initialHero.tag);
695+
check(lightboxHero.flightShuttleBuilder).isNotNull();
696+
debugNetworkImageHttpClientProvider = null;
697+
});
698+
});
699+
}

0 commit comments

Comments
 (0)