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.

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

.position() and .offsetParent() return incorrect results in FF + recent versions of Chrome #3479

Open
craigkovatch opened this Issue Dec 31, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@craigkovatch

craigkovatch commented Dec 31, 2016

The W3 offsetParent algorithm changed on 9/15. It now includes ancestor elements with a CSS transform applied. Chrome and (maybe?) FF are in compliance with the new spec, while IE and Safari are not. As a result .position() is now returning incorrect results in these browsers.

IMO jQuery will probably need to implement its own ancestor traversal rather than relying on DOM-provided .offsetParent, otherwise results will forever depend on the browser+version of the client.

See: w3c/csswg-drafts#409

@gibson042 gibson042 referenced this issue Jan 5, 2017

Merged

Fix .offset() and .position() bugs #3487

4 of 4 tasks complete
@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 5, 2017

Member

I just submitted gh-3487, which should correct .offset() and .position() behavior in browsers implementing w3c/csswg-drafts@180b348 (although we should still add new tests for this issue), but I don't want to override host environment offsetParent logic. Instead, I think we should deprecate .offsetParent()gh-3487 already eliminates internal use.

Member

gibson042 commented Jan 5, 2017

I just submitted gh-3487, which should correct .offset() and .position() behavior in browsers implementing w3c/csswg-drafts@180b348 (although we should still add new tests for this issue), but I don't want to override host environment offsetParent logic. Instead, I think we should deprecate .offsetParent()gh-3487 already eliminates internal use.

@gibson042 gibson042 added the Offset label Jan 5, 2017

@craigkovatch

This comment has been minimized.

Show comment
Hide comment
@craigkovatch

craigkovatch Jan 5, 2017

Thanks for hopping on this!

Looks like your solution is reliant on getBoundingClientRect -- have you accounted for the way Chrome's return value for this API changes when pinch-to-zoomed? (See also https://bugs.chromium.org/p/chromium/issues/detail?id=489206)

Also, does this compute the correct value when translation or scaling transformations are present? I don't see anything in the tests around that property.

craigkovatch commented Jan 5, 2017

Thanks for hopping on this!

Looks like your solution is reliant on getBoundingClientRect -- have you accounted for the way Chrome's return value for this API changes when pinch-to-zoomed? (See also https://bugs.chromium.org/p/chromium/issues/detail?id=489206)

Also, does this compute the correct value when translation or scaling transformations are present? I don't see anything in the tests around that property.

@craigkovatch

This comment has been minimized.

Show comment
Hide comment
@craigkovatch

craigkovatch Jan 5, 2017

Also, my suggestion that these methods don't rely on offsetParent (whether DOM or jQuery) was because the results will differ depending on how recent the browser is. I'm not clear whether you meant you want to eliminate internal use if $.offsetParent() or use of both the jQuery and DOM methods.

My understanding was always that jQuery tries to hide these details "under the covers" so the author doesn't have to do browser-sniffing. Is that still the case?

craigkovatch commented Jan 5, 2017

Also, my suggestion that these methods don't rely on offsetParent (whether DOM or jQuery) was because the results will differ depending on how recent the browser is. I'm not clear whether you meant you want to eliminate internal use if $.offsetParent() or use of both the jQuery and DOM methods.

My understanding was always that jQuery tries to hide these details "under the covers" so the author doesn't have to do browser-sniffing. Is that still the case?

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 6, 2017

Member

Looks like your solution is reliant on getBoundingClientRect -- have you accounted for the way Chrome's return value for this API changes when pinch-to-zoomed? (See also https://bugs.chromium.org/p/chromium/issues/detail?id=489206)

Also, does this compute the correct value when translation or scaling transformations are present? I don't see anything in the tests around that property.

Explicitly no to both, and in fact we'll soon be documenting that .position() is not reliable for elements with transformed ancestors (cf. #3193 (comment) ), although that may change in the future (and if so, gh-3487 is a step towards it).

Also, my suggestion that these methods don't rely on offsetParent (whether DOM or jQuery) was because the results will differ depending on how recent the browser is. I'm not clear whether you meant you want to eliminate internal use if $.offsetParent() or use of both the jQuery and DOM methods.

My understanding was always that jQuery tries to hide these details "under the covers" so the author doesn't have to do browser-sniffing. Is that still the case?

It depends on the situation. In this case, it's far better to live with the host environment's definition of offsetParent (upon which box model properties depend) than to impose the CSSOM View algorithm and start diverging from DOM and CSS properties in a variety of painful and incompatible ways. In other words, .position() should report relative to the actual offset parent, not relative to what the offset parent should be.

Member

gibson042 commented Jan 6, 2017

Looks like your solution is reliant on getBoundingClientRect -- have you accounted for the way Chrome's return value for this API changes when pinch-to-zoomed? (See also https://bugs.chromium.org/p/chromium/issues/detail?id=489206)

Also, does this compute the correct value when translation or scaling transformations are present? I don't see anything in the tests around that property.

Explicitly no to both, and in fact we'll soon be documenting that .position() is not reliable for elements with transformed ancestors (cf. #3193 (comment) ), although that may change in the future (and if so, gh-3487 is a step towards it).

Also, my suggestion that these methods don't rely on offsetParent (whether DOM or jQuery) was because the results will differ depending on how recent the browser is. I'm not clear whether you meant you want to eliminate internal use if $.offsetParent() or use of both the jQuery and DOM methods.

My understanding was always that jQuery tries to hide these details "under the covers" so the author doesn't have to do browser-sniffing. Is that still the case?

It depends on the situation. In this case, it's far better to live with the host environment's definition of offsetParent (upon which box model properties depend) than to impose the CSSOM View algorithm and start diverging from DOM and CSS properties in a variety of painful and incompatible ways. In other words, .position() should report relative to the actual offset parent, not relative to what the offset parent should be.

@craigkovatch craigkovatch reopened this Jan 6, 2017

@craigkovatch

This comment has been minimized.

Show comment
Hide comment
@craigkovatch

craigkovatch Jan 6, 2017

(Whoops, wrong button^)

I certainly appreciate how what you're suggesting is the technically 'right' thing to do. But I'm concerned then that consumers of jQuery will all have to start writing checks as to which implementation of offsetParent the current browser supports, and code around that -- or not use .position()/.offset() at all. This doesn't seem like a positive step for what is a very fundamental library for the web.

(That situation, for what it's worth, is exactly how I found this bug. And I sent an email to several hundred other browser developers at my company as a warning "hey, don't rely on this method anymore, and check any existing uses".)

craigkovatch commented Jan 6, 2017

(Whoops, wrong button^)

I certainly appreciate how what you're suggesting is the technically 'right' thing to do. But I'm concerned then that consumers of jQuery will all have to start writing checks as to which implementation of offsetParent the current browser supports, and code around that -- or not use .position()/.offset() at all. This doesn't seem like a positive step for what is a very fundamental library for the web.

(That situation, for what it's worth, is exactly how I found this bug. And I sent an email to several hundred other browser developers at my company as a warning "hey, don't rely on this method anymore, and check any existing uses".)

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 6, 2017

Member

Hmm, after exploring live examples, I see what you mean. I guess I'd like the other PR to land, then we can update .offsetParent() to match the spec and add it back to .position().

Member

gibson042 commented Jan 6, 2017

Hmm, after exploring live examples, I see what you mean. I guess I'd like the other PR to land, then we can update .offsetParent() to match the spec and add it back to .position().

@gibson042 gibson042 added this to the 3.2.0 milestone Jan 6, 2017

@craigkovatch

This comment has been minimized.

Show comment
Hide comment
@craigkovatch

craigkovatch Jan 6, 2017

Fine by me! Thank you!

craigkovatch commented Jan 6, 2017

Fine by me! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment