Skip to content

scrollBehavior shouldn't use window.scrollTo #1189

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
TheDutchCoder opened this issue Nov 4, 2021 · 3 comments
Closed

scrollBehavior shouldn't use window.scrollTo #1189

TheDutchCoder opened this issue Nov 4, 2021 · 3 comments
Labels
enhancement New feature or request

Comments

@TheDutchCoder
Copy link

Version

4.0.12

Reproduction link

stackblitz.com

Steps to reproduce

In the demo (sorry for the rough CSS), click on the "About link". It has a hash and the router has a scrollBehavior function that will log 'woops'.

Nothing will happen, even though the router is told to scroll down 200 pixels. This is because internally, the router uses window.scrollTo, which isn't correct. It should use .scrollTo on whatever element hosts the router view.

The body is set to not scroll, but the internal router-view div is.

In the console, use window.scrollTo(0, 200) and nothing will happen.
In the console, use document.querySelector('#wat').scrollTo(0, 200) and the view will properly scroll.

I think the router should select the proper view first, before attempting to scroll.

What is expected?

The view to scroll to the given position

What is actually happening?

Nothing is scrolling because window.scrollTo doesn't work when the body isn't scrollable.


I'm not sure how difficult this is, because this is heavily CSS dependent, but I think it should be doable to select the closest router-view of the element requested to scroll to?

@LinusBorg
Copy link
Member

LinusBorg commented Nov 4, 2021

I think the router should select the proper view first, before attempting to scroll.

In your own provided example, the scrollable element is not the router-view itself, or the element that the router-view is nested in (<main>), it's the outermost container of App.vue, which is unrelaed to the "proper view".

Which serves as a nice demo that it's not really straightforward to figure out which element should be scrolled. add to that the fact that you could have scrollable elements nested within other scrollable elements, and it seems pretty obvious that we won't be able to make this auto-detectable without adding a bunch of overhead.

So this is not a bug, really. It's an implementation that only covers the default usecase, and leaves the rest to userland.

But we could talk bout how to open up the API to that one could tell the router which scroll parent to use (which still would not solve nested scrollables), and/or (I haven't tested this might already work) maybe just allow for this:

scrollBehavior(to, from, savedPosition) {
  scrollIntoView(document.querySelector(to.meta.scrollParent ?? 'body'))
}

@LinusBorg LinusBorg added the enhancement New feature or request label Nov 4, 2021
@TheDutchCoder
Copy link
Author

TheDutchCoder commented Nov 4, 2021

I think the router should select the proper view first, before attempting to scroll.

In your own provided example, the scrollable element is not the router-view itself, or the element that the router-view is nested in (<main>), it's the outermost container of App.vue, which is unrelaed to the "proper view".

Fair enough, it was a quickly throw together example, the same behavior happens when you actually do select a nested router-view's div.

The thing is, I think this part of the router is trying to mimic a native browser API, that actually still works, it just gets trapped by the router.

For example, if you inspect and edit the code and just add an anchor with href="#test" and assign id="test" to some element down in the view, then click the link, the browser scrolls just fine to that element.

I wonder if there could be a case where the router doesn't trap the hash at all and just let the browser take over? I haven't done a deep dive into the router's source, so I don't know what the options are here.

I like the idea of telling the router what element to use as the "parent". I was under the impression that already happened with { el: '#thing' } (which can just be any element).

@posva
Copy link
Member

posva commented Nov 4, 2021

Duplicate of vuejs/vue-router#1187

This needs to go through an RFC. One was open but it was too limited. I have a few ideas on mind but never had the time to put them on an RFC.

Note you can use an afterEach() hook to trigger the scroll manually and implement your own scrolling behaviour in the meantime.

Locking to avoid discussion forking.

@posva posva closed this as completed Nov 4, 2021
@vuejs vuejs locked and limited conversation to collaborators Nov 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants