Fix NavMenu route/subroute not selecting correct item

[vnbaaij]

Addresses #743

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-pebble-0dc79cb03-755.westeurope.3.azurestaticapps.net

[tlmii]

Do you think there's any value in trying to only match if an entire portion of a route (between /s) match?

In other words, right now, if the new route is /MyRouteWithALongName and a menu item has an href of /MyRoute, then it'd end up selected. That is probably not what is expected?

[vnbaaij]

Yes, trying to match the portion between /'s would be better indeed. Although the example you give is a bit strange/far fetched perhaps. As this is a NavMenu, one can expect that the routes match what is in the menu more or less.

How do you think we can/should solve it? Add a "/" at the end of both localPath and x.Href in line 203?

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-pebble-0dc79cb03-755.westeurope.3.azurestaticapps.net

[tlmii]

Although the example you give is a bit strange/far fetched perhaps. As this is a NavMenu, one can expect that the routes match what is in the menu more or less.

That's what I get for trying to put the comment in on mobile :)

A better example would be if the menu had these hrefs:

/
/container
/containers

And then someone tried to go to /containers. The current code would find /container, where "/containers".StartsWith("/container") == true and it's the first one that matches, so it'd be selected.

How do you think we can/should solve it? Add a "/" at the end of both localPath and x.Href in line 203?

The two strategies I can think of are:

1. As you said, ensure that both paths end in / (don't add it if its already there) and do the exact startswith match you're doing now. We did this in HttpRepl for similar purposes (needing to compare the hierarchy of a route).
2. Split the two paths by the / char and do exact matches on the string segments that come from it. As long as all n segments in the menu item array match the first n segments in the navigation path, it matches.

In both cases, .LocalPath already strips off the query params, so you don't need to worry about that.

The first option is clearly easier, though I have a nagging feeling it isn't quite as exhaustive of a solution. Still, I'm leaning towards that, especially as a first attempt.

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-pebble-0dc79cb03-755.westeurope.3.azurestaticapps.net

[tlmii]

@vnbaaij After playing with this for a bit today I think there's a situation that we might have missed. I haven't fully debugged it yet. But what I think is happening is this:

1. Menu has two items (for simplicity): / and /logs
2. The user is on / and clicks a button that navigates them to /logs/abcd
3. The new changes in the menu system correctly notice that /logs is a proper subset of /logs/abcd so it selects that item
4. But the menu system also tries to activate that button, which in turn navigates the user to /logs.

I'm guessing there needs to be a change in HandleCurrentSelectedChangedAsync somewhere so that it doesn't (via TryActivateMenuItemAsync) call menuItem.ExecuteAsync (which in turn calls NavigationManager.NavigateTo(Href)). But I'm not exactly clear on what that change needs to be at the moment.

Thoughts?

[Narvalex]

After the last update I have a crash on line 216:

fluentui-blazor/src/Core/Components/NavMenu/FluentNavMenu.razor.cs

Line 216 in ba4bf12

menuItem = _allItems.Values.First(x => x.Href == "/");

I have a System.InvalidOperationException: Sequence contains no matching element
It happens every time I go to the "/" route. Is it just me?

[tlmii]

@Narvalex looking at where the error occurs, is it correct to assume you don't have a menu item with a / route in your menu?

[Narvalex]

I do have the "/" route. When page loads does not throw an errror, but after that, whenever I go to another route and then return to the "/" route that exception is thrown

[vnbaaij]

I do have the "/" route. When page loads does not throw an errror, but after that, whenever I go to another route and then return to the "/" route that exception is thrown

I'm not seeing that on the demo site...

[vnbaaij]

@vnbaaij After playing with this for a bit today I think there's a situation that we might have missed. I haven't fully debugged it yet. But what I think is happening is this:

1. Menu has two items (for simplicity): / and /logs
2. The user is on / and clicks a button that navigates them to /logs/abcd
3. The new changes in the menu system correctly notice that /logs is a proper subset of /logs/abcd so it selects that item
4. But the menu system also tries to activate that button, which in turn navigates the user to /logs.

I'm guessing there needs to be a change in HandleCurrentSelectedChangedAsync somewhere so that it doesn't (via TryActivateMenuItemAsync) call menuItem.ExecuteAsync (which in turn calls NavigationManager.NavigateTo(Href)). But I'm not exactly clear on what that change needs to be at the moment.

Thoughts?

I've added a private bool _hasRouteParameters = false to the class. In the HandleNavigationManagerLocationChanged method I set this variable to true when detect there is a (are) route parameter(s) (this is what I changed in the last fix). Then in TryActivateMenuItemAsync I added the variable to the check that determines if the ExecuteAsyc must be called:

if (!actionArgs.Handled && !_hasRouteParameters)
{
    await menuItem.ExecuteAsync(actionArgs);
}

if (actionArgs.Handled && !_hasRouteParameters)
{
    await menuItem.SetSelectedAsync(true);
}

This prevents the menu from 'redirecting' itself to itself again. Seems like a good fix?

[Narvalex]

I do have the "/" route. When page loads does not throw an errror, but after that, whenever I go to another route and then return to the "/" route that exception is thrown

I'm not seeing that on the demo site...

Finally, after a lot work, I was able to reproduce the error. I created the issue #789 for this. Please let me know if you need anything else, @vnbaaij .