Skip to main content

Bug Tracker

Side navigation

#4512 closed bug (fixed)

Opened April 10, 2009 02:04AM UTC

Closed October 09, 2010 08:22PM UTC

Last modified March 09, 2012 10:45AM UTC

jquery-1.3.2.min.js "visible" selector doesn't run properly

Reported by: qiuzj Owned by: snover
Priority: high Milestone: 1.4.3
Component: css Version: 1.4.2
Keywords: Cc: dmethvin, snover
Blocked by: Blocking:
Description

HTML Code:

<table>

<tr style="display:none"><td>Value 1</td></tr>

<tr><td>Value 2</td></tr>

</table>

jQuery Code:

$("tr:visible")

Results:

[ <tr style="display:none"><td>Value 1</td></tr>,<tr><td>Value 2</td></tr> ]

Attachments (3)
  • 4512.diff (0.9 KB) - added by brandon May 18, 2009 01:50PM UTC.
  • test-all-elements.html (12.8 KB) - added by jvdneut July 01, 2010 01:16PM UTC.

    Test page for IE8 visibility problem

  • test2.html (1.2 KB) - added by brandon May 07, 2009 04:23PM UTC.
Change History (35)

Changed April 10, 2009 02:23AM UTC by qiuzj comment:1

Firefox:

[ <tr><td>Value 2</td></tr> ]

IE:

[ <tr style="display:none"><td>Value 1</td></tr>,<tr><td>Value 2</td></tr> ]

Changed May 06, 2009 03:50PM UTC by brandon comment:2

component: ajaxselector
milestone: 1.3.21.3.3
owner: → john

Changed May 06, 2009 03:54PM UTC by brandon comment:3

owner: johnbrandon

Changed May 06, 2009 03:55PM UTC by brandon comment:4

resolution: → invalid
status: newclosed

I'm unable to reproduce this. Please validate your HTML. If you are able to create a complete and clean test case, please feel free to reopen this ticket.

Changed May 07, 2009 04:24PM UTC by brandon comment:5

resolution: invalid
status: closedreopened

Found a complete test-case that is failing.

Changed May 07, 2009 04:30PM UTC by brandon comment:6

And realized my own test-case was flawed. I was looking at the td, not the tr.

The tr, although hidden with display: none, still has a width > 0. The height of the hidden tr is 0. The logic for the :visible/:hidden selectors was changed so that if an element has a width or a height, it is considered visible.

The reason it can have a height of 0 and width > 0 and still be visible is because a parent with uncleared floated elements is still visible but will have a height of 0.

Changed May 18, 2009 01:02PM UTC by benoit@gardentechno. comment:7

I just have a look on the source and I think the following code :

 return elem.offsetWidth > 0 || elem.offsetHeight > 0;
 

should be replaced by :

 return elem.offsetWidth > 0 && elem.offsetHeight > 0;
 

  • (jQuery/src/selector.js at line 984)
  • This works on firefox and ie6 event if the element is a TR

Changed May 18, 2009 01:16PM UTC by brandon comment:8

Replying to [comment:7 benoit@gardentechno.]:

I just have a look on the source and I think the following code :
>  return elem.offsetWidth > 0 || elem.offsetHeight > 0;
>  
should be replaced by :
>  return elem.offsetWidth > 0 && elem.offsetHeight > 0;
>  

Unfortunately an element can be "visible" even if one of its dimensions is 0. For example, a div that has un-cleared floated children will cause the parent's height to be 0 but the width will be > 0. Just because the height is 0 doesn't mean the element is hidden.

Changed May 18, 2009 01:22PM UTC by brandon comment:9

My thinking right now is to use the width/height checks for speed and if the result is ambiguous (one dimension is 0 and the other is > 0) then check the display property.

Changed May 18, 2009 03:39PM UTC by brandon comment:10

resolution: → fixed
status: reopenedclosed

fixed in r6357

Changed May 18, 2009 09:51PM UTC by vbabiy86 comment:11

resolution: fixed
status: closedreopened

I just checked out r6357 and it does not work in IE8 if I turn on compatibility mode it works fine.

Changed May 18, 2009 11:39PM UTC by dmethvin comment:12

Can you expand on "does not work" by providing a test case? Thanks.

Changed May 18, 2009 11:41PM UTC by brandon comment:13

Replying to [comment:12 dmethvin]:

Can you expand on "does not work" by providing a test case? Thanks.

I can verify that the patch does not work with the supplied test case in IE8. There is a thread in the dev mailing-list where I've posted another possible solution.

Changed June 23, 2009 05:29PM UTC by brandon comment:14

The other possible solution I posted was this.

Index: src/selector.js
===================================================================
--- src/selector.js     (revision 6357)
+++ src/selector.js     (working copy)
@@ -977,19 +977,21 @@
 jQuery.expr[":"] = jQuery.expr.filters;

 Sizzle.selectors.filters.hidden = function(elem){
-       var width = elem.offsetWidth, height = elem.offsetHeight;
-       return ( width === 0 && height === 0 ) ?
+       var width = elem.offsetWidth, height = elem.offsetHeight,
+               force = /tr/i.test( elem.tagName );
+       return ( width === 0 && height === 0 && !force ) ?
               true :
-               ( width !== 0 && height !== 0 ) ?
+               ( width !== 0 && height !== 0 && !force ) ?
                       false :
                       !!( jQuery.curCSS(elem, "display") === "none" );
 };

 Sizzle.selectors.filters.visible = function(elem){
-       var width = elem.offsetWidth, height = elem.offsetHeight;
-       return ( width === 0 && height === 0 ) ?
+       var width = elem.offsetWidth, height = elem.offsetHeight,
+               force = /^tr$/i.test( elem.tagName );
+       return ( width === 0 && height === 0 && !force ) ?
               false :
-               ( width > 0 && height > 0 ) ?
+               ( width > 0 && height > 0 && !force ) ?
                       true :
                       !!( jQuery.curCSS(elem, "display") !== "none" );
 };

Changed June 23, 2009 06:59PM UTC by brandon comment:15

resolution: → fixed
status: reopenedclosed

Fixed in r6413

Changed October 12, 2009 06:53PM UTC by alex88 comment:16

resolution: fixed
status: closedreopened

The same problem occurs for divs etc too (no only TRs), you can see it here:

http://jsbin.com/obavo

using ie8. Temp01 from irc channel suggested me this solution, replace:

force = /^tr$/i.test( elem.nodeName ); // ticket #4512

with:

force = true;

in nightly build rev. 6586, and now it's working fine.

Changed November 02, 2009 02:39PM UTC by davidserduke comment:17

See duplicate #4512

Changed November 11, 2009 10:35PM UTC by john comment:18

resolution: → fixed
status: reopenedclosed

Looks like this fix is still working, as far as I can tell.

Changed November 30, 2009 09:20AM UTC by januszjasinski comment:19

resolution: fixed
status: closedreopened

Replying to [comment:21 john]:

Looks like this fix is still working, as far as I can tell.

It is not working in IE8 as can be seen when going to http://jsbin.com/ijini and when loading the file here: http://dev.jquery.com/attachment/ticket/4512/test2.html

Changed November 30, 2009 11:00AM UTC by janusz comment:20

The only way I can get this to work is using the code below

