Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MaterialPageRoute incorrectly applies CupertinoPageRoute’s transition animation on iOS #95764

Open
weakvar opened this issue Dec 24, 2021 · 6 comments
Labels
a: animation Animation APIs a: fidelity Matching the OEM platforms better f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. platform-ios iOS applications specifically team-design Owned by Design Languages team triaged-design Triaged by Design Languages team

Comments

@weakvar
Copy link

weakvar commented Dec 24, 2021

If you use MaterialPageRoute, Flutter automatically adapts the transition animation depending on the platform: on Android it uses a transition from MaterialPageRoute, and on iOS from CupertinoPageRoute. It's a good idea, but the implementation needs to be improved.

Although MaterialPageRoute adapts the transition animation depending on the platform, it uses the wrong animation speed for transition from CupertinoPageRoute: instead of 400 ms animation speed required for CupertinoPageRoute, it uses 300 ms animation speed which is only suitable for MaterialPageRoute. It seems to be no big deal, but this makes CupertinoPageRoute's behavior feel completely different than it should. It doesn't feel like a native transition animation.

I have a question: is it normal MaterialPageRoute behavior on iOS or is it a bug? What was the point of using a transition animation from CupertinoPageRoute in MaterialPageRoute on iOS?

By the way, in one of the next releases there will be an improvement CupertinoPageRoute’s transition animation. Issue: #95511, PR: #95537. Briefly, the CupertinoPageRoute transition animation will have a new barrierColor like in native iOS. With the current implementation of how the transition animation works depending on platform, I guess those developers who are using MaterialPageRoute for both platforms won't be able to notice these improvements.

@nt4f04uNd nt4f04uNd added a: fidelity Matching the OEM platforms better f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. passed first triage labels Dec 24, 2021
@nt4f04uNd
Copy link
Member

That certainly looks like a fidelity issue - PageTransitionsTheme does specify transition builders, but not durations and other parameters of these transitions.

@willlockwood
Copy link
Contributor

willlockwood commented Dec 28, 2021

This could be a weird one to figure out. After a short bit of pondering, I can't imagine a way of doing this that both 1) wouldn't suck, and 2) wouldn't break some public APIs folks out there are using to configure durations.

I think the core problem is that the current API for configuring durations on routes is not straightforward, and not consistent:

  • You can define transitions durations as properties on the route
  • You can also override createAnimationController on certain routes to provide custom values, and if you do that, then the required-to-define transitionDuration fields become irrelevant, and you can't make the implementation of createAnimationController dependent on stuff like the platform on the Theme without manually injecting that info into the route
  • And then there are the imperative methods for pushing dialogs and modals—sometimes these don't make transition durations configurable, and sometimes they do, but sometimes they do by taking in durations as parameters, and sometimes they do by providing a whole AnimationController for the route to use.

I feel like the ways this could be solved would be to either:

  1. Build a custom solution for MaterialPageRoute that allows this to be configurable, which I think would necessarily further fracture the APIs for how to configure transition durations for routes, or
  2. Take this as an opportunity to decide on a flexible and consistent path forward that all TransitionRoutes can use to make durations easily configurable. And to make things clearer for devs, we may be able to do so in a way that feels similar to how other platform-specific transition configuration is done, hopefully making the whole transitions system easier to understand, document, and maintain.

I'm blocked on some other stuff I was working on atm so I'm gonna put some thoughts together on the options we have.

@nt4f04uNd
Copy link
Member

I haven't looked closely at it, but it seems, we could just add more properties to PageTransitionsTheme, which MaterialPageRoute already uses

@willlockwood
Copy link
Contributor

willlockwood commented Dec 28, 2021

I haven't looked closely at it, but it seems, we could just add more properties to PageTransitionsTheme, which MaterialPageRoute already uses

Well yes, and that probably is going to be what makes sense to do, but we'd also have to expose some part of the route where we can actually build the animation controller with access to a context from which you can pull the theme values. This would be the path of least resistance:

@override
Duration get transitionDuration {
  final ThemeData theme = Theme.of(navigator!.context);
  return theme.pageTransitionsTheme.something.duration(theme.platform);
}

@override
Duration get reverseTransitionDuration {
  final ThemeData theme = Theme.of(navigator!.context);
  return theme.pageTransitionsTheme.something.reverseDurations(theme.platform);
}

@override
Color? get barrierColor {
  final ThemeData theme = Theme.of(navigator!.context);
  return fullscreenDialog ? null : theme.pageTransitionsTheme.something.barrierColor(theme.platform);
}

The null checks on the navigator! here feel bad to me, in terms of how maintainable this strategy gets as we make more and more of these route characteristics configurable over time. It's not super clear how these values are used or why we can be sure that the null checks are safe (I'm not actually sure if the null check on barrierColor would always be safe).

The alternative I was imagining would be worth considering, would be something more like:

@override
AnimationController buildAnimationController(BuildContext context, TickerProvider vsync) {
  // Routes that currently take in a custom animationController could just return it here
  // The context here is the navigator context, but this method would only get called once during `install`
  final ThemeData theme = Theme.of(context);
  return AnimationController(
    duration: theme.pageTransitionsTheme.something.duration(theme.platform),
    reverseDuration: theme.pageTransitionsTheme.something.reverseDuration(theme.platform),
    vsync: vsync
  );
}

@override
Color? buildBarrierColor(BuildContext context) {
  final ThemeData theme = Theme.of(context); // This would be the `ModalScope`'s context, which feels more correct to me
  return fullscreenDialog ? null : theme.pageTransitionsTheme.something.barrierColor(theme.platform);
}

@willlockwood
Copy link
Contributor

After doing some more digging, I've found that the navigator! calls should probably be fine. Gonna take a crack at this the simple way 👍🏻

@deepak786
Copy link

It's the same issue for the web as well.

@flutter-triage-bot flutter-triage-bot bot added multiteam-retriage-candidate team-ios Owned by iOS platform team triaged-ios Triaged by iOS platform team labels Jul 8, 2023
@stuartmorgan stuartmorgan added team-design Owned by Design Languages team triaged-design Triaged by Design Languages team and removed team-ios Owned by iOS platform team triaged-ios Triaged by iOS platform team labels Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: animation Animation APIs a: fidelity Matching the OEM platforms better f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. platform-ios iOS applications specifically team-design Owned by Design Languages team triaged-design Triaged by Design Languages team
Projects
None yet
6 participants