Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #11755 #774

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@markelog
Copy link
Member

markelog commented May 11, 2012

jQuery Size - compared to last make
  253011    (+99) jquery.js
   93726    (+70) jquery.min.js
   33385     (+8) jquery.min.js.gz
.animate({opacity: to}, speed, easing, callback);
return this.filter(function() {
return jQuery.css( this, "display" ) === "none";
}).css( "opacity", 0 ).show().end().animate( {opacity: to}, speed, easing, callback );

This comment has been minimized.

Copy link
@rwaldron

rwaldron May 11, 2012

Member

cc @gnarf37

This comment has been minimized.

Copy link
@gnarf

gnarf May 11, 2012

Member

Should this be .animate({ opacity: to }, speed, easing, callback ) ?

This comment has been minimized.

Copy link
@markelog

markelog May 11, 2012

Author Member

or maybe

.animate({
    opacity: to 
}, speed, easing, callback );

What is the right way?

@gnarf

This comment has been minimized.

Copy link
Member

gnarf commented May 11, 2012

Any chance you can put together test cases for when this failed before, but passes now? I don't have any problems with these changes - but would like to have a test case for it.

@markelog

This comment has been minimized.

Copy link
Member Author

markelog commented May 11, 2012

@gnarf37 current code is not failing, this is just an improvement, ":hidden" selector do a lot of checks that can be avoided and this is also decreases amount of cases when display property set, which is might be a good thing. I guess i can add a couple tests for it. More about it here – http://bugs.jquery.com/ticket/11755

@gnarf

This comment has been minimized.

Copy link
Member

gnarf commented May 11, 2012

Yeah I read the ticket, but this is definitely going to be something that has a test that failed before, but passes now... Specifically the "inside a hidden element" case...

Check out http://jqbug.com/10045 and http://jqbug.com/10568 to see if these are things that might be fixed by this also.

I'm a little concerned that there are now going to be cases in which an item would be considered "hidden" that they are now going to be considered "shown" -- I.E. height: 0

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented May 18, 2012

@Orkel, can you check on the two tickets @gnarf37 mentioned and see if they may be fixed? If so it would be good to incorporate tests for that into this patch.

Also, I wonder how common the IE height:0 case might be. I just don't know.

@markelog

This comment has been minimized.

Copy link
Member Author

markelog commented May 21, 2012

Tried to come up with code and tests, that more or less cover all the issues, it did take some time, sorry.
Comparing to 63aaff5, it actually removes 6 bytes instead of adding 8. Pull does not fix #10568, but corrects #10045, i added some tests for it. More explanation of the last commit is here

@gnarf

This comment has been minimized.

Copy link

gnarf commented on src/effects.js in 7298730 May 22, 2012

I feel like this isHidden calling convention will fail when you $(".twoElements").fadeTo(1) -- elem in the isHidden function will be 1 for the second element.

@gnarf

This comment has been minimized.

Copy link

gnarf commented on src/effects.js in 7298730 May 22, 2012

Should probably be elem = el || elem if you want it callable via each and plain old isHidden( elem )

@gnarf

This comment has been minimized.

Copy link

gnarf commented on test/unit/effects.js in b2cd5d9 May 22, 2012

<3 this use of when :)

@gnarf

This comment has been minimized.

Copy link
Member

gnarf commented May 22, 2012

apparently my last comments were already dealt with ;) sorry @Orkel !

@gnarf

This comment has been minimized.

Copy link
Member

gnarf commented May 22, 2012

So, I brought this up briefly with @Orkel in IRC - https://github.com/jquery/jquery-ui/blob/master/ui/jquery.effects.core.js#L372-377 -- Why should :hidden be any different than this isHidden definition? I know we have some crazy checks in our :hidden selector that probably aren't necessary.

@markelog

This comment has been minimized.

Copy link
Member Author

markelog commented May 22, 2012

In animate case, we should test if element, not its parent are hidden, whereas users have a lot of cases when they need to know if some of the parents are hidden too. Even through it creates cases like this – http://jsfiddle.net/xRUrc/

But i guess, its not a common case, and users probably get used to it. Check for height and width = 0, is only reliable and fast way to determine if a some of the parents of the element are hidden. More info about it

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented May 22, 2012

@Orkel pretty much nailed it, the check for width/height of 0 catches the case where a parent is hidden, which is a lot faster than searching up the DOM tree for display:none. Or at least it was at the point it was changed, maybe some time around 1.2? And of course we've pretty much now said that width/height of 0 indicates hidden so we can't change that. Think of :hidden as meaning "takes up no space in the document".

The other meaning of :hidden is that foolish consistency one of input[type=hidden] which most people may not even realize it does (except when it bites them). I suppose we could deprecate that as part of the form shortcut changes in http://bugs.jquery.com/ticket/9400 ?

@markelog

This comment has been minimized.

Copy link
Member Author

markelog commented May 22, 2012

The thing is, input[type=hidden] does not take any space in document too – http://jsfiddle.net/cQge2/
so basically, it falls under definition of the hidden elements, this is why it selected for :hidden. We actually do not check for name or type of the element in hidden selector – https://github.com/jquery/jquery/blob/master/src/css.js#L446

Only way to deprecate it, is to add a special case for it.

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented May 22, 2012

Oh right, originally I think it had a special case until we used width/height 0. The :enabled filter still does have a special case for it: https://github.com/jquery/sizzle/blob/master/sizzle.js#L624

@dmethvin dmethvin closed this in ae20e73 May 23, 2012

markelog added a commit to markelog/jquery that referenced this pull request Jun 29, 2012

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.