Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

amp-lightbox-gallery: Different images for thumbnail and detail #21736

Closed
machal opened this issue Apr 5, 2019 · 33 comments
Closed

amp-lightbox-gallery: Different images for thumbnail and detail #21736

machal opened this issue Apr 5, 2019 · 33 comments

Comments

@machal
Copy link

machal commented Apr 5, 2019

Describe the new feature or change to an existing feature you'd like to see

Feature request for simple use case of the amp-lightbox-gallery component. Like in jQuery plugins:

<a href="detail.jpg">
  <amp-img src="thumbnail.jpg" lightbox>
</a>

Describe alternatives you've considered

I've tried to do it with srcset/sizes as recommended in #14900 and other issues and I've ended with following code:

<amp-img
  src="foto_1_tb.jpg"
  srcset="
    foto_1_tb.jpg 200w,
    foto_1.jpg 1024w
    "
  sizes="
    (max-width: 900px) calc((100vw - 8 * 0.25rem) / 4),
    200px
    "
  alt="" layout="responsive"
  width="200" height="200" 
  lightbox role="button" tabindex="0">
</amp-img>

Live website: https://www.vzhurudolu.cz/amp/kurzy/rychlost-nacitani

Issues I can see here:

@aghassemi
Copy link
Contributor

Thanks @machal for the feedback. I agree the correct solution is not very ergonomic.

Maybe we can have an API like:

<amp-img src="low-res" lightbox lightbox-src="super-high-res" > 

OR

<amp-img src="low-res.png" lightbox lightbox-image-id="myHighResImage" >
<amp-img hidden id="myHighResImage" src="high-res.png">

the benefit of second one is consistency with gallery thubnail API and allowing other attributes such as `srcset` and fallbacks without the need to expose those as additional attributes.

/cc @cathyxz

@cathyxz
Copy link
Contributor

cathyxz commented Apr 8, 2019

I still suggest using object-fit to crop your initial image because this allows <amp-lightbox-gallery> to animate the transition. I think with object-position, it might be possible to be a lot more precise with the cropping as well.

The reasoning here is because it's not possible to animate the transition from one image to a second image with a different aspect ratio because it's not possible to programmatically figure out how that translates, so supporting different aspect ratio means no transition animation.

If we are ok with doing no transition or doing a fade transition, then the second API will probably work better, since that allows us to still make use of srcset and sizes for handling responsiveness to different device widths.

@machal
Copy link
Author

machal commented Apr 11, 2019

@cathyxz Thanks for idea with object-fit. But I my case (and many other cases) it is impossible, because I want hand made crops in the thumbnail. No transition is OK in that case.

The case with <amp-img hidden> is fine.

But I think there should be an extra option with <a><amp-img> for extra simple cases (and jQuery developers). Mostly I don't really need srcset for detail image, because it is not downloaded with the page. Only one hires image could solve the problem.

You may consider it.

@cathyxz
Copy link
Contributor

cathyxz commented Apr 11, 2019

