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.

Sign up for GitHub

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

Jump to bottom

Test: ensure position/offset return mutable objects #3695

Merged
merged 2 commits into from Jul 10, 2017
Merged

Test: ensure position/offset return mutable objects #3695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
246d362
Test: ensure position/offset return mutable objects
jbedard Jun 15, 2017
7fcd1dd
Revert "Offset: Resolve strict mode ClientRect "no setter" exception"
jbedard Jul 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/offset.js
View file
Open in desktop
Expand Up @@ -140,10 +140,8 @@ jQuery.fn.extend( {

// Incorporate borders into its offset, since they are outside its content origin
parentOffset = jQuery( offsetParent ).offset();
parentOffset = {
top: parentOffset.top + jQuery.css( offsetParent, "borderTopWidth", true ),
left: parentOffset.left + jQuery.css( offsetParent, "borderLeftWidth", true )
};
parentOffset.top += jQuery.css( offsetParent[ 0 ], "borderTopWidth", true );
parentOffset.left += jQuery.css( offsetParent[ 0 ], "borderLeftWidth", true );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these need to be offsetParent, not offsetParent[ 0 ].

}
}

Expand Down
36 changes: 31 additions & 5 deletions test/unit/offset.js
View file
Open in desktop
Expand Up @@ -65,7 +65,7 @@ QUnit.test( "empty set", function( assert ) {
} );

QUnit.test( "disconnected element", function( assert ) {
assert.expect( 3 );
assert.expect( 4 );

var result = jQuery( document.createElement( "div" ) ).offset();

Expand All @@ -75,10 +75,11 @@ QUnit.test( "disconnected element", function( assert ) {
assert.equal( result.top, 0, "Retrieving offset on disconnected elements returns zeros (gh-2310)" );
assert.equal( result.left, 0, "Retrieving offset on disconnected elements returns zeros (gh-2310)" );
assert.equal( Object.keys( result ).length, 2, "Retrieving offset on disconnected elements returns offset object (gh-3167)" );
assert.equal( jQuery.isPlainObject( result ), true, "Retrieving offset on disconnected elements returns plain object (gh-3612)" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be actually trying to mutate the object here? The jQuery.isPlainObject() test is a bit of an indirect feature test and technically we could be returning a plain object with read-only properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I was just ensuring it wasn't a ClientRect. I could create a helper that checks both isPlainObject and ensures it is modifiable. WDYT?

} );

QUnit.test( "hidden (display: none) element", function( assert ) {
assert.expect( 3 );
assert.expect( 4 );

var node = jQuery( "<div style='display: none' />" ).appendTo( "#qunit-fixture" ),
result = node.offset();
Expand All @@ -91,10 +92,11 @@ QUnit.test( "hidden (display: none) element", function( assert ) {
assert.equal( result.top, 0, "Retrieving offset on hidden elements returns zeros (gh-2310)" );
assert.equal( result.left, 0, "Retrieving offset on hidden elements returns zeros (gh-2310)" );
assert.equal( Object.keys( result ).length, 2, "Retrieving offset on hidden elements returns offset object (gh-3167)" );
assert.equal( jQuery.isPlainObject( result ), true, "Retrieving offset on hidden elements returns plain object (gh-3612)" );
} );

QUnit.test( "0 sized element", function( assert ) {
assert.expect( 3 );
assert.expect( 4 );

var node = jQuery( "<div style='margin: 5px; width: 0; height: 0' />" ).appendTo( "#qunit-fixture" ),
result = node.offset();
Expand All @@ -104,10 +106,11 @@ QUnit.test( "0 sized element", function( assert ) {
assert.notEqual( result.top, 0, "Retrieving offset on 0 sized elements (gh-3167)" );
assert.notEqual( result.left, 0, "Retrieving offset on 0 sized elements (gh-3167)" );
assert.equal( Object.keys( result ).length, 2, "Retrieving offset on 0 sized elements returns offset object (gh-3167)" );
assert.equal( jQuery.isPlainObject( result ), true, "Retrieving offset on 0 sized elements returns plain object (gh-3612)" );
} );

QUnit.test( "hidden (visibility: hidden) element", function( assert ) {
assert.expect( 3 );
assert.expect( 4 );

var node = jQuery( "<div style='margin: 5px; visibility: hidden' />" ).appendTo( "#qunit-fixture" ),
result = node.offset();
Expand All @@ -117,6 +120,23 @@ QUnit.test( "hidden (visibility: hidden) element", function( assert ) {
assert.notEqual( result.top, 0, "Retrieving offset on visibility:hidden elements (gh-3167)" );
assert.notEqual( result.left, 0, "Retrieving offset on visibility:hidden elements (gh-3167)" );
assert.equal( Object.keys( result ).length, 2, "Retrieving offset on visibility:hidden elements returns offset object (gh-3167)" );
assert.equal( jQuery.isPlainObject( result ), true, "Retrieving offset on visibility:hidden elements returns plain object (gh-3612)" );
} );

QUnit.test( "normal element", function( assert ) {
assert.expect( 4 );

var node = jQuery( "<div>" ).appendTo( "#qunit-fixture" ),
offset = node.offset(),
position = node.position();

node.remove();

assert.equal( Object.keys( offset ).length, 2, "Retrieving offset on normal elements returns offset object (gh-3612)" );
assert.equal( jQuery.isPlainObject( offset ), true, "Retrieving offset on normal elements returns plain object (gh-3612)" );

assert.equal( Object.keys( position ).length, 2, "Retrieving position on normal elements returns offset object (gh-3612)" );
assert.equal( jQuery.isPlainObject( position ), true, "Retrieving position on normal elements returns plain object (gh-3612)" );
} );

testIframe( "absolute", "offset/absolute.html", function( assert, $, iframe ) {
Expand Down Expand Up @@ -350,7 +370,7 @@ testIframe( "static", "offset/static.html", function( assert, $ ) {
} );

testIframe( "fixed", "offset/fixed.html", function( assert, $, window ) {
assert.expect( 34 );
assert.expect( 38 );

var tests, $noTopLeft;

Expand All @@ -377,8 +397,12 @@ testIframe( "fixed", "offset/fixed.html", function( assert, $, window ) {
assert.ok( true, "Browser doesn't support scroll position." );
assert.ok( true, "Browser doesn't support scroll position." );
assert.ok( true, "Browser doesn't support scroll position." );
assert.ok( true, "Browser doesn't support scroll position." );
assert.ok( true, "Browser doesn't support scroll position." );

} else if ( window.supportsFixedPosition ) {
assert.equal( jQuery.isPlainObject( $( this.id ).offset() ), true, "jQuery('" + this.id + "').offset() is plain object" );
assert.equal( jQuery.isPlainObject( $( this.id ).position() ), true, "jQuery('" + this.id + "').position() is plain object" );
assert.equal( $( this.id ).offset().top, this.offsetTop, "jQuery('" + this.id + "').offset().top" );
assert.equal( $( this.id ).position().top, this.positionTop, "jQuery('" + this.id + "').position().top" );
assert.equal( $( this.id ).offset().left, this.offsetLeft, "jQuery('" + this.id + "').offset().left" );
Expand All @@ -390,6 +414,8 @@ testIframe( "fixed", "offset/fixed.html", function( assert, $, window ) {
assert.ok( true, "Fixed position is not supported" );
assert.ok( true, "Fixed position is not supported" );
assert.ok( true, "Fixed position is not supported" );
assert.ok( true, "Fixed position is not supported" );
assert.ok( true, "Fixed position is not supported" );
}
} );

Expand Down