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

Fix for scrolling up past images in the comments jerks you back down #845

Merged
merged 2 commits into from
Jun 23, 2023

Conversation

nahwneeth
Copy link
Contributor

@nahwneeth nahwneeth commented Jun 23, 2023

Closes #730
Closes #825

This happens only with images that go beyond the available canvas width. The attached videos shows 2 images one after another and the jerk down only occurs for the larger image which is of res 1600x1200 while only 990px was the available width.

This seems to be a problem with the CoilImagePlugin (or maybe that's how it's meant to behave) which doesn't call the image resolver if the canvas width is not known. The canvas width is obtained during the first paint call.

So, the initial bounds are 1600x1200 and during the first paint call, it realizes the image won't fit and reduces the bounds based on the canvas width. My assumption is this change in bounds is picked up by the lazy column and the jerk down happens trying to maintain the scroll position.

Other ways to test this:

  • Provide a custom image size resolver which always returns a fixed size and the jerk down will still happen.
  • Add a Modifier.onSizeChange() to the AndroidTextView and log the markdown & size. One can see that it initially takes up a large size and immediately reduces itself.

To solve this, I tried wrapping the AndroidTextView with a BoxWithConstraints and initialized the canvas width of AsyncDrawable with the maxWidth available. This seems to work. After the first paint call the canvas width is picked up and there is no (or minimum) change in bounds. Changing orientation of the device also works as intended.

bug.mov
fix.mov

Link to the post in the above videos.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Looks great now, thx!

@dessalines
Copy link
Member

I'll let @twizmwazin take a look before we merge.

@twizmwazin twizmwazin enabled auto-merge (squash) June 23, 2023 17:31
@twizmwazin twizmwazin merged commit e66e1c1 into LemmyNet:main Jun 23, 2023
1 check passed
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.

Post scrolling issues Scrolling up past images in the comments jerks you back down
3 participants