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 on table rows inside a relative div container gives different results based on scroll position #1708

Open
mgol opened this Issue Oct 16, 2014 · 7 comments

Comments

Projects
None yet
4 participants
@mgol
Member

mgol commented Oct 16, 2014

Originally reported by ripdog: http://bugs.jquery.com/ticket/15239

The docs for .position() clearly state that they return the "current position of an element relative to the offset parent." - explicitly not the position based on the viewport. However, .position does report different results based on where it is moved on the page due to scrolling.

Here's a fiddle: http://jsfiddle.net/6g5upsvq/ Click the button, then scroll and click it again. The .offsetTop direct from the DOM stays constant, but .position changes.

@mgol mgol added the Bug label Oct 16, 2014

@mgol mgol added this to the 3.0.0 milestone Oct 16, 2014

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 26, 2014

Member

@mikesherov Can I get your review on this?

Member

dmethvin commented Nov 26, 2014

@mikesherov Can I get your review on this?

@timmywil timmywil added the Offset label Jan 30, 2015

@timmywil timmywil removed this from the 3.0.0 milestone Jan 30, 2015

@timmywil timmywil added this to the 3.0.0 milestone May 5, 2015

@timmywil timmywil self-assigned this May 5, 2015

@timmywil timmywil closed this in 2d71594 May 12, 2015

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@markelog markelog removed this from the 1.12.0/2.2.0 milestone Feb 8, 2016

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 8, 2016

Member

1714 changes provoked new issues: #2836, #2828.

At the meeting, we decided to revert offending commit (in all three branches - 2.2-stable, 1.12-stable and master) and tackle this issue in 3.x

Member

markelog commented Feb 8, 2016

1714 changes provoked new issues: #2836, #2828.

At the meeting, we decided to revert offending commit (in all three branches - 2.2-stable, 1.12-stable and master) and tackle this issue in 3.x

@markelog markelog reopened this Feb 8, 2016

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Feb 8, 2016

Member

Changing this sounds like a breaking change so we might need to do it in 3.0.0 or wait for 4.0.0.

I've added the "Needs review" label so that we don't forget about this doubt of mine.

Member

mgol commented Feb 8, 2016

Changing this sounds like a breaking change so we might need to do it in 3.0.0 or wait for 4.0.0.

I've added the "Needs review" label so that we don't forget about this doubt of mine.

@mgol mgol added the Needs review label Feb 8, 2016

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 8, 2016

Member

Changing this sounds like a breaking change so we might need to do it in 3.0.0 or wait for 4.0.0.

Why?

Member

markelog commented Feb 8, 2016

Changing this sounds like a breaking change so we might need to do it in 3.0.0 or wait for 4.0.0.

Why?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Feb 8, 2016

Member

Hmm, OK, maybe not. I mean, this will surely break some code but it may be considered a bug fix due to the description in the API. I'd wait for fixes for edge cases that may break some code at least to a minor release, though.

I'll set the milestone to 3.1.0 for now so that we don't forget; we can reschedule later.

Member

mgol commented Feb 8, 2016

Hmm, OK, maybe not. I mean, this will surely break some code but it may be considered a bug fix due to the description in the API. I'd wait for fixes for edge cases that may break some code at least to a minor release, though.

I'll set the milestone to 3.1.0 for now so that we don't forget; we can reschedule later.

@mgol mgol removed the Needs review label Feb 8, 2016

@mgol mgol added this to the 3.1.0 milestone Feb 8, 2016

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Feb 9, 2016

Member

It's not clear exactly what is going on with this, whether the original report was even a bug or not. We could definitely use some better unit tests from somebody who knew what the right answers were. 😸

Member

dmethvin commented Feb 9, 2016

It's not clear exactly what is going on with this, whether the original report was even a bug or not. We could definitely use some better unit tests from somebody who knew what the right answers were. 😸

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@timmywil timmywil removed their assignment Sep 27, 2016

@timmywil timmywil modified the milestones: 3.2.0, 3.3.0 Mar 6, 2017

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Mar 27, 2017

Member

Per @mgol's comment, I think making this change in 4.0 is the safest option.

Member

timmywil commented Mar 27, 2017

Per @mgol's comment, I think making this change in 4.0 is the safest option.

@timmywil timmywil modified the milestones: 4.0.0, 3.3.0 Mar 27, 2017

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