Skip to content

Offset: fix wrong origin value #334

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

Merged
merged 1 commit into from
Apr 10, 2020
Merged

Offset: fix wrong origin value #334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/offset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@ var oldOffset = jQuery.fn.offset;
jQuery.fn.offset = function() {
var docElem,
elem = this[ 0 ],
origin = { top: 0, left: 0 };
bogus = { top: 0, left: 0 };

if ( !elem || !elem.nodeType ) {
migrateWarn( "jQuery.fn.offset() requires a valid DOM element" );
return origin;
Copy link
Member

Choose a reason for hiding this comment

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

We only want to return undefined for the !elem case. Some code including older older versions of jQuery UI would pass window and expect the top/left of 0. jquery/jquery#3249

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it expect {left:0,top:0} for window? Why both 1.12.4 and 2.2.4 return undefined as I tested?

Copy link
Member

Choose a reason for hiding this comment

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

It sure does, thanks for catching that! I don't have time to look at it right now but I must have been thinking of some other edge case with .offset()? Looks like that ticket is specifically about calling a method on window, not returning the bogus value.

Copy link
Member

Choose a reason for hiding this comment

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

@dmethvin Since the issue was about a breaking change in jQuery 3.0.0, it seems we shouldn't care about it here as Migrate 3.x is meant to aid with upgrading to the future jQuery 4.0. Am I missing anything?

return undefined;
}

docElem = ( elem.ownerDocument || window.document ).documentElement;
if ( !jQuery.contains( docElem, elem ) ) {
migrateWarn( "jQuery.fn.offset() requires an element connected to a document" );
return origin;
return bogus;
}

return oldOffset.apply( this, arguments );
Expand Down
10 changes: 5 additions & 5 deletions test/offset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ QUnit.test( ".offset()", function( assert ) {
} );

expectWarning( assert, ".offset() on window", function() {
assert.deepEqual(
assert.strictEqual(
jQuery( window ).offset(),
bogus, "window bogus top/left 0"
undefined, "window undefined"
);
} );

Expand All @@ -38,10 +38,10 @@ QUnit.test( ".offset()", function( assert ) {
);
} );

expectWarning( assert, ".offset() on plain object", function() {
assert.deepEqual(
expectWarning(assert, ".offset() on plain object", function () {
assert.strictEqual(
jQuery( { space: "junk", zero: 0 } ).offset(),
bogus, "plain object bogus top/left 0"
undefined, "plain object undefined"
);
} );
} );