fixes #9233, make sure runtimeStyle isn't affected by dimensions #562

Closed
wants to merge 2 commits into
from

2 participants

@mikesherov
jQuery Foundation member

may not have many use cases in the wild, but the fix was simple enough.

@rwaldron rwaldron commented on an outdated diff Oct 23, 2011
test/unit/dimensions.js
@@ -245,6 +245,21 @@ test("child of a hidden elem has accurate inner/outer/Width()/Height() see #944
$divNormal.remove();
});
+test("getting dimensions shouldnt modify runtimeStyle see #9233", function() {
+ var $div = jQuery( "<div>" ).appendTo( "body" ),
+ div = $div.get( 0 ),
+ runtimeStyle = div.runtimeStyle;
+
+ if ( runtimeStyle ) {
+ expect( 1 );
+
+ div.runtimeStyle.marginLeft = "12em";
+ div.runtimeStyle.left = "11em";
+ $div.outerWidth( true );
+ equal( div.runtimeStyle.left, "11em", "getting dimensions modifies runtimeStyle, see #9233" );
+ }
+});
+
@rwaldron
jQuery Foundation member
rwaldron added a line comment Oct 23, 2011

This test should be re-written to show that the correct behaviour happens in all supported browsers. Additionally, an expect() call should be made at the beginning, not within a condition - that defeats the entire purpose of setting expectations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rwaldron rwaldron and 2 others commented on an outdated diff Oct 23, 2011
test/unit/dimensions.js
@@ -245,6 +245,21 @@ test("child of a hidden elem has accurate inner/outer/Width()/Height() see #944
$divNormal.remove();
});
+test("getting dimensions shouldnt modify runtimeStyle see #9233", function() {
+ var $div = jQuery( "<div>" ).appendTo( "body" ),
@rwaldron
jQuery Foundation member
rwaldron added a line comment Oct 23, 2011

Whenever possible, append new elements to #qunit-fixture. This is super inconsistent across the entire test suite and we're all guilty of it, but we should start to make efforts to do the right thing in new tests. Unfortunately, sometimes tests will require appending stuff outside of #qunit-fixture, in those cases, be sure to remove the elements at the end of the test.

@timmywil
jQuery Foundation member
timmywil added a line comment Oct 27, 2011

Actually, I'd recommend never using #qunit-fixture as a selector in the tests. We should have a container of our own that QUnit doesn't depend on and always use that.

@mikesherov
jQuery Foundation member
mikesherov added a line comment Oct 27, 2011
@rwaldron
jQuery Foundation member
rwaldron added a line comment Oct 28, 2011

i misread your comment earlier... we absolutely should be appending temporary junk elements to #qunit-fixture, because it's guaranteed that they are cleaned up after every test. #qunit-fixture's sole purpose is to be a place for adding throw away junk elements.

@mikesherov
jQuery Foundation member
mikesherov added a line comment Oct 28, 2011

so... to qunit-fixute or not to qunit-fixture... that is the question?

@rwaldron
jQuery Foundation member
rwaldron added a line comment Oct 28, 2011

Use QUnit correctly, put the element in #qunit-fixture. Thanks.

@timmywil
jQuery Foundation member
timmywil added a line comment Oct 28, 2011

Sorry to confuse you. Rick is right. For now qunit-fixture is the way to go. Body is incorrect. I was making a comment on something that I want to change about all of our tests, but the current HTML in index.html wouldn't allow for it yet.

@mikesherov
jQuery Foundation member
mikesherov added a line comment Oct 28, 2011

No problem, and just so no one is confused any further, we are in a discussion on a line note from a commit that was overridden 4 days ago. I already updated the PR to use #qunit-fixture 4 days ago: mikesherov@7c08794

Ah, the joys of Github's line note discussion threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timmywil
jQuery Foundation member

Landed in commit fa0e801.

@timmywil timmywil closed this Oct 28, 2011
@davidpenuelab davidpenuelab pushed a commit that referenced this pull request Dec 3, 2013
@mikesherov mikesherov Landing pull request 562. Make sure runtimeStyle isn't affected by di…
…mensions. Fixes #9233.

More Details:
 - #562
 - http://bugs.jquery.com/ticket/9233
fa0e801
@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@mikesherov mikesherov Landing pull request 562. Make sure runtimeStyle isn't affected by di…
…mensions. Fixes #9233.

More Details:
 - jquery#562
 - http://bugs.jquery.com/ticket/9233
1917508
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment