Skip to content

Conversation

@justinmc
Copy link
Contributor

@justinmc justinmc commented Jan 25, 2024

Programmatically performing navigation during a Cupertino back gesture was causing buggy state. This PR attempts to fix it by canceling the gesture when any navigation happens.

Currently this is just a proof of concept. It only fixes the case of a pop happening during the gesture, but weird things also happen with a push. I tried it, and if I hold the drag gesture so that it's more than halfway back, then it gets stuck when the push happens.

Listening to isCurrent instead of using PopScope didn't work because the change came after the drag gesture had already ended.

CC @maRci002

Fixes #141268

The example app I was ultimately using to test this
import 'dart:async';

import 'package:flutter/cupertino.dart';
import 'package:flutter/gestures.dart';
import 'package:flutter/material.dart';

class AppScrollBehavior extends MaterialScrollBehavior {
  const AppScrollBehavior();

  @override
  Set<PointerDeviceKind> get dragDevices => const {...PointerDeviceKind.values};
}

void main() {
  runApp(const MainApp());
}

class MainApp extends StatelessWidget {
  const MainApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const CupertinoApp(
      home: ScreenOne(),
    );
  }
}

class ScreenOne extends StatelessWidget {
  const ScreenOne({super.key});

  @override
  Widget build(BuildContext context) {
    return CupertinoPageScaffold(
      child: Center(
        child: TextButton(
          onPressed: () {
            Navigator.of(context).push(
              CupertinoPageRoute(
                title: 'Two',
                builder: (_) => const ScreenTwo(),
              ),
            );
          },
          child: const Text('Go to Screen Two'),
        ),
      ),
    );
  }
}

class ScreenTwo extends StatefulWidget {
  const ScreenTwo({super.key});

  @override
  State<ScreenTwo> createState() => _ScreenTwoState();
}

class _ScreenTwoState extends State<ScreenTwo> {
  int seconds = 5;
  late Timer timer;

  @override
  void initState() {
    super.initState();

    timer = Timer.periodic(
      const Duration(seconds: 1),
      (Timer timer) {
        final ModalRoute<dynamic>? route = ModalRoute.of(context);
        if (!mounted || route == null || !route.isCurrent) {
          timer.cancel();
          return;
        }
        setState(() => seconds--);

        if (seconds == 0) {
          timer.cancel();
          //pop();
          //maybePop();
          navigate();
        }
      },
    );
  }

  @override
  void dispose() {
    timer.cancel();
    super.dispose();
  }

  void pop() {
    Navigator.of(context).pop();
  }

  void maybePop() {
    Navigator.of(context).maybePop();
  }

  void navigate() {
    Navigator.of(context).push(
      CupertinoPageRoute(
        title: 'Three',
        builder: (_) => const ScreenThree(),
      ),
    );
  }

  @override
  Widget build(BuildContext context) {
    return PopScope(
      canPop: true,
      onPopInvoked: (didPop) => print('ScreenTwo onPopInvoked didPop? $didPop'),
      child: CupertinoPageScaffold(
        child: Center(
          child: Column(
            mainAxisSize: MainAxisSize.min,
            children: [
              Text('Auto pop in: $seconds'),
              const SizedBox(height: 8),
              TextButton(
                onPressed: pop,
                child: const Text('Pop'),
              ),
              const SizedBox(height: 8),
              TextButton(
                onPressed: maybePop,
                child: const Text('Maybe Pop'),
              ),
              const SizedBox(height: 8),
              TextButton(
                onPressed: navigate,
                child: const Text('Go to Screen Three'),
              ),
            ],
          ),
        ),
      ),
    );
  }
}

class ScreenThree extends StatelessWidget {
  const ScreenThree({super.key});

  @override
  Widget build(BuildContext context) {
    return PopScope(
      canPop: true,
      onPopInvoked: (didPop) =>
          print('ScreenThree onPopInvoked didPop? $didPop'),
      child: const CupertinoPageScaffold(
        child: Center(
          child: Text('Hello World!'),
        ),
      ),
    );
  }
}
pop push
Screen.Recording.2024-02-01.at.1.50.46.PM.mov
Screen.Recording.2024-02-01.at.1.55.08.PM.mov

