Using `navigatorBuilder` with a widget that contains Overlay results in crash

[rydmike]

Using GoRouter.navigatorBuilder with Overlay fails

I was trying use the navigatorBuilder to always wrap the content in a complex responsice scaffold wrapper and noticed that it fails if there is an Overlay widget in the widget in the wrapping widget in the navigatorBuilder.

We can use the repo example nested_nav.dart to demonstrate the error. Wrap the text in the navigatorBuilder in the example with a Tooltip, eg so:

    // show the current router location as the user navigates page to page; note
    // that this is not required for nested navigation but it is useful to show
    // the location as it changes
    navigatorBuilder: (context, child) => Material(
      child: Column(
        children: [
          Expanded(child: child!),
          Padding(
            padding: const EdgeInsets.all(8),
            child: Tooltip(  // <== This  will break the app.
              message: _router.location,
              child: Text(_router.location),
            ),
          ),
        ],
      ),
    ),

This will result in this error:

Or in text form:

======== Exception caught by widgets library =======================================================
The following assertion was thrown building Tooltip("/family/f1", dirty, state: _TooltipState#b100b(ticker inactive)):
No Overlay widget found.

Tooltip widgets require an Overlay widget ancestor for correct operation.

The most common way to add an Overlay to an application is to include a MaterialApp or Navigator widget in the runApp() call.

The specific widget that failed to find an overlay was: Tooltip
  "/family/f1"
The relevant error-causing widget was: 
  Tooltip Tooltip:file:////code/flutter/csells/go_router/example/lib/nested_nav.dart:69:20

The error is of course well explained, but first felt a bit surprising, it is saying it does not have an Overlay in any parent, which it would get via MaterialApp or a Navigator. Well we clearly have a MaterialApp above it in the three, The built widget in the navigatorBuilder even gets the theme from it. However, a MaterialApp created via the .router interface apparently does not have a Navigator a this stage, we don't see a Navigator until much later in the tree, and the Overlay is I think actually created by the Navigator not by the MaterialApp directly. (The error message is thus not entirely correct imo)

This is a bit of a complication, well for my use case anyway.

This entire issue does of course go back to the not yet fully supported setup of nested navigation that I'm trying out different options for. I guess I in the navigatorBuilder I could include an extra Navigator that then become a higher root than the GoRouter's Navigator, thus introducing the real missing nested Navigator.

With Navigator 1.0 and using imperative routing that is actually how I solved nested navigation. The part in the widget tree that needed nested navigation, just contained a new Navigator and inserted via it navigated to widgets in that point in the tree.

It seems I really need support for that in GoRouter to be able to build the routing I need. But I will return in another issue on that topic and relate it to the open nested navigation issue as well.

Conclusion

The current version of the navigatorBuilder in GoRouter cannot be used to insert any widget that would work in a normal MaterialApp, this was a surprise and not expected.

[csells]

@lulupointu you were looking at Overlays and nested navigation weren't you?

[lulupointu]

Yes I did, I fixed the issue by manually inserting an Overlay on top of navigationBuilder, fixing this type of issues.

[esDotDev]

I believe you have this same issue when you try and use MaterialApp.builder to wrap a scaffold in Nav 1, which has always been a pain point. Nice fix!!

[rydmike]

You are right, I have seen that mentioned somewhere too. Feels like the Overlay fix should be included in the SDKs MaterialApp.builder too.

[csells]

Yes I did, I fixed the issue by manually inserting an Overlay on top of navigationBuilder, fixing this type of issues.

@lulupointu What does the code look like to manually insert the Overlay? Is that something we should just do?

[lulupointu]

@lulupointu What does the code look like to manually insert the Overlay? Is that something we should just do?

Yes, you can have a look at the TabbedRoutes repo its in go_router.dart and its documented.

[esDotDev]

Definitely would be appreciated if the router makes sure there is always an overlay on top, this would be needed for dialogs, drop-down menus or bottom sheets that are shown within the scaffold as well,

[csells]

@lulupointu What does the code look like to manually insert the Overlay? Is that something we should just do?

Yes, you can have a look at the TabbedRoutes repo its in go_router.dart and its documented.

@lulupointu I do not see any use of an Overlay widget in your https://github.com/lulupointu/go_router fork. What does the Overlay code look like?

If I just take the most basic go_router example and add a navigatorBuilder with the child wrapped with an Overlay, navigation still happens but the widget tree is not re-rendered:

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

void main() => runApp(App());

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

  static const title = 'GoRouter Example: Declarative Routes';

  @override
  Widget build(BuildContext context) => MaterialApp.router(
        routeInformationParser: _router.routeInformationParser,
        routerDelegate: _router.routerDelegate,
        title: title,
      );

  final _router = GoRouter(
    routes: [
      GoRoute(
        path: '/',
        builder: (context, state) => const Page1Screen(),
      ),
      GoRoute(
        path: '/page2',
        builder: (context, state) => const Page2Screen(),
      ),
    ],
    // navigatorBuilder w/ Overlay
    navigatorBuilder: (context, state, child) => Overlay(
      initialEntries: [
        OverlayEntry(
          builder: (context) => child,
        )
      ],
    ),
  );
}

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

  @override
  Widget build(BuildContext context) => Scaffold(
        appBar: AppBar(title: const Text(App.title)),
        body: Center(
          child: Column(
            mainAxisAlignment: MainAxisAlignment.center,
            children: [
              ElevatedButton(
                onPressed: () => context.go('/page2'),
                child: const Text('Go to page 2'),
              ),
            ],
          ),
        ),
      );
}

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

  @override
  Widget build(BuildContext context) => Scaffold(
        appBar: AppBar(title: const Text(App.title)),
        body: Center(
          child: Column(
            mainAxisAlignment: MainAxisAlignment.center,
            children: [
              ElevatedButton(
                onPressed: () => context.go('/'),
                child: const Text('Go to home page'),
              ),
            ],
          ),
        ),
      );
}

