Skip to content

Added additional boolean parameter "replace" to NavigationManager.NavigateTo #25752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion src/Components/Components/src/NavigationManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -84,18 +84,41 @@ protected set
}
}

/// <summary>
/// Navigates to the specified URI.
/// </summary>
/// <param name="uri">The destination URI. This can be absolute, or relative to the base URI
/// (as returned by <see cref="BaseUri"/>).</param>
public void NavigateTo(string uri)
=> NavigateTo(uri, forceLoad: false);

/// <summary>
/// Navigates to the specified URI.
/// </summary>
/// <param name="uri">The destination URI. This can be absolute, or relative to the base URI
/// (as returned by <see cref="BaseUri"/>).</param>
/// <param name="forceLoad">If true, bypasses client-side routing and forces the browser to load the new page from the server, whether or not the URI would normally be handled by the client-side router.</param>
public void NavigateTo(string uri, bool forceLoad = false)
public void NavigateTo(string uri, bool forceLoad)
{
AssertInitialized();

// For back-compatibility, we must call the (string, bool) overload of NavigateToCore from here,
// because that's the only overload guaranteed to be implemented in subclasses.
NavigateToCore(uri, forceLoad);
}

/// <summary>
/// Navigates to the specified URI.
/// </summary>
/// <param name="uri">The destination URI. This can be absolute, or relative to the base URI
/// (as returned by <see cref="BaseUri"/>).</param>
/// <param name="options">Provides additional <see cref="NavigationOptions"/>.</param>
public void NavigateTo(string uri, NavigationOptions options)
{
AssertInitialized();
NavigateToCore(uri, options);
}

/// <summary>
/// Navigates to the specified URI.
/// </summary>
Expand All @@ -104,6 +127,15 @@ public void NavigateTo(string uri, bool forceLoad = false)
/// <param name="forceLoad">If true, bypasses client-side routing and forces the browser to load the new page from the server, whether or not the URI would normally be handled by the client-side router.</param>
protected abstract void NavigateToCore(string uri, bool forceLoad);

/// <summary>
/// Navigates to the specified URI.
/// </summary>
/// <param name="uri">The destination URI. This can be absolute, or relative to the base URI
/// (as returned by <see cref="BaseUri"/>).</param>
/// <param name="options">Provides additional <see cref="NavigationOptions"/>.</param>
protected virtual void NavigateToCore(string uri, NavigationOptions options) =>
throw new NotImplementedException($"The type {GetType().FullName} does not support supplying {nameof(NavigationOptions)}. To add support, that type should override {nameof(NavigateToCore)}(string uri, {nameof(NavigationOptions)} options).");

/// <summary>
/// Called to initialize BaseURI and current URI before these values are used for the first time.
/// Override <see cref="EnsureInitialized" /> and call this method to dynamically calculate these values.
Expand Down
27 changes: 27 additions & 0 deletions src/Components/Components/src/NavigationOptions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;

namespace Microsoft.AspNetCore.Components
{
/// <summary>
/// Additional options for navigating to another URI
/// </summary>
[Flags]
public enum NavigationOptions
{
/// <summary>
/// Use default options
/// </summary>
None = 0,
/// <summary>
/// Bypasses client-side routing and forces the browser to load the new page from the server, whether or not the URI would normally be handled by the client-side router.
/// </summary>
ForceLoad = 1,
/// <summary>
/// Indicates that the current history entry should be replaced, instead of pushing the new uri onto the browser history stack.
/// </summary>
ReplaceHistoryEntry = 2
}
}
9 changes: 9 additions & 0 deletions src/Components/Components/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,14 @@ Microsoft.AspNetCore.Components.DynamicComponent.Parameters.set -> void
Microsoft.AspNetCore.Components.DynamicComponent.SetParametersAsync(Microsoft.AspNetCore.Components.ParameterView parameters) -> System.Threading.Tasks.Task!
Microsoft.AspNetCore.Components.DynamicComponent.Type.get -> System.Type!
Microsoft.AspNetCore.Components.DynamicComponent.Type.set -> void
Microsoft.AspNetCore.Components.NavigationManager.NavigateTo(string! uri) -> void
Microsoft.AspNetCore.Components.NavigationManager.NavigateTo(string! uri, Microsoft.AspNetCore.Components.NavigationOptions options) -> void
*REMOVED*Microsoft.AspNetCore.Components.NavigationManager.NavigateTo(string! uri, bool forceLoad = false) -> void
Microsoft.AspNetCore.Components.NavigationManager.NavigateTo(string! uri, bool forceLoad) -> void
Microsoft.AspNetCore.Components.NavigationOptions
Microsoft.AspNetCore.Components.NavigationOptions.ForceLoad = 1 -> Microsoft.AspNetCore.Components.NavigationOptions
Microsoft.AspNetCore.Components.NavigationOptions.None = 0 -> Microsoft.AspNetCore.Components.NavigationOptions
Microsoft.AspNetCore.Components.NavigationOptions.ReplaceHistoryEntry = 2 -> Microsoft.AspNetCore.Components.NavigationOptions
static Microsoft.AspNetCore.Components.ParameterView.FromDictionary(System.Collections.Generic.IDictionary<string!, object?>! parameters) -> Microsoft.AspNetCore.Components.ParameterView
virtual Microsoft.AspNetCore.Components.NavigationManager.NavigateToCore(string! uri, Microsoft.AspNetCore.Components.NavigationOptions options) -> void
virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.DispatchEventAsync(ulong eventHandlerId, Microsoft.AspNetCore.Components.RenderTree.EventFieldInfo? fieldInfo, System.EventArgs! eventArgs) -> System.Threading.Tasks.Task!
21 changes: 14 additions & 7 deletions src/Components/Server/src/Circuits/RemoteNavigationManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,28 +67,35 @@ public void NotifyLocationChanged(string uri, bool intercepted)
/// <inheritdoc />
protected override void NavigateToCore(string uri, bool forceLoad)
{
Log.RequestingNavigation(_logger, uri, forceLoad);
NavigateToCore(uri, forceLoad ? NavigationOptions.ForceLoad : NavigationOptions.None);
}

