Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FlowBuilder breaks Navigator 2.0 system level routing (deep links specifically) #117

Open
pstromberg98 opened this issue Nov 29, 2023 · 20 comments
Assignees
Labels
bug Something isn't working

Comments

@pstromberg98
Copy link

pstromberg98 commented Nov 29, 2023

Describe the bug
Not sure if this is a "defect" of flow_builder, but it might be worth mentioning in the docs.

Using FlowBuilder alongside AutoRoute (and probably any Navigator 2.0 based solution) will break deep linking. This is because flow_builder sets its own handler for the SystemNavigation.instance method channel thus hijacking all system level routing calls (didPopRoute, didPushRoute, etc).

To Reproduce
Steps to reproduce the behavior:

  1. Set up project with AutoRoute
  2. Set up deep linking (https://docs.flutter.dev/ui/navigation/deep-linking)
  3. Set initial route to contain a FlowBuilder widget
  4. Try to deep link to app in foreground (xcrun simctl openurl booted https://{your-domain}/)
  5. AutoRoute doesn't receive deep link (deepLinkBuilder callback isn't ran)

Expected behavior
Not sure. If FlowBuilder isn't supposed to be used with Navigator 2.0 routing packages then I think it should be mentioned in the documentation or README. If we want it to be able to coexist with Navigator 2.0 routing packages then I would expect FlowBuilder to not interfere/break them.

Additional context
I have only tested this with AutoRouter and have confirmed the issue (and that removing FlowBuilder fixes it).

Screenshot 2023-11-28 at 5 31 55 PM

@felangel
Copy link
Owner

Thanks for reporting this! This is definitely a bug imo and FlowBuilder should both support deep linking itself as well as work nicely with existing packages. Are you able to share a minimal reproduction sample? Thanks!

@felangel felangel added bug Something isn't working waiting for response Waiting for additional information labels Nov 29, 2023
@felangel felangel self-assigned this Nov 29, 2023
@pstromberg98
Copy link
Author

@felangel Sure! I'll throw something together soon.

@vasilich6107
Copy link

vasilich6107 commented Mar 1, 2024

Hi @felangel here is a repro repop.
Check the uri text at the top of the screen
After opening the flow page - universal links are not woking anymore

https://github.com/artflutter/flow_builder_unilinks_bug

Screen.Recording.2024-03-01.at.9.49.11.AM.mov

@vasilich6107
Copy link

vasilich6107 commented Mar 9, 2024

Hi
@felangel could you take a look

@vasilich6107
Copy link

Good evening @felangel
It would be nice to get a response
Thanks

@felangel
Copy link
Owner

Good evening @felangel
It would be nice to get a response
Thanks

Hey sorry for the slow response I’ll try to take a look either today or tomorrow 👍

@felangel felangel removed the waiting for response Waiting for additional information label May 21, 2024
@vasilich6107
Copy link

Hi @felangel
Did you have a chance to take a look?

@felangel
Copy link
Owner

Hi @felangel Did you have a chance to take a look?

Sorry this slipped through the cracks! Will try to find some time in the next few days to look 👍

@vasilich6107
Copy link

Hi @felangel just a friendly reminder :-)

@uberchilly
Copy link

uberchilly commented Jul 14, 2024

A sImilar thing happened to me as well on the web: When I navigate to a screen that has FlowBuilder browser back (which should be the same as deep-linking) stops working

@vasilich6107
Copy link

Hi @felangel
Any updates?

@vasilich6107
Copy link

vasilich6107 commented Sep 20, 2024

  • knok knok @felangel
  • who's there?
  • it's me, the bug

@felangel
Copy link
Owner

  • knok knok @felangel
  • who's there?
  • it's me, the bug

Apologies for the delay! Planning to look at this once I get back from FlutterCon next week, thanks for the bump!

@Peetee06
Copy link
Contributor

Peetee06 commented Nov 4, 2024

hey @felangel, just came across this and thought I'll give it another bump. :)

@felangel
Copy link
Owner

felangel commented Nov 4, 2024