https://user-images.githubusercontent.com/2568253/148506322-f2629617-4af6-46ff-bf7b-60cd7364c888.mov

[lulupointu]

@lulupointu I do not see any use of an Overlay widget in your https://github.com/lulupointu/go_router fork. What does the Overlay code look like?

I think it's mainly useful when building inside navigatorBuilder. For example if you build a scaffold with a bottom navigation bar inside navigatorBuilder, the bottom navigation bar will need an overlay. This is also shown in the first comment of this thread which put a Tooltip in navigatorBuilder.

If I just take the most basic go_router example and add a navigatorBuilder with the child wrapped with an Overlay, navigation still happens but the widget tree is not re-rendered

Yes, that's because Overlay is not declarative, you need to rebuild it manually. Basically you build it once and then you call _overlayEntry!.markNeedsBuild() whenever needed. Here is where I do it in my go_router implementation.

[csells]

@esDotDev @rydmike @lulupointu I have a branch that wraps an Overlay around the Navigator but it still doesn't quite solve all of the problems, e.g. there's no Navigator to use to show a dialog from the widgets created by the navigatorBuilder, which makes sense but is irritating.

On the other hand, it's easy enough to wrap the widget return by navigatorBuilder inside a single page Navigator (shown in the shared scaffold sample) and seems to solve all problems, including requiring no need to change the implementation of GR. Thoughts?

[esDotDev]

Right, nesting a Navigator with a single home page above the router is one way to get everything to work properly, but always felt pretty cludgy to me.

Usually I will just work around this by assigning a global key to the MaterialApp.navigatorKey, and pass that into my appManager/appBloc in order to show dialogs, pages and bottom sheets etc.

The issue with missing an Overlay though is particularly a pain, because many material widgets use ToolTips, so you pretty much always need it, and not having it just leads to various material widgets just blowing up. It's not as easy to workaround as the Navigator stuff.

So, tl;dr, kinda breaks down like:

• 2nd Overlay - required all the time (whether handled by 2nd Navigator, or an Overlay widget)
• 2nd Navigator - may be required, but there are other workarounds to this problem.

Though, to be fair, if you want full-screen dialogs and sheets that cover your scaffolding, there really is no choice but to use a 2nd Navigator, so I dunno... It's complicated!

[csells]

The use of a root Navigator fixes my Overlay problems. I assume because the former provides one of the latter.

[csells]

re: a 2nd navigator -- that's something a GR user can do.

re: a 2nd overlay -- that's only really something I can do for them, but I agree that it's a lighter weight solution to the problem.

If I could figure out how to call showDialog using your global navigator key trick, then that would be worth exploring in combo w/ 2nd overlay.

[esDotDev]

Should be able to just do something like: showDialog(_navKey.currentContext!); but again, that approach won't cover anything outside of the Navigator bounds, so in cases where you want full-screen dialogs to cover some scaffolding, you are basically forced to add a wrapping Navigator (which yes, does solve the Overlay issue as a side-effect).

In both cases the GR user can do this themselves by wrapping either an Overlay or a Navigator inside of the navigator builder (I think?), so this is really a quality of life thing. To what degree should things "just work".

My personal opinion is that GR adding a Navigator is a bit too much implicit behavior, and is not always needed, but adding an Overlay is probably a nice pragmatic thing to do, with no real downsides, and is virtually always needed.

[csells]

I don't actually think a GR user can fix the problem themselves with an Overlay; at least, I couldn't make that work. For the navigatorBuilder to get called with the right stuff, I had to integrate the Overlay deep into the delegate. On the other hand, the Navigator provides an Overlay itself, so it sounds like I just did it wrong.

[esDotDev]

I think you might be right, I expected this to work, but it's not rebuilding things properly, despite calling markNeedsBuild.

OverlayEntry? entry;
GoRouter goRouter = GoRouter(
  navigatorBuilder: (_, __, navigator) {
    entry ??= OverlayEntry(builder: (_)  => MyScaffold(body: navigator));
    entry!.markNeedsBuild();
    return Overlay(initialEntries: [entry!]);
  }

It's odd, breakpoints say the content is rebuilt, but the overlay is just never painted.

Wrapping Navigator is definitely most straightforward, and fixes all dialog and overlay issues, but it's unfortunately verbose:

GoRouter goRouter = GoRouter(
  navigatorBuilder: (_, __, navigator) {
    return Navigator(
      onPopPage: (_, __) => false,
      pages: [
        MaterialPage(
          child: MyScaffold(body: navigator),
        )
      ]);
    },
    routes: [ ... ]

Could have an addExternalNavigator option to GoRouter so we can get the nice default behavior out of the box, but it's not magic? The type of page shouldn't matter as I don't believe it is ever animated.

Then we'd get:

GoRouter goRouter = GoRouter(
  addExternalNavigator: true,
  navigatorBuilder: (_, __, navigator) => MyScaffold(body: navigator),
  routes: [ ... ]

Although, I guess we could just bury the Navigator inside of MyScaffold if we wanted, keeping the GoRouter definition more readable.