Skip to content

Fix 'back' button following navigation to external URL. Fixes #10732 #10839

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

Merged
merged 1 commit into from
Jun 6, 2019

Conversation

SteveSandersonMS
Copy link
Member

Reported here: #10732 (comment)

This issue first appeared in preview 6 via #10062

It particularly affects the new auth UIs in preview 6, since many of them involve navigating to external URLs. Example: User clicks 'log in', goes to login screen, presses 'back' because they don't really want to log in, nothing happens.

Technical details

The way we handle external navigations is:

  1. User clicks a link. We don't yet know whether the target URL is external or not, so we ask the router to handle it.
  2. Router receives the URL, decides it's external, and issues a JS interop call to do a full-page load of the target URL
  3. Browser navigates to target URL

This leads to browser-specific behavioral differences:

  • On Chrome, it does do a full-page load, so it initially seems OK. But it doesn't replace the current history entry, which may be flagged as a client-side navigation. Then if the user clicks 'back', it tries to do a client-side navigation back.
  • On Firefox, it does do a full-page load, and it replaces the history entry with one that's not flagged as client-side navigation. Then if the user clicks back, it does a full-page load to go backwards.

Between these, Firefox's behavior is more useful for our scenario, because:

  • If the developer wants to force-load, that implies they really want to tear down all the client-side state. As such it doesn't make sense that a subsequent 'back' should not also cross the same state barrier and restart the app.
  • If the current URL isn't even part of the SPA (e.g., it's some other non-SPA page on the same domain), then client-side navigation will break the 'back' button completely, as the non-SPA page won't even handle onpopstate. This is what was reported as issue Server Side Blazor - Links to razor pages don't work correctly #10732.

To handle this, the only cross-browser-consistent mechanism I've found is to replace the current history entry with a bogus one (literally any relative URL), then use location.replace to replace that with the real one. Since it's no longer the same URL you're already on, it creates a new history entry flagged as server-side navigation, and since you use location.replace, the bogus one is eliminated from history.

Further, to avoid too much visible weirdness in the address bar, we can choose a 'bogus' URL that looks almost identical to the real one, e.g., by appending a ? character. This never gets sent to the server nor triggers client-side navigation, so it doesn't matter what the exact value is.

// triggering browser-specific behavior issues.
// For details about what this fixes and why, see https://github.com/aspnet/AspNetCore/pull/10839
const temporaryUri = uri + '?';
history.replaceState(null, '', temporaryUri);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Does this run in to the same issue if you hit go back + forward + back?

Copy link
Member Author

Choose a reason for hiding this comment

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

In my experiments, that sequence of actions worked fine after this fix.

The fix causes the temporary client-side history entry to get replaced with a regular server-side one, so all back-or-forward sequences cause a full page load when you go across that barrier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I've only been trying this on desktop browsers, and only in the specific case where it's about navigation to an external URL.

I don't think it's likely to cause regressions in any other case, since the new code path only applies when you're doing a force-load to the same URL you're already on, which is extremely unlikely to be something you're doing in any other case/

@pranavkm
Copy link
Contributor

pranavkm commented Jun 4, 2019

Would we want to address this in preview6? We could do a QB bar check at the very least (cc @danroth27 \ @mkArtakMSFT)

@SteveSandersonMS
Copy link
Member Author

I agree this definitely warrants consideration for inclusion in preview 6, hence getting this PR ready immediately.

@Eilon Eilon added the area-blazor Includes: Blazor, Razor Components label Jun 4, 2019
@SteveSandersonMS SteveSandersonMS changed the base branch from master to release/3.0-preview6 June 5, 2019 09:52
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/fix-back-10732 branch from 6c528ac to dbbfe60 Compare June 5, 2019 09:57
@javiercn javiercn force-pushed the stevesa/fix-back-10732 branch 2 times, most recently from 90baf32 to 056db04 Compare June 5, 2019 22:07
@natemcmaster
Copy link
Contributor

FYI @wtgodbe @mmitche here is another aspnet P6 QB-approved change.

@javiercn @SteveSandersonMS Ping me when PR checks are green and I can merge.

@wtgodbe
Copy link
Member

wtgodbe commented Jun 5, 2019

Code check failed:

error : Generated code is not up to date in src/Components/Browser.JS/dist/Debug/blazor.server.js. You might need to regenerate the reference assemblies or project list (see docs/ReferenceAssemblies.md and docs/ReferenceResolution.md) [F:\workspace_work\1\s\src\Components\Browser.JS\dist\Debug\blazor.server.js]

@SteveSandersonMS @natemcmaster @javiercn what's needed to fix this?

@javiercn
Copy link
Member

javiercn commented Jun 5, 2019

Updating some baseline. I'm running the code right now

@javiercn javiercn force-pushed the stevesa/fix-back-10732 branch from 056db04 to 0619bd1 Compare June 5, 2019 23:02
@mkArtakMSFT mkArtakMSFT merged commit 3697b47 into release/3.0-preview6 Jun 6, 2019
@ghost ghost deleted the stevesa/fix-back-10732 branch June 6, 2019 00:43
@mkArtakMSFT
Copy link
Contributor

@natemcmaster we're good to go!

@guidevnet
Copy link

Is #10842 fixed with this change?

@SteveSandersonMS
Copy link
Member Author

@guidevnet No, that's unrelated.

@guidevnet
Copy link

@SteveSandersonMS Ok, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants