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

Anchor links appear at bottom of page instead of top #400

Closed
tao opened this issue Sep 18, 2021 · 8 comments · Fixed by #476
Closed

Anchor links appear at bottom of page instead of top #400

tao opened this issue Sep 18, 2021 · 8 comments · Fixed by #476

Comments

@tao
Copy link

tao commented Sep 18, 2021

I found that on Firefox / Firefox mobile / Chrome that when I use an anchor link, sometimes it loads it at the bottom of the page instead of the top.

--------------
| #anchor    |   <-- expected position after page load
|            |
|            |
|            |
|            |
--------------
--------------
|            |
|            |
|            |
|            |
| #anchor    |   <-- the anchor appears at the bottom instead
--------------

I believe I've narrowed this down to the following css:

        scroll-behavior: smooth;

If I remove this scroll-behavior then the issue disappears but my app requires smooth scroll so I don't disorientate the user when jumping between paragraphs.

My pages are long articles with anchors for each paragraph and they are numbered starting at anchor#1 up to anchor#40 for example.

If I am at anchor#3 and click on a higher number like anchor#15 it'll scroll down and put that anchor at the bottom of the page... so it must be scrolling until the anchor is in-screen but not to the top of the viewport.

If I am at anchor anchor#35 for example, and click a lower number like anchor#4 then it looks like it scrolls up the page and puts the anchor at the top of the page because it's higher up. This must be because the anchor only appears into view at the top of the page and then it stops. This bug happens when you load new pages, but not when jumping between paragraphs on the same page.

So I only found the problem going from lower numbers like anchor#3 to a higher one like anchor#15 where the next link has a much higher Y position on the new page. As soon as the anchor appears at the bottom of the page it must stop the scroll calculation and leave the anchor at the bottom instead of the top of viewport.

@tao
Copy link
Author

tao commented Sep 18, 2021

Here's a demo (https://github.com/tao/turbo-scroll-bug).

  1. Start a server: php -S localhost:9000 and visit http://localhost:9000/one.html
  2. Scroll down to the bottom and click the link to two.html#10 (works fine)
  3. Click the link to go back to one.html#25 (the paragraph will be at the bottom of the page instead of the top)

This happens on Firefox. On Chrome, the turbo link behaviour seems to puts the link in the center of both pages.

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Nov 25, 2021
Closes hotwired#400

Before finishing a Visit by scrolling to an element, wait for an
additional animation frame (with the `nextAnimationFrame()` utility
which calls [requestAnimationFrame][] behind the scenes).

[requestAnimationFrame]: https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame
@seanpdoyle
Copy link
Contributor

Thank you for providing such a simple example @tao!

I've opened #476. Locally, I was unable to re-produce the issue with those changes.

For now, that PR is only the implementation changes. If this resolves your issue, I'll try and transform your example repository into the Turbo test suite.

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Nov 26, 2021
Closes hotwired#400

Invoke `Visit.goToSamePageAnchor()` earlier in the
`BrowserAdapter.visitStarted()` delegate method.

If a `Visit` is to an anchor on the same page, invoke it at the same
time a simulated request that's pre-populated from a `VisitResponse`
instance in the hopes that the asynchronous Visit delegate callbacks
won't be in competition with the same-page scrolling asynchrony.
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Nov 26, 2021
Closes hotwired#400

Invoke `Visit.goToSamePageAnchor()` earlier in the
`BrowserAdapter.visitStarted()` delegate method.

If a `Visit` is to an anchor on the same page, invoke it at the same
time a simulated request that's pre-populated from a `VisitResponse`
instance in the hopes that the asynchronous Visit delegate callbacks
won't be in competition with the same-page scrolling asynchrony.
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Nov 27, 2021
Closes hotwired#400

Before finishing a Visit by scrolling to an element, wait for an
additional animation frame (with the `nextAnimationFrame()` utility
which calls [requestAnimationFrame][] behind the scenes).

[requestAnimationFrame]: https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Nov 27, 2021
Closes hotwired#400

When processing a `Visit`, invoke `Visit.performScroll()` while loading
a response, loading a Snapshot, or executing a same-page anchor
navigation. After in-lining those calls to more appropriate points in
the Visit lifecycle, this commit removes the `performScroll()` call from
the `Visit.render()` method.
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Nov 27, 2021
Closes hotwired#400

When processing a `Visit`, invoke `Visit.performScroll()` while loading
a response, loading a Snapshot, or executing a same-page anchor
navigation. After in-lining those calls to more appropriate points in
the Visit lifecycle, this commit removes the `performScroll()` call from
the `Visit.render()` method.
@tao
Copy link
Author

tao commented Nov 28, 2021

Thanks, this does seem to solve the example problem that I linked on both Firefox and Chrome... but having trouble with my production site first and I'm not 100% yet what's going on yet or if I'm just making a mistake including the new code.

On Chrome it still seems to place the content in the centre.

On Firefox, when I update the url manually in the url bar then it loads the page and scrolls down to the correct place each time, but it seems like when I click on a link inside the page and turbo replaces the content then it still loads the anchor link at the bottom of the page.