A few thoughts here:

  1. For the <a><amp-img/></a> use case, maybe having an on="tap:myLightboxId.open" action could solve that. If there are certain cases where we do no transition, we could also allow tap actions to trigger opening a lightbox specified by a lightboxId. This would require a slight refactor to the open_ function in amp-lightbox-gallery.js and expanding the list of TAP_ELIGIBLE_TAGS but is fairly reasonable to do.
  2. For images with different crop, I wanted to note the existing <picture> tag. I think that is the existing browser standard for doing art direction and different crops (already supported by most major browsers: https://caniuse.com/#search=picture). Do we want to keep in line with that and support <picture> with different <source> objects for crop? (The syntax isn't super dev-friendly, but it is an existing standard.)

Option 1 here and

<amp-img src="low-res.png" lightbox lightbox-image-id="myHighResImage" >
<amp-img hidden id="myHighResImage" src="high-res.png">

both allow more developer control as opposed to leaving it up to the browser's discretion.

@machal
Copy link
Author

machal commented Apr 12, 2019

The idea with <picture> is interesting. You mean something like this?

<picture>
  <amp-img src="thumbnail.jpg" media="(max-width: 599px)" 
    layout="responsive" width="200" height="200"  alt="" lightbox>
  <amp-img src="detail.jpg" media="(min-width: 600px)"
    layout="responsive" width="1600" height="900" alt=""></amp-img>
</picture>

Looks good for me and – because it is the standard – it should be one of the options.

But for lot of developers it will be hard to think/write in comparison with <a><amp-img/></a> or two <amp-img/> elements. So I think amp-lightbox-gallery should accept both.

Another problem is that my initial srcset/sizes solution is somewhere working but undocumented. Should I open new issue in docs repo?

@sparhami
Copy link

You can do crops with a combination of transform: scale(blah) and object-position on the <img> within the <amp-img> to do the crop. I do have a pending PR (still need to figure out if it is something we want) to support animating those types of crops correctly: ampproject/animations#18. Here is a demo of what such an animation would look like: https://sparhami.github.io/animations/docs/demo/zoom-crop/ and what the CSS needed to do the cropping looks like. This uses a simple object-position of center bottom, but you can move the horizontal and vertical axes by a percentage amount, pixel amount, or a combination of the two using calc.

One problem here would be specifying the scale / object position for the child img. This would need to be done via CSS, since you cannot add inline styles to the img directly. One possibility would be to have something like:

amp-img img {
  transform: var(--img-transform);
  object-position: var(--img-object-position);
}
<amp-img src="..." style="--img-transform: scale(2); --img-object-position: center bottom"></amp-img>

I suppose one useful thing would be to have a tool to generate the correct DOM structure / CSS when cropping to a specific portion of an image.

@sparhami
Copy link

This is still a work in progress, but here is a tool where you can choose an image, then select an area to crop and it will give you the HTML you need:

https://sparhami.github.io/animations/docs/tools/img-cropper/?output=amp

@maciejmackowiak
Copy link

Thanks @machal for the feedback. I agree the correct solution is not very ergonomic.

Maybe we can have an API like:

<amp-img src="low-res" lightbox lightbox-src="super-high-res" > 

OR

<amp-img src="low-res.png" lightbox lightbox-image-id="myHighResImage" >
<amp-img hidden id="myHighResImage" src="high-res.png">

the benefit of second one is consistency with gallery thubnail API and allowing other attributes such as `srcset` and fallbacks without the need to expose those as additional attributes.

/cc @cathyxz

@aghassemi
Any updates on this?
I think this would be a perfect solution.

@archon810
Copy link

I'd love to see this implemented soon as well because it makes the lightbox totally unusable in the most generic case of having smaller thumbs and larger full sized images.

Right now it doesn't seem to be possible to have a gallery with low-res image thumbs because once tapped, the image that is then loaded into view is the thumb and not the full size. That'd be like the web existing without <a href='full.jpg'><img src='thumb.jpg'></a> and only having <a href='full.jpg'><img src='full.jpg'></a> or <a href='thumb.jpg'><img src='thumb.jpg'></a> - can you imagine the bandwidth waste and speed loss in the former case and potato quality of the latter? Even using srcset and intermediate sizes doesn't give us the correct solution.

What am I missing? How's it possible amp-lightbox-gallery doesn't support the most basic use case for lightbox galleries?

@sparhami
Copy link

I'd love to see this implemented soon as well because it makes the lightbox totally unusable in the most generic case of having smaller thumbs and larger full sized images.

Right now it doesn't seem to be possible to have a gallery with low-res image thumbs because once tapped, the image that is then loaded into view is the thumb and not the full size. That'd be like the web existing without <a href='full.jpg'><img src='thumb.jpg'></a> and only having <a href='full.jpg'><img src='full.jpg'></a> or <a href='thumb.jpg'><img src='thumb.jpg'></a> - can you imagine the bandwidth waste and speed loss in the former case and potato quality of the latter? Even using srcset and intermediate sizes doesn't give us the correct solution.

What am I missing? How's it possible amp-lightbox-gallery doesn't support the most basic use case for lightbox galleries?

You can already do this. You can use srcset to specify different resolutions of the image to use. AMP will pick the correct one for your smaller image and use a higher resolution one for the lightbox. For an example, see the following glitch: https://southern-tempo.glitch.me/

This does:

<amp-img
    layout="fixed"
    width="135" height="90"
    lightbox
    srcset="https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach-small.jpg?v=1566332064756 338w,
            https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach.jpg?v=1566331576281 1350w"
    sizes="135px"
</amp-img>

This uses causes the lower resolution thumb to be picked when the content is in the page, and a higher resolution one to be picked for the gallery. There is no extra bandwidth being wasted here to download the high resolution image.

The page also contains examples of cropping using object-fit and a transform. Notice that all three cases download the lower resolution source.

Is there a particular example page where this is not working for you?

@machal
Copy link
Author

machal commented Aug 21, 2019

Let's summarize this issue. There are two topics here:

1) Implement <a href="detail.jpg"><amp-img></a> in amp-lightbox-gallery

