search

d-oliveros / ngSmoothScroll

AngularJS directives for animating smooth scrolling to elements.
276 stars 110 forks source link

Doesn't scroll to element if it is offset by a top margin... #18

Open davidmyersdev opened 9 years ago

davidmyersdev commented 9 years ago

Hello!

I'm really liking this module, but it seems that it doesn't take into account the ending element's top margin. I'm not sure if this is a bug or there is something I'm missing, but I thought I'd bring it to your attention.

Thanks!

adamreisnz commented 9 years ago

I think this is a bug in Firefox. I've encountered a similar issue when trying to scroll to the top element of the page which was offset by a margin. Firefox doesn't seem to take the top margin into account for determining the document.body.scrollHeight, causing ngSmoothScroll to stop scrolling prematurely due to the comparison:

(window.innerHeight + currentLocation) >= document.body.scrollHeight

At the start of the animation, when you have scrolled down almost all the way of the page, this comparison will be triggered incorrectly, because the document body scroll height is too little, causing ngSmoothScroll to stop the animation before it even started.

In Chrome this problem does not occur, as the margin is added correctly to the document body scroll height.

See this fiddle to see the problem (open it in Firefox and Chrome) and notice how in Firefox the scroll height is reported as 1000px, which is exactly the element's height, but excludes the margin (and body padding):

https://jsfiddle.net/xvbaoakg/

You can work around this problem by either commenting out the comparison in the smooth scroll library, or avoiding to use margins on your elements and add a padder element with padding instead.

adamreisnz commented 9 years ago

Actually, just found a proper fix for this problem. Instead of using document.body.scrollHeight, use this to determine the correct document height:

var docHeight = Math.max(
    Math.max(document.body.scrollHeight, document.documentElement.scrollHeight),
    Math.max(document.body.offsetHeight, document.documentElement.offsetHeight),
    Math.max(document.body.clientHeight, document.documentElement.clientHeight)
);

Updated fiddle: https://jsfiddle.net/xvbaoakg/1/

Will create a PR for this fix.