I'm going to try debug it a bit more over the next few days and see if I can figure something out... perhaps it's some javascript code interfering with the page render step.

anchor-link-demo.mov

@tao
Copy link
Author

tao commented Nov 29, 2021

It appears that the new update in the pull request with this commit (0406188) plus the addition of this snippet mentioned in #426 does solve my problem.

If I only use this snippet on the current build of turbo.es2017-umd.js then it doesn't work.

Screen.Recording.2021-11-29.at.11.47.21.mov

@tao
Copy link
Author

tao commented Nov 29, 2021

The initial problem seems to be solved, so I'm happy with that solution.

However, another problem does occur, but it doesn't seem related as it happens whether I use the pull request or not, and with or without the snippet. So might be best to open another ticket for this. In this video below, you can see I'm on the top of the page, visit a link and it appears at the top of the view in the correct position... but when I return to the starting page it loads it at the bottom?

Perhaps when I visit the anchor link it stores the scroll position which is half way down the page, and when you return to the cached page it doesn't restore the scroll position of the cached page?

back-bug.mov

It appears to work up to a certain point, so links 1-13 allow you to return to the top of the previous page... but at 14 in this example it goes to the bottom... so maybe after the current page length is greater than the cached page length it puts it at the bottom?

Screen.Recording.2021-11-29.at.12.27.24.mov

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jul 16, 2022
Closes hotwired#400

When processing a `Visit`, invoke `Visit.performScroll()` while loading
a response, loading a Snapshot, or executing a same-page anchor
navigation. After in-lining those calls to more appropriate points in
the Visit lifecycle, this commit removes the `performScroll()` call from
the `Visit.render()` method.
@seanpdoyle
Copy link
Contributor

Was this resolved by #571?

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jul 16, 2022
Closes hotwired#400

When processing a `Visit`, invoke `Visit.performScroll()` while loading
a response, loading a Snapshot, or executing a same-page anchor
navigation. After in-lining those calls to more appropriate points in
the Visit lifecycle, this commit removes the `performScroll()` call from
the `Visit.render()` method.
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jul 16, 2022
Closes hotwired#400

When processing a `Visit`, invoke `Visit.performScroll()` while loading
a response, loading a Snapshot, or executing a same-page anchor
navigation. After in-lining those calls to more appropriate points in
the Visit lifecycle, this commit removes the `performScroll()` call from
the `Visit.render()` method.
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jul 16, 2022
Closes hotwired#400

When processing a `Visit`, invoke `Visit.performScroll()` while loading
a response, loading a Snapshot, or executing a same-page anchor
navigation. After in-lining those calls to more appropriate points in
the Visit lifecycle, this commit removes the `performScroll()` call from
the `Visit.render()` method.
@seanpdoyle
Copy link
Contributor

@tao I've rebased #476, could you verify whether or not it still fixes the behavior like you mentioned in #400 (comment)?

@tao
Copy link
Author

tao commented Jul 18, 2022

@seanpdoyle I tried again with #476 and If I disable scroll-behavior: smooth then it seems like it does work as expected and the other related issues I pointed out in my videos all seem like they've been resolved and working correctly on my production site when I tested it.

With smooth scroll enabled it still does lots of strange things on different browsers, especially on Firefox it's giving me strange behaviour. Chrome seems like it works with the smooth scrolling enabled much better but loads anchor links in the middle of the page.

But if I include this script again #426 just after I include the turbo script then it seems to all work correctly without any issues and smooth scrolling enabled.

        <script src="/js/turbo.es2017-umd.js" defer></script>
        <script type="text/javascript">
            document.addEventListener(`turbo:load`, () => {
                document.documentElement.style.scrollBehavior = `smooth`
            })

            document.addEventListener(`turbo:before-visit`, () => {
                document.documentElement.style.scrollBehavior = `unset`
            })
        </script>

So it's all looking good, I'm going to continue testing it tomorrow and see if I can finally include turbo on my site.

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jul 28, 2022
Closes hotwired#400

When processing a `Visit`, invoke `Visit.performScroll()` while loading
a response, loading a Snapshot, or executing a same-page anchor
navigation. After in-lining those calls to more appropriate points in
the Visit lifecycle, this commit removes the `performScroll()` call from
the `Visit.render()` method.
@dhh dhh closed this as completed in #476 Jul 29, 2022
dhh pushed a commit that referenced this issue Jul 29, 2022
* Perform scrolling prior to Visit completion

Closes #400

When processing a `Visit`, invoke `Visit.performScroll()` while loading
a response, loading a Snapshot, or executing a same-page anchor
navigation. After in-lining those calls to more appropriate points in
the Visit lifecycle, this commit removes the `performScroll()` call from
the `Visit.render()` method.

* Fix flaky Form Submission Test

Resolves a [flaky test][] by replacing a call to `nextBeat()` with a
helper that will wait until the target element (in this case,
`turbo-frame#frame`) dispatches a particular event (in this case,
`turbo:frame-load`).

[flaky test]: https://github.com/hotwired/turbo/runs/7570036350?check_suite_focus=true#step:12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants