Page MenuHomePhabricator

[web] Inherit min-height property for media elements
Changes PlannedPublic

Authored by patryk on Jul 17 2023, 3:03 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 2:30 PM
Unknown Object (File)
Sat, Nov 23, 7:16 AM
Unknown Object (File)
Sat, Nov 9, 10:30 AM
Unknown Object (File)
Sat, Nov 9, 8:57 AM
Unknown Object (File)
Fri, Nov 1, 3:47 AM
Unknown Object (File)
Sep 26 2024, 5:55 PM
Unknown Object (File)
Sep 26 2024, 5:55 PM
Unknown Object (File)
Sep 26 2024, 5:50 PM
Subscribers

Details

Summary

Fix for ENG-4400.
This diff fixes the issue when the rendered height of the image (or video) is less than 50px (equal to min-height). In such cases, the image doesn't cover the container completely, resulting in the absence of rounded corners.

Depends on D8483, because this issue is created by ENG-4371 fix.

Test Plan

Upload some photos, specifically, upload an image that is really long (width greater than 4500px) and a common-sized image. Then resize the browser window to full screen and see if the expected results match.

Expected result for long photos:

exp1.png (310×1 px, 23 KB)

Test photo:
3b3f04b9-ceee-41e0-97f8-b5a4ce429387.png (110×4 px, 175 KB)

Expected result for common-sized photo:

exp2.png (419×1 px, 56 KB)

Test photo:
0a8b8540155c4f95.png (194×1 px, 38 KB)

Note: screenshots were made for screen with 1440px width.

Diff Detail

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

Event Timeline

patryk edited the test plan for this revision. (Show Details)
patryk added reviewers: bartek, tomek, kamil, inka.
web/media/media.css
24

This line defines expected behavior when width changes when height equals min-height (media element should be "zoomed-in"):
Before:

2130.png (284×453 px, 27 KB)

After:
2131.png (271×480 px, 29 KB)

web/media/media.css
23–24

These properties are set in this selector because the same issue (with not maintaining min-height) happens in chat input bar:

lol.png (174×135 px, 8 KB)

Can you add some more things to the Test Plan?

  1. Testing really long images to make sure they work correctly too
  2. Testing small images with normal aspect ratios, to make sure they don't get "zoomed in" to the size of the container, and instead stay the small size

Testing small images with normal aspect ratios, to make sure they don't get "zoomed in" to the size of the container, and instead stay the small size

Regarding this, the goal is to test an image with a normal aspect ratio, but one that is smaller than the max-width, so that we can guarantee that it appears correctly.

Recently I've tested this for images smaller than current min-width and min-height (all equal to 50px) and actually they are being "zoomed-in". In addition when we upload an small squared image (e.g. 30px x 30px), min-width is not maintained and we obtain something like this (image is an upload multimedia icon):

square.png (93×268 px, 5 KB)

When we set min-width: inherit; the problem is solved but the image is still enlarged. I will post another comment soon when I find a workaround for this issue.

Thanks for testing those cases!

Regarding the previous comment, to fix zoomed-in small images problem, we could set min-width: inherit or min-height: inherit only when image height or width > 50px (current value for min-width/height):

// multimedia.react.js
// Remember to remove min-height: inherit from media.css!!
// ...
const elementStyle = React.useMemo(() => {
  if (!dimensions) {
    return undefined;
  }
  const { width, height } = dimensions;
  // Resize the image to fit in max width while preserving aspect ratio
  const calculatedWidth =
    Math.min(MAX_THUMBNAIL_HEIGHT, height) * (width / height);
  return {
    background: placeholderImage
      ? `center / cover url(${placeholderImage})`
      : undefined,
    width: `${calculatedWidth}px`, // (*)
    aspectRatio: `${width} / ${height}`,
    minWidth: height < 50 ? undefined : 'inherit', // <- change here
    minHeight: width < 50 ? undefined : 'inherit', // <- change here
  };
}, [dimensions, placeholderImage]);
// ...

However, implementing this change may lead to issues with images that have a small height and a "normal" width, such as 520x30px:

expl1.png (166×765 px, 35 KB)

Calculated width is smaller than actual width of message container, thus the image is cut. What we could do is to comment out (*) and now our images will fit the message contnainer and maintain aspect ratio. This change unfortunately affects displaying thumbnail when encrypted images are loaded (because thumbnails have different dimensions than original image). We could workaround this by adding additional logic when to set calculated width. Also, above changes would affect image grid (images are cut on the left and right, because grid cell has no enough width to display image entirely) :
stacked-images.png (434×1 px, 66 KB)

Second solution is to remove min-width and min-height entirely from .multimediaImage class (or at least from message container, we could leave min-width/height in chat input bar) and obtain something like this:

after_removing_mins.png (494×787 px, 51 KB)

But this makes loaders and error indicators cropped for small images (but we could resize this indicators to fit current image dimensions).

@ashoat please let me know what you think about this.

I can't see your attachments.

Ultimately, we're looking for a solution here that doesn't have any bad trade-offs:

  • Aspect ratios should always be maintained. Images should be displayed with enough space for loaders
  • Images that are too large should be scaled down to fit within the width constraints
  • Images that are too small should be "zoomed in" to fit the minimum width/height constraints so there is space for the loaders
  • Transitions between thumbhash, thumbnail, and media should be smooth. There should be no jittering or changes in dimensions during transitions
  • The solution should work on all modern browsers
  • The solution should work for both images and videos

Do you have any solution that works with all of these constraints? If yes, then go ahead and implement it. If no, you'll need to keep looking.

Thanks for attaching! In addition to my above comments, I can leave some guidance on the image grid. The image grid is treated somewhat differently... we basically want to fit the image into the space available in the grid. For small images we can "zoom" them in, and for larger images we can "zoom" them out. Let me know if you have any questions about this, or my previous comment.

Ultimately I'm hoping you can take these requirements and find a solution that works for all of them!