Side navigation
#7641 closed enhancement (wontfix)
Opened November 26, 2010 06:39PM UTC
Closed January 18, 2012 03:33AM UTC
Last modified March 10, 2012 04:54AM UTC
Animating style to "auto" and "" (empty string) values
Reported by: | darccyclarke | Owned by: | lrbabe |
---|---|---|---|
Priority: | low | Milestone: | None |
Component: | effects | Version: | 1.6.1 |
Keywords: | Cc: | ||
Blocked by: | Blocking: |
Description
Have been doing some testing and realized trying to animate height / width to the "auto" value doesn't work. Submitting my quick patch that seems to solve this.
Note: there may be a better way then to clone the node to get it's default values but I thought it worked well in this case. This shoul be pretty heavily scrutinized since I've only bounced the idea off a few other people.
Add the following before line #6471 (Unminified jQuery 1.4.4 version):
if ( val === "auto" && ( name === "height" || name === "width" ) ){ var $temp = jQuery(self).clone().css(name,"auto").appendTo("body"); val = $temp.css(name); $temp.remove(); }
Attachments (0)
Change History (19)
Changed November 26, 2010 10:23PM UTC by comment:1
Changed November 29, 2010 07:32PM UTC by comment:2
Ditto. In fact, CSS Transitions currently are pretty shit when going from 'auto' to a value or vice versa.
This should work.. but its tricky to.
The patch here won't cut it.
But I'd like this feature fo sho
Changed November 29, 2010 09:12PM UTC by comment:3
_comment0: | Ater talking with Paul a bit about this I've come up with a slightly different version of my proposed patch. Instead of appending the cloned element to the body I insert it before itself. And instead of removing the element completely Paul suggested detaching it. \ \ {{{ \ if ( val === "auto" && ( name === "height" || name === "width" ) ){ \ var $temp = jQuery(self).clone().css(name,"auto").insertBefore(self); \ val = $temp.css(prop); \ $temp.detach(); \ } \ }}} \ \ I think this is a pretty solid solution. Again, it's open to criticism if someone can find the flaw in obtaining the intended CSS attributes value this way. \ \ Thoughts? → 1291066233773110 |
---|
Ater talking with Paul a bit about this I've come up with a slightly different version of my proposed patch. Instead of appending the cloned element to the body I insert it before itself.
if ( val === "auto" && ( name === "height" || name === "width" ) ){ var $temp = jQuery(self).clone().css(name,"auto").insertBefore(self); val = $temp.css(prop); $temp.remove(); }
I think this is a pretty solid solution. Again, it's open to criticism if someone can find the flaw in obtaining the intended CSS attributes value this way.
Thoughts?
An example of the implementation in a separate function, not in the core as proposed: http://jsfiddle.net/darcyclarke/JNRpK/21/
Changed December 01, 2010 06:36AM UTC by comment:4
component: | unfiled → effects |
---|---|
keywords: | → auto animate |
priority: | undecided → low |
resolution: | → patchwelcome |
status: | new → closed |
This seems like a cool feature/enhancement, but it really seems like one of those that you'll need to take the reigns on and create a pull request along with several unit tests showing it works.
I'm a little worried about a flash of weird content, or an accidental rerunning of script elements (by injecting them again), or more commonly just performance hits.
I'd love to see it work though.
Changed May 12, 2011 07:32PM UTC by comment:5
Using jQuery 1.5.2. Not only does animating the height/width of an element to "auto" not work, but it causes a JavaScript error in IE 7 and IE 8. I don't mind that it doesn't work (I'm really just passing it through as a dummy value), but it'd be nice if it didn't throw an error.
Could it work? It doesn't seem like it'd be too hard to figure out what the, e.g., height of an element would be if no styles were applied to it modifying its height, which is exactly what the "auto" property is. Am I wrong?
Changed June 07, 2011 12:59PM UTC by comment:6
milestone: | 1.5 |
---|---|
resolution: | patchwelcome |
status: | closed → reopened |
patch and unit tests here: https://github.com/jquery/jquery/pull/408
Changed June 07, 2011 01:01PM UTC by comment:7
milestone: | → 1.next |
---|---|
owner: | → lrbabe |
status: | reopened → assigned |
version: | 1.4.4 → 1.6.1 |
Changed June 07, 2011 03:40PM UTC by comment:8
summary: | Animating height / width to "auto" → Animating style to "auto" and "" (empty string) values |
---|
Changed July 12, 2011 08:09PM UTC by comment:9
From the pull:
'Correct me if I'm wrong, but won't this only work if there are uncomputed styles set that we can fall back to? For instance, the default value of width is "auto" and unless a width is set somewhere in the user css that isn't auto, we still can't do the animation.'
The main issue I see is that even the best browsers will return "auto" sometimes for _computed_ styles, so I'm not sure we can fully support animating auto.
Changed July 19, 2011 03:01PM UTC by comment:10
_comment0: | ''(sorry, I just noticed your comment after answering on Github)'' \ \ The problem of the "auto" value returned for computed style is also present in other part of the effects code, see how jQuery.fx.cur() deals with it. \ This didn't prevent us from writing the effect component ;-) \ \ Having basic support for "auto" and "" is better than no support at all. → 1311088101786084 |
---|
''(sorry, I just noticed your comment after answering on Github)''
The problem of the "auto" value returned for computed style is also present in other part of the effects code, see how jQuery.fx.cur() deals with it.
This didn't prevent us from writing the effect component ;-)
Having ''almost complete'' support for "auto" and "" is better than no support at all.
Changed September 24, 2011 06:08PM UTC by comment:11
keywords: | auto animate → 1.8-discuss |
---|
Tabling for 1.8 discussion. Some of the team, including myself, still have concerns about adding support for these values.
Changed November 23, 2011 03:23PM UTC by comment:12
The PR is now here: https://github.com/jquery/jquery/pull/515
additional info on why this patch is harmless has been included
Changed December 13, 2011 01:15PM UTC by comment:13
description: | Have been doing some testing and realized trying to animate height / width to the "auto" value doesn't work. Submitting my quick patch that seems to solve this. \ \ Note: there may be a better way then to clone the node to get it's default values but I thought it worked well in this case. This shoul be pretty heavily scrutinized since I've only bounced the idea off a few other people. \ \ Add the following before line #6471 (Unminified jQuery 1.4.4 version): \ {{{ \ if ( val === "auto" && ( name === "height" || name === "width" ) ){ \ var $temp = jQuery(self).clone().css(name,"auto").appendTo("body"); \ val = $temp.css(name); \ $temp.remove(); \ } \ }}} \ \ → Have been doing some testing and realized trying to animate height / width to the "auto" value doesn't work. Submitting my quick patch that seems to solve this.\ \ Note: there may be a better way then to clone the node to get it's default values but I thought it worked well in this case. This shoul be pretty heavily scrutinized since I've only bounced the idea off a few other people.\ \ Add the following before line #6471 (Unminified jQuery 1.4.4 version):\ {{{\ if ( val === "auto" && ( name === "height" || name === "width" ) ){\ var $temp = jQuery(self).clone().css(name,"auto").appendTo("body");\ val = $temp.css(name);\ $temp.remove();\ }\ }}}\ \ |
---|
-1, This requires a bit more than the patch implies, no? Animating to auto width would mean the cloned element can't just be appended to the body as the patch implies. I could be persuaded if there was a simple enough way to do this.
Changed December 13, 2011 03:54PM UTC by comment:14
+0, If there is an easy, short way, then do it. Else, document it and move on.
Changed December 13, 2011 05:09PM UTC by comment:15
description: | Have been doing some testing and realized trying to animate height / width to the "auto" value doesn't work. Submitting my quick patch that seems to solve this.\ \ Note: there may be a better way then to clone the node to get it's default values but I thought it worked well in this case. This shoul be pretty heavily scrutinized since I've only bounced the idea off a few other people.\ \ Add the following before line #6471 (Unminified jQuery 1.4.4 version):\ {{{\ if ( val === "auto" && ( name === "height" || name === "width" ) ){\ var $temp = jQuery(self).clone().css(name,"auto").appendTo("body");\ val = $temp.css(name);\ $temp.remove();\ }\ }}}\ \ → Have been doing some testing and realized trying to animate height / width to the "auto" value doesn't work. Submitting my quick patch that seems to solve this. \ \ Note: there may be a better way then to clone the node to get it's default values but I thought it worked well in this case. This shoul be pretty heavily scrutinized since I've only bounced the idea off a few other people. \ \ Add the following before line #6471 (Unminified jQuery 1.4.4 version): \ {{{ \ if ( val === "auto" && ( name === "height" || name === "width" ) ){ \ var $temp = jQuery(self).clone().css(name,"auto").appendTo("body"); \ val = $temp.css(name); \ $temp.remove(); \ } \ }}} \ \ |
---|
+0, If this doesn't take a lot of bytes I'm fer it, otherwise I'm agin' it. Animating things set to auto
naturally raises a flag with me, sorry that some people expect it to work somehow.
Changed December 13, 2011 05:28PM UTC by comment:16
description: | Have been doing some testing and realized trying to animate height / width to the "auto" value doesn't work. Submitting my quick patch that seems to solve this. \ \ Note: there may be a better way then to clone the node to get it's default values but I thought it worked well in this case. This shoul be pretty heavily scrutinized since I've only bounced the idea off a few other people. \ \ Add the following before line #6471 (Unminified jQuery 1.4.4 version): \ {{{ \ if ( val === "auto" && ( name === "height" || name === "width" ) ){ \ var $temp = jQuery(self).clone().css(name,"auto").appendTo("body"); \ val = $temp.css(name); \ $temp.remove(); \ } \ }}} \ \ → Have been doing some testing and realized trying to animate height / width to the "auto" value doesn't work. Submitting my quick patch that seems to solve this.\ \ Note: there may be a better way then to clone the node to get it's default values but I thought it worked well in this case. This shoul be pretty heavily scrutinized since I've only bounced the idea off a few other people.\ \ Add the following before line #6471 (Unminified jQuery 1.4.4 version):\ {{{\ if ( val === "auto" && ( name === "height" || name === "width" ) ){\ var $temp = jQuery(self).clone().css(name,"auto").appendTo("body");\ val = $temp.css(name);\ $temp.remove();\ }\ }}}\ \ |
---|
-1
Changed December 14, 2011 04:00PM UTC by comment:17
-1
Changed January 02, 2012 04:51PM UTC by comment:18
@mikesherov My current patch has nothing to do with the code posted when the bug was opened. It doesn't involve cloning elements anymore, see https://github.com/jquery/jquery/pull/515
@jaubourg The proposed patch adds exactly 58B to jquery minified and compressed
It saddens me to see some "-1" without any explanation. I wish I could understand and maybe answer your concerns.
Changed January 18, 2012 03:33AM UTC by comment:19
description: | Have been doing some testing and realized trying to animate height / width to the "auto" value doesn't work. Submitting my quick patch that seems to solve this.\ \ Note: there may be a better way then to clone the node to get it's default values but I thought it worked well in this case. This shoul be pretty heavily scrutinized since I've only bounced the idea off a few other people.\ \ Add the following before line #6471 (Unminified jQuery 1.4.4 version):\ {{{\ if ( val === "auto" && ( name === "height" || name === "width" ) ){\ var $temp = jQuery(self).clone().css(name,"auto").appendTo("body");\ val = $temp.css(name);\ $temp.remove();\ }\ }}}\ \ → Have been doing some testing and realized trying to animate height / width to the "auto" value doesn't work. Submitting my quick patch that seems to solve this. \ \ Note: there may be a better way then to clone the node to get it's default values but I thought it worked well in this case. This shoul be pretty heavily scrutinized since I've only bounced the idea off a few other people. \ \ Add the following before line #6471 (Unminified jQuery 1.4.4 version): \ {{{ \ if ( val === "auto" && ( name === "height" || name === "width" ) ){ \ var $temp = jQuery(self).clone().css(name,"auto").appendTo("body"); \ val = $temp.css(name); \ $temp.remove(); \ } \ }}} \ \ |
---|---|
keywords: | 1.8-discuss |
milestone: | 1.next → None |
resolution: | → wontfix |
status: | assigned → closed |
I agree that animating width/height auto should probably work "out of the box," but I am not quite sure that this patch will do that job - or if any patch could do that job accurately.