-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Recent regression in display of image embeds without pre-existing dimensions data #6800
Comments
It wouldn't cause any flash/jump at all, just need to set css |
I think that would make images with aspect ratios > 1 get a lot of unwarranted empty space though |
On that note, I think a lot of people would really appreciate a setting to disable image cropping in general (in favor of letterboxing) |
Tbh, in practical terms, no one would see or mind the shift because using a laptop, phone etc... you only see the first 2 or 3 posts, and not the ones below until you scroll, and the shift is fast enough to not be seen on the first 2 or 3. If you were going to use bars, use the theme color, white for the light theme, black or dark for the dark theme? This seems like we are going backwards, making a fix for a problem that isn't really a problem. |
Maybe give the user an option to choose either on their page, based on preference. Removing to go backwards doesn't seem the best. With that said. if it speeds up the servers then ok, but most software resizes images to fit in today's world or gives the option for letter box based on theme colors. |
Chiming in to add a little detail: @GlitchyPSIX and I noticed that, clearly, a lot (if not all?) of images posted by 3rd party clients/bots are affected since the It might just me misunderstanding or looking at the wrong thing though so maybe this is a red herring. |
To expand on this, the fact that this field isn't required but also didn't exist until a few months ago makes me think the PDS would (should?) backfill that data if missing? Footnotes
|
This issue is across all platforms for the official app as of 1.95 (I personally confirmed it on iOS, android, and both mobile web and desktop web). Third party bluesky apps like TOKIMEKI and deck blue are still displaying images normally. |
Yeah, I hear that it's frustrating that this isn't being enforced (fwiw enforcing it would have been a breaking change — which is impossible in Lexicon). I also want to point out that as long as the client receives images without dimensions, with the old implementation, the layout shifts were inevitable which affect every post below the image in question in the feed. Layout shifts on scroll are inexcusable for a feed app. I totally hear the frustration but we needed to plug this perf/UX problem, and it is necessary for the clients to be aware that this field needs to be sent. So some kind of intervention was necessary. I'll need to double check with the backend team whether there is any way those can be backfilled automatically. I would certainly prefer that myself too. My understanding of the constraints is that this was not possible, but I'll ask again. |
You prolly already do all this, given this thought already... But in case helpful. We do the ratio on upload(server side) when not provided as you mentioned. And store in state. For images that don't have ratio for any reason, we do that ratio only on image request server side then send and store for next time. Or store and send. That way you don't have to do the entire back log at once or really ever. I'm guessing you do something similar/or already thought of this. I guess you could start a cron job to do it, but better to do on need/request just once. |
Potential fix / improvement PR #6828 |
Re: server stuff, I'll have to sync with the backend team on this, but probably after the US holidays. |
The only downside of using css object-fit: contain; is there will be a lot/a ton of unused blank space in the feed between text, horizontal images, and even posts in some cases in the posts. As a poster above mentioned. And in the light theme, an unusual large gray background of totally empty gray space between text and images. It's ok as a temp fix, till replaced. But is unsightly in practical use, with lots of posts/in a feed, it is better than the current cropped out square that's there now, but not as good as the original functionality that was already in there prior to this change. I just tested it by changing all the page CSS, there is a lot of empty space between stuff on horizontal images. Note: I never saw the shifts in layout at any point, but truthfully users are used to dynamic content in a layout loading pretty much in web pages. IMHO contain isn't a long term fix. |
I appreciate you and all of the bluesky team working hard to help and on this platform. And you working diligently on this. I know you are all pulled thin. So we appreciate it. When you string a few posts into a feed using the CSS contained posts together in the light theme you will find that the feed may double/triple/quadruple etc... in length to read, for the same amount of posts and not look that good doing it. In the end, I'm just a user of the software so you'll need to do what's best for you. But this still feels like a change that should be rolled back until a good/true fix can be implemented/communicated to third party clients. Best, -C |
Ok, bear with me, I have a thought: If:
|
Here's my last post, I'm passionate about this platform, hope that's ok. =) You now have over 20 million users on the system you can't make system breaking changes for them without getting your third party client apps and posts submission and posts scheduling apps involved(communication and mandatory fields/data etc...). Now you are forced into fixing it. Making a plan on the go/fly.... I like you. And the platform. You are no longer small. Was this change that broke the system an emergency? Before the change? Were third party client apps and posts submission and posts scheduling apps let known of the changes beforehand now being mandatory so they could update their apps before the system breaking change? You can't fix a communication issue with a client side tech fix. And one that really needs a tech fix to be fixed on the upload instead. You can only bandage it. Temp fix it or roll it back and then communicate. Here's what my page looks like after the last system breaking change(basically unusable): Here's what it will look like after the next layout breaking change: Is there an emergency here needing this first system breaking change? Or can the code age out by providing the right field data in the post submission API? And communication? Or by a change in post upload processing? I have made adjustments/workarounds to get my new posts to be viewable on my page. So with the right communication to people the original problem breaking change if rolled back now ages out fast(possibly a week) and is only for old posts. But without the right communication this problem gets worse/bandaged. Just some thoughts... Right now, all my older posts on my page are unusable visually basically. So usable but ugly is better than not usable. But still... And with that, hope you all do well. Apologize for the passion. Best, -C |
Hi Bluesky team. We (the bavarian environmental agency) joined bluesky and promote it. I'm a fan of your platform. But the change you introduced completely destroyed our posts: https://bsky.app/profile/lfu.bayern.de |
I'll double-check with the backend crew if it's possible to somehow fill these in from the server instead of relying on the I am fairly certain that we're not going to roll back this change — it has to be done at some point because we can't tolerate layout shifts (jumping content) in feeds, and having even a single post like this in a feed causes everything below it to jump. Having multiple means multiple jumps. This causes performance issues for everyone else — having a single post in a feed like this is enough to hurt all content that appears below it. We just can't keep doing it like this. We do plan to deploy #6828 soon which, while not ideal, should improve how images with missing dimensions are displayed. I do agree we want to emphasize this field in the documentation. I would really appreciate pointers on which pages you'd find it most helpful to see mentioned. AFAIK we're not able to make this an actually required field because lexicons have to be backwards-compatible. I.e. we can't actually break old posts. So that is not an option. But we can strongly recommend to include it. Re: communication, for a change like this, it is pretty much impossible to communicate beforehand and hope that everybody will hear about it. Most major clients I'm aware of (maybe all by now?) have already been sending the image dimensions correctly, but there will always be a long tail of one-off scripts, libraries, etc. For better or worse, displaying them suboptimally in the app is the most effective way to actually ensure that the change gets noticed. I 100% agree it's frustrating for older posts, but delaying the change is even worse because even more posts will be made before the change takes effect the next time. That's the motivation for ripping the bandaid. But again, I also fully agree that ideally this should not have been an issue in the first place. I hope that this will be solvable on the backend, so I'll check on that. For now, #6828 and documentation updates (please let me know if you see a page that needs to be changed) would hopefully improve this. Thanks! |
@gaearon thank you for your detailed explanation. #6828 would be very necessary to be rolled out as soon as possible. I don't want to sound rude, but for a platform that promotes maximum openness such a breaking change not communicated is not very trustworthy. You can not break functionality based on an optional api field. |
I like you all. Do the 3rd party apps have to nominally register to use your API to submit posts for people to the system? If not, then that should be fixed right away. Because they should. And it would solve your communication to them easily/knowing who to communicate to/problems to, them being registered. If they don't have to isn't that in and of itself an issue? The rest... I can see you are running down that path. It hurts me to see it. It especially hurt my page. =) There are so many alternatives.
|
there is no versioning and migrations cannot happen because AT Protocol is a generic data store, the only mitigation for this is if Bluesky's AppView can ask the CDN to fill in the missing metadata instead of serving as is. though I think it'd still be preferred if bots/clients were to properly fill this metadata by themselves, instead of relying on this (soon to be implemented) missing fill behavior.
|
#6828 should be live on the web, we'll get it to native apps soon as well. |
I've checked with the backend crew — we should be able to eventually solve this in the future (even for old posts) by changing how AppView is involved with the CDN but not something we can do quickly right now. |
Cool! |
@gaearon thanks for the fast fix |
I appreciate that displaying images poorly is an approach that's being taken to communicate the change, but as Damien points out, there are accounts that have been creating these kinds of images since before the field existed. For accounts such as one I run or Damien's, the idea that this older content is simply permanently broken and it will never be possible to fix it is... frustrating? Suboptimal? Especially since people do regularly go back to older posts on these accounts, and so their media is less ephemeral than the average post. It doesn't make me feel good about any future changes that might happen. If backfilling this data for older posts is possible, it would be really appreciated. |
The proper fix for this is to rewire how our server communicates with the CDN, at which point this will become a non-issue and would work for old images too. Right now we're in an unfortunate transient state where we can't fix it properly yet. |
Well, to be clear, I think this can be made to work for old images too, can't promise that yet. Maybe we can explore some compromises like accepting layout shifts in profiles (e.g. when visiting old posts) but using the current behavior in feeds. Then at least perf loss would be contained. |
Improving the docs bluesky-social/bsky-docs#290 |
Steps to Reproduce
Taking this post as an example
https://bsky.app/profile/macthemes.bsky.social/post/3lbxkfazqdx2f
It showed "as expected" on 1.94
But shows up with
objectFit: cover
on 1.95, which obstructs a bunch of the imageI haven't checked if the issue happens on mobile or not but I woud assume so?
It seems like the regression was caused by the fallback square aspect ratio introduced in #6474
Which makes sense! Layout shifts suck, and it's an understandable fix, I just feel that retroactively changing the way images are displayed isn't great either?
My bot has been posting images for way longer than the
aspectRatio
field has been available onapp.bsky.embed.images
records so it feels "unfair" 😛I fixed my posting code so it won't happen for future records, but I wonder if a middle ground "fix" would be to:
It might cause a slight flash/jump, but it's better than a layout shift and more importantly, wouldn't retroactively "hide" content for images that lacked aspect ratio data and displayed fine until 1.95
Attachments
No response
What platform(s) does this occur on?
Web (Desktop), Web (Mobile)
Device Info
No response
What version of the app are you using?
1.95
Additional Information
No response
The text was updated successfully, but these errors were encountered: