Skip to content

[WIP] Android 10 transition by default #51538

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.

Sign up for GitHub

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

Jump to bottom
Closed
shihaohong wants to merge 3 commits into from
Closed

[WIP] Android 10 transition by default #51538

shihaohong wants to merge 3 commits into from

Conversation

@shihaohong
Copy link
Contributor

@shihaohong shihaohong commented Feb 27, 2020

Description

As new OEM behaviors are created, we should be staying up to date. This PR introduces a breaking change that sets the default MaterialPageRoute transition to the Android 10 transition.

Android 10 activity transition animation

To keep previous behavior, one would need to modify the PageTransitionsTheme in their MaterialApp theme as follows:

      theme: ThemeData(
        pageTransitionsTheme: PageTransitionsTheme(
          builders: {
            TargetPlatform.android: FadeUpwardsPageTransitionsBuilder(),
          },
        ),
        // rest of theme
      ),

Related Issues

Fixes #43277

Tests

I added the following tests:

  • Updated previous tests to check if the default MaterialPageRoute uses the ZoomPageTransitionsBuilder

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@shihaohong shihaohong added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Feb 27, 2020
@shihaohong shihaohong mentioned this pull request Feb 27, 2020
Closed
9 tasks
@shihaohong shihaohong changed the title Implement default Android 10 transition Android 10 transition by default Feb 27, 2020
@shihaohong
Copy link
Contributor Author

shihaohong commented Feb 27, 2020

This change causes some internal tests to fail. At a glance, it doesn't look like it will be easy to fix and figure out. I may have to delay this a bit since this would add quite a bit of work for me and I'm already working through a few other breaking changes.

@shihaohong shihaohong changed the title Android 10 transition by default [WIP] Android 10 transition by default Feb 27, 2020
@Zazo032 Zazo032 mentioned this pull request Mar 6, 2020
Closed
@AlexV525
Copy link
Member

AlexV525 commented Jul 28, 2020

Since we have landed a new transition, can we try again with tests?

AlexV525
AlexV525 reviewed Jul 29, 2020
View reviewed changes
packages/flutter/test/material/page_test.dart

expect(widget1Scale.scale.value, greaterThan(1.0));
expect(widget2Scale.scale.value, lessThan(1.0));
expect(widget2Opacity.opacity.value, lessThan(1.0));
Copy link
Member

@AlexV525 AlexV525 Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to migrate these tests, and it fails at here, with the actual value 1.0 .

The following TestFailure object was thrown running a test:
  Expected: a value less than <1.0>
  Actual: <1.0>
   Which: is not a value less than <1.0>

I'm wondering why it would be 1.0 after only 50 milliseconds, that doesn't seem correct.

Copy link
Contributor Author

@shihaohong shihaohong Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure why either and would have to look at your code. Do you have a branch with the attempted migration you can link me to?

Copy link
Member

@AlexV525 AlexV525 Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a branch with the attempted migration you can link me to?

Not yet. Should I file a PR for this?

Copy link
Contributor Author

@shihaohong shihaohong Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue I ran into originally was that it resulted in breaking changes internally at Google and I did not really know what the sources of the issue were. I would hold off on trying to get the Android 10 transition by default since it would require a Googler to fix the internal tests/implementations if we want to move forward.

Copy link
Member

@AlexV525 AlexV525 Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would hold off on trying to get the Android 10 transition by default since it would require a Googler to fix the internal tests/implementations if we want to move forward.

Because those tests/implementations are invisible to me, the only I can do is reach out to you.

If it re-opened at the future, please let me know. :)

Copy link
Member

@AlexV525 AlexV525 Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should we try again to see if it breaks them?

Copy link
Contributor Author

@shihaohong shihaohong Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure it will continue to break the tests. I remember that they were mostly tests that rely strictly on the widgets contained in the existing default Android transition.

@shihaohong
Copy link
Contributor Author

shihaohong commented Jul 29, 2020

I'm going to close this out for now because I haven't been able to work on it and I don't want to keep this PR open and stale.

@shihaohong shihaohong closed this Jul 29, 2020
@shihaohong shihaohong deleted the default-andr-10 branch December 3, 2020 11:33
@AlexV525
Copy link
Member

AlexV525 commented May 16, 2021

I'll bring up a new PR for this, if you think it's the right time to push forward.

@AlexV525 AlexV525 mentioned this pull request May 17, 2021
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexV525 AlexV525 AlexV525 left review comments

Assignees

No one assigned

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make Android 10 Activity Animation the default animation

3 participants

@shihaohong @AlexV525 @googlebot