felangel / flow_builder

Flutter Flows made easy! A Flutter package which simplifies navigation flows with a flexible, declarative API.
https://pub.dev/packages/flow_builder
MIT License
389 stars 63 forks source link

Cannot pop the page with Android back button when using Navigator.push<Type> #84

Closed orestesgaolin closed 2 years ago

orestesgaolin commented 2 years ago

Describe the bug When using flow_builder the Android back button can break if using navigation push with return type argument.

To Reproduce Steps to reproduce the behavior:

  1. Execute the below sample on Android device
  2. Press Navigate with <void>, then navigate back using Android back button
  3. Press Navigate with <NavigationResult?>, then navigate back using Android button
  4. In the second case the page is not popped
  5. Press back button again, the app closes
import 'package:flow_builder/flow_builder.dart';
import 'package:flutter/material.dart';

enum NavigationResult {
  save,
  cancel,
}

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

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: FlowBuilder<int>(
        state: 1,
        onGeneratePages: (state, pages) => [
          const MaterialPage(
            child: MyHomePage(),
          ),
        ],
      ),
    );
  }
}

class MyHomePage extends StatelessWidget {
  const MyHomePage({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Home'),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            ElevatedButton(
              onPressed: () {
                Navigator.of(context).push(DetailsPage.route('1'));
              },
              child: const Text('Navigate with <void>'),
            ),
            ElevatedButton(
              onPressed: () {
                Navigator.of(context)
                    .push<NavigationResult?>(DetailsPage.resultRoute('1'));
              },
              child: const Text('Navigate with <NavigationResult?>'),
            ),
          ],
        ),
      ),
    );
  }
}

class DetailsPage extends StatelessWidget {
  const DetailsPage({Key? key}) : super(key: key);

  static Route<NavigationResult?> resultRoute(String id) =>
      MaterialPageRoute<NavigationResult?>(
        builder: (_) => const DetailsPage(),
        settings: const RouteSettings(name: '/test'),
      );

  static Route<void> route(String id) => MaterialPageRoute(
        builder: (_) => const DetailsPage(),
        settings: const RouteSettings(name: '/test'),
      );

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Details'),
        centerTitle: true,
      ),
      body: const Center(
        child: Text('Details Page'),
      ),
    );
  }
}

Expected behavior A clear and concise description of what you expected to happen.

Screenshots

https://user-images.githubusercontent.com/16854239/163337695-5d28d4a1-9973-435c-9eac-69b6a313c4f6.mp4

Logs

Running on Android 9, but this also is a problem on Android 12

name: navigation_error
description: A new Flutter project.

publish_to: "none"

version: 1.0.0+1

environment:
  sdk: ">=2.16.2 <3.0.0"

dependencies:
  flutter:
    sdk: flutter

  cupertino_icons: ^1.0.2
  flow_builder: ^0.0.6

dev_dependencies:
  flutter_test:
    sdk: flutter

  flutter_lints: ^1.0.0

flutter:
  uses-material-design: true
[✓] Flutter (Channel stable, 2.10.4, on macOS 12.3 21E230 darwin-arm, locale pl-PL)
[✓] Android toolchain - develop for Android devices (Android SDK version 32.0.0)
[✓] Xcode - develop for iOS and macOS (Xcode 13.3.1)
[✓] Chrome - develop for the web
[✓] Android Studio (version 2021.1)
[✓] VS Code (version 1.66.1)
[✓] Connected device (3 available)
[✓] HTTP Host Availability

Additional context Add any other context about the problem here.

felangel commented 2 years ago

Hey @orestesgaolin I took a closer look and the issue in the example is the nested routes are of type NavigationResult? whereas the FlowBuilder is of type int. As a result, when you try to pop the typed route, a TypeError occurs because int isn't of type NavigationResult?. You can resolve the issue by adjusting the type of the FlowBuilder to align:

import 'package:flow_builder/flow_builder.dart';
import 'package:flutter/material.dart';

enum NavigationResult {
  initial,
  save,
  cancel,
}

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: FlowBuilder<NavigationResult>(
        state: NavigationResult.initial,
        onGeneratePages: (state, pages) => [
          const MaterialPage<void>(
            child: MyHomePage(),
          ),
        ],
      ),
    );
  }
}

class MyHomePage extends StatelessWidget {
  const MyHomePage({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Home'),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            ElevatedButton(
              onPressed: () {
                Navigator.of(context).push(DetailsPage.route('1'));
              },
              child: const Text('Navigate with <void>'),
            ),
            ElevatedButton(
              onPressed: () {
                Navigator.of(context)
                    .push<NavigationResult?>(DetailsPage.resultRoute('1'));
              },
              child: const Text('Navigate with <NavigationResult?>'),
            ),
          ],
        ),
      ),
    );
  }
}

class DetailsPage extends StatelessWidget {
  const DetailsPage({Key? key}) : super(key: key);

  static Route<NavigationResult?> resultRoute(String id) =>
      MaterialPageRoute<NavigationResult?>(
        builder: (_) => const DetailsPage(),
        settings: const RouteSettings(name: '/test'),
      );

  static Route<void> route(String id) => MaterialPageRoute(
        builder: (_) => const DetailsPage(),
        settings: const RouteSettings(name: '/test'),
      );

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Details'),
        centerTitle: true,
      ),
      body: const Center(
        child: Text('Details Page'),
      ),
    );
  }
}

Let me know if that helps 👍

orestesgaolin commented 2 years ago

I'm afraid it's not going to resolve my issue, as the difference in FlowBuilder type and the navigation result is done on purpose. In the app we have a top-level FlowBuilder that is handling the AppState based navigation, whereas from within the nested pages we want to navigate using classic Navigator and handle its results in place.

Moreover, the worrying part is that you can easily navigate back from the MaterialPageRoute<NavigationResult?> using the AppBar's BackButton, but not using the physical Android back button.

aquadesk commented 2 years ago

I have a similar problem.

I open camera from one of the pages under FlowBuilder, and when I hit device back button, FlowBuilder popped up and leave the camera open. When this happen, I can go back to the app.

aquadesk commented 2 years ago

I've created a reproducible repo https://github.com/aquadesk/flow_builder/commit/738bc8af668f19f256a5669f8057810955faade5#diff-555a6f9605c2ef6cb47d1607cffce980fd0d5104e2fbd7268bd6bb1c30018621R132-R169

Spin it up and tap into onboarding and on the second page, tap the text on the page to open asset picker and hit device back button.

aquadesk commented 2 years ago

I've created a PR not sure if this will fix this problem but could be related.

wolfenrain commented 2 years ago

I'm afraid it's not going to resolve my issue, as the difference in FlowBuilder type and the navigation result is done on purpose. In the app we have a top-level FlowBuilder that is handling the AppState based navigation, whereas from within the nested pages we want to navigate using classic Navigator and handle its results in place.

Moreover, the worrying part is that you can easily navigate back from the MaterialPageRoute<NavigationResult?> using the AppBar's BackButton, but not using the physical Android back button.

I have been looking into this today, and it seems the AppBar's BackButton calls maybePop without a result value.

If we do the same in FlowBuilder._pop and use the example from @orestesgaolin it works as expected. But I don't think this is the desired solution, will dive into this some more to see what this entails and if there are better ways to fix this.

wolfenrain commented 2 years ago

I have opened a PR with a fix that I already discussed with Felix: #89

@orestesgaolin can you if this fixes the problem you were having? If it does I can try and add some tests for this.

orestesgaolin commented 2 years ago

Sure, gonna test this in the app we're having a problem

orestesgaolin commented 2 years ago

Seems to be fixed with the db316a2d77163a9f07f9be6473c01d2dd67243cd, thanks!