[Request] In-app Prompt/onLeave confirmation

[cee-chen]

Describe the feature:

The current AppMountParameters.onAppLeave API only works in the case where user leaves the entire app, but it doesn't handle a case where a user is navigating within the app to another page.

Describe a specific use case for the feature:

In the Enterprise Search app, we have several pages with complex forms/data where we use React Router Prompt component to prevent users from navigating away from the page without saving changes. We specifically need the ability to confirm/prevent navigation for users still within our app if possible.

CC @yakhinvadim, @scottybollinger - feel free to include more details if I've missed any!

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[pgayvallet]

For the curious:

• Prompt - https://github.com/ReactTraining/react-router/blob/master/packages/react-router/modules/Prompt.js
• History (who the heck codes that way?) - https://github.com/ReactTraining/history/blob/master/packages/history/index.ts

So, after taking a quick look, I think we have two options here:

Adapt AppMountParameters.onAppLeave to support intra-app navigation

We could just enhance our existing API to allow to (optionally) block intra-app navigation.

The way onAppLeave works currently is simply by 'intercepting' navigateToApp and handle the 'should we navigate' logic here:

kibana/src/core/public/application/application_service.tsx

Lines 208 to 216 in 4584a8b

	const navigateToApp: InternalApplicationStart['navigateToApp'] = async (
	appId,
	{ path, state, replace = false }: NavigateToAppOptions = {}
	) => {
	const currentAppId = this.currentAppId$.value;
	const navigatingToSameApp = currentAppId === appId;
	const shouldNavigate = navigatingToSameApp
	? true
	: await this.shouldNavigate(overlays, appId);

We could adapt AppLeaveHandler to support this use case.

type AppLeaveHandler = (
  factory: AppLeaveActionFactory,
  nextAppId?: string
) => AppLeaveAction;
// =>
type AppLeaveHandler = (
  factory: AppLeaveActionFactory,
  nextAppId?: string,
  isSameApp: boolean
) => AppLeaveAction;

Changing AppLeaveHandler would be a breaking change, but the implementation seems way better, even if it means renaming a few things (AppLeaveHandler would probably become something like BeforeNavigationHandler) and adapting the (few) existing consumers to handle this new isSameApp parameter

The main problem is, this approach would only work with navigations based on application.navigateToApp. Even if this is the recommended way to navigate, and if <RedirectAppLinks> uses it under the hood, it would not work with applications using plain react-router-dom's <Link> component for example, as those just use history.push

Pros:

• Better UX. we're capable of displaying a real modal instead of just relying on the beforeunload trick prompting an ugly alert.
• We're enhancing our API instead of introducing a new one

Cons:

• Only work with navigateToApp based navigation.

Implement ScopedHistory.block

This API is currently unimplemented because we thought that AppMountParameters.onAppLeave was answering all use cases regarding navigation blocking. Of course we didn't think about intra-app.

We could just implement this API. I did some tests, and I don't think it would be harder than (famous last words) just calling parentHistory.block while keeping a reference of the unregistration callbacks, and perform the necessary cleanup in setupHistoryListener when we tear down the history because the user navigated out of scope.

kibana/src/core/public/application/scoped_history.ts

Lines 291 to 297 in 4584a8b

	unlisten();
	this.isActive = false;
	return;
	}
	/**
	* Track location keys using the same algorithm the browser uses internally.

If we were to do that, we have two options:

• ScopedHistory.block could block all kind of navigations
• ScopedHistory.block could only block intra-app navigation, and we would still rely on onAppLeave to block external navigation

Even if option a) would mean that we have two APIs doing (partially) the same thing, I think that option b) is just terrible developer experience, as both APIs needs to be called and refreshed/updated, especially as the blocking logic may differ from one app page to the other.

Note that one option could be to create our own Prompt component to support both use cases. It could look like

<OurPrompt
   history={history}
   onAppLeave={onAppLeave}
   message="Are you sure..."
/>

However react-history's Prompt uses the router context to avoid explicitly passing the history instance. We could also provide such context if we want to...

<AppLeaveContext history={history} onAppLeave={onAppLeave} />
   // down the tree
   <OurPrompt  message="Are you sure..." />
</AppLeaveContext>

Not sure if this isn't just over-engineering though. If we do implement ScopedHistory.block, I would just go with block all kind of navigations

Overall

Overall, I'm mixed:

I don't really see any valid reason to not implement ScopedHistory.block, as long as we perform the necessary cleanup by tearing down the block handlers when the history gets out of scope. So I think we may want to do that anyway. Also, this is the most resilient approach, as it will handle all history-based navigations, not only the ones triggered via navigateToApp

I also think that the onAppLeave/AppLeaveHandler solution is a better end user experience (default confirm modal in 2021... :feelsbadmad: ) But navigation blocking is still a niche / edge case scenario so I guess we should go with the safest option for now.

@joshdover wdyt?

[joshdover]

I don't really see any valid reason to not implement ScopedHistory.block, as long as we perform the necessary cleanup by tearing down the block handlers when the history gets out of scope. So I think we may want to do that anyway. Also, this is the most resilient approach, as it will handle all history-based navigations, not only the ones triggered via navigateToApp

I lean pretty heavily towards the implement ScopedHistory.block option with the block all kind of navigations behavior for these reasons:

• The resiliency / robustness of being sure that this API covers all navigation scenarios
• The simplicity of both the implementation in Core and the API for developers

I also think that the onAppLeave/AppLeaveHandler solution is a better end user experience (default confirm modal in 2021... :feelsbadmad: ) But navigation blocking is still a niche / edge case scenario so I guess we should go with the safest option for now.

Is this a limitation of the history.block API or just the <Prompt /> component from react-router? It seems that history.block is flexible enough to allow us to implement an enhanced <KibanaPrompt /> component to render a modal for in-page navigations and fallback to onbeforeunload.

[pgayvallet]

Is this a limitation of the history.block API or just the component from react-router? It seems that history.block is flexible enough to allow us to implement an enhanced

history implementation is fully synchronous, see allowTx in https://github.com/ReactTraining/history/blob/master/packages/history/index.ts. <Prompt> just calls history.block when mounted and the returned unregister callback when unmounted. I don't think we could show a real UI component (or any kind of asynchronous confirmation) without forking/recoding the whole history implementation, which doesn't feel like something we may want to do.

[joshdover]

history implementation is fully synchronous

FYI we are still on v4.x which has quite a bit different implementation: https://github.com/ReactTraining/history/blob/v4/modules/createBrowserHistory.js#L284.

It appears to support callbacks via the getUserConfirmation config option that can be supplied when calling createBrowserHistory. I believe we could leverage this to render a prettier UI?

[pgayvallet]

Oh, I missed that. If that works, it's actually quite great

[pgayvallet]

So, it does work! Just need to cleanup the ScopedHistory.block implementation now.