Ability to cancel a Navigation event

[MariovanZeist]

Added a LocationChanging event on the NavigationManager so users are able to cancel navigation events.

LocationChanging events can be canceled for calling NavigationManager.NavigateTo directly (except when setting the forceload flag),
Clicking an href element
Performing a history.back or history.forward function or clicking the back / forward history buttons in the browser.

For performance reasons and to minimize Server/browser interactions (Especially when using ServerSide Blazor)
the JS Runtime will only call into C# code when an event listener has been added otherwise every navigation event would incur an extra Client/Server roundtrip when presumably in most cases no Listener will be registered.

Addresses #23886

[mrpmorris]

Wow :)

[captainsafia]

Looks good overall! Left some feedback inline.

[MariovanZeist]

Api review and recommendations.

NavigateTo

When the forceLoad flag is true in the function

public void NavigateTo(string uri, bool forceLoad = false)

I have opted to not call the LocationChanging event, but I am not sure if that was a good decision.

The main use case I foresee for the LocationChanging event will be the "There are unsaved data on this page, do you want to save" scenarios.
In those cases, it might still be better to call the LocationChanging event, so the code can take some action.

I can think of 3 ways we could handle this scenario.

1. Leave as is. When the developer chooses to forceLoad, he/she probably doesn't want the LocationChanging to be fired.
2. Call LocationChanging but when it returns ForceLoad anyway. (So ignore the cancel)
3. Call LocationChanging but respect Cancel.

I personally lean towards option 3, because that gives the developer the most flexibility, but I can find good arguments for any of these options.

In Cases 2 and 3 we should Add the ForceLoad flag to the LocationChanging event, so the developer knows what was intended.
We might want to Combine this with my proposal in #25752 by using
NavigationOptions but that is just a thought

LocationChangingEventArgs

The current LocationChangingEventArgs Implementation is defined as follows

public class LocationChangingEventArgs : EventArgs
{
    public string Location { get; }
    public bool IsNavigationIntercepted { get; }
    public bool Cancel { get; set; }
}

This means that if multiple eventhandlers are registered an eventhandler could revert the Cancel by setting it to false again

We could change it to:

public class LocationChangingEventArgs : EventArgs
{
    public string Location { get; }
    public bool IsNavigationIntercepted { get; }
    public bool IsCanceled() { get; }
    public void Cancel()
}

In which case the Cancel will persist, and can't be reset back to false by subsequent eventhandlers.

[ShaunCurtis]

Forgive me for asking this (polite) question if I've misunderstood the above discussion. Why are you trying to modify NavigationManager? It's just the messenger, surfacing Javascript/HTML navigation events into Blazor. NavigationManager doesn't router/reload the page, Router does. So if you are looking to modify/manage router behaviour, why not do it in the router?

There's another issue here issues/14962 covering this subject and a link to some work I've done on it.

[MariovanZeist]

@ShaunCurtis This PR Adds a LocationChanging event, which is fired before A LocationChanged event.
It's added to the NavigationManager because that's the earliest opportunity to intercept and cancel the navigation request, without it ever firing a LocationChanged event. The Router reacts to LocationChanged events and that's too late to cancel the navigation without introducing some side effects, As at that point the only way to "cancel" a route change is to Navigate again to the previous page, so instead of not navigating, you end up Navigating twice.

Some of these Side effects include.

1. Multiple calls To LocationChanged, Users that Hook into the LocationChanged event will see these events which may trigger unwanted code. (example would be a call to an Api to retrieve some data)
2. Browser History state is compromised. Every time you cancel route navigation this way, 2 more entries are added to the browser history state.

So if you want to cancel a routechange because for example your model is dirty you could use something like this.

    public interface IPageState
    {
        bool IsDirty { get; }
    }

    public class CheckDirty : ComponentBase, IDisposable
    {
        [Inject] NavigationManager NavigationManager { get; set; }
        [Inject] IPageState PageState { get; set; }
        protected override void OnInitialized()
        {
            NavigationManager.LocationChanging += HandleLocationChanging;
        }

        private void HandleLocationChanging(object? sender, LocationChangingEventArgs e)
        {
            if (PageState.IsDirty)
            {
                e.Cancel = IsDirty;
            }
        }

        public void Dispose()
        {
            NavigationManager.LocationChanging -= HandleLocationChanging;
        }
    }

[mrpmorris]

@MariovanZeist I prefer args.Cancel () so another handler can't assume it's the only interested party and set it back to false.

[MariovanZeist]

@mrpmorris I can see the benefits of your proposal. I am going to add that to my Api Review section above, so as to see what other people think about it. The main reason I had for the Cancel being a settable property was consistency with other EventArgs. But that may be an inadequate justification.

[captainsafia]

Thanks for prepping this list of API review items, @MariovanZeist! We'll be doing an API review for this on Monday and will report back.

[ShaunCurtis]

Hi Mario,

