Skip to content

Offset: Don't warn against usage on disconnected elements #359

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
May 20, 2020
Merged

Offset: Don't warn against usage on disconnected elements #359

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
18 changes: 2 additions & 16 deletions src/jquery/offset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,13 @@ import { migrateWarn } from "../main.js";

var oldOffset = jQuery.fn.offset;

function isAttached( elem ) {
return jQuery.contains( elem.ownerDocument, elem );
}

jQuery.fn.offset = function() {
var elem = this[ 0 ],
bogus = { top: 0, left: 0 };
var elem = this[ 0 ];

if ( !elem || !elem.nodeType ) {
if ( elem && ( !elem.nodeType || !elem.getBoundingClientRect ) ) {
migrateWarn( "jQuery.fn.offset() requires a valid DOM element" );
return arguments.length ? this : undefined;
}

if ( !isAttached( elem ) ) {
migrateWarn( "jQuery.fn.offset() requires an element connected to a document" );

// Only return the bogus value for the getter.
if ( !arguments.length ) {
return bogus;
}
}

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

expectWarning( assert, ".offset() on disconnected node", function() {
expectNoWarning( assert, ".offset() on disconnected node", function() {
assert.deepEqual(
jQuery( document.createElement( "div" ) ).offset(),
bogus, "disconnected bogus top/left 0"
);
} );

expectWarning( assert, ".offset() as setter on disconnected node", 2,
expectNoWarning( assert, ".offset() as setter on disconnected node",
function() {
var offset,
$elemInitial = jQuery( "<div />" )
Expand All @@ -55,7 +55,7 @@ QUnit.test( ".offset()", function( assert ) {
assert.strictEqual( offset.left, 99, "proper left offset" );
} );

expectWarning( assert, ".offset() on empty set", 2, function() {
expectNoWarning( assert, ".offset() on empty set", function() {
var $empty = jQuery();

assert.strictEqual( $empty.offset(), undefined, ".offset() returns undefined" );
Expand Down
5 changes: 2 additions & 3 deletions warnings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,8 @@ This is _not_ a warning, but a console log message the plugin shows when it firs
**Solution**: It is almost always a mistake to use `.removeAttr( "checked" )` on a DOM element. The only time it might be useful is if the DOM is later going to be serialized back to an HTML string. In all other cases, `.prop( "checked", false )` should be used instead.

### JQMIGRATE: jQuery.fn.offset() requires a valid DOM element
### JQMIGRATE: jQuery.fn.offset() requires an element connected to a document

**Cause:** In earlier versions of jQuery, the `.offset()` method would return a value of `{ top: 0, left: 0 }` for some cases of invalid input. jQuery 3.0 throws errors in some of these cases. The selected element in the jQuery collection must be a DOM element that is currently in a document. Text nodes, the `window` object, plain JavaScript objects, and disconnected elements are not valid input to the `.offset()` method. jQuery *may* throw an error in those cases but in general does not guarantee specific results with invalid inputs.
**Cause:** In earlier versions of jQuery, the `.offset()` method would return a value of `{ top: 0, left: 0 }` for some cases of invalid input. jQuery 3.0 throws errors in some of these cases. The selected element in the jQuery collection must be a DOM element that has a `getBoundingClientRect` method. Text nodes, the `window` object, and plain JavaScript objects are not valid input to the `.offset()` method. jQuery *may* throw an error in those cases but in general does not guarantee specific results with invalid inputs.

**Solution**: Do not attempt to get or set the offset information of invalid input.

Expand Down Expand Up @@ -264,4 +263,4 @@ See jQuery-ui [commit](https://github.com/jquery/jquery-ui/commit/c0093b599fcd58

**Cause:** jQuery 3.5.0 changed the way it processes HTML strings. Previously, jQuery would attempt to fix self-closed tags like `<i class="test" />` that the HTML5 specification says are not self-closed, turning it into `<i class="test"></i>`. This processing can create a [security problem](https://nvd.nist.gov/vuln/detail/CVE-2020-11022) with malicious strings, so the functionality had to be removed.

**Solution:** Search for the reported HTML strings and edit the tags to close them explicitly. In some cases the strings passed to jQuery may be created inside the program and thus not searchable. Migrate warning messages include a stack trace that can be used to find the location of the usage in the code.
**Solution:** Search for the reported HTML strings and edit the tags to close them explicitly. In some cases the strings passed to jQuery may be created inside the program and thus not searchable. Migrate warning messages include a stack trace that can be used to find the location of the usage in the code.