dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.44k stars 10.03k forks source link

[Blazor] Navigating to a page will scroll to top before rendering new page #53863

Open danielchalmers opened 9 months ago

danielchalmers commented 9 months ago

Is there an existing issue for this?

Describe the bug

Using NavigationManager.NavigateTo to change pages while you are partially scrolled down the page will visibly and jarringly scroll to the top before navigating to the new page.

Expected Behavior

The user should keep seeing the same viewport until the new page has rendered.

I know there's going to be a technical reason for this, but it just looks so unpleasant and I don't know how to work around it.

Steps To Reproduce

This was done on .NET MAUI Blazor Hybrid with MudBlazor. If a repo is needed, please let me know and I'll happily go make one.

Exceptions (if any)

No response

.NET Version

8.0.100

Anything else?

You can see in this video that I scroll down then click a button to navigate the page which then scrolls back up before navigating to the new page.

https://github.com/dotnet/aspnetcore/assets/7112040/4722d3e4-431b-430f-a5b0-2334a056f6fc

It looks really bad for the user.

SteveSandersonMS commented 9 months ago

If a repo is needed, please let me know and I'll happily go make one.

Thanks, yes, I think we will need a repro (as a public GitHub repo) because that shouldn't happen by default. Please make sure the repro is minimal and doesn't use MudBlazor or other external libraries, so we can see it's a problem with Blazor itself.

danielchalmers commented 9 months ago

If a repo is needed, please let me know and I'll happily go make one.

Thanks, yes, I think we will need a repro (as a public GitHub repo) because that shouldn't happen by default. Please make sure the repro is minimal and doesn't use MudBlazor or other external libraries, so we can see it's a problem with Blazor itself.

@SteveSandersonMS

Hi, it's occurring before the OnInitializedAsync event.

https://github.com/dotnet/aspnetcore/assets/7112040/d7e7702c-3b2a-4551-aa8c-8b4867b6a922

(it pauses because it hits the breakpoint)

I must be doing a bit more work in my app because it's much harder to see on a fresh template without the breakpoint.

SteveSandersonMS commented 9 months ago

Thanks, that does explain it. I think this is a behavior that has always existed in Blazor since the beginning, though it requires a particular combination of things to happen in order to produce the UX weirdness you are observing.

Minimal repro

In an app with global interactivity:

  1. In page A which is big enough to be scrollable, add a button at the bottom that does programmatic navigation: <button @onclick="@(() => NavigationManager.NavigateTo("b"))">Do nav</button>
  2. In page B, cause it to block the CPU for a while on startup (e.g., in OnInitialized, add Thread.Sleep(1000)).
  3. Run the app and click the button in page A. Observe it scrolls to the top before page B is rendered.

Explanation

It's desirable to navigate to the top when the user moves to a new page. However, the client-side code doesn't really know which render corresponds to a "new page", so it uses the following heuristic:

This works correctly as long as, after each navigation is triggered, the next render is the one for the new page. And that's always the case given that navigation occurs synchronously between those two steps above, and that the new page always renders synchronously. It does not work if:

Workaround

You can work around this by suppressing the rendering of the old page when you know you're navigating away from it, e.g.:

<button @onclick="@DoNavigation">Do nav</button>

@code {
    bool suppressRender;

    protected override bool ShouldRender()
        => !suppressRender;

    private void DoNavigation()
    {
        suppressRender = true;
        NavigationManager.NavigateTo("/");
    }
}

Longer-term solution

We could consider having "scroll to top" be triggered by something inside Router that happens only after it knows it's completed a render after changing the current page component. There may be lots of subtleties that make this difficult, though.

danielchalmers commented 9 months ago

@SteveSandersonMS

Really appreciate the detailed the response. Weird thing is that the workaround works on my sample project but not on the production app. Even protected override bool ShouldRender() => false; seems to do nothing at all. Do you know what could cause that? I don't inherit from anything.

SteveSandersonMS commented 9 months ago

protected override bool ShouldRender() => false; will stop all except the first render from your component. I'm not sure of any reason why it could fail to have an effect.

However it's possible that your real-world app triggers other component renders besides the one you've suppressed rendering (i.e., in between when you do the navigation and when the new page renders itself). That would prevent the workaround from working.

danielchalmers commented 9 months ago

For future reference: The culprit was the MudBlazor button which probably triggers a render at the same time it calls OnClick->Navigate because you can see it scroll to the top even with ShouldRender() => false. You do not see the same with a regular <button>.

https://github.com/MudBlazor/MudBlazor/discussions/8151

Geccoka commented 8 months ago

I'm also using MudBlazor and had this issue too.

I've figured out a workaround that works in my application in a .NET7 Blazor Server app. For some reason that I don't understand, after I've put a NavigationLock with an empty OnBeforeInternalNavigation handler, the page only scrolled up after the new page has rendered, here is the solution.

Relevant part of App.razor:

<Router AppAssembly="@typeof(App).Assembly">
    <Found Context="routeData">
        ...
        <NavigationLock OnBeforeInternalNavigation="@EventCallback.Empty" />
    </Found>
    ...
</Router>

I've modified nothing else in my app.

danielchalmers commented 8 months ago

@Geccoka

Weird, I'm not getting the same result on MAUI Hybrid 8.0:

<Router AppAssembly="@typeof(MauiProgram).Assembly">
    <Found Context="routeData">
        <RouteView RouteData="@routeData" DefaultLayout="@typeof(MainLayout)" />
        @*<FocusOnNavigate RouteData="@routeData" Selector="h1" />*@

        @* https://github.com/dotnet/aspnetcore/issues/53863#issuecomment-1952669759 *@
        <NavigationLock OnBeforeInternalNavigation="@EventCallback.Empty" />
    </Found>
</Router>

or Blazor Server 7.0

<Router AppAssembly="@typeof(App).Assembly">
    <Found Context="routeData">
        <RouteView RouteData="@routeData" DefaultLayout="@typeof(MainLayout)" />
    @* https://github.com/dotnet/aspnetcore/issues/53863#issuecomment-1952669759 *@
    <NavigationLock OnBeforeInternalNavigation="@EventCallback.Empty" />
    </Found>
    <NotFound>
        <PageTitle>Not found</PageTitle>
        <LayoutView Layout="@typeof(MainLayout)">
            <p role="alert">Sorry, there's nothing at this address.</p>
        </LayoutView>
    </NotFound>
</Router>

Are you using NavigationManager.NavigateTo?

Geccoka commented 8 months ago

Yes, I'm using NavigationManager.NavigateTo, but after further testing I must say this workaround only solves the issue in certain cases. There are places in my app where this doesn't work :(. But putting an await Task.Delay(100) to the OnBeforeInternalNavigation handler solves it :). Ofc this will delay every navigation by 100ms.