hey @felangel, just came across this and thought I'll give it another bump. :)

Thanks will try to finally get to this this week 🤞

@vasilich6107
Copy link

vasilich6107 commented Nov 20, 2024

Hey @pstromberg98 @Peetee06 @uberchilly @felangel
I would like to invite you in 9 days to celebrate 1 year of our bug

Grab your favorite food and drinks.

We can do a meme party in comments :-)

@Peetee06
Copy link
Contributor

@vasilich6107 haha 😂👍🏻

Let's help @felangel out, he has more than enough on his plate.

I'll look into this on the weekend and build the necessary skills to open a PR with a fix.

@vasilich6107
Copy link

vasilich6107 commented Nov 20, 2024

@Peetee06
You can use reproduction code and video from
#117 (comment)

@Peetee06
Copy link
Contributor

Peetee06 commented Nov 21, 2024

Update:

It looks like this issue stems from this line:
SystemChannels.navigation.setMethodCallHandler(_handleSystemNavigation);

Setting the custom method call handler replaces the one set by WidgetsBinding.initInstances:

Future<bool> _handleNavigationInvocation(MethodCall methodCall) {
    return switch (methodCall.method) {
      'popRoute' => handlePopRoute(),
      'pushRoute' => handlePushRoute(methodCall.arguments as String),
      'pushRouteInformation' => _handlePushRouteInformation(methodCall.arguments as Map<dynamic, dynamic>),
      // Return false for unhandled method.
      _ => Future<bool>.value(false),
    };
  }

Because of that, the system method call pushRouteInformation with the deep link route is unhandled.

I couldn't find a way to restore that handler, because it's private.

Unsure if re-initializing the WidgetsBinding could be a workaround, because I don't know if that would cause other side effects.

Will work on understanding the purpose of the _pop interceptor, as that seems to be the main reason to set a custom method call handler. After having understood it, I'll look into other ways to achieve the same behavior without unwanted side effects.

Edit: As far as I understand, this was introduced in #14 to fix issue #12. Because it's related to the system back button, a way could be to use the BackButtonListener widget instead of replacing the system navigation method call handler. Looking into this.

@Peetee06
Copy link
Contributor

Peetee06 commented Nov 22, 2024

I looked into the BackButtonListener and removing the _SystemNavigationObserver.

The BackButtonListener needs a Router above it in the widget tree. So this doesn't work for MaterialApps without router. Adding our own router in the FlowBuilder didn't help to work around this. I assume because the root WidgetsApp that comes with MaterialApp will handle the back button event.

We could implement the _handlePushRouteInformation method of the WidgetsBinding. But that might have unwanted side effects, because we don't respect the observers.

Future<bool> _handlePushRouteInformation(Map<dynamic, dynamic> routeArguments) async {
    final RouteInformation routeInformation = RouteInformation(
      uri: Uri.parse(routeArguments['location'] as String),
      state: routeArguments['state'] as Object?,
    );
    for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
      if (await observer.didPushRouteInformation(routeInformation)) {
        return true;
      }
    }
    return false;
  }

I also tried to add our own WidgetsBindingObserver that overrides the didPopRoute method and add it to the WidgetBindings' observers. But again, the WidgetsApp is added to the observers before it and handles the popRoute method, preventing our observer from doing so.

Unsure where to go from here. Maybe there are other solutions that I didn't find, yet?

Edit:
Maybe the LocalHistoryRoute could be an option here?

/// A mixin used by routes to handle back navigations internally by popping a list.
///
/// When a [Navigator] is instructed to pop, the current route is given an
/// opportunity to handle the pop internally. A [LocalHistoryRoute] handles the
/// pop internally if its list of local history entries is non-empty. Rather
/// than being removed as the current route, the most recent [LocalHistoryEntry]
/// is removed from the list and its [LocalHistoryEntry.onRemove] is called.
///
/// See also:
///
///  * [Route], which documents the meaning of the `T` generic type argument.
mixin LocalHistoryRoute<T> on Route<T>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants