Page MenuHomePhabricator

[web] Fix stretched images bug
ClosedPublic

Authored by patryk on Jul 12 2023, 1:00 AM.
Tags
None
Referenced Files
F3386321: D8483.diff
Fri, Nov 29, 4:13 AM
Unknown Object (File)
Mon, Nov 25, 6:05 PM
Unknown Object (File)
Mon, Nov 25, 5:57 PM
Unknown Object (File)
Sat, Nov 23, 6:21 AM
Unknown Object (File)
Sat, Nov 9, 3:05 PM
Unknown Object (File)
Sat, Nov 9, 2:34 PM
Unknown Object (File)
Sat, Nov 9, 2:34 PM
Unknown Object (File)
Sat, Nov 9, 12:58 PM
Subscribers

Details

Summary

Fix for ENG-4371.

Screenshots for encrypted images:

screen3.png (747×1 px, 38 KB)

screen4.png (760×871 px, 69 KB)

Unencrypted images:

screen2.png (752×1 px, 43 KB)

screen1.png (756×1 px, 53 KB)

Test Plan

Run web app and check if images are stretched.

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D8483
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

patryk held this revision as a draft.
patryk added reviewers: bartek, ashoat, tomek, inka.
patryk edited the summary of this revision. (Show Details)
bartek requested changes to this revision.Jul 12 2023, 1:42 AM

Generally I'd go the other way around:
Firstly, fix the stretching issue AND avoid that jump when it finishes loading. This is the crucial issue here

Then figure out how to deal with the too large spinner

