Ignore query/fragment in ShouldMatch of NavLink by default but allow overriding ShouldMatch

[ilonatommy]

Breaking change to the behavior of ShouldMatch method in NavLink

Description

Appending a query to home url that is marked as NavLink.All in the navigation layout, resulted in un-selecting it in the navigation tab. This was reported as confusing for the users.
In #32168 a property to skip query in the uri comparison was proposed, but it does not solve such cases as appended fragments etc. After the discussion (see below), it was decided that the original intention of NavLink.All wasn't matching queries and we should start ignoring them. This may break a possibly few apps that relied on the previous behavior, so we are exposing ShouldMatch for inheritance. Users can create their own version of NavLink, e.g. NavLinkIgnoreQueryAndFragmentString (see tests).

Fixes #31312.

[ilonatommy]

CanFollowLinkToDefaultPageWithHash_NoTrailingSlash and similar are already testing both: queries and fragments with expected behavior. A better approach would be to follow the design from #32168 (comment).

The option of exposing ShouldMatch for inheritance looks more suitable - queries are not the only cases we could choose to ignore, there are also fragments and that might not be the end of the possibilities.

[javiercn]

Overall looks great.

Some comments:

• I'm not opposed to us exposing ShouldMatch, but I'm not sure how much it's needed.
• I believe that we should fix this in NavLink directly.
  • I find hard to believe that relying on not matching on the query string is a behavior people will have taken a dependency on, especially since the way we compare is extremely unpredictable, as the query string parameters need to be on the same exact order every time.
  • At most we can consider an AppCompatSwitch to revert to the old behavior

@SteveSandersonMS in case you have strong opinions about it. I think the original intent of Match.Prefix and Match.All was to match based on the path, and that we didn't account for the query string or the hash at the time.

It doesn't make sense to do matching on exact urls on something containing a query string because query string parameters are not ordered
So for example, /a/b?c=d&e=f is the same url as /a/b?e=f&c=d, it doesn't make sense that if the link is for the first url, it doesn't match the second

[SteveSandersonMS]

@SteveSandersonMS in case you have strong opinions about it. I think the original intent of Match.Prefix and Match.All was to match based on the path, and that we didn't account for the query string or the hash at the time.

The original intent was focused on the case where you have links to

• /
• /something

... and you want:

• / to be treated as the current URL only when you're at the URL / (so you'd use Match.All)
• /something to be treated as the current URL when you're at /something or /something/{id} (so you'd use Match.Prefix)

Treating Match.All as including the query/hash, as we do today, isn't ideal for this case. It would be unusual to want a link to /?page=2 not to highlight the link to /. So the proposed new behavior of ignoring the query/hash seems like an improvement in this specific (and common) case.

There is possibly a case where the change would be bad, e.g., if you had a tabview with links like:

• /mypage?tab=first
• /mypage?tab=second

Now if <NavLink> ignores querystring by default, you'd lose highlighting on the links, and would have to subclass NavLink to get the correct highlighting back. At least I think so - please correct me if I've got this wrong. BTW I have built URL/link structures like this in Blazor before but can't estimate how common it is externally.

If we were building things from scratch here I'd be comfortable defaulting to "ignore query/hash" and force people who want distinguishable links to ?tab=first and ?tab=second to do extra work. However, taking this as a breaking change is a harder call to make, even with an appcompat switch. I think it's reasonable for you, @lewing, @ilonatommy etc to make that call (and if you do choose to take the breaking change, just ensure it's called out through whatever process we advertise such breaking changes).

[javiercn]

@SteveSandersonMS thanks for the input.

I think we should go ahead and make the change now with a breaking change announcement and if we want to be extra careful, an app compat switch.

I think it'll be more common for people to trip into the current behavior than for the second case to apply.