$("#clicktoshoworhide").click(function() {  
var elem = $('#elementtoshoworhide')[0];
if(elem.style.display == 'none')
  $('#elementtoshoworhide').show();
    else
  {
    $('#elementtoshoworhide').hide();                       
}

Changed November 30, 2009 10:50PM UTC by dmethvin comment:21

resolution: → fixed
status: reopenedclosed

janusz, you're using jQuery 1.3.2 for your test and the fix is in the nightlies.

Changed December 04, 2009 07:06AM UTC by cipher comment:22

resolution: fixed
status: closedreopened

This is not fixed. In IE8 offsetWidth and offsetHeight are not zero for UL's as well.

I have tested the nightly jQuery file and is(':visible') does not work correctly (along with .toggle()) with UL's.

changing the code to:

force = /^(?:tr|ul)$/i.test(...);

has fixed the problem; I still don't like selectively testing certain elements for display:none. I think that if the elements height and width are 0, it's hidden; if not, it might be hidden but display:none should still be checked.

Changed December 05, 2009 08:01PM UTC by john comment:23

resolution: → fixed
status: reopenedclosed

@cipher. I created a test case but I got the result that I expected:

<script src="dist/jquery.js"></script>
<script>
$(window).load(function(){
        alert( $("ul:first").is(":hidden") );
        alert( $("ul:last").is(":hidden") );
});
</script>
<ul style="width:0;height:0;"></ul>
<ul style="display:none;"></ul>

The first one was false (height/width of zero, but still visible - correct) the second one was true (display: none, not visible).

Changed July 01, 2010 01:25PM UTC by jvdneut comment:24

resolution: fixed
status: closedreopened

Replying to [comment:27 john]:

I have encountered the same problem using jQuery 1.4.2 running on IE8 in standards mode but for the TD and TH tags.

I wrote the following test code to try and detect all the elements that were causing problems. It is by no means complete, but it does show that the problem occurs on more elements than just TR's (specifically it also occurs on BUTTON, TD, TH, CODE, and Q)

http://dev.jquery.com/attachment/ticket/4512/test-all-elements.html

I also discovered that the problem only occurs on IE8 running in "standards mode", meaning that pages that do not specify a doctype or include a <meta http-equiv="X-UA-Compatible" content="IE=7" /> do work correctly.

The problem is, as stated earlier, related to the offsetWidth and offsetHeight of the elements in question not being equal to 0 when hidden.

For my own purposes I modified the jQuery file to read:

skip = /^(tr|td|th)$/.test(elem.nodeName.toLowerCase());

instead of:

skip = elem.nodeName.toLowerCase() === "tr";

Changed October 05, 2010 03:22AM UTC by snover comment:25

cc: → dmethvin, snover
milestone: 1.41.4.3
need: ReviewPatch
priority: majorhigh
version: 1.3.21.4.2

So, just to clarify, the *principal* (but not only) issue here is that IE gives table elements a width and height whenever there is an additional table cell in the row that is not hidden but which also has a height.

We can work around that. This bug is far more nefarious, however, and I feel like laughing and crying at the same time.

In doing research for this issue, I have discovered that are some inexplicable rules dictating when offsetWidth/offsetHeight are non-zero:

1. button inside label (but not input)

2. q inside q (but not code inside code, div inside div, or span inside span)

3. any table cell as long as there is at least one table cell in the row that is not hidden and is not empty

4. any element followed by an inline element as long as the first element in the parent is a text node

That last one is particularly interesting. As far as I can tell, none of these rules are related to hasLayout, either.

In conclusion, I think we can safely patch the issue with :visible selector and tables, but unless someone can figure out the voodoo that is going on behind the scenes in IE’s warped little brain, someone will eventually be haunted by this shortcut again in another permutation.

Changed October 05, 2010 04:58AM UTC by snover comment:26

OK, so, modification to the rules, based off this plainly ridiculous test case.

1. Innermost nested q breaks if any of the parent q is followed by another node (other elements like span or code are OK)<br>

2. Any td or th or tr as long as there is at least one table cell in the row that is not hidden and not empty<br>

3. button preceeded by a button or non-block node<br>

4. Some voodoo combinations of inline elements, which also sometimes affect block-level elements but not usually

If someone can figure out that voodoo, I will be super-duper impressed.

FML Trident is a mess! Interestingly, IE9b1 in IE8 and IE9 modes pass, but IE9b1 in IE9 mode required the addition of a check to avoid testing html/body/head/etc..

Changed October 05, 2010 08:21AM UTC by snover comment:27

OK.

So, the reason that q appears on the surface to be special is because IE8 generates content. This is not immediately obvious in the above test-case because jsfiddle sets the content to "". That is not the same as setting the content to none, however, so the generated text nodes are still created. This makes q appear special but it is apparently not so much.

danheberden and I worked on this for a few hours, and the pattern seems to most closely match this description:

IE8 fails to return offsetWidth/Height properly for hidden elements whenever the element has inline siblings on both sides, or whenever the element is an only child and its parent has an inline next sibling. (Text nodes are apparently considered to be inline elements for the purposes of these decisions within Trident.)

I know what you are thinking—this sounds like a bug that one would expect to see from hasLayout—but forcing layout does not solve the problem, and the buggy elements are a mix of elements with and without layout. So, no luck there.

I think that the most important takeaway from this analysis is not the actual criteria under which IE8 screws up; rather, it is that the behaviour changes depending upon the existence or non-existence of ''generated content'', which means that it appears there is no way of 100% reliably detecting the conditions under which a specific hidden element may not correctly return offsetWidth/Height.

Testing for this condition is very easy, and I have written a patch to detect it. Unfortunately, once we know that offsetWidth/Height is unreliable, it looks like we’ll be forced to walk up the DOM every time someone wants to determine if an element is hidden in IE8. :(

Changed October 05, 2010 09:34AM UTC by snover comment:28

owner: brandonsnover
status: reopenednew

Changed October 05, 2010 09:34AM UTC by snover comment:29

status: newassigned

Changed October 05, 2010 08:30PM UTC by snover comment:30

need: PatchCommit

I would just like to mention that the final statement in my last comment was wrong; offsetWidth/Height are only wrong when the element itself has display: none set. If an ascendant is display: none, then offsetWidth/Height are still zero. Yay!

Pull request

Changed October 09, 2010 03:54AM UTC by snover comment:31

#6895 is a duplicate of this bug.

Changed October 09, 2010 08:22PM UTC by john comment:32

resolution: → fixed
status: assignedclosed

Changed October 09, 2010 08:22PM UTC by john comment:33

component: selectorcss

Changed October 31, 2010 08:44AM UTC by snover comment:34

#5670 is a duplicate of this ticket.

Changed November 19, 2010 10:10AM UTC by snover comment:35

#5567 is a duplicate of this ticket.