Fix: broken pop dialog

[JesusHdez960717]

📜 Description

When a dialog is showed, and the back button in Android is pressed, the dialog don't pop, instead the back action is executen in the background screen

The error was given because the use of an navigation widget in the widget tree prevent the back action from actually poping the dialog (btw, I'm not 100% sure why this bug is reproduced, but I know that with this change it get solved)

Before:

    return Navigator(
      onGenerateRoute: (_) {
        return MaterialPageRoute<void>(
          builder: (context) {
            return Theme(...);
          }
        ),
      },
    );

After:

    return Overlay(
      initialEntries: [
        OverlayEntry(
          builder: (context) => Theme(...),
        ),
      ],
    );

💡 Motivation and Context

This PR solve the Dialog not popping issue
This issue is reported in:

• Issue 194
• Issue 205

💚 How did you test it?

To tested, run the next code in the latest version of the plugin:

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

void main() {
  runApp(
    const BetterFeedback(
      child: MyApp(),
    ),
  );
}

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

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: const MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatelessWidget {
  final String title;

  const MyHomePage({Key? key, required this.title}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(title),
      ),
      body: Center(
        child: ElevatedButton(
            onPressed: () {
              FeedbackController controller = BetterFeedback.of(context);
              if (controller.isVisible) {
                controller.hide();
              } else {
                controller.show((feedback) {
                  //to see in console that the feedback is working ok
                  print(feedback.text);
                });
              }
            },
            child: Text('Toggle')),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: () {
          showDialog(
              context: context,
              builder: (_) {
                return AlertDialog(
                  title: Text("Dialog"),
                  content: Container(),
                );
              });
        },
        tooltip: 'Increment',
        child: const Icon(Icons.add),
      ),
    );
  }
}

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• I updated the docs if needed (don't needed)
• All tests passing
• No breaking changes

🔮 Next steps

• Get this solution tested and validate that it works
• In case I don't break anything else 😄, approve the PR

[JesusHdez960717]

@ueman have you been able to review this PR?

[ueman]

Unfortunately, it's not this easy to fix the issue. If you try running the example, click on the 'toggle feedback mode' button, and then when you use the drop down in the feedback view, it's broken, since there's no navigator anymore.

[JesusHdez960717]

Okey thanks, let me review it and see if I can figure out anything else to fix it

[JesusHdez960717]

Hi @ueman , I just submit another commit with the fix for the 'dropdown need navigator ancestor'

Solution: I wrap the feedback-builder with a navigation and it worked.

Description: From one side, if fixes the error when something in this builder need a Navigation as ancestor,
and for the other side, because this builder only cover the bottom part of the overlay and not the hole app, it does not affect the navigation.

BTW, the other solution I found was to do the same thing from the 'developer' side, and wrap the feedback-builder with the navigation, and work as same, the difference is that it give the responsibility to the developer and should be an explicit explanation in this package docs to avoid future errors

[ueman]

Can you open a dialog from the custom feedback builder?

Solution: I wrap the feedback-builder with a navigation and it worked.

That's awesome. Can you also extend the example or test with a dialog, so that this behavior doesn't regress in the future?

BTW, the other solution I found was to do the same thing from the 'developer' side, and wrap the feedback-builder with the navigation, and work as same, the difference is that it give the responsibility to the developer and should be an explicit explanation in this package docs to avoid future errors

I don't like this solution. The developer shouldn't have to figure this out on his own, it should just work. It's the expected behavior from a consumer's point of view, so not enabling that seems like a bad design.

[codecov]

Codecov Report

Attention: Patch coverage is 76.74419% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 83.43%. Comparing base (471cc7b) to head (e9b1734).
Report is 10 commits behind head on master.

Files	Patch %	Lines
feedback/lib/src/feedback_widget.dart	80.26%	15 Missing ⚠️
feedback/lib/src/feedback_bottom_sheet.dart	50.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #293      +/-   ##
==========================================
- Coverage   83.77%   83.43%   -0.35%     
==========================================
  Files          20       20              
  Lines         635      640       +5     
==========================================
+ Hits          532      534       +2     
- Misses        103      106       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JesusHdez960717]

Updated the example.

You can successfully open a dialog from the custom-feedback, the problem is that because it's in a different navigator it get show only in the bottom part of app (and I don't know if that is the intended behavior)

I attached images of how it would look:

Basic Image with nothing open:

Image with both dialogs open, one from base scaffold and one from custom-feedback:

[ueman]

With the current code, the dialog would be shown properly, and not just in the bottom sheet. It's a very tricky problem :/

[ueman]

Maybe @caseycrogers has some ideas on how to fix the navigator issue?

[JesusHdez960717]

At this point I consider that maybe we must put the options on the table and sacrifice something, the options I see are:

1 - Leave the plugin as is with the impossibility of using the dialogs if you use this package (because they dont pop)
2 - Update it with the changes that allow the normal use of navigation and dialogs poping correctly, but the behavior of the dialogs within the custom feedback builder are only shown at the bottom

[caseycrogers]

I'm happy to dig into this this weekend and see if I can come up with a fix. Flutter's navigation handling for the Android back button is a dumpster fire, but it's a dumpster fire I've dove into a couple times before so there's a good chance to make everyone happy here.

All that said, showing a dialogue within a feedback flow and then having it properly respond to the back gesture is such a niche within a niche as you said it may also be reasonable to just pick one compromise or the other.

[JesusHdez960717]

@ueman @caseycrogers any update guys?

[ueman]

I'll plan to merge this, but I'm currently busy 😅 Thank you very much for this tho!

[JesusHdez960717]

Thanks to you, for me it is a pleasure be able to contribute to a package as awesome as this one

[JesusHdez960717]

Thanks @ueman for the approvement