julianshapiro / velocity

Accelerated JavaScript animation.
VelocityJS.org
MIT License
17.29k stars 1.56k forks source link

Unexpected behavior of slide animation using Stop command #744

Closed R-G-B closed 7 years ago

R-G-B commented 7 years ago

Hi all, I'm using bootstrap and trying to animate my dropdowns with this code (in jquery):

$('.dropdown').on('show.bs.dropdown', function (e) {
    $(this).find('.dropdown-menu').first().stop(true, true).slideDown();
});
$('.dropdown').on('hide.bs.dropdown', function (e) {
    e.preventDefault();
    $(this).find('.dropdown-menu').first().stop(true, true).slideUp(400, function(){
        $(this).parent().removeClass('open');
    });
});

and everything works well even when the user clicks fast at the dropdown many times in a row: https://i.gyazo.com/8a2f4b3bcc02cf6b60756c33c8b996fa.gif But when I'm trying to use velocity instead of jquery with this code:

$('.dropdown').on('show.bs.dropdown', function (e) {
    $(this).find('.dropdown-menu').first().velocity("stop").velocity("slideDown", { duration: 300 });
});
$('.dropdown').on('hide.bs.dropdown', function (e) {
    e.preventDefault();
    $(this).find('.dropdown-menu').first().velocity("stop").velocity("slideUp", { duration: 300, complete: function() { 
        $(this).parent().removeClass('open'); 
    } });
});

in the same situation (fast clicking many times in a row) dropdowns are gradually collapsing with each click, so after maybe 10 clicks the whole dropdown is completely collapsed and there is no way to show it without refreshing the page: https://i.gyazo.com/6fae1ab9c39b01d7a76d1a5b4a0f21ee.gif It looks like velocity is trying to do slideUp before slideDown animation is completed, but during this process it is straying from the correct numbers of element's height/margin/padding etc, so when dropdown is "killed" after many clicks and there is no way to show it, his styles looks like this:

<ul class="dropdown-menu" style="display: none; overflow: hidden; height: 0.0751667px; padding-top: 0.00025814px; padding-bottom: 0.00129072px; margin-top: 0px; margin-bottom: 0px;">
    ...
</ul>

and when we are trying to click on it, it is not changing, except "display: block" property, but it's height remains near 0px, so nothing changes visually:

<ul class="dropdown-menu" style="display: block; overflow: hidden; height: 0.0751667px; padding-top: 0.00025814px; padding-bottom: 0.00129072px; margin-top: 0px; margin-bottom: 0px;">
    ...
</ul>

Any ideas how to fix velocity behavior?

febLey commented 7 years ago

I'm encountering the same issue, fixed it temporarily by replacing the stop with finish

if (_link.classList.contains(classActive)) {
    _link.classList.remove(classActive)
    $subNav.velocity('finish').velocity('slideUp', { duration: 300 });
}
else {
    _link.classList.add(classActive)
    $subNav.velocity('finish').velocity('slideDown', { duration: 300 });
}
R-G-B commented 7 years ago

@febLey Thanks a lot! But what about consequences of this approach?

Rycochet commented 7 years ago

http://velocityjs.org/#finish - Explanation is in there, basically it will mean that the animation will be finished (ie, jump to final values), unlike "stop" which will stop it exactly where it is when called.

R-G-B commented 7 years ago

@Rycochet thanks, but it seems I've got another problem now, after replacing "stop" with "finish". Steps to get the problem:

  1. Open website in vertical/horizontal orientation
  2. Expand some dropdown menu
  3. Change the orientation
  4. Collapse the dropdown and try to expand it again
  5. You will have something like this: https://i.gyazo.com/f0eba0b4201d150c82f21bb72f799454.gif Moreover, number of "blinks" depends on the orientation changes quantity (4 repeating blinks on my gif = 4 changes from horizontal to vertical orientation and vice versa). When I'm changing "finish" to "stop", it solves this problem, but returns the problem, described in the first post. Any ideas how to fix all this stuff together?
Rycochet commented 7 years ago

@R-G-B I don't know your code (you've not linked to the site or given a JSFiddle), but sounds like you're attaching events at the wrong time - and hence triggering the animations at the wrong time. "stop" isn't preventing the problem, it's hiding it - hence the duplicate animations.

R-G-B commented 7 years ago

@Rycochet sorry, I'm not very familiar with jsfiddle, but I'll try to show you what is going on. http://jsfiddle.net/45fx1Lda/1/ - try to look this fiddle at the small resolution and try to click many times on the dropdown menu named Dropdown: image

it will disappear, as described in the first message https://github.com/julianshapiro/velocity/issues/744#issue-204563276 And after replacing stop with finish (row 58 and 62), we've got another problem, described 6 days ago https://github.com/julianshapiro/velocity/issues/744#issuecomment-285768274: http://jsfiddle.net/b9ogs6hw/2/

Rycochet commented 7 years ago

Will try to have a better look later today - but the version of Velocity you're running on there (1.2.2) is over two years old...

R-G-B commented 7 years ago

Sorry, didn't notice that, updated fiddles: http://jsfiddle.net/45fx1Lda/1/ http://jsfiddle.net/b9ogs6hw/2/

Rycochet commented 7 years ago

I couldn't duplicate this on Chrome, but looking at the code I noticed on major issue that's looking like the probable cause -

animateDropdowns() is adding event handlers with .on(...) - but it's getting called with every window resize event (maybe make sure you remove the events before adding so it doesn't duplicate).

$('.dropdown').off('show.bs.dropdown hide.bs.dropdown');
if ($('.hidden-xs').is(":visible")) {
   ...
R-G-B commented 7 years ago

But this issue doesn't depends on browser, it is visible even on mobiles, maybe I'm not very good at explaining the problem and GIFs aren't very informative, so I decide to record the new one and indicate mouse clicks (you have to click fast and many times in a row to reproduce the major issue): https://gyazo.com/811ab8331861f9fe1e64d7b36a146ca4 (hope now its clear to you) I tried to unbind event handlers with something like $('.dropdown').unbind(); but unfortunately nothing changes.

Rycochet commented 7 years ago

Ah - I was clicking on the wrong thing, will have another look at things (sorry, lack of time in general).

You use .off() to remove event handlers, .bind/.unbind were depreciated many years ago (you're already using .on) :-)

R-G-B commented 7 years ago

At your convenience, of course - I'm surprised at all that you have time for me :) About .off() - I've tried it in both ways, but didn't notice any changes (or maybe I'm doing something wrong)

Rycochet commented 7 years ago

Don't know if .off would make any difference - but you're adding the same event handler multiple times - and not by reference (ie defining it and passing the function name), which is really just a bad idea in general (it's the sort of thing that can cause other "undocumented features" to turn up) ;-)

Rycochet commented 7 years ago

Ok, having a quick look through the code, and not sure exactly what can be done to fix it - it's the expected behaviour of how it animates -

Looking at the code it definitely looks like it needs to be rewritten, but right now trying to think my way through it (after work) isn't helping.

One thing you can do (for the meantime) is resetting the css after it finishes slideUp -

$(this).find('.dropdown-menu').first().velocity("stop").velocity("slideUp", { duration: 300, complete: function() { $(this).attr("style", "display:none;").parent().removeClass('open'); } });

I've literally just added the reset of .attr("style", "display:none;") in there - it still looks weird with fast clicking, but now it at least lets you carry on after you let it finish.

R-G-B commented 7 years ago

@Rycochet thanks a lot, it works!