Implement slightly magical CSS soln. to thumbnail sizing #1912

lukebarnard1 · 2018-05-18T09:13:01Z

See http://www.mademyday.de/css-height-equals-width-with-pure-css.html for details on how this works.

It relies on being able to set padding-bottom to a percentage which is the aspect ratio of the thumbnail.

This change attempts to be restricted to sizing of the thumbnail, the logic of when to display a thumbnail, spinner for decryption should be the same. It could use some improvement though as the conditions on displaying certain views are quite unclear.

Fixes element-hq/element-web#6722

As the slightly nicer alternative to fixupHeight being applied once we actually have a timelineWidth. The niceness comes from not needing timelineWidth, which means we can implement at render time with CSS. (Despite still calculating aspect ratios when we render.)

dbkr

Should we be using the same logic for stickers? Since this just removes the width / height fixing for stickers without replacing it with anything.

dbkr · 2018-05-18T09:48:38Z

res/css/views/messages/_MImageBody.scss

+.mx_MImageBody_thumbnail_container {
+    // Prevent the padding-bottom (added inline in MImageBody.js) from
+    // effecting elements below the container.


affecting ;)

dbkr · 2018-05-18T09:51:42Z

src/components/views/messages/MImageBody.js

+                "display": "flex",
+                "alignItems": "center",
+                "width": "100%",
+            }}>


Can we put these in the stylesheet?

We can indeed!

lukebarnard1 · 2018-05-21T10:52:00Z

Should we be using the same logic for stickers? Since this just removes the width / height fixing for stickers without replacing it with anything.

In theory they can be sized in the same way as images, albeit with their natural width/height halved on non hdpi displays.

Edit: remove annoyingly big screenshots.

The benefits of this: - One code path for determining spinner/placeholder and it's position for loading images/stickers. This includes spinner used in e2e decryption of images. - Very small definition for MStickerBody, only overriding the minimal differences is has from MImageBody. The disadvantages: - Slightly more complicated MImageBody, but hopefully not less readable.

lukebarnard1 · 2018-05-21T16:03:43Z

Should we be using the same logic for stickers?

I may have gone overboard by factoring out all shared logic between stickers and images. As explained in the commit message:

The benefits of this:

One code path for determining spinner/placeholder and it's position for loading images/stickers. This includes spinner used in e2e decryption of images.
Very small definition for MStickerBody, only overriding the minimal differences is has from MImageBody.

The disadvantages:

Slightly more complicated MImageBody, but hopefully not less readable.

dbkr

otherwise lgtm

dbkr · 2018-05-21T16:58:18Z

src/components/views/messages/MImageBody.js

+                    "display": showPlaceholder ? undefined : 'none',
+                    // Constrain width here so that spinner appears central to the loaded thumbnail
+                    "max-width": content.info.w + "px",
+                }}>


Why put the thing in the dom with display: none if there's no placeholder (as opposed to not putting it in at all?)

That's fair, the placeholder doesn't need to be there at all

dbkr · 2018-05-21T16:58:34Z

src/components/views/messages/MImageBody.js

-        // which may well be much smaller than the 800x600 bounding box.
+                <div style={{display: !showPlaceholder ? undefined : 'none'}}>
+                    { img }
+                </div>


same as above re: display: none

The <img> does have to be in the DOM so that it can load during which time the placeholder is shown.

oh right, of course

ara4n · 2018-05-22T09:52:35Z

src/components/views/messages/MImageBody.js

