Skip to content

Commit 1556d48

Browse files
lakshya1goellakshya1goel
committed
lightbox: Prevent hero animation between message lists
Fixes: #930
1 parent 0494723 commit 1556d48

File tree

3 files changed

+158
-9
lines changed

3 files changed

+158
-9
lines changed

‎lib/widgets/content.dart

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,7 @@ class MessageImage extends StatelessWidget {
649649
Navigator.of(context).push(getImageLightboxRoute(
650650
context: context,
651651
message: message,
652+
messageImageContext: context,
652653
src: resolvedSrcUrl,
653654
thumbnailUrl: resolvedThumbnailUrl,
654655
originalWidth: node.originalWidth,
@@ -657,7 +658,7 @@ class MessageImage extends StatelessWidget {
657658
child: node.loading
658659
? const CupertinoActivityIndicator()
659660
: resolvedSrcUrl == null ? null : LightboxHero(
660-
message: message,
661+
messageImageContext: context,
661662
src: resolvedSrcUrl,
662663
child: RealmContentNetworkImage(
663664
resolvedThumbnailUrl ?? resolvedSrcUrl,

‎lib/widgets/lightbox.dart

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,39 +21,53 @@ 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({
25+
required this.messageImageContext,
26+
required this.src,
27+
});
2528

26-
final int messageId;
29+
/// The [BuildContext] of the image in the message list that's being expanded
30+
/// into the lightbox. Used to coordinate the Hero animation between this specific
31+
/// image and the lightbox view.
32+
///
33+
/// This helps ensure the animation only happens between the correct image instances,
34+
/// preventing unwanted animations between different message lists or between
35+
/// different images that happen to have the same URL.
36+
final BuildContext messageImageContext;
37+
38+
/// The image source URL. Used to match the source and destination images
39+
/// during the Hero animation, ensuring the animation only occurs between
40+
/// images showing the same content.
2741
final Uri src;
2842

2943
@override
3044
bool operator ==(Object other) {
3145
return other is _LightboxHeroTag &&
32-
other.messageId == messageId &&
46+
other.messageImageContext == messageImageContext &&
3347
other.src == src;
3448
}
3549

3650
@override
37-
int get hashCode => Object.hash('_LightboxHeroTag', messageId, src);
51+
int get hashCode => Object.hash('_LightboxHeroTag', messageImageContext, src);
3852
}
3953

4054
/// Builds a [Hero] from an image in the message list to the lightbox page.
4155
class LightboxHero extends StatelessWidget {
4256
const LightboxHero({
4357
super.key,
44-
required this.message,
58+
required this.messageImageContext,
4559
required this.src,
4660
required this.child,
4761
});
4862

49-
final Message message;
63+
final BuildContext messageImageContext;
5064
final Uri src;
5165
final Widget child;
5266

5367
@override
5468
Widget build(BuildContext context) {
5569
return Hero(
56-
tag: _LightboxHeroTag(messageId: message.id, src: src),
70+
tag: _LightboxHeroTag(messageImageContext: messageImageContext, src: src),
5771
flightShuttleBuilder: (
5872
BuildContext flightContext,
5973
Animation<double> animation,
@@ -226,6 +240,7 @@ class _ImageLightboxPage extends StatefulWidget {
226240
const _ImageLightboxPage({
227241
required this.routeEntranceAnimation,
228242
required this.message,
243+
required this.messageImageContext,
229244
required this.src,
230245
required this.thumbnailUrl,
231246
required this.originalWidth,
@@ -234,6 +249,7 @@ class _ImageLightboxPage extends StatefulWidget {
234249

235250
final Animation<double> routeEntranceAnimation;
236251
final Message message;
252+
final BuildContext messageImageContext;
237253
final Uri src;
238254
final Uri? thumbnailUrl;
239255
final double? originalWidth;
@@ -317,7 +333,7 @@ class _ImageLightboxPageState extends State<_ImageLightboxPage> {
317333
child: InteractiveViewer(
318334
child: SafeArea(
319335
child: LightboxHero(
320-
message: widget.message,
336+
messageImageContext: widget.messageImageContext,
321337
src: widget.src,
322338
child: RealmContentNetworkImage(widget.src,
323339
filterQuality: FilterQuality.medium,
@@ -599,6 +615,7 @@ Route<void> getImageLightboxRoute({
599615
int? accountId,
600616
BuildContext? context,
601617
required Message message,
618+
required BuildContext messageImageContext,
602619
required Uri src,
603620
required Uri? thumbnailUrl,
604621
required double? originalWidth,
@@ -611,6 +628,7 @@ Route<void> getImageLightboxRoute({
611628
return _ImageLightboxPage(
612629
routeEntranceAnimation: animation,
613630
message: message,
631+
messageImageContext: messageImageContext,
614632
src: src,
615633
thumbnailUrl: thumbnailUrl,
616634
originalWidth: originalWidth,

‎test/widgets/lightbox_test.dart

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import 'dart:async';
2+
import 'dart:math';
23

34
import 'package:checks/checks.dart';
45
import 'package:clock/clock.dart';
@@ -10,12 +11,18 @@ import 'package:video_player_platform_interface/video_player_platform_interface.
1011
import 'package:video_player/video_player.dart';
1112
import 'package:zulip/api/model/model.dart';
1213
import 'package:zulip/model/localizations.dart';
14+
import 'package:zulip/model/narrow.dart';
15+
import 'package:zulip/model/store.dart';
1316
import 'package:zulip/widgets/app.dart';
1417
import 'package:zulip/widgets/content.dart';
1518
import 'package:zulip/widgets/lightbox.dart';
19+
import 'package:zulip/widgets/message_list.dart';
1620

21+
import '../api/fake_api.dart';
1722
import '../example_data.dart' as eg;
1823
import '../model/binding.dart';
24+
import '../model/content_test.dart';
25+
import '../model/test_store.dart';
1926
import '../test_images.dart';
2027
import 'dialog_checks.dart';
2128
import 'test_app.dart';
@@ -197,6 +204,128 @@ class FakeVideoPlayerPlatform extends Fake
197204
void main() {
198205
TestZulipBinding.ensureInitialized();
199206

207+
group('LightboxHero', () {
208+
late PerAccountStore store;
209+
late FakeApiConnection connection;
210+
211+
final channel = eg.stream();
212+
final imageSrcUrlStr = 'https://chat.example/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp';
213+
final imageSrcUrl = Uri.parse(imageSrcUrlStr);
214+
final message = eg.streamMessage(stream: channel,
215+
topic: 'test topic', contentMarkdown: ContentExample.imageSingle.html);
216+
217+
Future<void> setupMessageListPage(WidgetTester tester) async {
218+
addTearDown(testBinding.reset);
219+
final subscription = eg.subscription(channel);
220+
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot(
221+
streams: [channel], subscriptions: [subscription]));
222+
store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
223+
connection = store.connection as FakeApiConnection;
224+
await store.addUser(eg.selfUser);
225+
226+
connection.prepare(json:
227+
eg.newestGetMessagesResult(foundOldest: true, messages: [message]).toJson());
228+
229+
await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
230+
child: MessageListPage(initNarrow: const CombinedFeedNarrow())));
231+
232+
await tester.pumpAndSettle();
233+
}
234+
235+
testWidgets('Hero animation occurs smoothly when opening lightbox from message list', (tester) async {
236+
prepareBoringImageHttpClient();
237+
238+
await setupMessageListPage(tester);
239+
240+
final messageContentFinder = find.byWidgetPredicate((widget) =>
241+
widget is MessageContent && widget.message.id == message.id
242+
);
243+
final messageListImageFinder = find.descendant(
244+
of: messageContentFinder,
245+
matching: find.byType(RealmContentNetworkImage)
246+
);
247+
final initialImagePosition = tester.getRect(messageListImageFinder);
248+
await tester.tap(messageListImageFinder);
249+
await tester.pump();
250+
// pump to start hero animation
251+
await tester.pump();
252+
253+
final heroAnimationDuration = Duration(milliseconds: 300);
254+
final steps = 150;
255+
final stepDuration = heroAnimationDuration ~/ steps;
256+
List<Rect> animatedPositions = [];
257+
for (int i = 1; i <= steps; i++) {
258+
await tester.pump(stepDuration);
259+
260+
final animatedFlightImageFinder = find.byWidgetPredicate((widget) =>
261+
widget is RealmContentNetworkImage && widget.src == imageSrcUrl
262+
);
263+
animatedPositions.add(tester.getRect(animatedFlightImageFinder));
264+
}
265+
266+
final totalDistance = sqrt(
267+
pow(initialImagePosition.top - animatedPositions.last.top, 2) +
268+
pow(initialImagePosition.left - animatedPositions.last.left, 2));
269+
270+
Rect previousPosition = initialImagePosition;
271+
double maxStepDistance = 0.0;
272+
for (final position in animatedPositions) {
273+
final stepDistance = sqrt(
274+
pow(position.top - previousPosition.top, 2) +
275+
pow(position.left - previousPosition.left, 2));
276+
maxStepDistance = max(maxStepDistance, stepDistance);
277+
check(position).not((pos) => pos.equals(previousPosition));
278+
279+
previousPosition = position;
280+
}
281+
check(maxStepDistance).isLessThan(0.03 * totalDistance);
282+
283+
debugNetworkImageHttpClientProvider = null;
284+
});
285+
286+
testWidgets('no hero animation occurs between different message list pages for same image', (tester) async {
287+
prepareBoringImageHttpClient();
288+
289+
await setupMessageListPage(tester);
290+
291+
final imageFinder = find.byWidgetPredicate((widget) =>
292+
widget is RealmContentNetworkImage && widget.src == imageSrcUrl
293+
);
294+
final firstImagePosition = tester.getRect(imageFinder);
295+
final firstElement = imageFinder.evaluate().single;
296+
297+
connection.prepare(json:
298+
eg.newestGetMessagesResult(foundOldest: true, messages: [message]).toJson());
299+
await tester.tap(find.descendant(
300+
of: find.byType(StreamMessageRecipientHeader),
301+
matching: find.text('test topic')));
302+
await tester.pumpAndSettle();
303+
304+
final secondImagePosition = tester.getRect(imageFinder);
305+
final secondElement = imageFinder.evaluate().single;
306+
307+
await tester.tap(find.byType(BackButton));
308+
await tester.pump();
309+
310+
final heroAnimationDuration = Duration(milliseconds: 300);
311+
final steps = 150;
312+
final stepDuration = heroAnimationDuration ~/ steps;
313+
for (int i = 0; i < steps; i++) {
314+
await tester.pump(stepDuration);
315+
316+
final currentImages = imageFinder.evaluate();
317+
check(currentImages).unorderedEquals([firstElement, secondElement]);
318+
319+
check(tester.getRect(
320+
find.byElementPredicate((e) => e == firstElement))).equals(firstImagePosition);
321+
check(tester.getRect(
322+
find.byElementPredicate((e) => e == secondElement))).equals(secondImagePosition);
323+
}
324+
325+
debugNetworkImageHttpClientProvider = null;
326+
});
327+
});
328+
200329
group('_ImageLightboxPage', () {
201330
final src = Uri.parse('https://chat.example/lightbox-image.png');
202331

@@ -216,6 +345,7 @@ void main() {
216345
unawaited(navigator.push(getImageLightboxRoute(
217346
accountId: eg.selfAccount.id,
218347
message: message ?? eg.streamMessage(),
348+
messageImageContext: navigator.context,
219349
src: src,
220350
thumbnailUrl: thumbnailUrl,
221351
originalHeight: null,

0 commit comments

Comments
 (0)