Enable gesture immediately on the underlying page when swiping back #140704

toda-bps · 2023-12-28T17:51:57Z

Introduce Navigator.popGestureStarted.
Enable gesture immediately on the underlying page when swiping back using CupertinoRouteTransitionMixin.
Add cupertino/page_transition_test.dart.

The behavior fixed in this pull request will probably not be completely identical to iOS native, but it will improve the user experience over the current state.
(See also: #48225 (comment))

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.

google-cla · 2023-12-28T17:52:01Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

MitchellGoodwin · 2024-01-02T17:44:12Z

Hi @toda-bps. Thank you for taking the time to put this PR together. Looks like there's some test failures in packages/flutter/test/cupertino/route_test.dart and packages/flutter/test/material/page_test.dart.

toda-bps · 2024-01-02T18:27:07Z

Looks like there's some test failures in packages/flutter/test/cupertino/route_test.dart and packages/flutter/test/material/page_test.dart.

@MitchellGoodwin

In my local environment, the tests cupertino/route_test.dart and material/page_test.dart pass.
(Also, all other tests have passed.)

route_test	page_test

Where can I find detailed logs of test failures?

Edit: These screenshots were a mistake: I checked out the wrong commit.

toda-bps · 2024-01-02T18:40:03Z

@MitchellGoodwin I found the log: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8760417236475823985/+/u/run_test.dart_for_framework_tests_shard_and_subshard_libraries/test_stdout.

I'll try to fix them.

MitchellGoodwin · 2024-01-02T18:41:47Z

You can find logs by selecting "Details" next to a failed check -> then "View more details on flutter-dashboard" near the bottom -> then looking under Steps and Logs. Usually test_stdout is the most useful, but navigating through the block of text can be tough. At a glance, the tests do look related. If it's not failing locally, try rebasing, and see if they still pass. If they do, rebasing would rerun the tests on Github and it may be something unrelated. If they still fail, then there may be something different between your local and the testing environment that is causing the split. Or the tests running on different platforms is causing issues that aren't on your local.

Edit: oh, you found it while I was typing. Oops.

* Introduce Navigator.popGestureStarted. * Enable gesture immediately on the underlying page when swiping back using CupertinoRouteTransitionMixin. * Add cupertino/page_transition_test.dart.

toda-bps · 2024-01-03T08:39:02Z

I tried to fix the test failure, but realized that it was difficult to improve the swipeback behavior while preserving the existing behavior using the previous methods.

So, I implemented another method that allows Navigator to have a separate userGestureInProgress that indicates the start of the pop gesture.

Thanks to iOS App Freeze for ~0.5 second on Swipe Back Gesture #48225 (comment) and other comments

This method improves swipeback behavior while allowing existing tests to pass.

However, I don't think this method is optimal because it adds a public property with limited use to Navigator, and I think it might be better to stop this PR.

I'd like to ask for some advice.

MitchellGoodwin · 2024-01-09T21:28:58Z

I tried to fix the test failure, but realized that it was difficult to improve the swipeback behavior while preserving the existing behavior using the previous methods.

So, I implemented another method that allows Navigator to have a separate userGestureInProgress that indicates the start of the pop gesture.

Thanks to iOS App Freeze for ~0.5 second on Swipe Back Gesture #48225 (comment) and other comments

This method improves swipeback behavior while allowing existing tests to pass.

However, I don't think this method is optimal because it adds a public property with limited use to Navigator, and I think it might be better to stop this PR.

I'd like to ask for some advice.

@chunhtai I'm not super familiar with navigator design. Can you take a look at this concern?

flutter-dashboard · 2024-03-08T22:23:31Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

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.

chunhtai · 2024-05-01T19:18:12Z

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

@@ -3516,6 +3516,9 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res

  bool get _usingPagesAPI => widget.pages != const <Page<dynamic>>[];

+  /// Whether the route was started popping by user gesture.
+  bool popGestureStarted = false;


This is a confusing name, should probably called inAutomatedPopGesture, or finalizingPopGesture

This property contradict the userGestureInProgressNotifier, which is unfortunate.

It looks like userGestureInProgressNotifier needs a new name like userGestureDrivenPopInProgress, or update its logic to match its name. The former will be a breaking change, the latter will require making cupertinoPageTransition more resilient (See

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

Line 543 in 5a41e1e

// Keep the userGestureInProgress in true state since AndroidBackGesturePageTransitionsBuilder

)

chunhtai · 2024-05-01T19:36:01Z

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

@@ -957,8 +957,9 @@ class _ModalScopeState<T> extends State<_ModalScope<T>> {
                                builder: (BuildContext context, Widget? child) {
                                  final bool ignoreEvents = _shouldIgnoreFocusRequest;
                                  focusScopeNode.canRequestFocus = !ignoreEvents;


if you want user interaction, we should probably also allow request focus

Piinks · 2024-05-22T18:10:21Z

Hey @toda-bps would you like to continue working on this PR and address the above feedback?

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository f: routes Navigator, Router, and related APIs. labels Dec 28, 2023

toda-bps force-pushed the fix/48225 branch from d8e85c5 to 20adbbe Compare December 29, 2023 01:07

toda-bps mentioned this pull request Dec 29, 2023

iOS App Freeze for ~0.5 second on Swipe Back Gesture #48225

Open

github-actions bot added the f: material design flutter/packages/flutter/material repository. label Jan 2, 2024

toda-bps force-pushed the fix/48225 branch 2 times, most recently from 1da4ed3 to a944492 Compare January 3, 2024 07:43

github-actions bot removed the f: material design flutter/packages/flutter/material repository. label Jan 3, 2024

Enable gesture immediately on the underlying page when swiping back.

5d6c0b8

* Introduce Navigator.popGestureStarted. * Enable gesture immediately on the underlying page when swiping back using CupertinoRouteTransitionMixin. * Add cupertino/page_transition_test.dart.

toda-bps force-pushed the fix/48225 branch from a944492 to 5d6c0b8 Compare January 3, 2024 07:58

toda-bps changed the title ~~Fix CupertinoPageTransition back gesture behavior~~ Enable gesture immediately on the underlying page when swiping back Jan 3, 2024

Piinks requested a review from chunhtai May 1, 2024 19:13

chunhtai reviewed May 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable gesture immediately on the underlying page when swiping back #140704

Enable gesture immediately on the underlying page when swiping back #140704

toda-bps commented Dec 28, 2023 •

edited

google-cla bot commented Dec 28, 2023

MitchellGoodwin commented Jan 2, 2024

toda-bps commented Jan 2, 2024 •

edited

toda-bps commented Jan 2, 2024

MitchellGoodwin commented Jan 2, 2024 •

edited

toda-bps commented Jan 3, 2024

MitchellGoodwin commented Jan 9, 2024

flutter-dashboard bot commented Mar 8, 2024

chunhtai May 1, 2024

chunhtai May 1, 2024

chunhtai May 1, 2024

Piinks commented May 22, 2024

Enable gesture immediately on the underlying page when swiping back #140704

Are you sure you want to change the base?

Enable gesture immediately on the underlying page when swiping back #140704

Conversation

toda-bps commented Dec 28, 2023 • edited

Pre-launch Checklist

google-cla bot commented Dec 28, 2023

MitchellGoodwin commented Jan 2, 2024

toda-bps commented Jan 2, 2024 • edited

toda-bps commented Jan 2, 2024

MitchellGoodwin commented Jan 2, 2024 • edited

toda-bps commented Jan 3, 2024

MitchellGoodwin commented Jan 9, 2024

flutter-dashboard bot commented Mar 8, 2024

chunhtai May 1, 2024

Choose a reason for hiding this comment

chunhtai May 1, 2024

Choose a reason for hiding this comment

chunhtai May 1, 2024

Choose a reason for hiding this comment

Piinks commented May 22, 2024

toda-bps commented Dec 28, 2023 •

edited

toda-bps commented Jan 2, 2024 •

edited

MitchellGoodwin commented Jan 2, 2024 •

edited