Thank you for taking the time to reply. I had forgotten about the two trip issue to fix the displayed url - I shouldn't have as I had to write code to deal with it!. I agree with your logic. When you write code for the current framework you sometimes don't raise your perspective high enough to see the whole picture. What got me started on this subject was the whole issue of dirty model data, and what is and is not dirty a dirty model - there are other issues with the edit form controls.

On ForceLoad, I would respect cancel. I can see scenarios in teams where the main navigation controls, such as a left side nav bar are maintained by a junior programmer who sets ForceLoad because he doesn't know any better! If I have a dirty model in an edit form I don't want to exit without user confirmation. period.

[SteveSandersonMS]

Thanks for this, @MariovanZeist! This is really impressive. Thanks for doing the E2E tests as well!

---

Regarding this part: https://github.com/dotnet/aspnetcore/pull/24417/files#diff-b761f1cc117bcbb43cdf980fda82ee8dR157

That seems a bit problematic, both because it's discarding "forward" entries, and because it's pushing new things onto the history stack that mean the back/forward history will contain unexpected entries if navigation is later allowed.

There is a different way this could be approached. Instead of using history.pushState to revert, you could use history.go(delta). This allows the forward/back stack to be retained completely. Here's a sketch of how it could work: https://gist.github.com/SteveSandersonMS/1924c7c0c46a6434518665924b2a97b4. Do you think we should consider that kind of approach?

---

Besides that, about the forceLoad parameter. I see there's open discussion about whether force-loaded navigations should be cancellable or not. Rather than having to guess what developers want, would it work to include the "forceLoad" flag on the OnNavigating event args, so that developers can choose how to account for it in their own "block-or-not" logic?

[MariovanZeist]

@pranavkm Sorry I didn't see the Working label until I pushed some changes.
I have some other issues I am working on (implementing @SteveSandersonMS suggestions)
You want me to hold off on working on those changes for now?

[pranavkm]

Sorry, I accidentally marked a PR as working.

[MariovanZeist]

@captainsafia I am trying to create an E2E test to validate the fix I made at 82605a1

The test would be similar to the CanGoBackFromNotAComponent, But it needs an initialized Navigationmanager.
According to this remark in the above-mentioned tests:

aspnetcore/src/Components/test/E2ETest/Tests/RoutingTest.cs

Lines 350 to 353 in fe6a3fc

	// Now click back
	// Because of how the tests are structured with the router not appearing until the router
	// tests are selected, we can only observe the test selector being there, but this is enough
	// to show we did go back to the right place and the Blazor app started up

I am not sure if this is possible. Can you advise?

[captainsafia]

Thanks for your patience with this API review, @MariovanZeist!

We took a look at this and posted some ideas on how to improve this. Let me know if you have any follow-up questions or need help with any of the follow-on work.

[captainsafia]

Feedback from API review: instead of using a global currentUri tracker and leveraging pushState to go back, we can directly use history.back().

[MariovanZeist]

I am working on some other changes related to this, especially steves comment: #24417 (comment) There are some corner cases I need to consider to properly implement this, so it isn't a straightforward change. So this part is still a Work In Progress

[MariovanZeist]

As we want async/await capabilities when handling a LocationChange I had to modify this function to an async void (Yikes 🤔) so too not to introduce breaking changes to the current API Surface.
The best solution would be to not only change this function to an async Task , but to also Change void Navigationmanager.NavigateTo to Task Navigationmanager.NavigateTo.
So I would like some advice on how we would want to proceed. (The WebAssemblyNavigationManager has a similar approach )
I am aware I am not catching any exceptions for the moment, and thrown exceptions will basically be "eaten" I will change it when we have made a decision if we want to introduce breaking changes, or not. (or have another better option)

[captainsafia]

@MariovanZeist and I chatted a bit about the potential confusion that we would have in the framework if we were to merge this PR. We'd have two different concepts around navigation/location changing: the OnNavigateAsync attribute we added in the router as part of the lazy-loading work and the LocationChanging listener in the NavigationManager. I worry that having these two similar-yet-different concepts in the framework will be confusing to people.

We should figure out a way to reconcile the two concepts before merging this PR. @MariovanZeist Maybe we should head back to the original issue for this and hash out a design that accounts for:

• The distinction between navigation cancellation and a general location changing event
• What to do with the OnNavigateAsync method now that we're introducing this API

[ChristianWeyer]

Do we have any indication of whether this will make it into 6.0 or not? .cc @mkArtakMSFT

[javiercn]

@ChristianWeyer It's in our radar and I believe we plan to get it in, however currently the team is working on other bigger items (Hot reload, AoT, Blazor Desktop, etc.) so it might take us a bit until it makes it in.

[ChristianWeyer]

OK, thanks for the quick update. Much appreciated.

[captainsafia]

@ChristianWeyer Yep -- since this feature does have a fair bit of complexity we're taking some time to draft up a design doc (#28527) so that we've covered all bases before implementation.

[captainsafia]

Closing this until we've got the design squared away. We'll use this PR as a basis for discussions moving forward.