Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Fixed scroll doesn't stop at the right position #15

Closed
wants to merge 4 commits into from

Conversation

itsPG
Copy link
Contributor

@itsPG itsPG commented Dec 18, 2013

This fix make sure that the scroll will stop at the right position after the animation.

@cferdinandi
Copy link
Owner

Cool idea on closing that weird little gap that sometimes occurs when scrolling. I don't like the "snapping" effect this implementation creates, though. I wonder if there's a better way to do this?

@itsPG
Copy link
Contributor Author

itsPG commented Dec 18, 2013

Yes, you are right. I didn't notice the snapping.

@itsPG
Copy link
Contributor Author

itsPG commented Dec 18, 2013

After watching percentage variable, I noticed that the animation ended with 0.8~1.0 and found the bug in the if statement.

@cferdinandi
Copy link
Owner

In testing this on all of the easing types included with the index.html file, I'm still seeing some minor "snapping" at the end. I believe the problem is related to how the scrolling intervals are calculated. They often end up being non-whole numbers (2.4px, for example), but browsers can only work in whole pixels. As a result, you sometimes end up with a remainder of a few pixels at the end.

There are three options I can think of:

  1. Ignore it, which is what the current version of the script does.
  2. Reconcile that remainder by adding a final scroll (which is what in one way or another both of your fixes do).
  3. Come up with a better way to calculate intervals.

I personally prefer option 1 over option 2, but option 3 is certainly the best of the bunch.

@itsPG
Copy link
Contributor Author

itsPG commented Dec 20, 2013

I found that there's a problem in window.scrollTo() function.
For example, If you call window.scrollTo(0, 1234); console.log(window.pageYOffset);
The result may be 1233. And the browser will fall into infinite loop.

@cferdinandi
Copy link
Owner

Still seeing the janky stuff at the end of the scroll.

@variousauthors
Copy link

I've noticed, with the fixed header, that if I am at the top of a page, with 5 anchors, and I click a link that would take me to the 3rd anchor, that the gap is bigger than if I had clicked on the second link. I'm using master and just downloaded your smooth-scroll today.

@cferdinandi
Copy link
Owner

Can you please share a link to a demo?

@itsPG
Copy link
Contributor Author

itsPG commented Jan 11, 2014

http://itsPG.org/

@cferdinandi
Copy link
Owner

Sorry, I meant @variousauthors. I'd like to see the problem in a working example.

@itsPG
Copy link
Contributor Author

itsPG commented Jan 11, 2014

Sorry, I misunderstood. I thought you're talking about the same problem on http://rivvr.io/ .

@gavinsawyer
Copy link

I deleted my website and tried again to make this work. I don't any scripts, so there is nothing interfering, but I found the problem. When you make the easing too fast it stops before it should. I just made a commit with the changes I made. They won't be on my website though, only in the repo.
Edit: this also explains why @variousauthors had the problem where the gap is bigger with the further away anchor. Because it calculates a faster speed.

@cferdinandi
Copy link
Owner

How fast are we talking, @gavinsawyer? I've tested down to 500ms without any issues. I wouldn't recommend any lower than that, though.

@variousauthors
Copy link

Hmm... a lower scroll speed won't really do for a single-page layout. Here is a link to the page I was talking about. If you click the second link, it will scroll perfectly to the correct section... but all the other links have the problem I've described.

These changes were merged into which branch?

@cferdinandi
Copy link
Owner

@variousauthors - the behavior I'm seeing looks like it's caused not by the speed of the scroll, but by the rounded off scrolling distance that can sometimes leave a bit of a gap above the anchor target.

@a-v-l
Copy link
Contributor

a-v-l commented Jan 15, 2014

I don't have time to dig into this right now. But the height of the gap is clearly a result from scroll distance divided by scroll speed (sort of). The results are always the same – even cross browsers (quick test on Mac: Chrome & Firefox). So it should be possible to calculate the gap from this two parameters and somehow add that to the stoping position. I will give it a try as soon as I can…

@cferdinandi
Copy link
Owner

@a-v-l - I was in the process of drafting a response when I had an "ah-ha" moment.

TL;DR: I just pushed an updated (v2.14) that fixes this issue.

The Long Version:

Before adding easing support, the script used some really basic math to figure out when to stop animating. Basically, it measured the distance to the anchor target, divided that by the number of milliseconds it should take to animate, and repeated itself that many times.

Unfortunately, this often resulted in fractional numbers, and since browsers can only work in whole pixels, you'd often end up with a few left over—the small gap above the anchor target. As you noted, it is in fact influenced by scroll speed as well.

When the script was updated for easing support, the old animation function was replaced with some new, fancier math that scrolls directly to the top of the anchor target. Unfortunately, the function that determines when to stop looping the animation function never got updated, so it was still using the fractional, rounded off numbers.

I just updated that way the script calculates when to stop. The new way is actually simpler, more accurate, and more performant, so that's awesome.

Thanks to @itsPG and @variousauthors for your help as well!

@variousauthors
Copy link

Awesome! I will give it a try.

@variousauthors
Copy link

I gave it a try, and it still didn't work right away. It was better, but not perfect. I did some snooping, and what I realized was that the target anchor itself had a height, and that wasn't being accounted for. I added fat_anchor = $(anchor).height(); to the getEndLocation function, and now it is purrfect. This might not be a problem for everyone, but I thought I might as well add that here.

@cferdinandi
Copy link
Owner

@variousauthors - Glad you found a fix that worked for you. Just an FYI: You can write that variable without jQuery. It'll be just as short and more performant.

var fatAnchor = anchor.offsetHeight;

And if you do choose to use jQuery, you don't need to wrap anchor in $(...). It's already an identified element. So, using jQuery:

var fatAnchor = anchor.height();

@variousauthors
Copy link

Yes! I'm slowly starting to wake up to the fact that the native javascript API is cool. I'm one of those children of the turn of the decade who grew up hating getElementById and was so overcome by the magic of jQuery that it just completely replaced JS in our brains. (that is a thing). Thanks for the tip!

@cferdinandi
Copy link
Owner

In all fairness, the native JS API was so inconsistently implemented across browsers until recently that you'd be insane not to use jQuery. And many of the best modern features (querySelectorAll and forEach) exist today because of jQuery.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants