Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fixes #11293, don't swap to measure dimensions unless the element is display:none #807

Closed
wants to merge 1 commit into from

2 participants

@mikesherov
Collaborator

http://jqbug.com/11293

Running "compare_size:files" (compare_size) task
Sizes - compared to master
    251207       (+44)  dist/jquery.js                                         
     92011       (+29)  dist/jquery.min.js                                     
     33094        (+9)  dist/jquery.min.js.gz     

One note:

  • I really dislike that for elements that have offsetWidth 0, we attempt to make them visible by swapping out some css properties. Users really shouldn't be asking for measurements on hidden elements. I suppose we need it for back-compat, but I just really dislike it.
@mikesherov
Collaborator

Some other thoughts: So I suppose the initial idea behind jQuery.swap was a display:none element to get changed to block and be made invisible for measurement. Theoretically, this can also make inline-block and inline elements into block elements. So, I purposely only excluded elems with table style display values.

However, if we're willing to say that only display:none should be swapped, then I can update this PR to reflect that.

@mikesherov
Collaborator

Actually, I'm going to go ahead and do that. I think it'll also fix http://bugs.jquery.com/ticket/11522

@mikesherov
Collaborator

Updated size:

Running "compare_size:files" (compare_size) task
Sizes - compared to master
    251226       (+63)  dist/jquery.js                                         
     92010       (+28)  dist/jquery.min.js                                     
     33091        (+6)  dist/jquery.min.js.gz   

Also, this should theoretically fix http://bugs.jquery.com/ticket/11522

@rwaldron
Collaborator

Sorry, but I'm not landing anything that will conflict with the strip_iife branch (eg the doomed jQuery.defaultDisplay)

@mikesherov
Collaborator

@rwldrn , I'll revert that portion. Thanks for the heads up. I can make that optimization in another PR.

@mikesherov
Collaborator

@rwldrn, updated.

@rwaldron
Collaborator

@mikesherov rebase and I can land it :)

@mikesherov
Collaborator

@rwldrn rebased.

@mikesherov
Collaborator

Ugh, I rebased incorrectly. Give me a few minutes to correct.

@mikesherov mikesherov closed this
@mikesherov
Collaborator

Ugh, I rebased incorrectly. Give me a few minutes to correct.

@mikesherov mikesherov reopened this
@mikesherov
Collaborator

OK, this time it's right, I promise.

@rwaldron rwaldron closed this
@mikaelkaron mikaelkaron referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@markelog markelog referenced this pull request from a commit in markelog/jquery
@mikesherov mikesherov Don't swap to measure dimensions unless the element is display:none. #…
…807 Fixes #11293
a0d77a0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 7, 2012
  1. @mikesherov
This page is out of date. Refresh to see the latest.
Showing with 11 additions and 1 deletion.
  1. +1 −1  src/css.js
  2. +10 −0 test/unit/dimensions.js
View
2  src/css.js
@@ -480,7 +480,7 @@ jQuery.each([ "height", "width" ], function( i, name ) {
jQuery.cssHooks[ name ] = {
get: function( elem, computed, extra ) {
if ( computed ) {
- if ( elem.offsetWidth !== 0 ) {
+ if ( elem.offsetWidth !== 0 || curCSS( elem, "display" ) !== "none" ) {
return getWidthOrHeight( elem, name, extra );
} else {
return jQuery.swap( elem, cssShow, function() {
View
10 test/unit/dimensions.js
@@ -299,6 +299,16 @@ test("outerWidth(true) returning % instead of px in Webkit, see #10639", functio
equal( el.outerWidth(true), 400, "outerWidth(true) and css('margin') returning % instead of px in Webkit, see #10639" );
});
+test( "getting dimensions of zero width/height table elements shouldn't alter dimensions", function() {
+ expect( 1 );
+
+ var table = jQuery("<table><tbody><tr><td></td><td>a</td></tr><tr><td></td><td>a</td></tr></tbody></table>").appendTo("#qunit-fixture"),
+ elem = table.find("tr:eq(0) td:eq(0)");
+
+ table.find("td").css({ margin: 0, padding: 0 });
+ equal( elem.width(), elem.width(), "width() doesn't alter dimension values" );
+});
+
test("box-sizing:border-box child of a hidden elem (or unconnected node) has accurate inner/outer/Width()/Height() see #10413", function() {
expect(16);
Something went wrong with that request. Please try again.