Skip to content

Fixes layout shifts caused by lazy loading images#441

Open
Lemonn wants to merge 1 commit intoFriendsOfFlarum:1.xfrom
Lemonn:master
Open

Fixes layout shifts caused by lazy loading images#441
Lemonn wants to merge 1 commit intoFriendsOfFlarum:1.xfrom
Lemonn:master

Conversation

@Lemonn
Copy link
Copy Markdown

@Lemonn Lemonn commented Aug 11, 2025

**Fixes #323

Changes proposed in this pull request:

Adds a database table which stores the image width and height after the image has been finally saved. Then, these values are used whenever an image is rendered with the image-preview template.

Reviewers should focus on:

To be able to use template variables inside CSS, the security feature DisallowUnsafeDynamicCSS from S9E needs to be disabled. I tried to find a way to only do this if the template and variable are of a matching type, but could not find such an option. I do not see any direct security implications, as the variable used inside the CSS is a purely server-side computed value. This prevents any user from exploiting CSS variables, despite enabling them. If this is absolutely not an option, JavaScript could be used to read the aspect ratio from an option-* tag and generate the proper CSS.

…n scrolling threads with images.

- Adds a model which stores the image width and height after the image has been finally saved.
- Uses this width / height as aspect ratio in the image-preview.blade.php template.
@Lemonn Lemonn requested a review from a team as a code owner August 11, 2025 16:49
Copy link
Copy Markdown
Member

@DavideIadeluca DavideIadeluca left a comment

Choose a reason for hiding this comment

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

Interesting proposition! Can you please sort out the PHPStan and testing issues and then request a review again?

@Lemonn
Copy link
Copy Markdown
Author

Lemonn commented Aug 12, 2025

Sure, I need to change another thing anyway. Currently, this fix only works with local storage, as I read the image from local storage after it has been finally saved. This does not work with remote storage adapters. I need to change it so that it also works with remote storage adapters. If I do this, I will probably also add a check that adds the data for older uploads when they are requested.

@Lemonn
Copy link
Copy Markdown
Author

Lemonn commented Aug 12, 2025

I fixed the non-test related issues and the PHPStan stuff. Now the images are correctly loaded by the Downloader. Also, if an image is requested via the template, which does not yet have metadata attached, the metadata is generated.

But I'm a bit frustrated regarding the integration tests. In my local installation, I could for example hide a file, without a problem. But for some reason the test for this case fails with response code 500.
https://github.com/Lemonn/flarum_fof_upload/actions/runs/16908456108/job/47903684071#step:8:38
image

I really would like to dig into the error. But the output for the tests is not enough, to be able to pinpoint the issue. I haven't found a good way to recreate the exact test environment locally. Ideally there would be a docker container, which resembles the test environment. As my local installation, which is based on crazymax/flarum, works, I suspect that some differences in the loaded modules, versions, etc. are the cause for the failing tests.

@huoxin233
Copy link
Copy Markdown

huoxin233 commented Dec 27, 2025

From what I see, it only get metadata for new image, should there be a script to extract metadata for exsiting images also?

@Lemonn
Copy link
Copy Markdown
Author

Lemonn commented Dec 28, 2025

From what I see, it only get metadata for new image, should there be a script to extract metadata for exsiting images also?

Yes, that's my fault, as I did not update this pull request because I wanted to fix the test issues first and then gave up. Look at my fork here: https://github.com/Lemonn/flarum_fof_upload/tree/8342bc890b1944d3e123f096a6b2b8802c5e83dd. There should be everything as described in the previous post.

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.

Uploaded images cause layout shifts

3 participants