Although commenters suggested many useful ways how to do it with srcset and object-fit, they do not solve my issue:

In my case (and many other cases) it is impossible, because I want hand made crops in the thumbnail. No transition is OK in that case.

2) Update docs with srcset solution

I've added issue for the docs team: ampproject/amp.dev#2804

@maciejmackowiak
Copy link

@sparhami
Your solution works only for fixed width images but what if I'd like to have responsive thumbnail image?
When I use sizes then AMP is adding inline styles, or am I doing something wrong?
https://angry-blade.glitch.me/

So I'm afraid that the case mentioned by @archon810 is not solved with your solution.
Why there is not simple solution like for example this one mentioned by @machal :

<amp-img src="low-res" lightbox lightbox-src="super-high-res" >

@sparhami
Copy link

So you can leave off sizes, and amp-img will generate them for you. For example this glitch:

https://furtive-octagon.glitch.me

  <amp-img
    layout="responsive"
    width="135" height="90"
    lightbox
    srcset="https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach-small.jpg?v=1566332064756 338w,
            https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach.jpg?v=1566331576281 1350w">
  </amp-img>

For some reason, this isn't picking the correct source in Chrome devtools device emulation mode, but the sizes media query is being generated correctly. If you don't do device emulation, and just narrow the window, it should work correctly in Chrome / Firefox.

You can check which source was used by finding the <img> and checking $0.currentSrc in the console.

This will pick the smaller resolution on a narrow screen, and the larger one on a wider screen. Note that the sizes attribute will be recomputed as you resize the screen, which may cause the higher resolution image to be downloaded. Also note, once the higher resolution image in downloaded, the browser may just use that, even if you resize smaller. You can hard refresh the page or load without cache to test it again.

@maciejmackowiak
Copy link

maciejmackowiak commented Aug 22, 2019

@sparhami
I don't think that it can be solved this way.
I've loaded Your example page in incognito mode in chrome and take a look at this:

obraz

it loads the big image, so that is not the solution.

@sparhami
Copy link

Screenshot from 2019-08-22 14-46-48

In the screenshot above, I loaded the page with a window width of 327 pixels (which is under the 338 declared in the srcset), and got the small image. After resizing it I correctly get the larger image.

What window width are you using? It should definitely load the smaller one if the page was loaded with the correct dimensions. When I load the page with a width of 337 pixels, I correctly get the smaller image. With 339, I correctly get the larger image.

Note that with srcset, if you go past a smaller size (in the example above, if your width is over 339 pixels), it will jump up to the next size and does not try to find the nearest size. For the example, I created only two sizes, but in practice, you will want to generate more intermediate steps.

I believe the image picked also takes into the display pixel density, window.devicePixelRatio, so you might need to account for that when testing out which image gets picked. For example, if you have a device pixel ratio of 2, it may choose the higher resolution image when the window width goes over 169 rather than 338.

