Skip to content
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

Hash routing to named element #47320

Merged
merged 37 commits into from
Apr 6, 2023
Merged

Hash routing to named element #47320

merged 37 commits into from
Apr 6, 2023

Conversation

surayya-MS
Copy link
Member

@surayya-MS surayya-MS commented Mar 20, 2023

Hash routing to named element

Description

Implemented navigation to the named element via anchor element or Navlink component with href containing '#' or using NavigationManager to navigate to url containing '#'.

<a href="#test1">Anchor to test1</a>
<NavLink href="#test1">NavLink to test1</NavLink>
<a href="/counter#test2">Anchor to test1</a>

There are 2 scenarios:

  1. Scrolling to the element on the same page

    • Anchor element (or NavLink component which renders as anchor) with href starting with '#'
    • Anchor with href with the same url but with changed hash

    For this scenario I changed NavigationManager.ts performInternalNavigation which is called in the method that overrides default behavior of anchor elements to handle this scenario too.

  2. Internal navigation

    • Anchor element with href that contains '#' linking to another page
    • using NavigationManager to navigate to another page or even the same page

    For this scenario I changed Router OnAfterRenderAsync to check the url and if it contains '#' then call a js script to scroll to the element.

Also I had to change behavior of FocusOnNavigate component because the page would flicker - scroll to the named element and then scroll to the FocusOnNavigate Selector. The ts script that FocusOnNavigate uses now prevents scrolling when focusing on the selector.

Fixes #8393

@surayya-MS surayya-MS requested a review from a team as a code owner March 20, 2023 14:56
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Mar 20, 2023
@surayya-MS surayya-MS requested a review from javiercn March 20, 2023 14:57
@surayya-MS surayya-MS enabled auto-merge (squash) March 20, 2023 16:35
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

The change looks reasonable, but I think we need E2E tests for it, and we need to address the issue with the layering, as we don't want to add a reference to JS interop directly to Microsoft.AspNetCore.Components

@surayya-MS surayya-MS requested a review from javiercn March 27, 2023 14:07
@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Mar 28, 2023

I think there's also a third scenario we should consider. Or at least, a variation of scenario 2 in the PR description:

  1. (B) Internal navigation with async data loading
    • Anchor tag goes to chat#message1234
    • The chat page renders a series of tags like <a name="message1234">...</a> but they are rendered asychronously because of some loading process that occurs first

The question is, how long is the async delay allowed to be? How do we know when to stop waiting? In the existing implementation in this PR, it has to be zero delay - we only support navigating to anchors that exist synchronously after the navigation. But I think in a very large number of cases, people have no choice but to load stuff asynchronously. At the same time, we don't want to wait indefinitely for the anchor tag to be added, since maybe the user has already started doing unrelated scrolling and will be inconvenienced by the scroll position changing unexpectedly later.

Scenarios like this make me think that a purely client-side implementation might be simpler and more flexible. That is:

  • When the JS-side code intercepts a navigation, if it contains a #hash, it stores that and sets some kind of timer or other cancellable token (and on all navigations, cancel any prior pending state)
  • After each renderbatch is applied, the JS-side code checks if there is a pending #hash and if so, tries to find it in the page.
    • If it's found, scroll to that point and clear all this temporary #hash state away
    • If not, just keep waiting
  • If the timer expires, clear the temporary #hash state
  • Also, during the initial page startup process, if the initial URL contains a #hash, set up the equivalent temporary state

Then none of the .NET-side changes would be necessary. This is at least more general than the existing solution, at the cost that while a #hash is pending we have to search the DOM for it after every renderbatch (which I think is OK because this doesn't happen often and in most cases it would stop after the next renderbatch).

The one issue is extreme cases where it takes, say, 20+ seconds for the async loading process to complete. We probably don't want to wait that long. So we'd have to accept the limitation that scrolling to the hash is not guaranteed and will only happen if you render the corresponding content within, say, 5 seconds. This does get us into the realm of making subjective judgements about UX but I think that's unavoidable.

What do you think? Was this sort of approach considered already?

@surayya-MS
Copy link
Member Author

I will add more e2e tests soon

surayya-MS and others added 2 commits April 5, 2023 17:00
suppress scrolling when Router re-renders;
add more e2e tests
@surayya-MS
Copy link
Member Author

As was discussed, we decided to go with the previous design where scrolling happens in Router.OnAfterRenderAsync. Thanks @SteveSandersonMS for coming up with the solution to suppress scrolling when Router re-renders.

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

This is really great! Thanks so much for continuing to sort out all the details and being flexible about the overall approach.

I would have approved this now (since this is 99% done) but just noticed that auto-merge is enabled. There are just a few minor details to resolve as per the other comments, and so I don't want to cause this to merge too soon 😄

@surayya-MS surayya-MS disabled auto-merge April 6, 2023 14:45
@SteveSandersonMS SteveSandersonMS self-requested a review April 6, 2023 14:46
@SteveSandersonMS
Copy link
Member

Also, superb work on the E2E tests. Those look really good.

surayya-MS and others added 3 commits April 6, 2023 16:51
Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
2. Prevent scrolling in focusOnNavigate
3. Removed test that checks scrolling to a named element with FocusOnNavigate
@surayya-MS surayya-MS enabled auto-merge (squash) April 6, 2023 16:20
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Great!

@surayya-MS surayya-MS merged commit 85573e6 into dotnet:main Apr 6, 2023
@ghost ghost added this to the 8.0-preview4 milestone Apr 6, 2023
@surayya-MS surayya-MS deleted the hashNav branch April 6, 2023 19:57
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.

Hash routing to named element
4 participants