Define durations/barrier colors on `PageTransitionsBuilder`s, and remove transitions for web/non-mobile platforms #104380

willlockwood · 2022-05-23T02:41:35Z

This PR:

Fixes There should be no default page transitions for desktop and web platforms. #99052
Fixes MaterialPageRoute incorrectly applies CupertinoPageRoute’s transition animation on iOS #95764
Fixes [WEB] Need more PageTransitionTheme for web transition #41564
Reintroduces changes that were added in Removed default page transitions for desktop and web platforms. #82596 and then reverted in Revert "Removed default page transitions for desktop and web platforms. (#82596)" #89997

Problems/requirements:

When running on web, regardless of the target platform, routes should have no default animation, and transitions should be immediate.
ZoomPageTransitionsBuilder should not define the default animation for any non-mobile platforms. Non-mobile platforms should also have no default transition animations, and transitions should be immediate.
MaterialPageRoutes should have the same exact transitions as CupertinoPageRoutes when using the CupertinoPageTransitionsBuilder (currently, the barrierColor, transitionDuration, reverseTransitionDuration, and ignorePointerDuringTransitions are all inconsistent between the two implementations)
In spite of all of the documentation written around it, understanding how routes behave by default while transitioning is currently more difficult than it should be, largely because the configuration for these default behaviors is spread across a lot of different classes/files.

Solution

The goal of this PR is to make material route transitions as visible on all platforms (plus web): easier to understand and configure, and to have higher fidelity to the platform-specific native transitions behavior.

This approach does that by taking the responsibility of defining platform-specific transition animation behavior largely within the PageTransitionsBuilder classes that currently only own the building of the transitions themselves. So before, these classes would only be used for building transitions, but now they are the main place that configuration for platform-specific transition animations would happen.

This means that now, the PageTransitionsTheme can own only the mapping of target platforms to platform-specific transition classes (the buildTransitions method on the theme has been deleted).

Similarly, now, the MaterialRouteTransitionMixin can just use the PageTransitionsTheme to get the right PageTransitionsBuilder, and delegate most of the visual configuration to the builder class. For Cupertino transitions, rather than duplicating the behavior for durations/barrier colors across the CupertinoPageRoute and CupertinoPageTransitionsBuilder implementations, that has remained consolidated under the CupertinoRouteTransitionsMixin, which is consulted by the builder so CupertinoPageRoutes and MaterialPageRoutes on iOS can share the same exact implementation (fixing #95764).

This ability to further configure more of the animations for specific platforms was the main thing blocking us from fixing #99052, so I've included fixes to those as well. Now, there is a new NoAnimationPageTransitionsBuilder for web and non-mobile platforms, so animations will only happen by default on android, iOS, and fuchsia (when not also on web).

Alternatives considered:

Keeping the builders only responsible for building transitions: this would require us to add a new map of builders to each new thing we want to be configurable (e.g., a map for durations, a separate map for reverse durations, etc.) I started with this implementation, and it became clear quickly that it was not going to scale well, even just to support the four fields I wanted to add for this PR.

Recordings:

Web

Before:

Screen.Recording.2022-05-22.at.2.43.26.PM.mov

After:

Screen.Recording.2022-05-22.at.2.41.33.PM.mov

Mobile

Before:

Screen.Recording.2022-05-22.at.10.05.54.PM.mov

After:

Screen.Recording.2022-05-22.at.10.07.32.PM.mov

willlockwood · 2022-05-23T03:19:44Z

packages/flutter/lib/src/widgets/routes.dart

@@ -1428,8 +1428,7 @@ abstract class ModalRoute<T> extends TransitionRoute<T> with LocalHistoryRoute<T
    }
    _ignorePointerNotifier.value = !isCurrent ||
        (navigator?.userGestureInProgress ?? false) ||
