firka/flutter
firka/flutter
5
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Refactored HeroController logic to handle complex cases (#150027)

Browse Source 
previous pr https://github.com/flutter/flutter/pull/140675

fixes https://github.com/flutter/flutter/issues/115358
fixes https://github.com/flutter/flutter/issues/110426
fixes https://github.com/flutter/flutter/issues/88578

most of the issue here is that herocontroller try to deal with multiple push/pops in one frame. This pr introduce a new navigator observer api didChangeTop, which fired when the top most route change regardless the sequence of the operations.

One of the uncertainty is how to infer the hero flight direction, but the current logic in this PR seems to be cover all the test cases
This commit is contained in:
chunhtai 2024-08-27 18:53:24 -07:00 committed by GitHub
parent e1eaad17ad
commit 88bee39f54
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 303 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

73
packages/flutter/lib/src/widgets/heroes.dart
View File

@ -834,34 +834,31 @@ class HeroController extends NavigatorObserver {
final Map<Object, _HeroFlight> _flights = <Object, _HeroFlight>{};
@override
void didPush(Route<dynamic> route, Route<dynamic>? previousRoute) {
assert(navigator != null);
_maybeStartHeroTransition(previousRoute, route, HeroFlightDirection.push, false);
}
@override
void didPop(Route<dynamic> route, Route<dynamic>? previousRoute) {
void didChangeTop(Route<dynamic> topRoute, Route<dynamic>? previousTopRoute) {
assert(topRoute.isCurrent);
assert(navigator != null);
if (previousTopRoute == null) {
return;
}
// Don't trigger another flight when a pop is committed as a user gesture
// back swipe is snapped.
if (!navigator!.userGestureInProgress) {
_maybeStartHeroTransition(route, previousRoute, HeroFlightDirection.pop, false);
}
}
@override
void didReplace({ Route<dynamic>? newRoute, Route<dynamic>? oldRoute }) {
assert(navigator != null);
if (newRoute?.isCurrent ?? false) {
// Only run hero animations if the top-most route got replaced.
_maybeStartHeroTransition(oldRoute, newRoute, HeroFlightDirection.push, false);
_maybeStartHeroTransition(
fromRoute: previousTopRoute,
toRoute: topRoute,
isUserGestureTransition: false,
);
}
}
@override
void didStartUserGesture(Route<dynamic> route, Route<dynamic>? previousRoute) {
assert(navigator != null);
_maybeStartHeroTransition(route, previousRoute, HeroFlightDirection.pop, true);
_maybeStartHeroTransition(
fromRoute: route,
toRoute: previousRoute,
isUserGestureTransition: true,
);
}
@override
@ -894,29 +891,37 @@ class HeroController extends NavigatorObserver {
// If we're transitioning between different page routes, start a hero transition
// after the toRoute has been laid out with its animation's value at 1.0.
void _maybeStartHeroTransition(
Route<dynamic>? fromRoute,
Route<dynamic>? toRoute,
HeroFlightDirection flightType,
bool isUserGestureTransition,
) {
void _maybeStartHeroTransition({
required Route<dynamic>? fromRoute,
required Route<dynamic>? toRoute,
required bool isUserGestureTransition,
}) {
if (toRoute == fromRoute ||
toRoute is! PageRoute<dynamic> ||
fromRoute is! PageRoute<dynamic>) {
return;
}
final PageRoute<dynamic> from = fromRoute;
final PageRoute<dynamic> to = toRoute;
final Animation<double> newRouteAnimation = toRoute.animation!;
final Animation<double> oldRouteAnimation = fromRoute.animation!;
final HeroFlightDirection flightType;
switch ((isUserGestureTransition, oldRouteAnimation.status, newRouteAnimation.status)) {
case (true, _, _):
case (_, AnimationStatus.reverse, _):
flightType = HeroFlightDirection.pop;
case (_, _, AnimationStatus.forward):
flightType = HeroFlightDirection.push;
default:
return;
}
// A user gesture may have already completed the pop, or we might be the initial route
switch (flightType) {
case HeroFlightDirection.pop:
if (from.animation!.value == 0.0) {
if (fromRoute.animation!.value == 0.0) {
return;
}
case HeroFlightDirection.push:
if (to.animation!.value == 1.0) {
if (toRoute.animation!.value == 1.0) {
return;
}
}
@ -924,8 +929,8 @@ class HeroController extends NavigatorObserver {
// For pop transitions driven by a user gesture: if the "to" page has
// maintainState = true, then the hero's final dimensions can be measured
// immediately because their page's layout is still valid.
if (isUserGestureTransition && flightType == HeroFlightDirection.pop && to.maintainState) {
_startHeroTransition(from, to, flightType, isUserGestureTransition);
if (isUserGestureTransition && flightType == HeroFlightDirection.pop && toRoute.maintainState) {
_startHeroTransition(fromRoute, toRoute, flightType, isUserGestureTransition);
} else {
// Otherwise, delay measuring until the end of the next frame to allow
// the 'to' route to build and layout.
@ -933,13 +938,13 @@ class HeroController extends NavigatorObserver {
// Putting a route offstage changes its animation value to 1.0. Once this
// frame completes, we'll know where the heroes in the `to` route are
// going to end up, and the `to` route will go back onstage.
to.offstage = to.animation!.value == 0.0;
toRoute.offstage = toRoute.animation!.value == 0.0;
WidgetsBinding.instance.addPostFrameCallback((Duration value) {
if (from.navigator == null || to.navigator == null) {
if (fromRoute.navigator == null || toRoute.navigator == null) {
return;
}
_startHeroTransition(from, to, flightType, isUserGestureTransition);
_startHeroTransition(fromRoute, toRoute, flightType, isUserGestureTransition);
}, debugLabel: 'HeroController.startTransition');
}
}

29
packages/flutter/lib/src/widgets/navigator.dart
View File

@ -793,6 +793,17 @@ class NavigatorObserver {
/// The [Navigator] replaced `oldRoute` with `newRoute`.
void didReplace({ Route<dynamic>? newRoute, Route<dynamic>? oldRoute }) { }
/// The top most route has changed.
///
/// The `topRoute` is the new top most route. This can be a new route pushed
/// on top of the screen, or an existing route that becomes the new top-most
/// route because the previous top-most route has been popped.
///
/// The `previousTopRoute` was the top most route before the change. This can
/// be a route that was popped off the screen, or a route that will be covered
/// by the `topRoute`. This can also be null if this is the first build.
void didChangeTop(Route<dynamic> topRoute, Route<dynamic>? previousTopRoute) { }
/// The [Navigator]'s routes are being moved by a user gesture.
///
/// For example, this is called when an iOS back gesture starts, and is used
@ -3962,12 +3973,14 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
for (final NavigatorObserver observer in _effectiveObservers) {
NavigatorObserver._navigators[observer] = null;
}
_effectiveObservers = <NavigatorObserver>[];
super.deactivate();
}
@override
void activate() {
super.activate();
_updateEffectiveObservers();
for (final NavigatorObserver observer in _effectiveObservers) {
assert(observer.navigator == null);
NavigatorObserver._navigators[observer] = this;
@ -3981,12 +3994,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
_debugLocked = true;
return true;
}());
assert(() {
for (final NavigatorObserver observer in _effectiveObservers) {
assert(observer.navigator != this);
}
return true;
}());
assert(_effectiveObservers.isEmpty);
_updateHeroController(null);
focusNode.dispose();
_forcedDisposeAllRouteEntries();
@ -4011,6 +4019,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
];
}
_RouteEntry? _lastTopmostRoute;
String? _lastAnnouncedRouteName;
bool _debugUpdatingPage = false;
@ -4437,9 +4446,15 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
// notifications.
_flushRouteAnnouncement();
final _RouteEntry? lastEntry = _lastRouteEntryWhereOrNull(_RouteEntry.isPresentPredicate);
if (lastEntry != null && _lastTopmostRoute != lastEntry) {
for (final NavigatorObserver observer in _effectiveObservers) {
observer.didChangeTop(lastEntry.route, _lastTopmostRoute?.route);
}
}
_lastTopmostRoute = lastEntry;
// Announce route name changes.
if (widget.reportsRouteUpdateToEngine) {
final _RouteEntry? lastEntry = _lastRouteEntryWhereOrNull(_RouteEntry.isPresentPredicate);
final String? routeName = lastEntry?.route.settings.name;
if (routeName != null && routeName != _lastAnnouncedRouteName) {
SystemNavigator.routeInformationUpdated(uri: Uri.parse(routeName));

238
packages/flutter/test/widgets/heroes_test.dart
View File

@ -2347,6 +2347,244 @@ Future<void> main() async {
expect(tester.getSize(find.byKey(smallContainer)), const Size(100,100));
});
testWidgets('Can add two page with heroes simultaneously using page API.', (WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/115358.
const String heroTag = 'foo';
final GlobalKey<NavigatorState> navigator = GlobalKey();
final Key smallContainer = UniqueKey();
final Key largeContainer = UniqueKey();
final MaterialPage<void> page1 = MaterialPage<void>(
child: Center(
child: Card(
child: Hero(
tag: heroTag,
child: Container(
key: largeContainer,
color: Colors.red,
height: 200.0,
width: 200.0,
),
),
),
),
);
final MaterialPage<void> page2 = MaterialPage<void>(
child: Center(
child: Card(
child: Hero(
tag: heroTag,
child: Container(
color: Colors.red,
height: 1000.0,
width: 1000.0,
),
),
),
),
);
final MaterialPage<void> page3 = MaterialPage<void>(
child: Center(
child: Card(
child: Hero(
tag: heroTag,
child: Container(
key: smallContainer,
color: Colors.red,
height: 100.0,
width: 100.0,
),
),
),
),
);
final HeroController controller = HeroController();
await tester.pumpWidget(
MaterialApp(
navigatorKey: navigator,
home: Navigator(
observers: <NavigatorObserver>[controller],
pages: <Page<void>>[page1],
onPopPage: (_, __) => false,
),
)
);
// The initial setup.
expect(find.byKey(largeContainer), isOnstage);
expect(find.byKey(largeContainer), isInCard);
expect(find.byKey(smallContainer, skipOffstage: false), findsNothing);
await tester.pumpWidget(
MaterialApp(
navigatorKey: navigator,
home: Navigator(
observers: <NavigatorObserver>[controller],
pages: <Page<void>>[page1, page2, page3],
onPopPage: (_, __) => false,
),
),
);
expect(find.byKey(largeContainer), isOnstage);
expect(find.byKey(largeContainer), isInCard);
expect(find.byKey(smallContainer, skipOffstage: false), isOffstage);
expect(find.byKey(smallContainer, skipOffstage: false), isInCard);
await tester.pump();
// The hero started flying.
expect(find.byKey(largeContainer), findsNothing);
expect(find.byKey(smallContainer), isOnstage);
expect(find.byKey(smallContainer), isNotInCard);
await tester.pump(const Duration(milliseconds: 100));
// The hero is in-flight.
expect(find.byKey(largeContainer), findsNothing);
expect(find.byKey(smallContainer), isOnstage);
expect(find.byKey(smallContainer), isNotInCard);
final Size size = tester.getSize(find.byKey(smallContainer));
expect(size.height, greaterThan(100));
expect(size.width, greaterThan(100));
expect(size.height, lessThan(200));
expect(size.width, lessThan(200));
await tester.pumpAndSettle();
// The transition has ended.
expect(find.byKey(largeContainer), findsNothing);
expect(find.byKey(smallContainer), isOnstage);
expect(find.byKey(smallContainer), isInCard);
expect(tester.getSize(find.byKey(smallContainer)), const Size(100,100));
});
testWidgets('Can still trigger hero even if page underneath changes', (WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/88578.
const String heroTag = 'foo';
final GlobalKey<NavigatorState> navigator = GlobalKey();
final Key smallContainer = UniqueKey();
final Key largeContainer = UniqueKey();
final MaterialPage<void> unrelatedPage1 = MaterialPage<void>(
key: UniqueKey(),
child: Center(
child: Card(
child: Container(
color: Colors.red,
height: 1000.0,
width: 1000.0,
),
),
),
);
final MaterialPage<void> unrelatedPage2 = MaterialPage<void>(
key: UniqueKey(),
child: Center(
child: Card(
child: Container(
color: Colors.red,
height: 1000.0,
width: 1000.0,
),
),
),
);
final MaterialPage<void> page1 = MaterialPage<void>(
key: UniqueKey(),
child: Center(
child: Card(
child: Hero(
tag: heroTag,
child: Container(
key: largeContainer,
color: Colors.red,
height: 200.0,
width: 200.0,
),
),
),
),
);
final MaterialPage<void> page2 = MaterialPage<void>(
key: UniqueKey(),
child: Center(
child: Card(
child: Hero(
tag: heroTag,
child: Container(
key: smallContainer,
color: Colors.red,
height: 100.0,
width: 100.0,
),
),
),
),
);
final HeroController controller = HeroController();
await tester.pumpWidget(
MaterialApp(
navigatorKey: navigator,
home: Navigator(
observers: <NavigatorObserver>[controller],
pages: <Page<void>>[unrelatedPage1, page1],
onPopPage: (_, __) => false,
),
)
);
// The initial setup.
expect(find.byKey(largeContainer), isOnstage);
expect(find.byKey(largeContainer), isInCard);
expect(find.byKey(smallContainer, skipOffstage: false), findsNothing);
await tester.pumpWidget(
MaterialApp(
navigatorKey: navigator,
home: Navigator(
observers: <NavigatorObserver>[controller],
pages: <Page<void>>[unrelatedPage2, page2],
onPopPage: (_, __) => false,
),
),
);
expect(find.byKey(largeContainer), isOnstage);
expect(find.byKey(largeContainer), isInCard);
expect(find.byKey(smallContainer, skipOffstage: false), isOffstage);
expect(find.byKey(smallContainer, skipOffstage: false), isInCard);
await tester.pump();
// The hero started flying.
expect(find.byKey(largeContainer), findsNothing);
expect(find.byKey(smallContainer), isOnstage);
expect(find.byKey(smallContainer), isNotInCard);
await tester.pump(const Duration(milliseconds: 100));
// The hero is in-flight.
expect(find.byKey(largeContainer), findsNothing);
expect(find.byKey(smallContainer), isOnstage);
expect(find.byKey(smallContainer), isNotInCard);
final Size size = tester.getSize(find.byKey(smallContainer));
expect(size.height, greaterThan(100));
expect(size.width, greaterThan(100));
expect(size.height, lessThan(200));
expect(size.width, lessThan(200));
await tester.pumpAndSettle();
// The transition has ended.
expect(find.byKey(largeContainer), findsNothing);
expect(find.byKey(smallContainer), isOnstage);
expect(find.byKey(smallContainer), isInCard);
expect(tester.getSize(find.byKey(smallContainer)), const Size(100,100));
});
testWidgets('On an iOS back swipe and snap, only a single flight should take place', (WidgetTester tester) async {
int shuttlesBuilt = 0;
Widget shuttleBuilder(

8
packages/flutter/test/widgets/navigator_test.dart
View File

@ -4350,14 +4350,14 @@ void main() {
}
await tester.pumpWidget(build());
observer._checkInvocations(<Symbol>[#navigator, #didPush]);
observer._checkInvocations(<Symbol>[#navigator, #didPush, #didChangeTop]);
await tester.pumpWidget(Container(child: build()));
observer._checkInvocations(<Symbol>[#navigator, #didPush, #navigator]);
observer._checkInvocations(<Symbol>[#navigator, #didPush, #didChangeTop]);
await tester.pumpWidget(Container());
observer._checkInvocations(<Symbol>[#navigator]);
observer._checkInvocations(<Symbol>[]);
final GlobalKey key = GlobalKey();
await tester.pumpWidget(build(key));
observer._checkInvocations(<Symbol>[#navigator, #didPush]);
observer._checkInvocations(<Symbol>[#navigator, #didPush, #didChangeTop]);
await tester.pumpWidget(Container(child: build(key)));
observer._checkInvocations(<Symbol>[#navigator, #navigator]);
});