/// <inheritdoc />
protected override void NavigateToCore(string uri, NavigationOptions options)
{
Log.RequestingNavigation(_logger, uri, options);

if (_jsRuntime == null)
{
var absoluteUriString = ToAbsoluteUri(uri).ToString();
throw new NavigationException(absoluteUriString);
}

_jsRuntime.InvokeAsync<object>(Interop.NavigateTo, uri, forceLoad);
_jsRuntime.InvokeAsync<object>(Interop.NavigateTo, uri,
(options & NavigationOptions.ForceLoad) != 0,
(options & NavigationOptions.ReplaceHistoryEntry) != 0);
}

private static class Log
{
private static readonly Action<ILogger, string, bool, Exception> _requestingNavigation =
LoggerMessage.Define<string, bool>(LogLevel.Debug, new EventId(1, "RequestingNavigation"), "Requesting navigation to URI {Uri} with forceLoad={ForceLoad}");
private static readonly Action<ILogger, string, NavigationOptions, Exception> _requestingNavigation =
LoggerMessage.Define<string, NavigationOptions>(LogLevel.Debug, new EventId(1, "RequestingNavigation"), "Requesting navigation to URI {Uri} with options={options}");

private static readonly Action<ILogger, string, bool, Exception> _receivedLocationChangedNotification =
LoggerMessage.Define<string, bool>(LogLevel.Debug, new EventId(2, "ReceivedLocationChangedNotification"), "Received notification that the URI has changed to {Uri} with isIntercepted={IsIntercepted}");

public static void RequestingNavigation(ILogger logger, string uri, bool forceLoad)
public static void RequestingNavigation(ILogger logger, string uri, NavigationOptions options)
{
_requestingNavigation(logger, uri, forceLoad, null);
_requestingNavigation(logger, uri, options, null);
}

public static void ReceivedLocationChangedNotification(ILogger logger, string uri, bool isIntercepted)
Expand Down
2 changes: 1 addition & 1 deletion src/Components/Web.JS/dist/Release/blazor.server.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/Components/Web.JS/dist/Release/blazor.webassembly.js

Large diffs are not rendered by default.

8 changes: 6 additions & 2 deletions src/Components/Web.JS/src/Services/NavigationManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ export function navigateTo(uri: string, forceLoad: boolean, replace: boolean = f
history.replaceState(null, '', temporaryUri);
location.replace(uri);
} else if (replace){
history.replaceState(null, '', absoluteUri)
if (forceLoad) {
location.replace(uri);
} else {
history.replaceState(null, '', absoluteUri)
Copy link
Member

@SteveSandersonMS SteveSandersonMS Dec 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic looks wrong to me. What happens if isWithinBaseUriSpace(absoluteUri) is false, force is false, and replace is true?

What I'd expect is: since this is an external navigation, the force param is irrelevant, and the replace flag means we should be calling location.replace.

What the logic appears to do is call history.replaceState, which won't produce a useful behavior because it's only doing HTML5-style client-side navigation and won't trigger an actual external page load.

Here's the full truth table I'd expect (ignoring the Force-loading the same URL you're already on special case):

isWithinBaseUriSpace(absoluteUri) Force Replace Expected behavior
false false false location.href = url (append external page load to history)
false false true location.replace (replace with external page load on history)
false true false location.href = url (append external page load to history)
false true true location.replace (replace with external page load on history)
true false false performInternalNavigation(absoluteUri, false, false) (calls history.pushState)
true false true performInternalNavigation(absoluteUri, false, true) (calls history.replaceState)
true true false location.href = url (append external page load to history)
true true true location.replace (replace with external page load on history)

Do you agree with this list? Could you check how it corresponds to the implementation? I think the only combination that's currently wrong is the one I mentioned at the top of this comment.

I suspect that before this PR, the logic here was already wrong in that it didn't handle replace without also force. That was OK before because it wasn't really proper public API. But since this is becoming proper public API now, we should make sure all the combinations of parameters work correctly.

}
} else {
// It's either an external URL, or forceLoad is requested, so do a full page load
location.href = uri;
Expand All @@ -91,7 +95,7 @@ function performInternalNavigation(absoluteInternalHref: string, interceptedLink
// Since this was *not* triggered by a back/forward gesture (that goes through a different
// code path starting with a popstate event), we don't want to preserve the current scroll
// position, so reset it.
// To avoid ugly flickering effects, we don't want to change the scroll position until the
// To avoid ugly flickering effects, we don't want to change the scroll position until
// we render the new page. As a best approximation, wait until the next batch.
resetScrollAfterNextBatch();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,20 @@ public void SetLocation(string uri, bool isInterceptedLink)

/// <inheritdoc />
protected override void NavigateToCore(string uri, bool forceLoad)
{
NavigateToCore(uri, forceLoad ? NavigationOptions.ForceLoad : NavigationOptions.None);
}

/// <inheritdoc />
protected override void NavigateToCore(string uri, NavigationOptions options)
{
if (uri == null)
{
throw new ArgumentNullException(nameof(uri));
}

DefaultWebAssemblyJSRuntime.Instance.Invoke<object>(Interop.NavigateTo, uri, forceLoad);
DefaultWebAssemblyJSRuntime.Instance.Invoke<object>(Interop.NavigateTo, uri,
(options & NavigationOptions.ForceLoad) != 0,
(options & NavigationOptions.ReplaceHistoryEntry) != 0);
}
}
}
116 changes: 116 additions & 0 deletions src/Components/test/E2ETest/Tests/RoutingTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,122 @@ public void CanNavigateProgrammaticallyWithForceLoad()
});
}

