Fix scroll animation callback called twice #255

[hpihkala]

See issue #255.

When showing a bubble on a target outside view, Hopscotch attempts to smooth scroll the target into view. Scrolling is attempted via the use of YUI or jQuery when available. This issue is related to how the scroll position is animated using jQuery.

jQuery calls the animation callback for each of the matched elements in the selector. Hopscotch was selecting both the html element and the body element for adjusting the scrollTop value, leading to the animation callback being called twice.

Adjusting scrollTop on the html element seems questionable anyway, so just removing it fixes the issue.

[timlindvall]

Thanks for the investigation and fix. I'm concerned about the cross-browser implications of this. If I remember correctly, WebKit browsers supported setting scrollTop on body, while Firefox supported scrollTop on html instead. That said, this was a long while ago, so I'll need to do some investigation to see if this quirk has been fixed.

[hpihkala]

If it turns out that both are needed, another solution to the issue would be to use a boolean to make sure the callback gets called only once per animation. This is slightly less elegant but supports the quirk.

Please let me know if you prefer the alternative solution, I can close this PR and submit a new one.

[timlindvall]

Just tested this against Chrome and Firefox latest and it looks like the quirk is still there. =(

Feel free to keep this PR open (unless it's easier to draft a new PR), but I would recommend pushing a new update to your branch that ensures cb is only called once. Wrapping the jQuery.animate call in a function that encompasses this logic should work.

[timlindvall]

Hi @hpihkala,

Don't want to push back the 0.2.6 release any longer, so I'm going to pull this from that milestone. We can consider this for a later maintenance release, should one occur, or we can fix this in ES6 Refresh.

If you'd like myself or another contributor to take over the investigation and fix for this issue, let me know.

Thanks!

[jasminmistry]

@hpihkala @zimmi88

This issue still exists. Please merge it.