-        (ignorePointerDuringTransitions &&
-            (isTransitioning(animation) || isTransitioning(secondaryAnimation)));
+        ((isTransitioning(animation) || isTransitioning(secondaryAnimation)) && ignorePointerDuringTransitions);


Reordering this so that ignorePointerDuringTransitions can be last, to avoid issues looking up the theme with a context at the wrong time (like, after the route has been explicitly removed from the navigator, like for dropdown routes). In those cases, the isTransitioning methods should always prevent ignorePointerDuringTransitions from being evaluated, since the transitions are set explicitly to kAlwaysDismissed, and will in turn fail this first condition

willlockwood · 2022-05-23T12:52:56Z

packages/flutter/lib/src/cupertino/route.dart

-  }) : assert(builder != null),
-       assert(maintainState != null),
-       assert(fullscreenDialog != null) {


Figured I might as well delete these on files that I'm touching to make the code cleaner (all of the assertion deletions are for null-checks on non-nullable parameters). I'm just assuming that there's nothing wrong with that, but lemme know if you'd like me to put these back to make the diff easier to read through

willlockwood · 2022-05-23T13:01:07Z

packages/flutter/lib/src/material/page_transitions_theme.dart

+    final TargetPlatform platform = CupertinoRouteTransitionMixin.isPopGestureInProgress(route)
+        ? TargetPlatform.iOS
+        : Theme.of(route.navigator!.context).platform;
+    return builders[platform] ?? _defaultBuilder;
  }


It's worth calling out that this public method has been deleted. I did this so that the responsibility of this class could be simpler—rather than delegating the building of transitions to this class, which then delegates it to the builder, the MaterialRouteTransitionMixin just gets the builder (which is now the new primary function of this class) and then delegates implementation details directly to it. This also makes it easier to be sure that we're perfectly duplicating the logic for this iOS platform override, which we should do not only for building transitions, but also for barrier colors and durations

That said, I don't know if the deletion of this public member is contributing to any of the failing checks; I'll check that out

willlockwood · 2022-05-23T13:05:06Z

packages/flutter/test/material/page_test.dart

@@ -72,7 +73,7 @@ void main() {

    expect(find.text('Page 1'), isOnstage);
    expect(find.text('Page 2'), findsNothing);
-  }, variant: TargetPlatformVariant.only(TargetPlatform.android));
+  }, variant: TargetPlatformVariant.only(TargetPlatform.android), skip: kIsWeb); // [intended] Transitions disabled on web


I took all of these skips from the diff that was accepted in #82596 (cc @darrenaustin as an FYI)

willlockwood · 2022-05-23T13:18:56Z

Tried rebasing to see if there were just testing problems on master, but now I just have even more failing checks. Gonna try figuring out how to work through some of those after work today. Looking at the linux ones now, it seems like at least some of these failures are showing up as flakes in other places as well, but I'll keep taking a look.

At any rate, the implementation is where I wanted to put it, so I think this PR should be ready for at least some level of feedback?

cc @HansMuller in case you'd like to check it out, in particular since you were the original author of this stuff surrounding the transitions builders and transitions theme, that this PR is modifying

…ransitionsTheme for no web builders, update MaterialRouteTransitionMixin to reference the builders

…0 ms

…ion is unchanged)

…hat makes a difference

willlockwood · 2022-05-24T12:08:56Z

Gotta fix this approach, so moving this back to a draft. It's not always going to be safe to lookup Theme.of(context) using the navigator's context (specifically for the new ignorePointerDuringTransitions field I added, in cases where you remove routes, like on dropdown routes, the lookup fails the debugCheckActiveForAncestorLookup assertion).

If anyone has any thoughts about how I can work around this, lemme know; otherwise I'll keep looking into if there are ways we can maybe have some cached value of the builder or something like that. I think if we were to go this direction, we'd want to be able to reference it as much as we want

flutter-dashboard · 2022-05-24T12:11:06Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

willlockwood · 2022-05-25T13:39:55Z

