<amp-lightbox-gallery> images stretch beyond their width without clever CSS

[nainar]

"From Simon Rønbøl: ""You have to do some pretty hacky css to make it so that the image being displayed in the amp-ligthbox-gallery doesn’t stretch beyond the width of the image""

You can see this in the amp playground if you chose responsive under devices, and drag the page out, and then click one of the lower resolution images, the larger image will be stretched out, pixelating the image.

The way we solve this right now is that we use this bit of css to target the amp-image-viewer to style on the layout so that even though it is “fill” it won’t fill the screen, and center itself.

amp-image-viewer[layout=fill]{
    transform: translate(-50%, -50%);
    top: 50%;
    left: 50%;

    &> img{
        position: relative;
    }
}"

cc @morsssss who filed this.

[sparhami]

Not sure which page the issue is from, but you can use something like:

amp-lightbox-gallery .amp-carousel-slide img {
  object-fit: scale-down;
}

to make sure the rendered image never exceeds the native image dimensions, no hacky CSS required.

That said, auto lightbox should only lightbox when there is enough resolution to do so. If you have an image that you want to explicitly lightbox, you should ensure that there is a srcset with at least one image with enough resolution such that the image is not overly scaled in the lightbox.

[morsssss]

/cc @SimonHogstromRonbol - can you comment on this proposed solution?

[SimonHogstromRonbol]

what @sparhami is proposing, using object-fit is a great solution, I hadn't thought of that, it's much cleaner than what we had come up with.

shouldn't we make this the default though? I'm sure styling an img tag inside an amp element isn't ideal, I don't know enough about how wordpress handles css treeshaking through the AMP plugin, but i know that we would atleast have to whitelist this rule in our build process to not have it be shaken away.

also, ie 11 doesnt support object-fit, but i'm guessing that since it really isn't broken, that it just looks ugly when its scaled, we shouldnt go out of our way to make the solution compatible with ie 11?

[morsssss]

This is what I was wondering. We often tell people that AMP makes it easier to create beautiful websites and lovely interactions - and that it makes things easier for developers by doing the hard work for them.

If an experienced site maker had to hunt around to find a way to make <amp-lightbox-gallery> work correctly, others will surely have trouble as well. @sparhami , can we build this object-fit solution into the component? Or, if there's a solution that works for the older browsers shown here, that's even better!

[sparhami]

If we want to change this, we will want to update the code here:

https://github.com/ampproject/amphtml/blob/26295bc28d43750dc7fb1550c2b1841426a7ac2b/extensions/amp-image-viewer/0.1/amp-image-viewer.js#L358-L364

to something like:

if (this.sourceWidth_ < width || this.sourceHeight_ < height) {
  width = this.sourceWidth_;
  height = this.sourceHeight_;
}

This will make sure the pan-zoom state works correctly, where changing to object-fit: scale-down can result in extra empty space on the sides/top when zoomed in and panning.

One thing that might be a good idea is to generate a console warning when we hit this case, letting developers know that they should provide a higher resolution image, ideally using a srcset. Since the current behavior, to fill the available space, seems intentional in the code I think we should put a bit more thought into this before changing the behavior here.

[cathyxz]

Interesting. I think this was 100% intentional on mobile, but there have been discussions about where this doesn't look ideal on desktop. You can either:

1. Make not-fill entire page (only stretch to image dimensions) the default UI on desktop, but keep stretch-to-fill on mobile. This is probably fine since desktop doesn't do pan/zoom anyway.
2. Make this behavior (only stretch to image dimensions) a optional behavior gated by attribute.

The former would be a trivial code change, since no pan/zoom behavior needs to be changed (yet).

[SimonHogstromRonbol]

i like solution 2 a lot, because that way, if I feel like I don't want an image stretched on mobile eigther, I could apply that attribute, to get that behavior on there too.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.