Page MenuHomePhabricator

Portrait images in PagePreview popups are notably cut off (zoomed in)
Closed, ResolvedPublic

Assigned To
Authored By
thiemowmde
Jun 16 2020, 10:34 AM
Referenced Files
F34611432: image.png
Aug 20 2021, 2:51 PM
F34611384: image.png
Aug 20 2021, 2:29 PM
F34607427: image.png
Aug 19 2021, 10:59 PM
F34607432: image.png
Aug 19 2021, 10:59 PM
F34607233: image.png
Aug 19 2021, 8:30 PM
F34607238: image.png
Aug 19 2021, 8:30 PM
F34586217: Screen Shot 2021-08-06 at 9.01.50 AM.png
Aug 6 2021, 4:02 PM
F34586200: Screen Shot 2021-08-06 at 8.26.28 AM.png
Aug 6 2021, 4:02 PM

Description

Portrait images (called "tall" in the code, i.e. their height is larger than their width) are massively cut on all 4 sides:

Screenshot from 2020-06-15 14-36-12.png (371×536 px, 99 KB)

The blue rectangle in this screenshot is the actual thumbnail, cut off on all 4 sides. For reference (notice how the building on the right is more visible):

Screenshot from 2020-06-15 14-36-16.png (319×267 px, 71 KB)

This does not happen for landscape images. I tried to understand why this happens, when it was introduced, and if it was a concisus decision to do so. As far as I can tell, no, this is a bug. This happens because:

  • The PageImages API is used to request the thumbnail. It's requested with a single 320 number to be the length of the largest side.
  • The API returns a thumbnail with, in this example, 240 x 320 pixels.
  • However, Popups uses this image as an unscaled background image for a rectangle that is at most 203 x 250 pixels. The responsible code doesn't even care how large the thumbnail is. It always shows a 203 x 250 cutout only. It's like this code expects the thumbnail to be 250 pixels high, but it's not.

Reported here: https://www.mediawiki.org/wiki/Topic:Vo876rts2zg2bvrh

A proper algorithm would request a thumbnail that fits in a 320 x 250 rectangle. MediaWiki can do this, but unfortunately the PageImages API doesn't. There is a single thumbsize parameter only. So a fixed algorithm might look like this:

  • Request a thumbnail that fits in 320 x 320 pixels.
  • If it's a landscape image, just use it as it is. The popup is 320 pixels wide as well. All landscape images fit perfectly.
  • If it's a portrait image, scale it down so it fits in the available 250 pixel height. "Scaling" here means simply setting the width and height to values that are scaled down a bit. The browser will do the scaling.

QA

Actual:

Screen Shot 2021-07-23 at 12.07.30 PM.png (702×1 px, 427 KB)

Image for reference:
Screen Shot 2021-07-23 at 12.07.41 PM.png (1×674 px, 552 KB)

Proposed:

Screen Shot 2021-07-23 at 12.10.13 PM.png (574×1 px, 338 KB)

Acceptance criteria

  • When fixing this issue, please consider adding the use case as a Storybook story so we can verify future changes don't regress.

Developer notes

We could use something like object-fit: fill to address these kind of portrait thumbnails but this may result in slightly stretched images and seems like the least risk prone solution here.

mwe-popups-is-tall .mwe-popups-thumbnail {
    object-fit: fill;
}

Event Timeline

ovasileva triaged this task as Medium priority.Jul 7 2020, 3:52 PM
ovasileva moved this task from Incoming to Needs Prioritization on the Web-Team-Backlog board.
LGoto subscribed.

This task was closed as part of backlog upkeep. If you believe it was closed in error, please respond on the ticket.

Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: alexhollender_WMF.

@alexhollender Is the problem statement clear, and do the proposals make sense to you? This change is not without risk so I want to make sure that the problem being solved is important enough to work on this. Essentially we'd change tall portraits to use object-fit fill rather than object-fit contain algorithm.

@alexhollender Is the problem statement clear, and do the proposals make sense to you? This change is not without risk so I want to make sure that the problem being solved is important enough to work on this. Essentially we'd change tall portraits to use object-fit fill rather than object-fit contain algorithm.

yes, the problem statement is clear and I agree with @thiemowmde that it would be a meaningful improvement

I don't know if there are certain image dimension restrictions already in place that would mitigate this, but wouldn't "object-fit" result in a distractingly squished image if the image is very tall? In general, this almost feels like we are altering wiki content to fit our design and UI, which makes me a bit nervous.