web/media/multimedia.react.js
163 ↗(On Diff #28611)
  1. We usually don't leave a commented-out code lines without a really good reason and additional comment
  2. This border radius can be moved into the CSS file, it doesn't need to be in the inline style where it's hard to find
This revision now requires changes to proceed.Jul 12 2023, 1:42 AM

Wouldn't commenting this code out result in the thumbhash having the wrong dimensions? Doesn't the thumbhash need the exact dimensions passed to it?

patryk edited the summary of this revision. (Show Details)

Fix thumbhash wrong dimensions and hide border when image tag doesn't have src attribute.

patryk retitled this revision from [web] Temporary fix for streched images bug to [web] Fix stretched images bug.Jul 13 2023, 1:19 AM
patryk edited the summary of this revision. (Show Details)
web/media/encrypted-multimedia.react.js
24 ↗(On Diff #28643)

There is no aspect-ratio attribute in CSSStyleDeclaration.

web/media/loadable-video.react.js
23 ↗(On Diff #28643)

There is no aspect-ratio attribute in CSSStyleDeclaration.

ashoat requested changes to this revision.Jul 13 2023, 5:45 AM

I tested this locally and it solves the issue! Great work @pklatka. Going to pass back to you with just some questions / minor suggestions inline

web/media/encrypted-multimedia.react.js
24 ↗(On Diff #28643)

This type might be useful in other contexts. For native we have native/types/styles.js – can you create web/types/styles.js, and define this as a new type there (eg. export type CSSStyle or something)?

web/media/media.css
23 ↗(On Diff #28643)

Can you explain this change and show a before / after screenshot?

26–28 ↗(On Diff #28643)

Can you explain this change and show a before / after screenshot? I'm also curious why we need not([src]) – maybe include a screenshot of what this would look like if we skipped that part

This revision now requires changes to proceed.Jul 13 2023, 5:45 AM
web/media/media.css
26–28 ↗(On Diff #28643)

I guess the not(src) refers to the thumbhash image where we haven't yet provided the actual decrypted src.

But good point - this is so unobvious, it's worth explaining and maybe adding a comment here

web/media/media.css
23 ↗(On Diff #28643)

Issue with long images.
Before:

issuelong.png (390×710 px, 39 KB)

After:
iii2.png (393×712 px, 39 KB)

26–28 ↗(On Diff #28643)

There is an issue when we load encrypted images. When img has no src, we can see white border for a very short time:

not-css2.png (1×1 px, 79 KB)

It is visible, because we set aspect-ratio. I will add comment there.

I need a lot more context. Please try to explain in detail the decisions behind each of these

web/media/media.css
23 ↗(On Diff #28643)

Where did you get this border-radius value? I tried to git grep in the codebase, but I wasn't able to find it:

# cd web/chat
# git grep border-radius
chat-input-bar.css:  border-radius: 8px;
chat-thread-list-item-menu.css:  border-radius: 4px;
chat-thread-list.css:  border-radius: 1.68px;
chat-thread-list.css:  border-radius: 15px;
chat-thread-list.css:  border-radius: 5px;
edit-text-message.css:  border-radius: 8px;
edit-text-message.css:  border-radius: 4px;
inline-engagement.css:  border-radius: 16px;
inline-engagement.css:  border-radius: 16px 0 0 16px;
inline-engagement.css:  border-radius: 0 16px 16px 0;
message-tooltip.css:  border-radius: 8px;
message-tooltip.css:  border-radius: 8px;
typeahead-tooltip.css:  border-radius: 8px;
typeahead-tooltip.css:  border-radius: 4px;

I'm also concerned that this CSS selector may inadvertently affect places we don't intend for it to affect. Can you identify all of the locations affected by this selector, and see if this border-radius is appropriate for all of them? For instance, does the span.multimedia > .multimediaImage > img, span.multimedia > .multimediaImage > video selector affect the ChatInputBar or the MultimediaModal?

High-level feedback: it shouldn't be necessary for a reviewer to investigate things like this... as the diff author, you should proactively identify places that might be confusing, and explain with inline comments where things are coming from / how decisions were made

26–28 ↗(On Diff #28643)

Weird. Is this really a border? If so, shouldn't we hide it by disabling the border, rather than disabling the visibility of the whole img? If we can disable the border, then I suspect the not([src]) part might not be necessary (since we don't want to show a border in any condition)

web/media/media.css
23 ↗(On Diff #28643)

From here. It looks like it is being set on message wrapper. No wonder that grep didn't catch that (I discovered this value when I was playing with CSS in browser). And yes, it affects ChatInputBar uploads, my bad, could have mentioned this. Probably that's why I used inline border-radius rather than using it in media.css. The main motivation to use inline border-radius was, as I wrote before, to round long images which render with small height (less than 50px). Because we set min-height: 50px here, images, as well as videos, with rendered height < 50px could not be rendered with borders. I suggest to return to inline declaration of border-radius.

26–28 ↗(On Diff #28643)

If border: 0/none or outline: 0/none worked, I would use it. It's the default "special" border that appears when you use an img element with an a src attribute set to something that doesn't exist (or you don't use src at all). We could also set some blank image as source to hide this border, but it would be the same as hiding this tag with CSS. You can read more here.

web/media/media.css
26–28 ↗(On Diff #28643)

Another resource which suggest using something similar to display: none is here.

web/media/media.css
23 ↗(On Diff #28643)
  1. We should avoid inline styles, and instead favor the application of specific classes. Instead of an inline style, you should define a new class that sets the border-radius that you want, and conditionally apply that class.
  2. Can you explain more why we need the border-radius to be applied here for long images? I don't understand what the borderRadius here works for most images, but doesn't work for long images. Is there some special behavior that min-height has that disables border-radius somehow?
26–28 ↗(On Diff #28643)

Thanks for explaining!! This is weird, but your links helped me understand why it's necessary

web/media/media.css
23 ↗(On Diff #28643)

expl.png (732×1 px, 213 KB)

Let's say we encounter this situation. User uploads "normal-sized" images and decides to upload a very long image. As you can see, this normal image covers completely our message container (temporary red color is not visible). However long images leave some free space, due to min-height: 50px. And because of that, our long image is not rounded, as image with normal size. So I've decided to add border-radius to avoid situation visible in the screenshot above. But what I found out today, is that we don't set this radius for every corner every time. We could inherit border settings from message container but we will end up getting something like this:
expl2.png (670×1 px, 260 KB)

Without red background:
expl3.png (815×1 px, 263 KB)

Because of min-height, there is quite a large gap between messages which I don't think matches our design. To solve this problem we have three options:

  1. Remove border-radius from img tag and don't care right now for this corner case.
  2. Round every corner of the image (set border-radius: 16px to img tag) only when rendered image height is less than min-height: 50px of message container.
  3. Inherit border settings for img tag and remove min-height: 50px. This however creates new issue: when image has extremely small rendered height, it will be hard to click on it and open multimedia modal :)

I feel like this issue fix should be a separate diff. Below I've attached some visualisations how second and third option could look like:
Second option:

expl4.png (565×1 px, 223 KB)

Third option:
expl7.png (560×1 px, 223 KB)

Thanks for the helpful visuals! That made it very clear what the problem is.

I agree that this should be handled in a separate diff. Can you create a separate Linear task for it, and update this diff to get rid of the border-radius changes?

I think my preferred solution would be if we could "zoom in" to the long image, and "clip" the left and right sides. So then it would appear with the min-height of 50px, and would still have the correct aspect ratio. Is something like this possible with eg. object-fit CSS?

patryk edited the summary of this revision. (Show Details)

Removed border-radius and added comment in media.css, added new type for CSS styles

It might be good to update these three existing sites where we use CSSStyleDeclaration to now use CSSStyle:

web/media/encrypted-multimedia.react.js:  +elementStyle?: ?Shape<CSSStyleDeclaration>,
web/media/loadable-video.react.js:  +elementStyle?: ?Shape<CSSStyleDeclaration>,
web/utils/typeahead-utils.js:  const textareaStyle: CSSStyleDeclaration = window.getComputedStyle(

It might be good to update these three existing sites where we use CSSStyleDeclaration to now use CSSStyle:

web/media/encrypted-multimedia.react.js:  +elementStyle?: ?Shape<CSSStyleDeclaration>,
web/media/loadable-video.react.js:  +elementStyle?: ?Shape<CSSStyleDeclaration>,
web/utils/typeahead-utils.js:  const textareaStyle: CSSStyleDeclaration = window.getComputedStyle(

Do we have a Linear task for that? If not, @patryk Can you create one?

This revision is now accepted and ready to land.Jul 19 2023, 12:35 AM

Sorry, forgot to add comment here that I've created task for that: ENG-4405

This revision was automatically updated to reflect the committed changes.