Fixes layout shifts caused by lazy loading images #441
Conversation
…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.
DavideIadeluca
left a comment
DavideIadeluca
left a comment
There was a problem hiding this comment.
Interesting proposition! Can you please sort out the PHPStan and testing issues and then request a review again?
|
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. |
|
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. 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. |
2 participants
**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.