I'm gonna reopen this in smaller pieces. Can't merge all parts of the current approach anyway now that #95757 has been reverted

braj065 · 2022-06-06T06:11:24Z

Once the smaller pieces are created, please mention them here so that i can follow them.

flutter-dashboard bot added 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. labels May 23, 2022

willlockwood commented May 23, 2022

View reviewed changes

willlockwood force-pushed the lockwood/page-transition-theme-expansion branch from e988b15 to 16b4d19 Compare May 23, 2022 12:17

willlockwood commented May 23, 2022

View reviewed changes

willlockwood added 8 commits May 23, 2022 20:00

Move more transitions details to PageTransitionsBuilder, update PageT…

242db32

…ransitionsTheme for no web builders, update MaterialRouteTransitionMixin to reference the builders

Give PageTransitionsBuilder class a default transition duration of 30…

0ca8ea4

…0 ms

Add test skip justification comments

8b4d46c

Fix test skip justification comments

bb42c49

Fix recursive import

d736f74

Fix grammar in doc comment

2a04336

A bunch of small updates to clean up the diff (functional implementat…

3a9b32a

…ion is unchanged)

Only test snack bar transitions on mobile platforms

f058140

willlockwood force-pushed the lockwood/page-transition-theme-expansion branch from 9d5b22a to f058140 Compare May 24, 2022 00:00

willlockwood added 3 commits May 23, 2022 20:16

Change default builder back to ZoomPageTransitionsBuilder to see if t…

e93de88

…hat makes a difference

Change default builder back to NoAnimationPageTransitionsBuilder

bdda475

Remove ignorePointerDuringTransitions from PageTransitionsBuilder

0f5dece

willlockwood marked this pull request as draft May 24, 2022 12:09

willlockwood changed the title ~~Define durations/barrier colors/pointer behavior on PageTransitionsBuilders, and remove transitions for web/non-mobile platforms~~ Define durations/barrier colors on PageTransitionsBuilders, and remove transitions for web/non-mobile platforms May 25, 2022

willlockwood closed this May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define durations/barrier colors on `PageTransitionsBuilder`s, and remove transitions for web/non-mobile platforms #104380

Define durations/barrier colors on `PageTransitionsBuilder`s, and remove transitions for web/non-mobile platforms #104380

willlockwood commented May 23, 2022 •

edited

willlockwood May 23, 2022 •

edited

willlockwood May 23, 2022

willlockwood May 23, 2022

willlockwood May 23, 2022

willlockwood commented May 23, 2022

willlockwood commented May 24, 2022

flutter-dashboard bot commented May 24, 2022

willlockwood commented May 25, 2022

braj065 commented Jun 6, 2022

Define durations/barrier colors on PageTransitionsBuilders, and remove transitions for web/non-mobile platforms #104380

Define durations/barrier colors on PageTransitionsBuilders, and remove transitions for web/non-mobile platforms #104380

Conversation

willlockwood commented May 23, 2022 • edited

Problems/requirements:

Solution

Alternatives considered:

Recordings:

Before:

After:

Before:

After:

willlockwood May 23, 2022 • edited

Choose a reason for hiding this comment

willlockwood May 23, 2022

Choose a reason for hiding this comment

willlockwood May 23, 2022

Choose a reason for hiding this comment

willlockwood May 23, 2022

Choose a reason for hiding this comment

willlockwood commented May 23, 2022

willlockwood commented May 24, 2022

flutter-dashboard bot commented May 24, 2022

willlockwood commented May 25, 2022

braj065 commented Jun 6, 2022

Define durations/barrier colors on `PageTransitionsBuilder`s, and remove transitions for web/non-mobile platforms #104380

Define durations/barrier colors on `PageTransitionsBuilder`s, and remove transitions for web/non-mobile platforms #104380

willlockwood commented May 23, 2022 •

edited

willlockwood May 23, 2022 •

edited