I don't know if there are certain image dimension restrictions already in place that would mitigate this, but couldn't this result in a distractingly squished image if the image is very tall? In general, this almost feels like we are altering wiki content to fit our design and UI, which makes me a bit nervous.

Hm, now I'm wondering if I'm misunderstanding what is being asked here. My understanding is:

We are not currently scaling "tall" images down. So given the following image & page preview:

image.png (428×1 px, 207 KB)

We are cropping the image on all four sides to make it fit the image box:

image.png (428×1 px, 326 KB)

When instead, to get more of the original image into the page preview, we could just scale the image down so the length of the shorter side matches the length of the shorter side of the image box, then only the top and bottom get cropped off:

image.png (428×1 px, 473 KB)

@thiemowmde can you please clarify if that understanding is correct?

@alexhollender, yes, this description sounds good. Any "squishing" is not acceptable, of course. The PageImages API doesn't even allow any squishing. This piece of code is safe. The behavior of all later code should be that of object-fit: cover;, see https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit.

However, there is something off. When I created this ticket the restraining factor was not the width (you talk about 215px) but the height (250px). The last step should be to "scale the image down so height = 250px".

But there is more going on. As of now even landscape images are heavily cut off. It appears like some recent change broke this even more than it was broken before:

Screenshot from 2021-08-03 09-06-44.png (457×434 px, 88 KB)

For comparison, this is the actual image as it appears in the article:

Screenshot from 2021-08-03 09-06-52.png (232×339 px, 12 KB)

Change 710322 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Popups@master] Storybook: Capture \"panorama\" style portrait and landscape examples

https://gerrit.wikimedia.org/r/710322

This possibly relates to T284643 as it seems something has recently changed with the preview height/width that is not to the original spec.
I've captured the two examples in a storybook for now.

I would ire on the side of caution when trying to optimize for every single page preview image. We've been here before (as have the apps team) when we worked on hero images. It's really impossible to get the algorithm right for every image without having additional metadata and I think we should seriously consider declining this under the umbrella of "its good enough".

I'm afraid I don't understand. The screenshots I posted above look seriously broken. Do you plan to revert the change that caused this?

Do you plan to revert the change that caused this?

@thiemowmde it wasn't clear to me this was a regression so I did the dirty work of trawling through git history to work out what happened here.

Since this keeps coming up on other tickets, I pulled down the original previews code (deaaf0961b7ae8d28) and with a few hacks was able to get it work. This is how this preview originally showed.

Screen Shot 2021-08-06 at 8.26.28 AM.png (862×748 px, 128 KB)

The entire preview was 320x368, of that disregarding the footer the preview was 320x321, the image portion making up 181px and the extract 140px.
Screen Shot 2021-08-06 at 8.42.27 AM.png (796×736 px, 119 KB)

Jumping forward to the modern day with storybook the preview is 320x394. 320x347 if we drop the footer. The extract is still 156px, but the image portion has grown to 191px high. Reducing the image height via min-height: 181px; fixes this particular issue but that likely breaks all the other issues.

It looks like this regression was introduced as part of 7b0937c5a.

We have deviated from the original spec in many ways and changed the height in numerous places: T272169, T246029, T268999 .. in particular the latter, which shows square page previews where the thumbnail is huge:

Screen Shot 2021-08-06 at 9.01.50 AM.png (1×834 px, 214 KB)

We could continue to fix edge cases in isolation but that doesn't seem like a good idea without a spec defining the options. I'd rather not continue to play whack-a-mole with edge cases as they arise as it's just causing different edge cases.... this should be blocked on T284643 and we should bear in mind this is likely a significant chunk of work if we push forward with doing this.

Change 710322 merged by jenkins-bot:

[mediawiki/extensions/Popups@master] Storybook: Capture \"panorama\" style portrait and landscape examples

https://gerrit.wikimedia.org/r/710322

alexhollender_WMF moved this task from Backlog to Needs Analysis on the Page-Previews board.

@Jdlrobson apologies for the delay here. These are the dimensions for page previews with images:

Horizontal

image.png (354×1 px, 204 KB)

Total size:
width: 450px
height: 242px

Text area:
width: 247px
height: 242px

Image:
width: 203px
height: 242px

Vertical

image.png (510×853 px, 311 KB)

Total size:
width: 320px
max-height: 394px

Text area:
width: 320px
max-height: 202px

Image:
width: 320px
height: 192px


Note: The sizes above should apply to all page previews. The only part that is variable is the height of the text area in vertical page previews, everything else is fixed.