Extract cupertino page transition out of material #9059

xster · 2017-03-28T21:13:50Z

For #8410 and #8726 step 1.

Moved page transitions from material to mountain view and cupertino. No logic change.

xster · 2017-03-28T21:16:31Z

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

+    // but not while being controlled by a gesture.
+    return new SlideTransition(
+      position: listenable,
+      child: new Material(


:(

So I only pulled in the one file for this. Perhaps I should attempt to somehow just use widget.PhysicalModel here?

CupertinoPageTransition should not include this Material widget. Instead, we should change MaterialPageRoute to supply the Material widget as the child for CupertinoPageTransition.

Maybe we should use PhysicalModel. If we look at the native iOS behaviour carefully, I think this has to know about the elevation since it actually animates the elevation too.

Removed Material, used PhysicalModel.

Updated the elevation a bit since it looked too small vs the native look.

iOS isn't based on a physical model. If there's some sort of shadow-casting object, then we should have a cupertino concept for it.

In the near term, I'd leave it out have the transition object just manage the motion design and have the visual design handled separately.

abarth · 2017-03-28T21:18:38Z

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

@@ -0,0 +1,142 @@
+import 'package:flutter/widgets.dart';


Missing copyright block.

Oops, done.

abarth · 2017-03-28T21:19:03Z

packages/flutter/lib/mountain_view.dart

+/// Flutter widgets implementing base Android UI components.
+///
+/// To use, import `package:flutter/mountain_view.dart`.
+


I'd probably add some more docs here explaining how this is different from material. Also, remove the blank line between the docs and the library directive.

abarth · 2017-03-28T21:19:31Z

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

+import 'package:flutter/widgets.dart';
+import 'package:meta/meta.dart';
+
+import '../material/material.dart'; // non ideal


We shouldn't import this library. Cupertino isn't allowed to depend on Material.

abarth · 2017-03-28T21:21:32Z

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

-      return new _CupertinoPageTransition(
+      return new CupertinoPageTransition(
        animation: new AnimationMean(left: animation, right: forwardAnimation),
        child: child


i.e., child: new Material(elevation: 8, child: child)

I'm a bit worried that the background color on the transition will cause a flash if the page doesn't paint an opaque background, but maybe it is required to do that anyway?

Oops, missed a comment. Wish github would just show review notes in chronological order. You're referring to this right? #9059 (comment)

I think I'm still favourable to just leaving holes in the route canvas and letting the user see into a shadow. But I think it won't 'flash' at the beginning of the transition since everything is offscreen for iOS and for Android opacity is 0. Was that what you meant?

Well to be very detailed, there is a single frame flash of the offscreen canvas leaking shadow into the screen. But that's going to go away too as we increase fidelity with the native look (where even the elevation animates from right to left).

abarth · 2017-03-28T21:22:27Z

packages/flutter/lib/src/mountain_view/page.dart

@@ -0,0 +1,32 @@
+import 'package:flutter/widgets.dart';


Missing license block

Hixie · 2017-03-28T22:46:01Z

Can you talk more about the purpose of the mountain_view library vs the material library?

Hixie · 2017-03-28T22:48:02Z

packages/flutter/lib/src/mountain_view/page.dart

+
+import 'package:flutter/widgets.dart';
+
+/// A page route transition used for Android and Fuchsia to transition between pages.


In practice it doesn't seem likely this will be used in fuchsia.

Hixie · 2017-03-28T22:49:38Z

packages/flutter/lib/src/mountain_view/page.dart

+import 'package:flutter/widgets.dart';
+
+/// A page route transition used for Android and Fuchsia to transition between pages.
+class MountainViewPageTransition extends AnimatedWidget {


This seems to me like it's just a non-hero Material page transition. (You can tell because it uses fastOutSlowIn, which is a Material curve). I don't really see the point of having this in its own library.

Hixie · 2017-03-28T22:50:45Z

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

+// found in the LICENSE file.
+
+import 'package:flutter/widgets.dart';
+import 'package:meta/meta.dart';


never import meta, instead import flutter/foundation

Hixie · 2017-03-28T22:52:10Z

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

+      position: listenable,
+      child: new PhysicalModel(
+        shape: BoxShape.rectangle,
+        color: const Color(0x00000000), // we don't need the canvas background, just the elevation


this seems dubious. What colour should be painted here? If the material is entirely transparent, then what's the shadow coming from?

Not sure I fully understand but it seems to work. I suspect the shadow is being casted by its children which is the behaviour we want I think.

The shadow is cast by the PhysicalModel rectangle.

This code seems wrong to me. I don't understand why the background would be explicitly given but be transparent. That's almost always a sign that something is wrong.

The color is a required field.

I looked a bit into the code and tested it on a sample app. Seems like the shadow is always drawn black regardless of the PhysicalModel's color or its child's color in https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/rendering/object.dart#L463 which seems like the desired behaviour.

I think one place where the color here might matter is if somehow the supplied child isn't a rectangle or has holes in it but I think seeing through the model onto the shadow behind it might be a clearer way to signal to the user that he/she needs to deal with the elevated hole than 'filling' it with some arbitrary color.

We are filling it with an arbitrary colour. :-)

I think if we want the background to be gray, we should set it to gray. I think, though, that iOS probably has an opinion of what the default colour should be here, probably some off-white, and we should use that. Consider if someone's route is just a Text widget. It would look weird to have a shadow, but not have a background. If we want a shadow, we should commit to a background, IMHO.

cc @jason-simmons who may be able to comment on the PhysicalModel API requiring a colour.
cc @abarth who may have opinions as well.

Ok, added in standard iOS background color

Hixie · 2017-03-28T22:52:28Z

(this will need tests)

abarth · 2017-03-28T22:58:45Z

Can you talk more about the purpose of the mountain_view library vs the material library?

The mountain_view library contains building blocks that are specific to Android but not specific to Material Design. For example, if we had a holo library, it would depend on mountain_view but not material.

Hixie · 2017-03-28T23:16:34Z

Does holo do the animate-from-the-bottom-using-fastOutSlowIn thing? What else would be common between Material and Holo that wouldn't also be common to Cupertino?

Hixie · 2017-03-28T23:19:23Z

Also if it's specific to Android (as opposed to ChromeOS or Andromeda or Fuchsia) then should we call it android/ rather than mountain_view/?

xster · 2017-03-29T02:26:11Z

@Hixie it's partly in #8410 and #8726 but it's mainly to distinguish between the [here's the strong opinion about how we think an full app experience should look like and probably the main part of the value add if you're trying to brand your own custom UI and by the way, here's a default one] vs [here's such basic platform specific building blocks for making an interface that matches your platform that you'll surprise users if you depart too much from it and is probably not a value add for your app to recreate].

Cupertino is currently the latter and Material is currently both which conceptually makes it strange for full customer experience developers who want to fully reject the Material app look and feel from the way they understand it (splashes, FABs, pastel color palettes) to have to import material.dart and have a MaterialApp root widget or tell them to rewrite all activity indicators etc which want a material ancestor. I guess it's a bit harder on Android since there hasn't been non Material system UI since 5.0 but the package is also good discipline to make us more consciously decide whether something we add should be reusable by custom designs or not by choosing the package to go into.

Whether to call it android vs mountain_view is a good point. I'm not too sure. Would fuschia allow full custom non-material apps? If so and the platform is meant to generally behave different like use a pinch out gesture to close pages etc then maybe we should call this android.

abarth · 2017-03-29T02:48:38Z

Would fuschia allow full custom non-material apps?

Yes. For example, moterm is one such app.

abarth · 2017-03-29T02:54:28Z

Does holo do the animate-from-the-bottom-using-fastOutSlowIn thing?

I'm not sure about the exact curve, but yes Holo did animate from the bottom.

What else would be common between Material and Holo that wouldn't also be common to Cupertino?

In principle, the ClampingScrollPhysics and GlowingOverscrollIndicator should be there, but that causes layering issues.

We could also factor out the interactive design from the drawer and it put there. (Android calls this the ViewDragHelper.)

I guess the pattern is a bunch of interaction/motion design that has been common through the various visual designs on Android.

Hixie · 2017-03-29T18:14:33Z

the interactive design from the drawer

This applies to iOS as well, so if we factor it out I guess it should go at the widgets layer.

I'm skeptical that there's enough here to justify an entire library. A library has a high cognitive cost -- it appears in all our docs in the layer diagram, it requires users to think about what they're importing, etc.

I'm not even sure that the "fly in from the bottom" feature is Android-only. Wouldn't there be cases where it's reasonable on iOS? (For example, pages that require explicit confirmation and can't be swiped away?)

xster · 2017-03-29T21:04:10Z

Ok, I removed the mountain_view package as discussed and added page transition routing test. Note the transition asserted in the test is still the incorrect behaviour. This PR is just a refactor + a test for existing behaviour.

Hixie · 2017-03-30T05:11:42Z

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

+    end: -FractionalOffset.topRight
+  );
+
+  CupertinoPageTransition({


nit: constructor first

You mean above the static final? Done

Hixie · 2017-03-30T05:11:55Z

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

+  CupertinoPageTransition({
+    Key key,
+    Animation<double> animation,
+    this.child


nit: trailing commas everywhere

Hixie · 2017-03-30T05:13:07Z

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

+class _CupertinoTransitionCurve extends Curve {
+  _CupertinoTransitionCurve(this.curve);
+
+  Curve curve;


Hixie · 2017-03-30T05:22:58Z

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

+  @override
+  void dispose() {
+    controller.removeStatusListener(handleStatusChanged);
+    controller = null;


if there's no particular reason to set it to null, we should skip that.

Hixie · 2017-03-30T05:23:46Z

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

+      navigator.pop();
+      assert(controller == null);
+    } else if (status == AnimationStatus.completed) {
+      dispose();


self-destructing objects are scary. Who owns this object?

We should document the lifecycle of this object in detail now that we're making it public.

I agree, it's a bit strange. Seems like the child owns itself by latching itself onto the parent's addStatusListener.

I guess something more conventional would be to have the parent have a closure listening to its own controller field's status and dispose the back gesture controller.

Moved lifecycle ownership to parent

…k to its parent

Hixie · 2017-03-30T22:20:25Z

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

+  }
+
+  void handleStatusChanged(AnimationStatus status) {
+    if (status == AnimationStatus.dismissed) {


nit: no braces

Hixie · 2017-03-30T22:21:54Z

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

    return _backGestureController;
  }

+  void handleBackGestureEnded (AnimationStatus status) {


nit: extraneous space before (

Hixie · 2017-03-30T22:31:25Z

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

+  CupertinoBackGestureController({
+    @required NavigatorState navigator,
+    @required this.controller,
+    @required this.onDisposed,


you don't need the onDisposed any more since you call it yourself.

# Conflicts: # packages/flutter/test/material/page_test.dart

xster · 2017-04-03T18:16:40Z

Gentle ping on this. It's a previous step for #9138

abarth · 2017-04-03T18:28:46Z

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

+const double _kMinFlingVelocity = 1.0;  // screen width per second.
+const Color _kBackgroundColor = const Color(0xFFEFEFF4); // iOS 10 background color.
+
+// Used for iOS.


It would be nice to have some dartdocs for this class now that it's public, but we can do that in another round if you prefer.

Ah yes. Done

abarth · 2017-04-03T18:29:05Z

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

+class CupertinoPageTransition extends AnimatedWidget {
+  CupertinoPageTransition({
+    Key key,
+    Animation<double> animation,


@required

Thanks. Done.

abarth · 2017-04-03T18:29:35Z

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

+  static final FractionalOffsetTween _kTween = new FractionalOffsetTween(
+    begin: FractionalOffset.topRight,
+    end: -FractionalOffset.topRight,
+  );


We usually make these file-level statics for no particular reason. This is fine too.

This is moved in the next PR. Though probably clearer inside the class. There will be 2 classes in the next PR with their own tween.

Oops, I remembered wrong. Will move to top level for consistency in next PR.

abarth · 2017-04-03T18:31:33Z

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

+    return (status == AnimationStatus.reverse || status == AnimationStatus.dismissed);
+  }
+
+  void handleStatusChanged(AnimationStatus status) {


Should this function be private?

Done in the next PR.

abarth · 2017-04-03T18:31:48Z

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

+
+// This class responds to drag gestures to control the route's transition
+// animation progress. Used for iOS back gesture.
+class CupertinoBackGestureController extends NavigationGestureController {


Dartdocs would be better now that this class is public.

abarth · 2017-04-03T18:32:05Z

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

    return _backGestureController;
  }

+  void handleBackGestureEnded(AnimationStatus status) {


Probably should be private.

Thanks. Done.

abarth · 2017-04-03T18:33:25Z

xster added 3 commits March 28, 2017 13:11

Moved stuff around yet

eeb8b50

Merge branch 'master' of github.com:flutter/flutter into mountainview

7751254

Fix depedencies

5ad0fe9

googlebot added the cla: yes label Mar 28, 2017

xster requested review from HansMuller and abarth March 28, 2017 21:14

xster commented Mar 28, 2017

View reviewed changes

abarth reviewed Mar 28, 2017

View reviewed changes

xster added 2 commits March 28, 2017 15:03

Add more dartdoc comments to packages

50c40cd

Remove Cupertino dependency on material

93359a5

Hixie reviewed Mar 28, 2017

View reviewed changes

xster mentioned this pull request Mar 29, 2017

Remove imports of meta #9081

Merged

Removed mountain_view package and added page transition test

034ae39

xster changed the title ~~Make a mountain_view package as sibling to cupertino~~ Extract cupertino page transition out of material Mar 29, 2017

Remove commented code

ff9739b

Hixie reviewed Mar 30, 2017

View reviewed changes

xster added 3 commits March 30, 2017 11:24

Some review notes

8306afb

Move the cupertino back gesture controller’s lifecycle management bac…

f455980

…k to its parent

Merge branch 'master' of github.com:flutter/flutter into mountainview

b9af506

Hixie reviewed Mar 30, 2017

View reviewed changes

xster added 3 commits March 30, 2017 16:44

Reviews

1849e44

Add background color

1239c56

Merge branch 'master' of github.com:flutter/flutter into mountainview

9581bb4

# Conflicts: # packages/flutter/test/material/page_test.dart

xster mentioned this pull request Mar 31, 2017

Make Cupertino page transitions match native behaviours #9138

Merged

final controller

df12876

abarth reviewed Apr 3, 2017

View reviewed changes

Review notes

2874a74

xster merged commit c7f98ef into flutter:master Apr 3, 2017

xster deleted the mountainview branch April 3, 2017 19:17

github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021


		import 'package:flutter/widgets.dart';

		/// A page route transition used for Android and Fuchsia to transition between pages.

Extract cupertino page transition out of material #9059

Extract cupertino page transition out of material #9059

Uh oh!

Conversation

xster commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Mar 28, 2017

Uh oh!

abarth commented Mar 28, 2017

Uh oh!

Hixie commented Mar 28, 2017

Uh oh!

Hixie commented Mar 28, 2017

Uh oh!

xster commented Mar 29, 2017

Uh oh!

abarth commented Mar 29, 2017

Uh oh!

abarth commented Mar 29, 2017

Uh oh!

Hixie commented Mar 29, 2017

Uh oh!

xster commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!