Page MenuHomePhabricator

Newcomer tasks: Suggested article image should be repeated when image width is less than card width
Closed, ResolvedPublicBUG REPORT

Description

This is a regression of T238282, and is occurring on Desktop and Mobile.

Expected:

Actual:

Event Timeline

RHo created this task.Feb 4 2020, 9:55 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 4 2020, 9:55 AM
RHo updated the task description. (Show Details)Feb 14 2020, 1:17 PM
RHo edited projects, added Growth-Team (Current Sprint); removed Growth-Team.
RHo renamed this task from Newcomer task: Suggested article image should be repeated when image width is less than card width to [Bug] Newcomer task: Suggested article image should be repeated when image width is less than card width.Feb 14 2020, 1:25 PM
RHo updated the task description. (Show Details)
Tgr added a comment.Apr 1 2020, 11:36 AM

Repeating the image looks weird and broken. Compare

with
.

RHo added a comment.Apr 1 2020, 3:34 PM

Repeating the image looks weird and broken. Compare

with
.

I actually find not repeating the image and having the white gaps looks more weird and broken. We already had agreed to implement the repeat-x image on T238282 which is also why this is filed as a regression bug. While this example of a person's face being shown is not that common and looks admittedly a bit off, it is preferable in terms of it occurring occasionally, vs having the white gaps appear all the times the image doesn't fill card width.

HOWEVER, I would be really interested if we could slightly tweak the implementation so that a slight blur to the repeated side images:

Option 1: Blur added to the repeated images
Option 2: Same image covering the background and blurred

The second option is the standard when mobile portrait video is shown in a landscape video player (eg mobile videos on YouTube).

Tgr added a comment.Apr 1 2020, 4:02 PM

Paging through a random topic, about half the non-fullwidth images where headshots where a part of the head got repeated, so I don't think it's that rare. And it's even more apparent when the background has a different color:

AFAIK there isn't any straightforward way to blur parts of the image only, but we could probably display a blurred version of the image with repeat-x or stretch, and then another unblurred instance on top of it. IE does not support CSS filters so the side images would appear unblurred, but I imagine we can live with that.

RHo added a comment.Apr 1 2020, 4:07 PM

Paging through a random topic, about half the non-fullwidth images where headshots where a part of the head got repeated, so I don't think it's that rare. And it's even more apparent when the background has a different color:

AFAIK there isn't any straightforward way to blur parts of the image only, but we could probably display a blurred version of the image with repeat-x or stretch, and then another unblurred instance on top of it. IE does not support CSS filters so the side images would appear unblurred, but I imagine we can live with that.

Yes I found this stackoverflow post which is not so straightforward. +1 to this tweak with the option to blur the *stretched width* background image). Also, yes to using CSS filter: blur and living without it on IE.

Tgr added a comment.Apr 1 2020, 5:11 PM

Which is your preferred option, stretch or repeat?

Tgr added a comment.Apr 1 2020, 5:12 PM

I wonder if it's possible to repeat + flip horizontally. That would make the edges line up nicely.

Tgr added a comment.Apr 1 2020, 6:54 PM

Here's a straightforward attempt, with filter:blur(5px). This looks pretty bad because the blur affects the image edge as well.


Can probably be fixed with SVG blur though, and that even works in IE.

RHo added a comment.Apr 1 2020, 7:05 PM

Here's a straightforward attempt, with filter:blur(5px). This looks pretty bad because the blur affects the image edge as well.


Can probably be fixed with SVG blur though, and that even works in IE.

Thanks for drumming that up so quickly! My pref is stretched blur instead of this repeat since if that may reduce the edge blurring issue, combined with SVG blur.

kostajh renamed this task from [Bug] Newcomer task: Suggested article image should be repeated when image width is less than card width to Newcomer task: Suggested article image should be repeated when image width is less than card width.Apr 2 2020, 1:43 PM
kostajh changed the subtype of this task from "Task" to "Bug Report".
MMiller_WMF renamed this task from Newcomer task: Suggested article image should be repeated when image width is less than card width to Newcomer tasks: Suggested article image should be repeated when image width is less than card width.May 4 2020, 3:39 AM
Tgr added a comment.May 20 2020, 3:44 PM

This is surprisingly hard to get right:

  • As shown above, CSS blur spreads the image outside its original area (could be addressed in a number of ways, overflow:hidden, clip, background-clip...) and more problematically fades the edges out. Or more precisely, assumes the image is on a white background and uses that white color for the convolution when near the edges. SVG has an option to avoid this (edgeMode) but it does not seem to be implemented in any browser. I guess we could scale the background image to extend beyond the card a bit and then clip it, so this would be manageable, although it would add to the weirdness factor.
  • Some images have transparent areas, where the blurred background version shows through. I don't really see a way to deal with this. If we add a white background, there will be an edge between that background and the stretched or repeated background image - the same problem this task is trying to solve.

So I'll fall back to adding repeat-x to the original image, which is trivial to do. It will look bad in some cases; but the composition approach would look bad in some others, and has a lot more moving parts that can go wrong.

Change 597577 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/extensions/GrowthExperiments@master] Make task card images repeat horizontally

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

Change 597577 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Make task card images repeat horizontally

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

What's was checked: arwiki, cswiki, kowiki, and viwiki betalabs. Also checked IE11, Safari 13, and Edge81 in Saucelabs. Also it was tested on a real mobile device (iPhone 6s).

@RHo for your review:

  • The number of images where the repeated image feature is noticeable is roughly 10% of all images (by noticeable I mean when an image has those repeated parts and a user may notice them). From those 10% half of the images have somewhat sub optimal image display (see the comparison below).
  • In betalabs, the images are cut off horizontally more than in production

(1) Images that are full-width images (seemed to be unaffected by the fix).

cswiki betalabsarwiki betalabskowiki betalabs

(2) Images where the repeated feature is noticeable but looks fine.
Note: In betalabs for Kniha_Mormonova the image from the info box was not used, for some reason.

cswiki betalabscswiki current productionwikipedia app (on iPHone 6s)

(3) Problematic images

cswiki betalabscswiki current productionwikipedia app (on iPHone 6s)

More examples (without comparison pictures)

What's was checked: arwiki, cswiki, kowiki, and viwiki betalabs. Also checked IE11, Safari 13, and Edge81 in Saucelabs. Also it was tested on a real mobile device (iPhone 6s).
@RHo for your review:

  • The number of images where the repeated image feature is noticeable is roughly 10% of all images (by noticeable I mean when an image has those repeated parts and a user may notice them). From those 10% half of the images have somewhat sub optimal image display (see the comparison below).

Thanks @Etonkovidova - I agree it's not the best for this ~5% to be very noticeably repeated-x, but it looks intentional rather than having the white empty spaces which appears imho to look like an error. Moving to PM for final word...

  • In betalabs, the images are cut off horizontally more than in production

Yes, this separate issue of not-great face detection is filed in T240034. IIRC the apps generally look better because they have implemented some face-detection on article lead images (though that is also not perfect – see T148926#3126381).

(3) Problematic images

cswiki betalabscswiki current productionwikipedia app (on iPHone 6s)
Tgr added a comment.May 21 2020, 2:48 PM
  • In betalabs, the images are cut off horizontally more than in production

I cannot reproduce that, the images have the exact same height for me (on desktop and mobile Chrome). Which is expected, the only thing that was changed is the background-repeat property.

Tgr added a comment.May 21 2020, 3:00 PM

@RHo looking at the app screenshots made me wonder why we don't just scale our images to full width. I originally assumed this problem occurs when the image is physically not wide enough to fill the card, but the app uses larger sizes so that cannot be the case (and I checked manually and indeed it isn't). So e.g.

instead of thislike this

Maybe that's just an artifact of what API we are using to get the image URL?

@RHo looking at the app screenshots made me wonder why we don't just scale our images to full width. I originally assumed this problem occurs when the image is physically not wide enough to fill the card, but the app uses larger sizes so that cannot be the case (and I checked manually and indeed it isn't). So e.g.

instead of thislike this

Maybe that's just an artifact of what API we are using to get the image URL?

Hi @Tgr - Yes indeed, this repeat-x is a workaround from the original request to have the image at full-width and centered. However @kostajh commented the following on T238280#5696825 regarding size-limits to thumbnails from RESTBase:

C. Image should be…

