Skip to content

fix a bug when android uses CupertinoPageTransitionsBuilder. #99919

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
ipcjs wants to merge 4 commits into from
Closed

fix a bug when android uses CupertinoPageTransitionsBuilder. #99919

ipcjs wants to merge 4 commits into from

Conversation

@ipcjs
Copy link
Contributor

@ipcjs ipcjs commented Mar 10, 2022 •
edited
Loading

When android uses iOS style PageTransitionsBuilder and iOS uses android style PageTransitionsBuilder, on android, swipe from the left edge of the screen doesn't work. This PR solves that problem.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 10, 2022
@ipcjs ipcjs changed the title Problems with android using CupertinoPageTransitionsBuilder. fix a bug when android uses CupertinoPageTransitionsBuilder. Mar 10, 2022
@HansMuller HansMuller requested a review from chunhtai March 18, 2022 22:04
chunhtai
chunhtai reviewed Mar 18, 2022
View reviewed changes
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

Do you have a reproducible code that demonstrate the crash

packages/flutter/lib/src/material/page_transitions_theme.dart Outdated
) {
TargetPlatform platform = Theme.of(context).platform;

if (CupertinoRouteTransitionMixin.isPopGestureInProgress(route))
Copy link
Contributor

@chunhtai chunhtai Mar 18, 2022

Choose a reason for hiding this comment

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

It seems like a weird way to override the platform. It may create unexpected behavior

Copy link
Contributor Author

@ipcjs ipcjs Mar 19, 2022

Choose a reason for hiding this comment

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

It's probably a legacy code.
So i deleted it😅

@ipcjs
Copy link
Contributor Author

ipcjs commented Mar 19, 2022 •
edited
Loading

Do you have a reproducible code that demonstrate the crash

Maybe i'm mistaken, the console is not reporting any error.
However, swipe to return is not valid, and the click event of the interface is blocked after returning via the back button.
Here is the Demo:
demo_page_transitions_theme.zip

@chunhtai
Copy link
Contributor

chunhtai commented Mar 21, 2022

It looks like the removed code was there to handle a corner case
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8820060409701693425/+/u/run_test.dart_for_framework_tests_shard_and_subshard_libraries/test_stdout

01:33 +2262 ~1: /b/s/w/ir/x/w/flutter/packages/flutter/test/material/page_test.dart: back gesture while OS changes                                                                                     
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: Offset:<Offset(500.0, 300.0)>
  Actual: Offset:<Offset(400.0, 301.3)>

When the exception was thrown, this was the stack:
#4      main.<anonymous closure> (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/material/page_test.dart:332:5)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)

This was caught by the test expectation on the following line:
  file:///b/s/w/ir/x/w/flutter/packages/flutter/test/material/page_test.dart line 332
The test description was:
  back gesture while OS changes
════════════════════════════════════════════════════════════════════════════════════════════════════

01:33 +2262 ~1 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/material/page_test.dart: back gesture while OS changes [E]                                                                              
  Test failed. See exception logs above.
  The test description was: back gesture while OS changes

I agree that code looks like a hack, but the solution in this PR should still handle this corner case

chunhtai
chunhtai reviewed Mar 21, 2022
View reviewed changes
packages/flutter/lib/src/material/page_transitions_theme.dart
/// or [FadeUpwardsPageTransitionsBuilder].
///
/// [MaterialPageRoute.buildTransitions] delegates to this method.
Widget buildTransitions<T>(
Copy link
Contributor

@chunhtai chunhtai Mar 21, 2022

Choose a reason for hiding this comment

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

The change makes sense to me. but it would be a breaking change. cc @HansMuller who may have better idea on how to fix this issue.

@chunhtai chunhtai requested a review from HansMuller March 21, 2022 21:15
HansMuller
HansMuller reviewed May 25, 2022
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

I'm afraid that this seems like a bit of a hack. I appreciate that it solves the reported problem. On the other hand it doesn't generalize and it appears to break the existing public API. Do we need a generic way to indicate that a page transition animation should not be recreated when route.navigator.userGestureInProgress is true?

@ipcjs ipcjs force-pushed the hotfix/page-transitions-theme branch from 5c98e2d to a02fbf3 Compare May 25, 2022 16:20
@ipcjs
Copy link
Contributor Author

ipcjs commented May 25, 2022

Sorry, I shouldn't have merged the master branch...😵

@ipcjs
Copy link
Contributor Author

ipcjs commented May 25, 2022

back gesture while OS changes is a very edge case and almost impossible to occur in actual code. Special handling of it doesn't feel too problematic.

As for BREAK CHANGE, PageTransitionsTheme is immutable, so _prevPageTranstionsBuilder can only be placed in MaterialRouteTransitionMixin. Maybe we can keep the PageTransitionsTheme.buildTransitions method for now and annotate it with @Deprecated, but not call it?

@HansMuller
Copy link
Contributor

HansMuller commented Aug 9, 2022

I don't think we want to pursue this PR (see #99919 (review)) so I'm going to close it. Thanks for the contribution.

@HansMuller HansMuller closed this Aug 9, 2022
@ipcjs ipcjs mentioned this pull request Oct 29, 2022
Merged
8 tasks
auto-submit bot pushed a commit that referenced this pull request Jul 5, 2023
@ipcjs
fix a bug when android uses CupertinoPageTransitionsBuilder... (#114303)
aaaea51 
When android uses iOS style `PageTransitionsBuilder` and iOS uses android style `PageTransitionsBuilder`, on android, swipe from the left edge of the screen doesn't work. This PR solves that problem.

#99919 introduced a breaking change, the pr re-implemented it <del>without introducing a breaking change.**</del>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chunhtai chunhtai chunhtai left review comments

@jmagman jmagman Awaiting requested review from jmagman

@Piinks Piinks Awaiting requested review from Piinks

@christopherfujino christopherfujino Awaiting requested review from christopherfujino

@keyonghan keyonghan Awaiting requested review from keyonghan

+1 more reviewer

@HansMuller HansMuller HansMuller left review comments

Reviewers whose approvals may not affect merge requirements

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.

3 participants

@ipcjs @chunhtai @HansMuller