Skip to content

Fix .offset() to work in ShadowDOM #2043

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

Closed
NekR wants to merge 9 commits into from
Closed

Fix .offset() to work in ShadowDOM #2043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6ae0de7
Offset: Fix .offset() to correctly work with ShadowDOM
NekR Jan 28, 2015
a97da28
Offset: Fix .offset() to correctly work with ShadowDOM
NekR Jan 28, 2015
f2790eb
Offset: Fix .offset() to correctly work with ShadowDOM
NekR Jan 28, 2015
7e12dc8
Offset: Fix .offset() to correctly work with ShadowDOM
NekR Jan 29, 2015
981cc26
Offset: Fix .offset() to correctly work with ShadowDOM
NekR Jan 30, 2015
65d9de9
Offset: Fix .offset() to correctly work with ShadowDOM
NekR Jan 30, 2015
ebf43fa
Offset: Fix .offset() to correctly work with ShadowDOM
NekR Jan 30, 2015
d0df2c6
Offset: Fix .offset() to correctly work with ShadowDOM
NekR Feb 19, 2015
2e7e1b1
Offset: Fix .offset() to correctly work with ShadowDOM
NekR Mar 2, 2015
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
26 changes: 12 additions & 14 deletions src/offset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ jQuery.offset = {
elem.style.position = "relative";
}

curOffset = curElem.offset();
curOffset = curElem.offset() || { top: 0, left: 0 };
Copy link
Member

@gibson042 gibson042 Feb 25, 2015

Choose a reason for hiding this comment

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

This relates to #2043 (comment) ... do we want to suppress exceptions when setting document-relative offset on no-layout elements? I'd like some other opinions here, but personally am inclined to drop this change and let the call throw.

Copy link
Member

@timmywil timmywil Feb 25, 2015

Choose a reason for hiding this comment

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

Ditto. +1 for throwing.

Copy link
Member

@markelog markelog Feb 26, 2015

Choose a reason for hiding this comment

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

Would need to mention it in the release notes

Copy link
Contributor Author

@NekR NekR Mar 1, 2015

Choose a reason for hiding this comment

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

This relates to #2043 (comment) ... do we want to suppress exceptions when setting document-relative offset on no-layout elements? I'd like some other opinions here, but personally am inclined to drop this change and let the call throw.

Do you suggest to just remove || { top: 0, left: 0 } or guard for curOffset and throw manually custom error?

Copy link
Member

@timmywil timmywil Mar 2, 2015

Choose a reason for hiding this comment

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

Per meeting, remove || { top: 0, left: 0 }.

Copy link
Member

@timmywil timmywil Mar 2, 2015

Choose a reason for hiding this comment

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

Actually, after further discussion, we'll leave as is and make this change separately. Sorry to confuse! Nothing further for you to do here @NekR

Copy link
Member

@gibson042 gibson042 Mar 2, 2015

Choose a reason for hiding this comment

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

Tracking in #2114.

curCSSTop = jQuery.css( elem, "top" );
curCSSLeft = jQuery.css( elem, "left" );
calculatePosition = ( position === "absolute" || position === "fixed" ) &&
Expand Down Expand Up @@ -82,28 +82,26 @@ jQuery.fn.extend({
});
}

var docElem, win,
var docElem, win, rect,
elem = this[ 0 ],
box = { top: 0, left: 0 },
doc = elem && elem.ownerDocument;

if ( !doc ) {
Copy link
Member

@gibson042 gibson042 Feb 3, 2015

Choose a reason for hiding this comment

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

I believe this can also go, along with the related "object without getBoundingClientRect" test, since all supported browsers have gBCR.

Copy link
Contributor Author

@NekR NekR Feb 3, 2015

Choose a reason for hiding this comment

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

I am not sure I got your message here. If you plan to remove that condition then you can remove it in separate thread. This PR is not about refacroting .offset() method, but about support ShadowDOM in it.

Copy link
Member

@gibson042 gibson042 Feb 3, 2015

Choose a reason for hiding this comment

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

Right; agreed.

Copy link
Member

@gibson042 gibson042 Mar 2, 2015

Choose a reason for hiding this comment

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

Tracking in #2115.

Copy link
Member

@timmywil timmywil May 5, 2015

Choose a reason for hiding this comment

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

Actually, that test is failing with this PR. I'm going to include #2115 when landing.

Copy link
Contributor Author

@NekR NekR May 6, 2015

Choose a reason for hiding this comment

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

@timmywil I am not sure about this, but as I remember that test did not worker even before I made any changes. Anyway, it's what all is resolved now, thanks!

return;
}

docElem = doc.documentElement;
rect = elem.getBoundingClientRect();

// Make sure it's not a disconnected DOM node
if ( !jQuery.contains( docElem, elem ) ) {
return box;
}
// Make sure element is not hidden (display: none) or disconnected
if ( rect.width || rect.height || elem.getClientRects().length ) {
win = getWindow( doc );
docElem = doc.documentElement;

box = elem.getBoundingClientRect();
win = getWindow( doc );
return {
top: box.top + win.pageYOffset - docElem.clientTop,
left: box.left + win.pageXOffset - docElem.clientLeft
};
return {
top: rect.top + win.pageYOffset - docElem.clientTop,
left: rect.left + win.pageXOffset - docElem.clientLeft
};
}
},

position: function() {
Expand Down
28 changes: 23 additions & 5 deletions test/unit/offset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,31 @@ test("object without getBoundingClientRect", function() {
equal( result.left, 0, "Check left" );
});

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

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

equal( result.top, 0, "Check top" );
equal( result.left, 0, "Check left" );
try {
result = jQuery( document.createElement("div") ).offset();
} catch ( e ) {}

ok( !result, "no position for disconnected element" );
});

test("hidden (display: none) element", function() {
expect(1);

var result,
node = jQuery("<div style='display: none' />").appendTo("#qunit-fixture");

try {
result = node.offset();
} catch ( e ) {}

node.remove();

ok( !result, "no position for hidden (display: none) element" );
});

testIframe("offset/absolute", "absolute", function($, iframe) {
Expand Down