[web] fix stretched encrypted images
ClosedPublic
Actions

Authored by ginsu on Dec 6 2023, 11:18 AM.

Details

Reviewers

bartek
ashoat

Commits

rCOMM5121a059d44a: [web] fix stretched encrypted images

Summary

The cause of this bug of this bug was that multimedia modal was passing the incorrect content dimensions to the full screen view modal. We were passing the dimensions state (which has an initial value of null) to the full screen modal, so when we tried to calculate the content dimensions for the full screen modal we were always returning early and not properly setting the dyanmicContentDimensions for the multimedia modal.

For the solution we actually don't want to pass in the dimensions state at all, but instead we want to pass in the initial media dimensions so that when we try and calculate the dynamic full screen dimensions of the media we can utilize the initial media dimensions to properly calculate what the dynamic dimensions of the media will be in this full screen view.

This is also what we were doing before I made this unintentional change in the refactor:
https://github.com/CommE2E/comm/blob/8ba42dce166acf4564e2baca3ad405a21d2c7fed/web/media/multimedia-modal.react.js#L164-L191

Linear task: https://linear.app/comm/issue/ENG-5685/imagemodal-on-web-stretches-images

Test Plan

Please see the demo video below

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Dec 6 2023, 11:18 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 6 2023, 11:19 AM

Harbormaster completed remote builds in B24833: Diff 34355.Dec 6 2023, 12:01 PM

ginsu requested review of this revision.Dec 6 2023, 12:01 PM

ginsu edited the summary of this revision. (Show Details)Dec 6 2023, 12:15 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added a reviewer: bartek.

ginsu attached a referenced file: F914548: Screen Recording 2023-12-05 at 4.35.46 PM.mov. (Show Details)

ginsu attached a referenced file: F914552: Screen Recording 2023-12-05 at 4.33.56 PM.mov. (Show Details)

Are you sure you throttled this enough? And cleared local cache before? The problem is that photos might be cached when loaded in-chat and modal loads already-cached image

IIRC expected behavior is to display blurred image (thumbhash) and spinner on top of it while actual image is being downloaded to be decrypted (decryption itself is very fast, that's true). I cannot see this step on your videos. Also I think these dimensions were required to display the thumbhash with the same dimensions as the real photo (without that, the thumbhash itself is sth like 100x100).

Screenshot 2023-12-07 at 12.54.59.png (100×273 px, 13 KB)

Are you sure you throttled this enough? And cleared local cache before?

I throttled to slow 3G, but I did not clear local cache in my demo video. When I did clear the local cache as well, I can see that my solution is not ideal and it causes a regression with the thumbhash. Will go back to the drawing board with this one and will work on a better solution

update

ginsu edited the summary of this revision. (Show Details)Dec 20 2023, 2:27 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu attached a referenced file: F990682: Screen Recording 2023-12-20 at 5.25.16 PM.mov. (Show Details)

ginsu added inline comments.

web/modals/full-screen-view-modal.react.js
87 ↗	(On Diff #34885)	Since the initial of value of `dynamicContentDimensions` was `null` we were always returning early and never were properly setting the `dynamicContentDimensions`