@justinmc justinmc self-assigned this Jan 25, 2024
@github-actions 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 Jan 25, 2024
child: Listener(
onPointerDown: _handlePointerDown,
behavior: HitTestBehavior.translucent,
return PopScope(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@goderbauer Is there a good way for a route to be informed if it's going to be popped or pushed over? This PopScope setup works for pops, but ideally I want to know if the route has something pushed on top of it, too. I tried watching for changes in PageRoute.isCurrent, but it comes in too late, after the recognizer thinks the drag has ended.

Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand what you need or are trying to do... Routes have didChangeNext that informs them if something is added (e.g. pushed) on top of them, but I don't know if this comes early enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I tried didChangeNext/Previous by adding a setState in them when I needed to rebuild, but it didn't rebuild fast enough. Actively reading isCurrent/isActive on the route instead of waiting for it to be passed down seems to be fast enough, though 👍 .

@maRci002
Copy link
Contributor

maRci002 commented Jan 25, 2024

I think the easiest solution would be to access the current _RouteLifecycle:

_RouteLifecycle currentState;

The Route could have a getter similar to this::

  bool get isOk {
    if (_navigator == null) {
      return false;
    }
    final _RouteEntry? currentRouteEntry = _navigator!
        ._firstRouteEntryWhereOrNull(_RouteEntry.isRoutePredicate(this));

    if (currentRouteEntry == null) {
      return false;
    }

    return !const <_RouteLifecycle>[
      _RouteLifecycle.popping,
      _RouteLifecycle.pushing
    ].contains(currentRouteEntry.currentState);
  }

@justinmc justinmc force-pushed the pop-during-back-gesture branch from 926724d to 45cc90b Compare January 31, 2024 23:52
@justinmc
Copy link
Contributor Author

justinmc commented Feb 1, 2024

@maRci002 I tried your approach but I always seemed to read _RouteLifecycle.idle at the time that the drag finished. Just checking isCurrent and isActive seems to work, though.

@justinmc justinmc marked this pull request as ready for review February 1, 2024 22:00
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@justinmc justinmc merged commit 3280be9 into flutter:master Feb 2, 2024
@justinmc justinmc deleted the pop-during-back-gesture branch February 2, 2024 19:27
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2024
tarrinneal pushed a commit to flutter/packages that referenced this pull request Feb 5, 2024
Manual roll Flutter from e02e207 to 0b5cd50 (46 revisions)

Manual roll requested by [email protected]

flutter/flutter@e02e207...0b5cd50

2024-02-05 [email protected] fix AppBar docs
for backgroundColor & foregroundColor (flutter/flutter#142430)
2024-02-04 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"Update gradle lockfiles template" (flutter/flutter#142889)
2024-02-04 [email protected] Update gradle lockfiles template
(flutter/flutter#140115)
2024-02-04 [email protected] Roll Flutter Engine from
20742e37e54e to f34c658b9600 (1 revision) (flutter/flutter#142876)
2024-02-03 [email protected] Roll Flutter Engine from
23763db72272 to 20742e37e54e (1 revision) (flutter/flutter#142850)
2024-02-03 [email protected] Roll Flutter Engine from
fee02145da8c to 23763db72272 (3 revisions) (flutter/flutter#142848)
2024-02-03 [email protected] Roll Flutter Engine from
9869d47a2736 to fee02145da8c (2 revisions) (flutter/flutter#142847)
2024-02-03 [email protected] Roll Flutter Engine from
78c63d3c2c68 to 9869d47a2736 (1 revision) (flutter/flutter#142842)
2024-02-02 [email protected] Roll Flutter Engine from
266d5d0b5588 to 78c63d3c2c68 (1 revision) (flutter/flutter#142836)
2024-02-02 49699333+dependabot[bot]@users.noreply.github.com Bump
github/codeql-action from 3.23.2 to 3.24.0 (flutter/flutter#142839)
2024-02-02 49699333+dependabot[bot]@users.noreply.github.com Bump
codecov/codecov-action from 3.1.6 to 4.0.1 (flutter/flutter#142838)
2024-02-02 [email protected] Update TextSelectionOverlay
(flutter/flutter#142463)
2024-02-02 [email protected] Roll Flutter Engine from
e29263212bfd to 266d5d0b5588 (5 revisions) (flutter/flutter#142832)
2024-02-02 [email protected] Fix CupertinoTextSelectionToolbar
clipping (flutter/flutter#138195)
2024-02-02 [email protected] Reland "Add support for Gradle Kotlin DSL
(#140744)" (flutter/flutter#142752)
2024-02-02 [email protected] Support navigation during a Cupertino
back gesture (flutter/flutter#142248)
2024-02-02 [email protected] Avoid depending on files from
build_system/targets other than from top level entrypoints in
flutter_tools. (flutter/flutter#142760)
2024-02-02 [email protected] Roll Packages from
5b48c44 to d37fb0a (14 revisions) (flutter/flutter#142812)
2024-02-02 [email protected] Add a link
the different possible Android virtual device configs
(flutter/flutter#142765)
2024-02-02 [email protected] Allow all iOS
tests to use either iOS 16 or 17 (flutter/flutter#142714)
2024-02-02 [email protected] Roll Flutter Engine from
b35153d00b2e to e29263212bfd (2 revisions) (flutter/flutter#142799)
2024-02-02 [email protected] Roll Flutter Engine from
dd4c79a6c864 to b35153d00b2e (10 revisions) (flutter/flutter#142783)
2024-02-02 [email protected] Wasm/JS Dual Compile with the
flutter tool (flutter/flutter#141396)
2024-02-02 [email protected] Reland: Added
ButtonStyle.foregroundBuilder and ButtonStyle.backgroundBuilder
(flutter/flutter#142762)
2024-02-01 [email protected] Use proto
name for emulator version and show cipd package version
(flutter/flutter#142262)
2024-02-01 [email protected] [github actions] ping actor of workflow
on cherry pick pr creation (flutter/flutter#142676)
2024-02-01 [email protected] Marks Linux_android_emu android
views to be unflaky (flutter/flutter#142590)
2024-02-01 [email protected] Implement `switch` expressions in
`lib/src/material/` (flutter/flutter#142634)
2024-02-01 [email protected] Roll Flutter Engine from
9beb7e82e081 to dd4c79a6c864 (1 revision) (flutter/flutter#142749)
2024-02-01 [email protected] Write Tests for API Example of
`form.0.dart` (flutter/flutter#142635)
2024-02-01 [email protected] Make leak_tracking bots sticked to the
left even if bot thinks they are non-flacky. (flutter/flutter#142744)
2024-02-01 [email protected] Upload
DerivedData logs in CI (flutter/flutter#142643)
2024-02-01 [email protected] Test codesigning xcframeworks in artifacts
(flutter/flutter#142666)
2024-02-01 [email protected] Fix gen_defaults test randomness
(flutter/flutter#142743)
2024-02-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"Added ButtonStyle.foregroundBuilder and ButtonStyle.backgroundBuilder"
(flutter/flutter#142748)
2024-02-01 [email protected] Roll Flutter Engine from
39415c3eed42 to 9beb7e82e081 (5 revisions) (flutter/flutter#142745)
2024-02-01 [email protected] Remove unused deprecated autoroll
mirror-remote flag (flutter/flutter#142738)
2024-02-01 [email protected] Fix leaks in tests.
(flutter/flutter#142677)
2024-02-01 [email protected] Roll Flutter Engine from
8c43332c6ffc to 39415c3eed42 (1 revision) (flutter/flutter#142740)
2024-02-01 [email protected] Remove verbose-system-logs on iOS perf
tests (flutter/flutter#142739)
2024-02-01 [email protected] Remove outdated arm64_armv7 check
(flutter/flutter#142737)
2024-02-01 [email protected] fix
CupertinoTabView's Android back button handling with PopScope
(flutter/flutter#141604)
2024-02-01 [email protected] Roll Flutter Engine from
68943afd62d1 to 8c43332c6ffc (8 revisions) (flutter/flutter#142726)
2024-02-01 [email protected] Unpin test
(flutter/flutter#141427)
...
dumazy added a commit to dumazy/flutter that referenced this pull request Feb 7, 2024
* master: (45 commits)
  Reverts "Update gradle lockfiles template" (flutter#142889)
  Update gradle lockfiles template (flutter#140115)
  Roll Flutter Engine from 20742e37e54e to f34c658b9600 (1 revision) (flutter#142876)
  Roll Flutter Engine from 23763db72272 to 20742e37e54e (1 revision) (flutter#142850)
  Roll Flutter Engine from fee02145da8c to 23763db72272 (3 revisions) (flutter#142848)
  Roll Flutter Engine from 9869d47a2736 to fee02145da8c (2 revisions) (flutter#142847)
  Roll Flutter Engine from 78c63d3c2c68 to 9869d47a2736 (1 revision) (flutter#142842)
  Roll Flutter Engine from 266d5d0b5588 to 78c63d3c2c68 (1 revision) (flutter#142836)
  Bump github/codeql-action from 3.23.2 to 3.24.0 (flutter#142839)
  Bump codecov/codecov-action from 3.1.6 to 4.0.1 (flutter#142838)
  Update TextSelectionOverlay (flutter#142463)
  Roll Flutter Engine from e29263212bfd to 266d5d0b5588 (5 revisions) (flutter#142832)
  Fix CupertinoTextSelectionToolbar clipping (flutter#138195)
  Reland "Add support for Gradle Kotlin DSL (flutter#140744)" (flutter#142752)
  Support navigation during a Cupertino back gesture (flutter#142248)
  Avoid depending on files from build_system/targets other than from top level entrypoints in flutter_tools. (flutter#142760)
  Roll Packages from 5b48c44 to d37fb0a (14 revisions) (flutter#142812)
  Add a link the different possible Android virtual device configs (flutter#142765)
  Allow all iOS tests to use either iOS 16 or 17 (flutter#142714)
  Roll Flutter Engine from b35153d00b2e to e29263212bfd (2 revisions) (flutter#142799)
  ...
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: cupertino flutter/packages/flutter/cupertino repository f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[iOS] Back gesture issue with concurrent manual and programmatic pop

3 participants