Skip to content
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

Stop setting container scrollTop to zero on render if collection has offset #164

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gwak
Copy link

@gwak gwak commented Dec 20, 2017

Add scroll test where a collection that is in the middle of the scroll container, offset by some amount, and when rendering it, it should not reset the existing container scroll position.
See comment

…l container, offset by some amount, and when rendering it, it should not reset the existing container scroll position
@gwak gwak changed the title Add Scroll test for when a collection is in the middle of its container Stop setting container scrollTop to zero on render if collection has offset Dec 20, 2017
@pzuraq
Copy link
Contributor

pzuraq commented Dec 20, 2017

Ok, I will be out for the next two weeks but will take a look at it as soon as I’m back. Thank you!

@gwak
Copy link
Author

gwak commented Jan 30, 2018

Hi, do you need anything else for the PR to be merged ?

@pzuraq
Copy link
Contributor

pzuraq commented Jan 30, 2018

I don't think this is quite the right fix unfortunately, and I haven't had the time to look at the issue and fix it myself. If you can rebase and dive in a bit deeper to try to get the right fix I can try to get it merged asap, I'm also available for guidance if needed.

scrollDiff is supposed to represent the delta between where VC thinks it is and what reality is. This is particularly important in dynamic collections - for instance, let's say you're scrolling upward and VC inserts an element that it recorded previously was 50px high. We insert the element and remeasure it, and it turns out that the content was actually 100px high. If we do nothing, it will appear to the user that the content jumped 50px downward, which will look really bad. In that case, the scrollDiff should be 50, and we should move the scroll position to fix it.

This PR would disable all of that logic if the VC had an offset in its container. It's surprising to me that no tests fail, definitely points to a lack of coverage in this area. The correct fix would account for this._collectionOffset when calculating the scrollDiff.

Feel free to reach out in the #e-smoke-and-mirrors on the Ember Community slack, or on here!

@runspired
Copy link
Contributor

I originally setup vertical-collection to allow for arbitrary padding on either side of the scrollable content in the scroll container to support loading states, table headers/footers and the like. It also helps for when the document body is the scroll container. Seems bad if this is broken :D

I read through the comments on the other thread and peeked at this. I agree this solution here isn't quite right, but if it gets rebased I'm sure we can massage it to a good place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants