Skip to content

Commit

Permalink
Make sure that .offset() doesn't fail against disconnected DOM nodes.…
Browse files Browse the repository at this point in the history 
… Fixes #4996.
  • Loading branch information
@jeresig
jeresig committed Sep 28, 2010
1 parent 7be1120 commit cf672a2
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 3 deletions.
12 changes: 9 additions & 3 deletions src/offset.js
View file
Expand Up @@ -5,7 +5,7 @@ var rtable = /^t(?:able|d|h)$/i,


if ( "getBoundingClientRect" in document.documentElement ) { if ( "getBoundingClientRect" in document.documentElement ) {
jQuery.fn.offset = function( options ) { jQuery.fn.offset = function( options ) {
var elem = this[0]; var elem = this[0], box;


if ( options ) { if ( options ) {
return this.each(function( i ) { return this.each(function( i ) {
Expand All @@ -21,8 +21,14 @@ if ( "getBoundingClientRect" in document.documentElement ) {
return jQuery.offset.bodyOffset( elem ); return jQuery.offset.bodyOffset( elem );
} }


var box = elem.getBoundingClientRect(), try {
doc = elem.ownerDocument, box = elem.getBoundingClientRect();

} catch(e) {
box = { top: elem.offsetTop, left: elem.offsetLeft };

This comment has been minimized.

Sign in to view

Copy link
@jitter

jitter Sep 29, 2010

Contributor

This may not work as expected as IE6/7 return -2 for offsetTop and offsetLeft for disconnected DOM nodes and not 0 as the other browsers do

This comment has been minimized.

Sign in to view

Copy link
@jeresig

jeresig Sep 29, 2010

Author Member

Yeah, the more I think about it the more obvious it is that it should just be 0 (it is disconnected, after all). Landed it in ec7ea3f

This comment has been minimized.

Sign in to view

Copy link
@jitter

jitter Sep 30, 2010

Contributor

Oops actually I'm wrong with my comment above. IE6/7 correctly return 0 for both elem.offsetTop and elem.offsetLeft. But they return 2 for docElem.clientTop and docElem.clientLeft. Which, if I understand correctly, means that IE6/7 by default assign <html> a border of 2px.

And because of Line 39-40
top = box.top + scrollTop - clientTop,
left = box.left + scrollLeft - clientLeft;

we end up with { top: -2, left: -2 }. But I don't know if it's worth to bother that { top: 0, left: 0 } gets returned in IE6/IE7 on detached elements.

To help the decision I made some quick tests and it seems that IE6 (can't test IE7 at the moment) only fails on elem.getBoundingClientRect() if we have a single detached element. e.g.
//tested with jquery before the try-catch-fix
$("

").offset(); //throws an Exception in IE6
$("

").offset(); //doesn't throw an Exception in IE6 but returns { top: -2, left: -2 };
$("

").children("p").offset(); //doesn't throw an Exception in IE6 but returns { top: -2, left: -2 };

The single detached element case could be easily checked by looking at elem.parentNode == null and then just return { top: 0, left: 0 } but for a detached structure we would need to walk up until parentNode == null and then check if that node is a HTMLDocument element or not. If it isn't the whole structure is detached and we should probably return { top: 0, left: 0 }. But this could get very costly if we start from deep-within a large detached dom-structure.

So I'm not really sure if we should do anything at all or just fix the test-case to not fail in IE6/7 :D
}

var doc = elem.ownerDocument,
body = doc.body, body = doc.body,
docElem = doc.documentElement, docElem = doc.documentElement,
win = getWindow(doc), win = getWindow(doc),
Expand Down
9 changes: 9 additions & 0 deletions test/unit/offset.js
View file
@@ -1,5 +1,14 @@
module("offset"); module("offset");


test("disconnected node", function() {
expect(2);

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

equals( result.top, 0, "Check top" );
equals( result.left, 0, "Check left" );
});

var supportsScroll = false; var supportsScroll = false;


testoffset("absolute"/* in iframe */, function($, iframe) { testoffset("absolute"/* in iframe */, function($, iframe) {
Expand Down

0 comments on commit cf672a2

Please sign in to comment.