amphtml: Responsive Layout for BREAKS aspect ratio when nested in

This bug breaks <amp-carousel>s containing <amp-img>s of different sizes completely.

The code block in question

<amp-img class="full-bleed" 
placeholder
src="https://placehold.it/300x450"
tabIndex=0
on="tap:my-lightbox"
width=400
height=400
layout="responsive"
role="button"></amp-img>
<amp-lightbox 
  id="my-lightbox"
  class="lightbox" 
  layout="nodisplay">
  <div class="lightbox-content">
      <amp-carousel height=400 width=400 type=slides>
        <amp-img src="https://placehold.it/300x450" layout="responsive" width=300 height=450></amp-img>
        <amp-img src="https://placehold.it/300x450" layout="responsive" width=300 height=450></amp-img>
        <amp-img src="https://placehold.it/300x400" layout="responsive" width=300 height=400></amp-img>
        <amp-img src="https://placehold.it/300x450" layout="responsive" width=300 height=450></amp-img>
        <amp-img src="https://placehold.it/500x200" layout="responsive" width=500 height=200></amp-img>
        </amp-anim>
      </amp-carousel>
  </div>
</amp-lightbox>

Everything works fine but the images fill the box without regards to aspect ratio, even with the declared height and width inside of the AMP-HTML tag. Normally when I declare a height and width and use responsive as the layout the <amp-img> maintains its aspect ratio.

screen shot 2016-01-22 at 1 55 16 pm

About this issue

Original URL
State: closed
Created 8 years ago
Comments: 25 (13 by maintainers)

Most upvoted comments

@johnnyshankman Forgot about this option. It’s possible to use layout=responsive or layout=fill for replaced content such as images to preserve aspect ratio in slides. Here’s an example:

http://jsbin.com/vuxite/edit?html,css,output

In a nutshell you have to see object-fit: contain on the encapsulated IMG element.

Let me know if this works for you. This would work for any replaced element: img, video, etc.

dvoytenko on Jan 22, 2016

@dvoytenko Just kidding I got antsy. IT WORKS!

Both of the solutions in the JS Bin works perfectly. 👍

The images maintain their aspect ratio and contain themselves inside the carousel box.

I think the object-fit + layout=“fill” workaround is great, and maybe should be documented because I’m sure tons of people will want this functionality from their slideshows/carousels.

My final working responsive amp-lightbox with slideshow goes a little like this:

<amp-img
placeholder
src="https://placehold.it/300x450"
tabIndex=0
on="tap:my-lightbox"
width=300
height=450
layout="responsive"
role="button"></amp-img>
<amp-lightbox 
  id="my-lightbox"
  class="lightbox" 
  layout="nodisplay">
  <div class="lightbox-content">
    <amp-carousel layout="responsive" height=320 width=320 type=slides>
      <amp-img src="https://placehold.it/300x450" layout="fill"></amp-img>
      <amp-img src="https://placehold.it/300x450" layout="fill"></amp-img>
      <amp-img src="https://placehold.it/300x400" layout="fill"></amp-img>
      <amp-img src="https://placehold.it/300x450" layout="fill"></amp-img>
      <amp-img src="https://placehold.it/500x200" layout="fill"></amp-img>
    </amp-carousel>
  </div>
</amp-lightbox>

and in the CSS

amp-carousel > amp-img > img{
  object-fit: contain;  
}

johnnyshankman on Jan 23, 2016

This is because carousel forces each slide (an immediate child) to be sized by the carousel’s size. The same rules cannot be applied to the children within the slide itself since we can’t make assumption about the design. E.g. it maybe perfectly justifiable to have content of a slide overflow for some reason.

The easiest change from your current sample is to update <amp-img> to use layout=fill. See http://jsbin.com/bibayat/edit?html,css,output

dvoytenko on Feb 2, 2017