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

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.

nit: extra empty line

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.

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.

Need a newline at the end of the file.

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:

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).

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

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.

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

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

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

nit: indentation

nit:

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

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.

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?

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.

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

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.

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.

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.

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.

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.

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

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.

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?