Page MenuHomePhabricator

Blank or white background images look abruptly cut off in Page Previews
Closed, DuplicatePublic

Description

Problem

Blank or white background images look abruptly cut off in Page Previews. Some examples:





Design

Add a 1px border of rgba(0,0,0,0.1) to the bottom or side edge of image (depending on orientation) in the page preview. The border should be overlapping the image. Here is a diagram to clarify the placement of the border:

Here are some examples that approximate the design, rendered in the browser via common.css:





Note:

  • make sure this works on RTL previews

Event Timeline

Nirzar created this task.Apr 3 2018, 12:32 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 3 2018, 12:32 AM
ovasileva triaged this task as Normal priority.Apr 3 2018, 9:03 AM
alexhollender removed alexhollender as the assignee of this task.Apr 9 2018, 5:00 PM
alexhollender added a subscriber: alexhollender.
ovasileva updated the task description. (Show Details)Apr 10 2018, 4:32 PM
ovasileva set the point value for this task to 1.

Change 425721 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Separate thumbnail from rest of page preview

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

Jdlrobson added a subscriber: Jdlrobson.

Who knew adding a bottom border could be so hard. IT seems the "a" element that surrounds the preview is display: inline. This combined with the mask and a bunch of margin-tops that get defined on the preview this is super super complicated.

I'm gonna move this back to 'todo' to signal I'm not currently working on this.
I'll come back to this after I've got the other large tasks I'm working on out of the way.

I've poked around with this (my vagrant is just killin' me today, so I'm writing my findings here...)

Like Jon mentions, the CSS is quite complex. Short of rewriting the DOM structure entirely, I think this border can be achieved (maybe) if we place a border-right on both the .mwe-popups-extract and footer when the thumbnail is to the side, and add a border-top to the .mwe-popups-extract when it's on top.

This approach would require .mwe-popups-extract to be padded with a padding instead of a margin, which according to the comments, might lead to issues in IE. If we can get around that somehow, then the following approach might be doable.

This is what I got so far:

CSS changeScreenshot
.mwe-popups-extract is changed to padding, border-right & negative margin is added to offset the border
footer is given same border treatment as .mwe-popups-extract
The SVG z-index is brought down so the border is on top
and same for the little triangle guy

We can also target the CSS so we only show the border when the thumbnail is present, using something like .mwe-popups-discreet ~ .mwe-popups-extract which will only match .mwe-popups-extract if it's a sibling of .mwe-popups-discreet (the thumbnail).

The approach outlined above was mostly working until I hit one snag. The situation in which the tall thumbnail is very narrow.
In that situation, we don't scale the thumbnail up, we leave the space between the text and thumbnail blank, which is not a problem until you try to put a border on the extract...


Looks good until you try to do this

@Jdrewniak what you have seems like the correct behavior to me. Of course, I'm not a designer. It might look even better if you always keep the width the same (and remove the x offset) regardless of the thumbnail dimensions so that the image can be centered. I tried using preserveAspectRatio on svg > image but couldn't quickly get it to behave like object-fit: cover for newer browsers.

Jdrewniak moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.

I tried using preserveAspectRatio on svg > image but couldn't quickly get it to behave like object-fit: cover

Yeah, it's tough. After doing some research, I was able to get the thumbnail to behave like background-size: cover and I made a codepen with the results, https://codepen.io/j4n/pen/deoNBP?editors=1100 I think it pretty much works...

The approach above changes the way the thumbnail is rendered in the SVG. Instead of rendering an image with a clip-path, it renders the clip-path as a polygon with the thumbnail as a "background image" (an SVG fill/pattern thingy). That fixes the problem with the border in my first attempt, by scaling the background-image to cover clip-path.

This requires some considerable reworking of the SVG code, but it would let us get rid of the math that positions/resizes the thumbnail image.

Scaling the thumbnail also raises the question of image quality, and whether it would degrade substantially if the image is scaled up too much.

Change 425721 abandoned by Jdlrobson:
Separate thumbnail from rest of page preview

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

@ovasileva we originally estimated this as a 1 but after looking into this it's turned out to be a lot more complicated and risky as the rendering code still predates the rewrite and doesn't make this easy. We feel we'll need to rewrite the entire rendering code here (a 8 pointer potentially) @Jdrewniak will create a task/link tasks relating to the problem here about what needs to be done. I'm going to thus throw this back into the backlog for further analysis.

Let's talk about this in the next product meeting.

Jdlrobson removed the point value for this task.
Jdrewniak added a comment.EditedApr 24 2018, 3:54 PM

In my initial implementation, I put a border on the text-extract to separate the thumbnail (see the Holy Roman Emperor image above). This proved to be a problem when the thumbnail was too narrow. After getting some feedback on this issue, I realized that maybe the solution could be to resize the popup to make it less wide in that situation, because, If the popup didn't include that extra white-space, then the first approach of adding a border could work. This might be preferable to rewriting the template markup and I've made a task in that direction T192928.

The approach outlined above was mostly working until I hit one snag. The situation in which the tall thumbnail is very narrow.
In that situation, we don't scale the thumbnail up, we leave the space between the text and thumbnail blank, which is not a problem until you try to put a border on the extract...


Looks good until you try to do this

@alexhollender - any thoughts on this?

@ovasileva @Jdrewniak I was hoping that T192928 might provide the solution here — the whole panel would be more narrow so in the case of the screenshot above there wouldn't be white space between the line and the image.

Jdlrobson changed the task status from Open to Stalled.May 7 2018, 9:56 PM

Looks like we want to stall this until T192928 is done.

@Jdrewniak what if we added a polyline shape programmatically? We'd have to export pointerSize from renderer and work out where the pokey is and adjust the starting Y coordinates based on it (and also take into account RTL), but given the mask is already made in JS that doesn't seem like the worst of ideas...

Output would look like

<svg>
  <image>
  <polyline stroke="rgba(0, 0, 0, 0.1)" points="0 0 0 242" stroke-width="1"></polyline>
</svg>

and code:

const line = document.createElementNS( nsSvg, 'polyline' );
	const isTall = className.indexOf( 'not-tall' ) === -1;
	const points = workOutPoints(isTall, pokeyOnTop);

	line.setAttribute( 'stroke', 'rgba(0, 0, 0, 0.1)' );
	line.setAttribute( 'points', points.join( ' ' ) );
	line.setAttribute( 'stroke-width', 1 );
This comment was removed by ovasileva.