zooniverse / front-end-monorepo

A rebuild of the front-end for zooniverse.org
https://www.zooniverse.org
Apache License 2.0
105 stars 29 forks source link

Markdown images (ThumbnailImage) have inconsistent sizing behaviour #6359

Open shaunanoordin opened 1 week ago

shaunanoordin commented 1 week ago

UI Bug

Package: lib-react-components Components: Media > ThumbnailImage, possibly Markdownz itself Noted as of: fef769f

Images in Markdown (e.g. as used in a Project's About page) can have inconsistent sizing behaviour. To be specific:

Analysis

Analysis of Sub-Issue 1: the chicken-and-egg hypothesis

Screenshot: we see the same animal image in two different kinds of tables. In both cases, the table layout logic seems to determine the width of left & right columns by looking for "fixed content" first - in this case, the text in the right column. It then leaves the remaining space for the "flexible content", meaning the image just fills up whatever space is left.

image

Analysis of Sub-Issue 2 & 2a:

Analysis of Sub-Issue 3:

Testing

To test this, simply try adding Markdown images with various explicit sizes (smaller than the original, larger than the original, etc) and in various situations (inside a table with a lot of other content, etc) to a test project's About page.

For example, heres my test project: https://www.zooniverse.org/lab/19242/about/results

And here's the Markdown code I used ``` ## Markdown Test This page is used to test how images are rendered in Markdown code on the Zooniverse. Specifically, how are _images with specified widths, inside tables,_ handled on both PFE and FEM? -------- **Test 1: Image in Table** ||| |:------------------:|-----| |![animal.jpeg](https://panoptes-uploads.zooniverse.org/project_attached_image/1a4198cb-635a-4c0f-806e-796f3ac3877f.jpeg =800x)|This is a cell in a table. To your left, you should see a picture of a prairie dog(? or a gopher? I'm not a biologist dangit). The [source image](https://panoptes-uploads.zooniverse.org/project_attached_image/1a4198cb-635a-4c0f-806e-796f3ac3877f.jpeg) is 206x183 pixels in size, but the markdown specifies that the image should be presented as 800px wide. Question: how does the image appear to you on the About Page? On PFE? On FEM?| -------- **Test 2: Image in less verbose Table** ||| |:------------------:|-----| |![animal.jpeg](https://panoptes-uploads.zooniverse.org/project_attached_image/1a4198cb-635a-4c0f-806e-796f3ac3877f.jpeg =800x)|Similar to table above, but text is shorter| -------- **Test 3: Image, no Table** Below, we see the same markdown code for the prairie dog(? or meerkat??) image, with the requested 800px width, but outside of the bounds of a table. How does it look like? On PFE? On FEM? ![animal.jpeg](https://panoptes-uploads.zooniverse.org/project_attached_image/1a4198cb-635a-4c0f-806e-796f3ac3877f.jpeg =800x) (On FEM, we expect the image to be displayed 206px wide, because the thumbnailer service won't increase the size of an image.) -------- **Test 4: Image, no Table, bigger** For fun, here's what it looks like when we say the image should be 1000px wide: ![animal.jpeg](https://panoptes-uploads.zooniverse.org/project_attached_image/1a4198cb-635a-4c0f-806e-796f3ac3877f.jpeg =1000x) (On FEM, this should fallback to a non-thumbnailed image, because [the thumbnailer service returns a 415 error at 1000px width and above.](https://thumbnails.zooniverse.org/1000x/panoptes-uploads.zooniverse.org/project_attached_image/1a4198cb-635a-4c0f-806e-796f3ac3877f.jpeg)) -------- The end (of the tests) ```

Now, compare the rendered Markdown on both PFE and FEM codebases:

Status

Priority analysis and dev work assignment pending.

We know that, as of 3 Oct, this is affecting at least one project.

shaunanoordin commented 1 week ago

Note

For the moment, we'll need to recommend that FEM project owners avoid using images in tables. (Slack) This will be the workaround while we consider a better fix.

eatyourgreens commented 1 week ago

The 999px limitation is set by these rules, so edit these regexes to allow bigger thumbnails.

https://github.com/zooniverse/thumbnailer/blob/a034b9123ccab5e55b316b5994460e369f55ddc2/nginx.conf#L42-L55

You’ll probably need to edit the URL rewrites in that file too, to catch URLs with bigger numbers.

EDIT: I've opened a PR to make that change.

eatyourgreens commented 1 week ago

The source image is 206x183 pixels in size, but the markdown specifies that the image should be presented as 800px wide.

Markdown doesn’t have a syntax for specifying image sizes, so FEM looks for the height and width in the image alt text. markdownz, on the other hand, uses a markdown convention that puts the height and width at the end of the URL. So that difference might cause problems.

Here’s the code that expects height and width to be in the alt text. https://github.com/zooniverse/front-end-monorepo/blob/fef769f4129af6e55d77a7c47f4c3195533315a8/packages/lib-react-components/src/Markdownz/Markdownz.js#L33-L39

EDIT: the storybook tests both PFE and FEM variations on markdown image sizing. https://zooniverse.github.io/front-end-monorepo/@zooniverse/react-components/?path=/story/components-markdownz--default

eatyourgreens commented 1 week ago

This needs testing, but removing the 100% height and width from the image CSS fixes the teeny tiny Grommet Image.

The test page with the dev console open to show the image height and width styles disabled, and the image displaying at the expected size.

So removing fill here might fix the problem, by letting the browser set the image height and width. https://github.com/zooniverse/front-end-monorepo/blob/fef769f4129af6e55d77a7c47f4c3195533315a8/packages/lib-react-components/src/Media/components/ThumbnailImage/ThumbnailImage.js#L63-L68

eatyourgreens commented 1 week ago

6360 looks like it will fix the tiny images in project About pages, after testing in Firefox and Chrome on Shaun's test project and WildWatch Burrowing Owl.

I marked that PR as closing this issue, but feel free to amend that, as this issue is actually several different issues (across multiple GitHub repo's.)

I’ve opened https://github.com/zooniverse/thumbnailer/pull/27 to increase the maximum thumbnail size from 999px to 9999px.

eatyourgreens commented 1 week ago

In PFE, the problem is resolved simply by setting the width/height attribute in the img element: e.g. <img src="meerkat-206x183.jpg" width=800 />

This doesn’t really solve the problem, because you get a low resolution, pixelated image. The subject images on the new home page are an example of images that look terrible because they are rendered larger than their natural size (#6333.)

The new code makes a deliberate design decision not to enlarge undersized images. If you want an 800px image on your About page, you have to upload a suitably sized image.

Conversely, images also look terrible if you size a large image down in the browser eg. <img src='big-photo-1000x400.jpg' width=100> will look terrible unless you resize the 1000px image down to 200px or smaller. Roughly speaking, your image should be, at most, twice the rendered size.

eatyourgreens commented 1 week ago

FWIW, the CSS that caused sub-issue 1 (tiny images) was pretty easy to find in the browser DOM inspector. The image component was styled to fit its container, but the container td itself has no explicit height or width.

You can reproduce the same bug in PFE by adding width: 100%; or width: auto; to .project-about-page .markdown img.

https://master.pfe-preview.zooniverse.org/projects/darkeshard/test-project-2022/about/results

width: auto;

A markdown image in PFE, styled with width:auto; so that it shrinks down to a tiny size inside a table.

width: 100%;

A markdown image in PFE, styled with width:100%; so that it shrinks down to a tiny size inside a table.
eatyourgreens commented 1 week ago

Screenshot: we see the same animal image in two different kinds of tables. In both cases, the table layout logic seems to determine the width of left & right columns by looking for "fixed content" first - in this case, the text in the right column. It then leaves the remaining space for the "flexible content", meaning the image just fills up whatever space is left.

Sorry, one other comment. The fixed size element in the DOM here is the img element, not the text node. Text in the DOM is flexible and reflows to fill whatever space is left after fixed-size elements like images have rendered. If an image has no height or width specified, this leads to cumulative layout shift (CLS), where the text on a web page shifts position after the images have loaded. Not related to this bug, but that's why FEM project classify pages have poor CLS scores in their Core Web Vitals. The classifier reflows after the subject image loads.

eatyourgreens commented 1 week ago

Accessibility for people with disabilities

When projects do use tables for layout in markdown, you should probably consider the layout on small screens, and the accessibility barriers for dyslexic or blind visitors too.

As well as being an accessibility barrier for screen readers, which can enter table mode and read the table content as tabular data, layout tables don't allow for a responsive layout that adapts to screen size.

Here’s the test page in PFE on my phone.

Shaun's test page on a phone, showing that the text layout becomes difficult to read.
eatyourgreens commented 1 week ago

Regarding sub-issue 2, which can't be fixed by a PR, here's a link to the documentation for the nginx image filter, which resizes the thumbnail images. It can shrink and sharpen an image, but it can't make it any bigger than it already is.

https://nginx.org/en/docs/http/ngx_http_image_filter_module.html