search

jlmakes / scrollreveal

Animate elements as they scroll into view.
https://scrollrevealjs.org/
22.3k stars 2.26k forks source link

reveal() reset when using afterReveal and scrolling fast #511

Open allaire opened 4 years ago

allaire commented 4 years ago

How to duplicate: https://jsfiddle.net/9vcytj2b/

Scroll up and down, you'll see that the header text and the footer text keeps animating. reset is set to false by default so this should not happen. Using cleanup: true make it works, but is not friendly when you use sync().

It works as expected when you comment out the afterReveal callbacks: https://jsfiddle.net/jxcnb2wp/

The fact that it works correctly without callbacks leave me thinking that there is a bug somewhere in there.

jlmakes commented 4 years ago

Thanks for the report and reproduction @allaire

I'm comparing changes in element state, and focusing on { revealed: false } being set after the element has revealed. revealed = false occurs three times in the source, and only one looks guilty:

https://github.com/jlmakes/scrollreveal/blob/66f44ab08c3cd0a3df7ced9b2e01e4ffcf809cb2/src/instance/functions/initialize.js#L7-L21

Line 10 seems an insufficient condition, and the following else block is being evaluated. What's more, that logic suggests the initialize() function is erroneously running more than once.

Which I just confirmed: initialize() is being invoked once on initial evaluation, and two more times for each reveal() call wrapped in option.afterReveal

I'm going to keep digging...

allaire commented 4 years ago

Hi @jlmakes, awesome thanks for the detailed reply. Let me know if you need help to test a potential fix.

Also, while testing ScrollReveal, I also experienced this issue: https://github.com/jlmakes/scrollreveal/issues/231

I know it's an old one, but I decided to reply instead of opening a new issue. I wasn't able to create a reproduction in a JSFiddle environment. Any advice how I can help with #231 ?

simonkuran commented 3 years ago

Any update on this? I'd like to be able to use the afterReveal option, but this bug is preventing me from implementing it.

jlmakes commented 3 years ago

@simonkuran I actually don't remember where I landed on this. Using option.afterReveal to invoke reveal() is not exactly a use case well suited to ScrollReveal's design. This is one of a few strategies I've seen surfaced by the community, reflecting a desire to have more control over what is revealed and when.

I'll take another look at this and see if there is something that can be improved for the current version of ScrollReveal. But for what it's worth, I am redesigning ScrollReveal 5 to offer more control to power users looking to coordinate their own reveals.