@maciejmackowiak
Copy link

maciejmackowiak commented Aug 22, 2019

I know how the srcset works, so thats why I'm pointing out that it's not the solution for the case mentioned byt @machal and @archon810
How can I use ligtbox gallery but with usage like this:
<a href='full.jpg'><img src='thumb.jpg'></a>
And that it work across all devices?
This solution looks perfect:
<amp-img src="low-res" lightbox lightbox-src="super-high-res" >

@archon810
Copy link

archon810 commented Aug 22, 2019

I'm testing this stuff on Chrome emulator for Windows with window.devicePixelRatio=2, and I can't make sense of the results and can't seem to get to the desired solution that @maciejmackowiak and I would want (well, sort of - see the ending).

The easiest way to test is in incognito and with Disable cache in the Network tab so that page refreshes always show uncached data.

We need to emulate the aforementioned scenario where we only have 2 images - a small thumb (always 215px wide in this example) and a potentially really large original image. No matter the device viewport, we'd like the thumb to load until clicked on, and then the original image to load - just what you'd expect from <a href='full.jpg'><img src='thumb.jpg'></a>.

Using your own images:
https://lightbox-300-1000.glitch.me/: -small loads until 336px (why?):

srcset="https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach-small.jpg?v=1566332064756 300w,
            https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach.jpg?v=1566331576281 1000w">

https://lightbox-300-10000.glitch.me/: -small loads until 867px (why?):

srcset="https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach-small.jpg?v=1566332064756 300w,
            https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach.jpg?v=1566331576281 10000w">

The last example would probably work for our purposes, but I'd really love to understand why it switches at that particular breakpoint, how universal the solution would be, and whether we're thinking about the whole thing the wrong way.

@maciejmackowiak
Copy link

@sparhami
So is it possible or not to to use a lightbox gallery having only two image sizes?
Because solution with srcset will never be reliable for all devices.

Like in this example:
<a href='full.jpg'><img src='thumb.jpg'></a>
Let assume that I cannot generate more thumbnails images but I still would love to use lightbox gallery.

Is it possible to implement this solution(or something similar):
<amp-img src="low-res" lightbox lightbox-src="super-high-res" >
Thanks
Maciej

@sparhami
Copy link

sparhami commented Aug 27, 2019

Ideally, srcset would be used with the auto generated sizes to pick an appropriate source depending on the resolution. Due to the behavior of sizes in AMP, it is not really possible to use it to manually specify which of the srcset to use.

#24223 adds an img-sizes attribute to <amp-img> to cover this gap. Once it lands, you can do something like:

  <amp-img
    layout="responsive"
    width="135" height="90"
    lightbox
    srcset="https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach-small.jpg?v=1566332064756 338w,
            https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach.jpg?v=1566331576281 1350w">
    img-sizes="338px"
  </amp-img>

to always pick the lower resolution image. You should also be able to just set img-sizes to something like 1px if you want to always grab the lowest resolution.

@archon810
Copy link

Rooting for #24223. Wonder when it might land in stable.

@archon810
Copy link

Can we get this issue moving toward a solution please?

@caroqliu
Copy link
Contributor

@archon810 Is the new disable-inline-width attribute sufficient for your use case?

@maciejmackowiak
Copy link

Hey @caroqliu
Can you please provide working example to use disable-inline-width for the case I've mentioned earlier?
Because it's not very well documented and I'm probably using it wrong.

So is it possible or not to to use a lightbox gallery having only two image sizes?
Because solution with srcset will never be reliable for all devices.

Like in this example:
<a href='full.jpg'><img src='thumb.jpg'></a>
Let assume that I cannot generate more thumbnails images but I still would love to use lightbox gallery.

We need a solution to have only two images, first one is always used for displaying it on a page and second one is always used for lightbox gallery.

Thanks in advance
Maciej

@caroqliu
Copy link
Contributor

caroqliu commented Mar 30, 2020