-            this.fixupHeight();
+    _messageContent(contentUrl, thumbUrl, content) {
+        // The maximum height of the thumbnail as it is rendered as an <img>
+        const maxHeight = Math.min(THUMBNAIL_MAX_HEIGHT, content.info.h);


what happens if the image (or thumbnail) has no explicit width & height?

If the event has an explicit thumbnail (e.g. e2e image), shouldn't we be looking at the thumbnails dims here?

(Apparently thumnbnail_info is provided for e2e and non-e2e images.)

Previously, we didn't use thumbnail_info in the process of calculating the height of the thumbnail in the timeline. We could do but it would be a change in functionality.

ara4n · 2018-05-22T09:53:13Z

src/components/views/messages/MImageBody.js

+    _messageContent(contentUrl, thumbUrl, content) {
+        // The maximum height of the thumbnail as it is rendered as an <img>
+        const maxHeight = Math.min(THUMBNAIL_MAX_HEIGHT, content.info.h);
+        // The maximum width of the thumbnail, as dictated by it's natural


ara4n · 2018-05-22T09:55:36Z

src/components/views/messages/MImageBody.js

-            this.fixupHeight();
+    _messageContent(contentUrl, thumbUrl, content) {
+        // The maximum height of the thumbnail as it is rendered as an <img>
+        const maxHeight = Math.min(THUMBNAIL_MAX_HEIGHT, content.info.h);


I'm still really surprised that we can't just set max-height: 600px on the image rather than calculating this in JS...

To be frank, using CSS/JS is irrelevant. We restrict the height of the image with max-height. If we use 600px then all thumbnails will be 600px high (because of the magical padding-bottom).

The image should be a maximum height of 600px or, if it's less, the given info.h.

ara4n · 2018-05-22T09:58:08Z

Just checking that this has been manually tested and checked for flickering in:

small image in e2e room (e.g. 320x240)
large image in e2e room (e.g. 1024x768)
small image in normal room
large image in normal room
sticker in e2e room
sticker in normal room
image with no explicit width & height metadata in e2e room
image with no explicit width & height metadata in normal room

...all both in a wide browser window and a narrow (<800px) window?

Also, are videos and other file types going to bite us in the same way?

lukebarnard1 · 2018-05-22T15:28:31Z

Also, are videos and other file types going to bite us in the same way?

Nothing was changed wrt other event types.

Edit: Although mx_MImageBody_thumbnail is used by MVideoBody...

Edit: Turns out MVideoBody loading spinner was actually broken by this PR before 538979a was committed. MVideoBody still does its own thumbnail sizing and I suggest we keep that out of scope for now. (Video thumbnails are broken on small timelines regardless of this PR).

lukebarnard1 · 2018-05-22T15:32:18Z

image with no explicit width & height metadata in e2e room
image with no explicit width & height metadata in normal room

These changes don't change our current failure mode under these circumstances, which is to not show any image in the timeline - we just show the "Download..." link.

ara4n · 2018-05-22T16:30:19Z

@dbkr, i'm finding it quite hard to gauge whether this is going to be a maintainability/stability improvement over your JS-based fix - wdyt? (post-GDPR)

dbkr · 2018-05-24T09:08:32Z

Hmm, I would quite like to get rid of the fixupHeight thing, especially the fact that it manipulates to dom directly after mounting and uses slightly different code paths for stickers and images. My inclination is this is probably worthwhile.

lukebarnard1 · 2018-05-24T10:00:42Z

To summarise the key changes:

No using the width of the timeline in the calculation of thumbnail sizes (so no waiting for the timeline to be rendered so that we can grab it's width)
No fixupHeight that makes changes to the DOM after render is called
Images and stickers use the same thumbnail sizing (whether e2e or otherwise)
The placeholders for images and stickers likewise use the same thumbnail sizing

ara4n · 2018-06-14T10:10:57Z

it looks net positive to me, but i'm scared of the risk of breaking something which is otherwise working right now, especially without test coverage. however, given we've manually tested it, i'm happy to risk it.

lukebarnard1 · 2018-06-14T10:17:12Z

Also, we'll get a lot of testing on /develop. Let's see what happens!

Prior to #1912, height fix up of image events without an `info` in their content would fail, setting `style.height = null + "px"`. Now that all thumbnail sizing is done through one path, we can fix the same problem for all cases (images, stickers, e2e/non-e2e) by handling images without `info` correctly. At the bare minimum, we use a null-guard that will make sure an image without an `info` does not appear in the timeline (as a spinner or otherwise until loaded). When loaded, we size it like any other image by using the natural dimensions of the loaded image in place of `info`. Note that we do not apply the same logic to images that *do* specify an `info` with `w` and `h` keys. If the aspect ratio of the image does not match that of the event, we use the one in `info` even when the image has loaded.

lukebarnard1 added 3 commits May 17, 2018 18:15

Fix e2e image thumbnail spinner containing box correct size

bbcf2fe

Remove fixupHeight call from MStickerBody

b41b9aa

lukebarnard1 assigned dbkr May 18, 2018

lukebarnard1 added 2 commits May 18, 2018 10:15

Adjust comment

d11442d

Add more comments to explain thumbnail sizing

7e7e2a7

dbkr reviewed May 18, 2018

View reviewed changes

dbkr removed their assignment May 18, 2018

lukebarnard1 added 2 commits May 18, 2018 11:29

Spelling/grammar

6699c4f

Move inline style to stylesheet

015093b

lukebarnard1 assigned dbkr May 18, 2018

Merge branch 'develop' into luke/kill-mimage-fixupheight

e9ae3de

dbkr reviewed May 21, 2018

View reviewed changes

dbkr assigned lukebarnard1 and unassigned dbkr May 21, 2018

lukebarnard1 added 2 commits May 22, 2018 10:43

Only include placeholder in DOM when necessary

e4f8c09

Remove spurious fixupHeight

fb5dd4a

lukebarnard1 assigned dbkr and unassigned lukebarnard1 May 22, 2018

ara4n reviewed May 22, 2018

View reviewed changes

lukebarnard1 assigned lukebarnard1 and unassigned dbkr May 22, 2018

Grammar

c249bee

Fix MVideoBody spinner

538979a

Merge branch 'develop' into luke/kill-mimage-fixupheight

2120858

lukebarnard1 merged commit 7029e9a into develop Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement slightly magical CSS soln. to thumbnail sizing #1912

Implement slightly magical CSS soln. to thumbnail sizing #1912

lukebarnard1 commented May 18, 2018 •

edited

Loading

dbkr left a comment

dbkr May 18, 2018

dbkr May 18, 2018

lukebarnard1 May 18, 2018

lukebarnard1 commented May 21, 2018 •

edited

Loading

lukebarnard1 commented May 21, 2018 •

edited

Loading

dbkr left a comment

dbkr May 21, 2018

lukebarnard1 May 21, 2018

dbkr May 21, 2018

lukebarnard1 May 21, 2018

dbkr May 24, 2018

ara4n May 22, 2018

lukebarnard1 May 22, 2018

ara4n May 22, 2018

ara4n May 22, 2018

lukebarnard1 May 22, 2018

ara4n commented May 22, 2018 •

edited by lukebarnard1

Loading

lukebarnard1 commented May 22, 2018 •

edited

Loading

lukebarnard1 commented May 22, 2018

ara4n commented May 22, 2018

dbkr commented May 24, 2018

lukebarnard1 commented May 24, 2018

ara4n commented Jun 14, 2018

lukebarnard1 commented Jun 14, 2018

Implement slightly magical CSS soln. to thumbnail sizing #1912

Implement slightly magical CSS soln. to thumbnail sizing #1912

Conversation

lukebarnard1 commented May 18, 2018 • edited Loading

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 commented May 21, 2018 • edited Loading

lukebarnard1 commented May 21, 2018 • edited Loading

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ara4n commented May 22, 2018 • edited by lukebarnard1 Loading

lukebarnard1 commented May 22, 2018 • edited Loading

lukebarnard1 commented May 22, 2018

ara4n commented May 22, 2018

dbkr commented May 24, 2018

lukebarnard1 commented May 24, 2018

ara4n commented Jun 14, 2018

lukebarnard1 commented Jun 14, 2018

lukebarnard1 commented May 18, 2018 •

edited

Loading

lukebarnard1 commented May 21, 2018 •

edited

Loading

lukebarnard1 commented May 21, 2018 •

edited

Loading

ara4n commented May 22, 2018 •

edited by lukebarnard1

Loading

lukebarnard1 commented May 22, 2018 •

edited

Loading