Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix for the ticket #13483 #1205

Closed
wants to merge 4 commits into from

5 participants

@ros3cin

The normal behavior for slideDown animation is to display an element that is hidden and make it return to original properties.

The condition that is triggered by this effect works as this:

  • If the element is not hidden and a slideDown was called, do nothing.

This condition is not considering the scenario where the animation was stopped, which means that the element is displayed, but not completely shown though.

So, I've added checking for this case:

  • If a slideDown animation was called on an element that is not hidden, check if we still have information of its original properties. If so, the animation did not finish yet ( because the animation.done has a cleanup for this information ), then continue to slideDown.
@rwaldron
Collaborator

Please read http://contribute.jquery.org/style-guide/js/ and fix any code that doesn't match.

Sign http://jquery.github.com/cla.html, if you haven't already

cc @mikesherov @gnarf37 please review

@gnarf
Owner

I feel like any valid fix should also handle slideUp in the same state, and be tied in with slideToggle somehow.

The togglers already handle this, i'm not getting why slideDown gives up on it, but slideToggle does what you'd expect ya know?

@mikesherov
Collaborator

A couple of things:

  1. The unit test is testing an assumed implementation, not the apparent behavior and side effects of a function.
  2. I agree that a valid fix here has to be in slidetoggle
@ros3cin

Hi, gnarf and mikesherov!
In the ticket, the reporter was aware that he could use slideToggle(), or do a workaround using .hide() before .stop(true).slideDown().
But he was worried because he wanted to make it without toggle.

If the problem was only in the case of toggling from slideUp to slideDown, slideToggle would certainly be the right way to do it.
But there's a scenario where slideToggle wouldn't help, reported in the ticket's comments by me.

  • Do a slideDown, stop it in the middle, then continue the slideDown. The animation won't continue. There is a workaround for this using toggle: stop(true).slideToggle(0).stop(true).slideToggle(duration). Because of this, I tried to make a fix. Is this ok? You can check the behavior here: http://jsfiddle.net/RNjub/

SlideUp do not get the same behavior because the condition to start/continue a slideUp animation is:

  • the element is still visible ( not hidden )

I really made a mistake in the unit test, misunderstood it!
I'll fix it as soon as possible, using async test, putting the scenarios I mentioned and not with an assumed implementation.

@ros3cin

I signed the CLA. \o