scaled to be full-width of card (See bottom of description for example of when this does not happen)
height is 128px (ratio 1:2 with image width)
is there any rules or algorithm used by Page Previews to center image previews on a person's face? Instead of cutting off faces?

There's not a great option here. Portrait orientation image thumbnails from restbase may or may not be under the minimum width (260px) we use for the card. We can stretch a 200px with image so that it's 260px but it will look pixelated and not great, in addition to already not looking that good because it's a portrait image being forced into a landscape orientation holder.

Another option would be to not show images at all if they're in portrait orientation, and just stick with the placeholder image.

I would be in favour of returning to the full-width solution if we can get larger resolution images from the same place that apps are getting these images! Moving back to Needs More Work since it sounds like this is worth exploring...

Tgr added a comment.May 21 2020, 3:45 PM

Yeah, we can just replace the size parameter in the URL to get what we want (unless the original image is narrower than 260px which should be very rare). The flip side is that it will load a bit slower as 260px is not a standard image width so the thumbnail is probably not pre-rendered. (Then again, I'm not sure the size offered by RESTBase is any different...)

  • In betalabs, the images are cut off horizontally more than in production

I cannot reproduce that, the images have the exact same height for me (on desktop and mobile Chrome). Which is expected, the only thing that was changed is the background-repeat property.

The height is is same, yes. I re-checked and realized that describing images in betalabs as "cut off horizontally " was not correct. The images in betalabs are displayed more close-up (and different centering?) than in production (may be there is a better term to describe it). I checked FF and mobile and below is another comparison set (separately for mobile and desktop).

  • Desktop (FF)
betalabsproduction
  • Mobile (iPhone 6s)
betalabsproduction
Tgr added a comment.May 22 2020, 11:17 AM

Yeah, we are talking about the same thing, I just can't reproduce it happening.
Looking at your first example, it doesn't even seem related to the image being narrower than the card.

I imagine this is due to images coming either from the action API or RESTBase, with slightly different resolution.

Change 598115 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Add mw.util.parseImageUrl

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

Change 598127 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Add URL template to the return data of mw.util.parseImageUrl

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

Change 598115 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.util: Add mw.util.parseImageUrl

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

Change 598154 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/extensions/GrowthExperiments@master] Resize image card thumbnails to be exactly 260 pixels

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

kostajh assigned this task to Tgr.May 29 2020, 1:09 PM
Tgr added a comment.EditedJun 1 2020, 6:15 PM

The patches improve thumbnail handling so we show a large enough thumbnail to fill the card whenever possible. Repetition should only be used now for thumbnails where the original, full-sized image is smaller than 260px, which I think in practice only happens for some company logos.

Change 598127 merged by jenkins-bot:
[mediawiki/core@master] Add URL template to the return data of mw.util.parseImageUrl

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

Change 598154 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Resize image card thumbnails to be exactly 260 pixels

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

Any image card thumbnails has thumbnailWidth: 260:

Note: It seems that betalabs displays images inherently different than production does (in terms of positioning). The example below from production looked different when they were in betalabs.

cswiki betalabscurrent production
RHo added a comment.EditedJun 2 2020, 12:44 PM

I've posted on T240034 about ways to improve faces being cut off, but in the interim can we consider making the image be positioned at background-position-y: 25% instead? This seems to prevent people's faces from being cut off at the forehead (when the image is vertically positioned at top) and also reduces the heads being cut off entire when the image is vertically centered.

A few examples in CS beta:

topbackground-position-y: 25%;

Making this update will mean also ensuring the image icon positioning when there is no thumbnail needs to be updated

Change 601807 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/extensions/GrowthExperiments@master] Improve suggested edits card image positioning

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

Change 601807 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Improve suggested edits card image positioning

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

Checked in betalabs and compared with production -the display seems to be really improved:

betalabsproduction

Here are some examples when the new positioning made the image display not so optimal, but such cases are really rare and the images are still displayed ok.

betalabsproduction

Making this update will mean also ensuring the image icon positioning when there is no thumbnail needs to be updated

Checked - the position of the thumbnail in betalabs is like on the first of your screenshots:

Thanks @Etonkovidova - agree this looks much improved now!

MMiller_WMF closed this task as Resolved.Jun 5 2020, 5:47 PM

Thank you. I know this was a lot of work, but I think it was worth it!