Thanks for clarifying -- to be clear, the <amp-img> on the page should always select the lower resolution src as well as fill responsively to the viewport?

The disable-inline-width attribute disables the inline styles applied as described earlier:

Your solution works only for fixed width images but what if I'd like to have responsive thumbnail image? When I use sizes then AMP is adding inline styles, or am I doing something wrong?

This means it should be possible to target the underlying img with CSS because the inline width does not have to be overridden.

@machal
Copy link
Author

machal commented Mar 31, 2020

@caroqliu I'm not sure, if it solves situation when we want:

  • one image file for thumbnail
  • different image file for full version in lightbox

As me and @maciejmackowiak wrote:

<a href='full.jpg'><img src='thumb.jpg'></a>

@caroqliu
Copy link
Contributor

caroqliu commented Apr 1, 2020

The disable-inline-width is an alternative to img-sizes mentioned earlier in this thread. Once the change is in, you should be able to do the following to achieve what you have described:

<amp-img
  layout="responsive"
  width="135"
  height="90"
  lightbox
  srcset="https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach-small.jpg?v=1566332064756 338w,
        https://cdn.glitch.com/e7729dec-fc32-4f68-8d6e-13ef08b88715%2Fbeach.jpg?v=1566331576281 1350w"
  sizes="337px"
  disable-inline-width
>
</amp-img>

@machal
Copy link
Author

machal commented Apr 2, 2020

@caroqliu Thanks for reply! But wasn't srcset attribute specified for images with the same content?

What I want with <a href='full.jpg'><img src='thumb.jpg'></a> is "art-direction". Thumbnail and full images does not have the same content. Thumbnail is for example crop of the full image, but it is impossible to create it automatically with object-fit.

And more – nobody still explained, why this so usual and popular code pattern like <a href='full.jpg'><img src='thumb.jpg'></a> is not possible in AMP.

@caroqliu
Copy link
Contributor

caroqliu commented Apr 2, 2020

Could you describe more what is not possible about the code pattern? I was able to create this working, valid sample: https://jsbin.com/vejasojiti/1/edit?html,output

@machal
Copy link
Author

machal commented Apr 8, 2020

@caroqliu This is not what we want. Your example contains <a target="_blank"> so large image opens in new browsers tab.

What we want is to open full size image in lightbox, like here: https://dbrekalo.github.io/simpleLightbox/

Originally we wanted to add this functionality with markup like this into AMP lightbox:

<a href="detail.jpg"><!-- <- Opens in lightbox -->
  <amp-img src="thumbnail.jpg">
</a>

@caroqliu
Copy link
Contributor

caroqliu commented Apr 8, 2020

I see, thank you for illustrating the issue further, and for your patience in communicating your goals to a few of us. 😄 This sounds like the UX is current achievable with the open action suggested earlier. Here is the relevant sample: https://jsbin.com/nuhifotowo/edit?html,output

I recognize that it's not the pattern you are most familiar with, nor is it very ergonomic. I'm happy to keep the issue open until the exact usage pattern described here is available in AMP, but as far as I'm aware we don't have the bandwidth to implement this soon.

@machal
Copy link
Author

machal commented Apr 9, 2020

@caroqliu Thank you very much, Caroline. I'm proposing this pattern not because I need it, but because of its simplicity which most jQuery users like. Will be great, if AMP will add it.

@stale
Copy link

stale bot commented Nov 6, 2021

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.

@stale stale bot added the Stale Inactive for one year or more label Nov 6, 2021
@stale stale bot closed this as completed Nov 16, 2021
@AaronMLB
Copy link

Please reopen and implement something to solve this issue!

I have photo galleries on user profiles. The full size images that end users upload could be of wildly different sizes and ratios.
I already ask them to crop a 3:2 thumbnail of whatever they upload, so that pre-exists as a common small version.

The large original I would like to open in the lightbox view, whilst the common, possibly cropped 3:2 thumbnail is visible on the page as the lightbox trigger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants