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

"Contain" images with missing dimensions instead of cropping them #6828

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 28, 2024

Addresses #6800

As noted in #6474, we don't want to use dynamically fetched dimensions on the client because it causes layout shifts for any post(s) after the image. In the worst case this can cause an entire cascade of relayouts which is absolutely terrible for performance.

However, as discussed in #6800, cropping can feel pretty disruptive. Instead, let's try "containing" when the aspect ratio is unknown.

This only affects images with missing dimensions. (Currently, it's up to the client to specify the dimensions.) In longer term it would be nice if the server could somehow fill out the missing ones, but fixing that is out of scope of this PR.

I had to disable the lightbox animation for such images because the initial sizing is going to be different in the "contains" layout. While disabling the animation, I noticed flashes on first and last frames, which the two last commits take care of.

Test Plan

Find an account with images that are missing dimensions, for example https://bsky.app/profile/wrtrstat.bsky.social

Verify that images with unknown aspect ratio are now "contained" instead of cropped.

Verify that opening them in the lightbox works, but skips the expanding animation.

Verify that opening images with dimensions remains fluid and animated.

Test all platforms.

Before

Screenshot 2024-11-28 at 17 36 26 Screenshot 2024-11-28 at 17 36 33

After

Screenshot 2024-11-28 at 17 36 11 Screenshot 2024-11-28 at 17 36 17

@arcalinea arcalinea temporarily deployed to cover-nodims - social-app PR #6828 November 28, 2024 18:32 — with Render Destroyed
Copy link

Old size New size Diff
8.14 MB 8.14 MB 0 B (0.00%)

Copy link
Contributor

@haileyok haileyok left a comment

Choose a reason for hiding this comment

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

Tested web and Android, works well.

@gaearon gaearon merged commit 5f4a0f2 into main Nov 28, 2024
6 checks passed
@gaearon gaearon deleted the cover-nodims branch November 28, 2024 23:29
@WriterStat
Copy link

Hi, I like you all.

It works better than the previous system breaking fix, but it is not a good fix long term for lack of mandatory field communication. Temp fix at best. The changes from the last update need to be rolled back to the prior original working code before the breaking changes were made for now instead. And mandatory fields need to be communicated to your 3rd party and client post submission ecosystem, even if only in rule, then the code itself will basically age out fast, will no longer be used for any new posts. Problem goes away by itself.

Change instead needs to happen on image upload api/fields required for your client post submission ecosystem.

See the new posts on #6800

The previous breaking change made before this one made my page on Bluesky basically unusable, I wasn't the only one, with 20 million. This one will allow it to be read, but will make Bluesky less visually usable/likeable, a longer process to read/see and longer pages to interact with and you will be stuck with it. Make the change in image submission/image upload instead to fill the mandatory fields needed to work. That fixes your original problem.

Or get/calculate the data needed and store it on the first image request call, so it doesn't need to run the hack code more than one time. Also fixes your original problem.

@ChrisArchitect
Copy link

Rolling this back yet? Going to squares with css contain breaking a long held standard for opengraph preview image dimensions to fix 'layout shift' or whatever is not a good look. And it not being emphasized in the API docs/examples about the aspectratio parameter means many many things have been thrown out of wack.

gaearon added a commit that referenced this pull request Dec 4, 2024
)

* Show unknown aspect as "contain" for autosize

* Fix a flash of wrong position when opening in lightbox

* Fix last frame flash on Android
gaearon added a commit that referenced this pull request Dec 4, 2024
)

* Show unknown aspect as "contain" for autosize

* Fix a flash of wrong position when opening in lightbox

* Fix last frame flash on Android
Signez pushed a commit to Signez/bsky-social-app that referenced this pull request Dec 26, 2024
…uesky-social#6828)

* Show unknown aspect as "contain" for autosize

* Fix a flash of wrong position when opening in lightbox

* Fix last frame flash on Android
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.

5 participants