[Fact]
public void CanNavigateProgrammaticallyValidateNoReplaceHistoryEntry()
{
//This test checks if default navigation does not replace Browser history entries
SetUrlViaPushState("/");

var app = Browser.MountTestComponent<TestRouter>();
var testSelector = Browser.WaitUntilTestSelectorReady();

app.FindElement(By.LinkText("TestHistory")).Click();

Browser.True(() => Browser.Url.EndsWith("/TestHistory", StringComparison.Ordinal));
Browser.Equal("This is the test history page.", () => app.FindElement(By.Id("test-info")).Text);


//We navigate to the /Other page
//This will also test our new NavigatTo(string uri) overload (it should not replace the browser history)
app.FindElement(By.Id("do-other-navigation")).Click();
Browser.True(() => Browser.Url.EndsWith("/Other", StringComparison.Ordinal));
AssertHighlightedLinks("Other", "Other with base-relative URL (matches all)");

Browser.Navigate().Back();
//After we press back, we should end up at the "/TestHistory" page so we know browser history has not been replaced
//If history would have been replaced we would have ended up at the "/" page
Browser.True(() => Browser.Url.EndsWith("/TestHistory", StringComparison.Ordinal));
AssertHighlightedLinks("TestHistory");

//For completeness, we will test if the normal NavigateTo(string uri, bool forceLoad) overload will also NOT change the browsers history
//So we basically repeat what we have done above
app.FindElement(By.Id("do-other-navigation2")).Click();
Browser.True(() => Browser.Url.EndsWith("/Other", StringComparison.Ordinal));
AssertHighlightedLinks("Other", "Other with base-relative URL (matches all)");

Browser.Navigate().Back();
Browser.True(() => Browser.Url.EndsWith("/TestHistory", StringComparison.Ordinal));
AssertHighlightedLinks("TestHistory");

// Because this was client-side navigation, we didn't lose the state in the test selector
Assert.Equal(typeof(TestRouter).FullName, testSelector.SelectedOption.GetAttribute("value"));

app.FindElement(By.Id("do-other-navigation-forced")).Click();
Browser.True(() => Browser.Url.EndsWith("/Other", StringComparison.Ordinal));

//We check if we had a force load
Assert.Throws<StaleElementReferenceException>(() =>
{
testSelector.SelectedOption.GetAttribute("value");
});

////But still we should be able to navigate back, and end up at the "/TestHistory" page
Browser.Navigate().Back();
Browser.True(() => Browser.Url.EndsWith("/TestHistory", StringComparison.Ordinal));
Browser.WaitUntilTestSelectorReady();
}


Comment on lines +464 to +465
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

[Fact]
public void CanNavigateProgrammaticallyWithReplaceHistoryEntry()
{
SetUrlViaPushState("/");

var app = Browser.MountTestComponent<TestRouter>();
var testSelector = Browser.WaitUntilTestSelectorReady();

app.FindElement(By.LinkText("TestHistory")).Click();

Browser.True(() => Browser.Url.EndsWith("/TestHistory", StringComparison.Ordinal));
Browser.Equal("This is the test history page.", () => app.FindElement(By.Id("test-info")).Text);


//We navigate to the /Other page, with replacehistroyentry enabled
app.FindElement(By.Id("do-other-navigation-replacehistoryentry")).Click();
Browser.True(() => Browser.Url.EndsWith("/Other", StringComparison.Ordinal));
AssertHighlightedLinks("Other", "Other with base-relative URL (matches all)");

Browser.Navigate().Back();
//After we press back, we should end up at the "/" page so we know browser history has been replaced
//If history would not have been replaced we would have ended up at the "/TestHistory" page
Browser.True(() => Browser.Url.EndsWith("/", StringComparison.Ordinal));
AssertHighlightedLinks("Default (matches all)", "Default with base-relative URL (matches all)");

// Because this was all with client-side navigation, we didn't lose the state in the test selector
Assert.Equal(typeof(TestRouter).FullName, testSelector.SelectedOption.GetAttribute("value"));
}

[Fact]
public void CanNavigateProgrammaticallyWithForceLoadAndReplaceHistoryEntry()
{
SetUrlViaPushState("/");

var app = Browser.MountTestComponent<TestRouter>();
var testSelector = Browser.WaitUntilTestSelectorReady();

app.FindElement(By.LinkText("TestHistory")).Click();

Browser.True(() => Browser.Url.EndsWith("/TestHistory", StringComparison.Ordinal));
Browser.Equal("This is the test history page.", () => app.FindElement(By.Id("test-info")).Text);

//We navigate to the /Other page, with replacehistroyentry and forceload enabled
app.FindElement(By.Id("do-other-navigation-forced-replacehistoryentry")).Click();
Browser.True(() => Browser.Url.EndsWith("/Other", StringComparison.Ordinal));

//We check if we had a force load
Assert.Throws<StaleElementReferenceException>(() =>
{
testSelector.SelectedOption.GetAttribute("value");
});

Browser.Navigate().Back();
//After we press back, we should end up at the "/" page so we know browser history has been replaced
Browser.True(() => Browser.Url.EndsWith("/", StringComparison.Ordinal));
Browser.WaitUntilTestSelectorReady();
}



[Fact]
public void ClickingAnchorWithNoHrefShouldNotNavigate()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<li><NavLink href="Other" Match=NavLinkMatch.All>Other with base-relative URL (matches all)</NavLink></li>
<li><NavLink href="/subdir/other?abc=123">Other with query</NavLink></li>
<li><NavLink href="/subdir/Other#blah">Other with hash</NavLink></li>
<li><NavLink href="/subdir/TestHistory">TestHistory</NavLink></li>
<li><NavLink href="/subdir/WithParameters/name/Abc">With parameters</NavLink></li>
<li><NavLink href="/subdir/WithParameters/Name/Abc/LastName/McDef">With more parameters</NavLink></li>
<li><NavLink href="/subdir/LongPage1">Long page 1</NavLink></li>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
@page "/TestHistory"
@inject NavigationManager NavigationManager

<div id="test-info">This is the test history page.</div>

<button id="do-other-navigation" @onclick=@(_ => NavigationManager.NavigateTo("Other"))>
Programmatic navigation (new overload)
</button>

<button id="do-other-navigation2" @onclick=@(_ => NavigationManager.NavigateTo("Other", false))>
Programmatic navigation
</button>

<button id="do-other-navigation-forced" @onclick=@(_ => NavigationManager.NavigateTo("Other", true))>
Programmatic navigation with force-load
</button>

<button id="do-other-navigation-replacehistoryentry" @onclick=@(_ => NavigationManager.NavigateTo("Other", NavigationOptions.ReplaceHistoryEntry))>
Programmatic navigation with history replace
</button>

<button id="do-other-navigation-forced-replacehistoryentry" @onclick=@(_ => NavigationManager.NavigateTo("Other", NavigationOptions.ReplaceHistoryEntry | NavigationOptions.ForceLoad))>
Programmatic navigation with force-load and history replace
</button>