Skip to content

fix a bug when android uses CupertinoPageTransitionsBuilder. - #99919

Closed
ipcjs wants to merge 4 commits into
flutter:masterfrom
ipcjs:hotfix/page-transitions-theme
Closed

fix a bug when android uses CupertinoPageTransitionsBuilder.#99919
ipcjs wants to merge 4 commits into
flutter:masterfrom
ipcjs:hotfix/page-transitions-theme

Conversation

@ipcjs

@ipcjs ipcjs commented Mar 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 p: material_ui material_ui package in flutter/packages 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

@chunhtai chunhtai left a comment

Copy link
Copy Markdown
Contributor

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

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

if (CupertinoRouteTransitionMixin.isPopGestureInProgress(route))

@chunhtai chunhtai Mar 18, 2022

Copy link
Copy Markdown
Contributor

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

@ipcjs ipcjs Mar 19, 2022

Copy link
Copy Markdown
Contributor Author

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

ipcjs commented Mar 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

chunhtai commented Mar 21, 2022

Copy link
Copy Markdown
Contributor

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
Comment thread packages/flutter/lib/src/material/page_transitions_theme.dart
/// or [FadeUpwardsPageTransitionsBuilder].
///
/// [MaterialPageRoute.buildTransitions] delegates to this method.
Widget buildTransitions<T>(

@chunhtai chunhtai Mar 21, 2022

Copy link
Copy Markdown
Contributor

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

@HansMuller HansMuller left a comment

Copy link
Copy Markdown
Contributor

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

ipcjs commented May 25, 2022

Copy link
Copy Markdown
Contributor Author

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

@ipcjs

ipcjs commented May 25, 2022

Copy link
Copy Markdown
Contributor Author

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

HansMuller commented Aug 9, 2022

Copy link
Copy Markdown
Contributor

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
6 of 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

framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ipcjs @chunhtai @HansMuller