@gnarf
Owner
src/effects.js
((5 lines not shown))
for ( index in props ) {
value = props[ index ];
if ( rfxtypes.exec( value ) ) {
delete props[ index ];
toggle = toggle || value === "toggle";
if ( value === ( hidden ? "hide" : "show" ) ) {
- continue;
+ //handle shown property if there is stored data, this means the slideDown() animation did not finish yet
+ if( value === "show" && dataShow[ index ] !== undefined ) {
+ //set the params so that the animation can proceed as if it was not stopped
+ style.display = "hidden";
@gnarf Owner
gnarf added a note

I'm not sure this is sane... I don't think we actually NEED to set hidden here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/effects.js
((5 lines not shown))
for ( index in props ) {
value = props[ index ];
if ( rfxtypes.exec( value ) ) {
delete props[ index ];
toggle = toggle || value === "toggle";
if ( value === ( hidden ? "hide" : "show" ) ) {
- continue;
+ //handle shown property if there is stored data, this means the slideDown() animation did not finish yet
+ if( value === "show" && dataShow[ index ] !== undefined ) {
+ //set the params so that the animation can proceed as if it was not stopped
+ style.display = "hidden";
+ hidden = true;
@gnarf Owner
gnarf added a note

Setting this internal "hidden" is probably enough

@rwaldron Collaborator

My question is: what is "display: hidden"?

@gnarf Owner
gnarf added a note

:smiley_cat:

@ros3cin
ros3cin added a note

I always change 'none' for 'hidden', sorry. XD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gnarf
Owner

After looking at the code again, this is in the prefilter anyway, the logic would indeed fix all the cases I was talking about.

Better test case definitely needed though, and take a look at my line comment about setting the display style to hidden, I am pretty sure this isn't needed.

@ros3cin

Yes, gnarf, I noticed the problem was also happening to fadeIn().
As you said, fixing that bit of code will be generic enough to fix the show based animations.
That line you mentioned is not needed indeed. I will take it out.

Thank you for the review, guys,
I'll make the changes you asked as soon as possible.

@ros3cin

Hey, I've made the changes.
Check it and tell me if there is anything more to change.
\o

src/effects.js
((5 lines not shown))
for ( index in props ) {
value = props[ index ];
if ( rfxtypes.exec( value ) ) {
delete props[ index ];
toggle = toggle || value === "toggle";
if ( value === ( hidden ? "hide" : "show" ) ) {
- continue;
+ //handle shown property if there is stored data, this means the slideDown() animation did not finish yet
@gnarf Owner
gnarf added a note

Please take another read through the style guide. Comments should be preceded by an empty line, also, there should be a space between the // and the comment body. I would also suggest rephrasing this:

// If there is dataShow left over from a stopped hide or show, we should pretend to be hidden

Also, now that I'm reading over this again, does someone have their head totally wrapped around the way this would behave with say a .hide().....stop().hide()

This path of code is VERY generic, and you should poke around with kid gloves in here :)

@ros3cin
ros3cin added a note

Can I change your suggestion to:

// If there is dataShow left over from a stopped hide or show and we are going to proceed with show, we should pretend to be hidden
?

Because the following condition only proceeds if we have value === "show"

@gnarf Owner
gnarf added a note

sure okay, now i remember why that worked... heh... so because its already "not hidden" during a failed hide, it works so we only need to do this cheat for value === "show" ... I forgot I figured that out earlier :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/effects.js
((19 lines not shown))
}
handled.push( index );
}
}
length = handled.length;
- if ( length ) {
- dataShow = jQuery._data( elem, "fxshow" ) || jQuery._data( elem, "fxshow", {} );
+ if ( !length ) {
+ jQuery._removeData( elem, "fxshow" );
+ }
+ else {
@gnarf Owner
gnarf added a note
} else {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
test/unit/effects.js
@@ -2025,4 +2025,66 @@ test( ".finish() calls finish of custom queue functions", function() {
div.remove();
});
+asyncTest( "slideDown() after stop() (#13483)", 2, function() {
+ var ul = jQuery( "<ul style='height: 100px;display: block'></ul>" ),
+ origHeight = ul.height();
+
+ //First test. slideUp() -> stop() in the middle -> slideDown() until the end
+ ul.slideUp(1000);
+ setTimeout( function() {
@gnarf Owner
gnarf added a note

Relying on setTimeout is pretty terrible to do in the unit tests, but there aren't many choices right now. Anyone have a better approach to suggest for these? Is this basically what .toggle().stop.toggle() checks are doing already?

@mikesherov Collaborator

Actually it's worse, these tests should use at least a full 1 second for timeouts because that's what firefox does when unfocused anyway.

@gnarf Owner
gnarf added a note

One of these days @mikesherov - you, me and a night with sinon?

@mikesherov Collaborator

yes, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
test/unit/effects.js
((12 lines not shown))
+ ul.slideDown( 1, function() {
+ equal( ul.height(), origHeight, "slideDown() after interrupting slideUp() with stop(). Height must be in original value" );
+
+ //Second test. slideDown() -> stop() in the middle -> slideDown() until the end
+ ul.slideUp( 1, function() {
+ ul.slideDown(1000);
+ setTimeout( function() {
+ ul.stop(true);
+ ul.slideDown( 1, function() {
+ equal( ul.height(), origHeight, "slideDown() after interrupting slideDown() with stop(). Height must be in original value" );
+ //cleanup
+ ul.remove();
+ start();
+ });
+ },
+ 500);
@mikesherov Collaborator

}, 500 );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
test/unit/effects.js
((17 lines not shown))
+ ul.slideDown(1000);
+ setTimeout( function() {
+ ul.stop(true);
+ ul.slideDown( 1, function() {
+ equal( ul.height(), origHeight, "slideDown() after interrupting slideDown() with stop(). Height must be in original value" );
+ //cleanup
+ ul.remove();
+ start();
+ });
+ },
+ 500);
+ });
+
+ });
+ },
+ 500);
@mikesherov Collaborator

}, 500 );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
test/unit/effects.js
((43 lines not shown))
+ ul.fadeIn( 1, function() {
+ equal( ul.css("opacity"), origOpacity, "fadeIn() after interrupting fadeOut() with stop(). Opacity must be in original value" );
+
+ //Second test. fadeIn() -> stop() in the middle -> fadeIn() until the end
+ ul.fadeOut( 1, function() {
+ ul.fadeIn(1000);
+ setTimeout( function() {
+ ul.stop(true);
+ ul.fadeIn( 1, function() {
+ equal( ul.css("opacity"), origOpacity, "fadeIn() after interrupting fadeIn() with stop(). Opacity must be in original value" );
+ //cleanup
+ ul.remove();
+ start();
+ });
+ },
+ 500);
@mikesherov Collaborator

}, 500 );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
test/unit/effects.js
((48 lines not shown))
+ ul.fadeIn(1000);
+ setTimeout( function() {
+ ul.stop(true);
+ ul.fadeIn( 1, function() {
+ equal( ul.css("opacity"), origOpacity, "fadeIn() after interrupting fadeIn() with stop(). Opacity must be in original value" );
+ //cleanup
+ ul.remove();
+ start();
+ });
+ },
+ 500);
+ });
+
+ });
+ },
+ 500);
@mikesherov Collaborator

}, 500 );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ros3cin

I'll read code style guide again and fix what you already pointed

@ros3cin

Code updated.
Waiting for feedback. \o

@dmethvin
Owner

This seems okay to me, @gnarf37 and @mikesherov can I get your signoff?

@gnarf
Owner

Seems good, the test could probably be asbtracted into a generator and also test show and hide with durations to be extra safe.

@gnarf
Owner

The body of this pull is solid though, don't let my test request stop it from landing, perhaps @ros3cin would be willing to do the updates to tests in an additional commit

@mikesherov
Collaborator

I'm cool if @gnarf37 is. Seems like it should be fine.

@ros3cin

Hi!
I can do tests with show and hide.
@gnarf37 How can I abstract test into a generator (what's this generator)?

@gnarf
Owner
@dmethvin
Owner

@ros3cin do you think you'll be able to add the unit tests in the next couple of days? I'd like to get this into 2.0 if possible.

@ros3cin

@dmethvin Sure. Sorry about the delay, I was finishing two projects from university last week, so I didn't have enough time. I got rid of them this morning. I'll be home at night and will start to work on these tests.

@gnarf
Owner

@dmethvin I really don't think this REQUIRES the tweak in the units to land... the units already added are good enough, but we can add some more after 2.0

@dmethvin dmethvin closed this in ea5c22e
@dmethvin
Owner

Thanks @ros3cin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 17, 2013
  1. @ros3cin
Commits on Mar 19, 2013
  1. @ros3cin
Commits on Mar 20, 2013
  1. @ros3cin
Commits on Mar 21, 2013
  1. @ros3cin

    Fix in code style

    ros3cin authored
This page is out of date. Refresh to see the latest.
Showing with 71 additions and 3 deletions.
  1. +11 −3 src/effects.js
  2. +60 −0 test/unit/effects.js
View
14 src/effects.js
@@ -304,21 +304,29 @@ function defaultPrefilter( elem, props, opts ) {
// show/hide pass
+ dataShow = jQuery._data( elem, "fxshow" ) || jQuery._data( elem, "fxshow", {} );
for ( index in props ) {
value = props[ index ];
if ( rfxtypes.exec( value ) ) {
delete props[ index ];
toggle = toggle || value === "toggle";
if ( value === ( hidden ? "hide" : "show" ) ) {
- continue;
+
+ // If there is dataShow left over from a stopped hide or show and we are going to proceed with show, we should pretend to be hidden
+ if ( value === "show" && dataShow[ index ] !== undefined ) {
+ hidden = true;
+ } else {
+ continue;
+ }
}
handled.push( index );
}
}
length = handled.length;
- if ( length ) {
- dataShow = jQuery._data( elem, "fxshow" ) || jQuery._data( elem, "fxshow", {} );
+ if ( !length ) {
+ jQuery._removeData( elem, "fxshow" );
+ } else {
if ( "hidden" in dataShow ) {
hidden = dataShow.hidden;
}
View
60 test/unit/effects.js
@@ -2025,4 +2025,64 @@ test( ".finish() calls finish of custom queue functions", function() {
div.remove();
});
+asyncTest( "slideDown() after stop() (#13483)", 2, function() {
+ var ul = jQuery( "<ul style='height: 100px;display: block'></ul>" ),
+ origHeight = ul.height();
+
+ // First test. slideUp() -> stop() in the middle -> slideDown() until the end
+ ul.slideUp( 1000 );
+ setTimeout( function() {
+ ul.stop( true );
+ ul.slideDown( 1, function() {
+ equal( ul.height(), origHeight, "slideDown() after interrupting slideUp() with stop(). Height must be in original value" );
+
+ // Second test. slideDown() -> stop() in the middle -> slideDown() until the end
+ ul.slideUp( 1, function() {
+ ul.slideDown( 1000 );
+ setTimeout( function() {
+ ul.stop( true );
+ ul.slideDown( 1, function() {
+ equal( ul.height(), origHeight, "slideDown() after interrupting slideDown() with stop(). Height must be in original value" );
+
+ // Cleanup
+ ul.remove();
+ start();
+ });
+ }, 500 );
+ });
+
+ });
+ }, 500 );
+});
+
+asyncTest( "fadeIn() after stop() (related to #13483)", 2, function() {
+ var ul = jQuery( "<ul style='height: 100px;display: block; opacity: 1'></ul>" ),
+ origOpacity = ul.css( "opacity" );
+
+ // First test. fadeOut() -> stop() in the middle -> fadeIn() until the end
+ ul.fadeOut( 1000 );
+ setTimeout( function() {
+ ul.stop( true );
+ ul.fadeIn( 1, function() {
+ equal( ul.css( "opacity" ), origOpacity, "fadeIn() after interrupting fadeOut() with stop(). Opacity must be in original value" );
+
+ // Second test. fadeIn() -> stop() in the middle -> fadeIn() until the end
+ ul.fadeOut( 1, function() {
+ ul.fadeIn( 1000 );
+ setTimeout( function() {
+ ul.stop( true );
+ ul.fadeIn( 1, function() {
+ equal( ul.css("opacity"), origOpacity, "fadeIn() after interrupting fadeIn() with stop(). Opacity must be in original value" );
+
+ // Cleanup
+ ul.remove();
+ start();
+ });
+ }, 500 );
+ });
+
+ });
+ }, 500 );
+});
+
})();
Something went wrong with that request. Please try again.