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

Responsive Layout for <amp-img> BREAKS aspect ratio when nested in <amp-carousel> #1531

Closed
johnnyshankman opened this issue Jan 22, 2016 · 25 comments

Comments

Projects
None yet
7 participants
@johnnyshankman
Copy link

commented Jan 22, 2016

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
screen shot 2016-01-22 at 1 55 08 pm

@johnnyshankman

This comment has been minimized.

Copy link
Author

commented Jan 22, 2016

Here is what is rendered when an <amp-img> is within any other standard element, in this context a <div>:

<div>
  <amp-img src="https://placehold.it/300x450" layout="responsive" width=300 height=450></amp-img>
</div>

screen shot 2016-01-22 at 1 57 54 pm

@johnnyshankman johnnyshankman changed the title Responsive Layout for <amp-img> loses aspect ratio inside <amp-carousel> Responsive Layout for <amp-img> BREAKS aspect ratio inside <amp-carousel> Jan 22, 2016

@johnnyshankman johnnyshankman changed the title Responsive Layout for <amp-img> BREAKS aspect ratio inside <amp-carousel> Responsive Layout for <amp-img> BREAKS aspect ratio when nested in <amp-carousel> Jan 22, 2016

@dvoytenko dvoytenko self-assigned this Jan 22, 2016

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2016

Yes, this is clearly a bug. We'll fix shortly. In the meantime, could you place each image in a div as a workaround?

@johnnyshankman

This comment has been minimized.

Copy link
Author

commented Jan 22, 2016

Let me give it a go and see what happens.

@johnnyshankman

This comment has been minimized.

Copy link
Author

commented Jan 22, 2016

Putting each <amp-img> element inside of a <div> does not quite fix the problem.

Now it respects the aspect ratio and the width, but not the height.

screen shot 2016-01-22 at 2 19 37 pm
Clips bottom of image

screen shot 2016-01-22 at 2 19 45 pm
Gets pinned to top of box

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2016

@johnnyshankman now it actually looks correct. Preserving the aspect ratio is actually what layout=responsive does.

In case of 500x200 case where it's pinned to top, you can always assign a class to the parent div to align the content in the center, e.g. via flexbox.

In case of 300x450 where it's clipped - there's no easy way to address it via CSS, since CSS is a bit challenge when dealing with height. If you find a way - let me know. That being said, since you know the aspect ratios ahead of time - you should be able to match them.

@johnnyshankman

This comment has been minimized.

Copy link
Author

commented Jan 22, 2016

So you're telling me that if I tell an <amp-img> with set height and width to be responsive within a carousel of set height and width, it shouldn't adjust its width in orderto make the the height fit within the carousel (while maintaining aspect ratio)?

I don't mean to say that makes the carousel useless but I'm pretty sure there is no valid way to make this work since I'd need to either use !important or inline styles to override the height. Neither of those are valid AMP markup.

The centering of the 500x200 is not an issue, as you have explained. I can fix that with CSS. So thank you for helping with that!

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2016

layout=responsive essentially means "responsive width". It takes all of the available width and calculates height automatically based on the aspect ratio. This is certainly a limitation. Unfortunately, however, our requirement is to ensure that all layouts are implemented via CSS in initial layout and we found no way to express aspect ration constraints based on height - only based on width. Certainly if you have recommendation, we'd like to hear.

@dvoytenko dvoytenko assigned erwinmombay and unassigned dvoytenko Jan 22, 2016

@johnnyshankman

This comment has been minimized.

Copy link
Author

commented Jan 22, 2016

Dang, that's a real game breaker for us.

Well, I'll report back if I can manage to produce a working/valid solution.

Unfortunately unless one of your engineers comes up with a working solution in mean time, I won't have more time to spike on this again for a few weeks which means you won't hear back from me for a little while. I'll continue to monitor this issue just in case that does happen though! crosses fingers

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2016

Hmm. This is a pretty typical problem. What do you do today to resolve it?

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2016

@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.

@johnnyshankman

This comment has been minimized.

Copy link
Author

commented Jan 22, 2016

@dvoytenko I eventually found that our API can pad the images via query parameters so I did some math in order to always pad the images into a square. I totally don't want to continue doing that though so I will try this solution first thing Monday morning and report back with my findings.

The JSBin looks promising though!!!! Thank you for that!

@johnnyshankman

This comment has been minimized.

Copy link
Author

commented 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;  
}

@rudygalfi rudygalfi added the Type: Bug label Jan 23, 2016

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2016

Awesome. Yes. We will document.

@dvoytenko dvoytenko closed this Jan 23, 2016

@webdados

This comment has been minimized.

Copy link

commented Feb 25, 2016

@johnnyshankman I've tried your approach and the carousel shows up on the top of the screen and not centered. Any other place I could chat with you?

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2016

You can post a link to your document here or try us on slack https://github.com/ampproject/amphtml/blob/master/DEVELOPING.md#slack-and-mailing-list

@webdados

This comment has been minimized.

Copy link

commented Mar 2, 2016

@dvoytenko Thanks!

If the images have all the same size, the carousel shows on top: http://filmspot.pt/artigo/o-gerente-da-noite-canal-amc-estreia-minisserie-de-espionagem-8126/amp/

If the images have different sizes, I'm making the carousel width and height the maximum width and height of the several images, it still shows on top, but images are being clipped: http://filmspot.pt/artigo/imagens-da-antestreia-mundial-de-star-wars-o-despertar-da-forca-7854/amp/

Just remove /amp/ on both urls to see the desktop (or mobile, depending on device) version.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Mar 2, 2016

@webdados Thanks for sharing the examples. These cases look fairly straightforward and not really related to images - the carousels themselves are not aligned vertically inside the lightboxes. One option you can do is to use display: flex like so:

.lightbox {
    display: flex;
    flex-direction: column;
    justify-content: center;
}
.lightbox amp-carousel {
  min-width: 100%;
}

There are a number of other approaches, but I prefer flex in cases like this.

Also, we are currently designing a carousel-based lightbox which should fit this case nicely in the future. Stay tuned!

@webdados

This comment has been minimized.

Copy link

commented Mar 2, 2016

@dvoytenko I will stay tuned!

Is flex fully supported by today's mobile devices?

(I've implemented your CSS rules and still got the vertical images clipped on the Star Wars example)

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Mar 2, 2016

@webdados It's supported essentially everywhere, but you may need to add vendor-prefixed properties as well. This is typically done for you by most of the CSS tools, though.

@dandv

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2016

@dvoytenko

Awesome. Yes. We will document.

Should we mention in the amp-carousel documentation that the user needs to add a CSS rule for the amp-img imp setting object-fit: contain;?

See this SO question. @adewale's answer is now out of date.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2016

@dandv We may do so, yes. So far we resisted because it's easy not to notice the improper original image size issues with the default object-fit: contain. So far, most of issues we've seen were due to the incorrect sizing rather than intentional image fitting in a different aspect ratio.

@Flowgram

This comment has been minimized.

Copy link

commented Feb 2, 2017

This still breaks if each image is wrapped in a div: http://jsbin.com/qukosutibo/edit?html,css,output

Notice how the tall images are getting cut off.

I need to wrap my images divs because eventually I'd like to include a caption below each one.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Feb 2, 2017

@Flowgram layout=responsive is explicitly a width-based layout. It takes the available width and calculates height based on the given ratio. In your example:

  <amp-carousel class="carousel1" height=400 width=400 type=slides>
    <div><amp-img src="https://placehold.it/300x950" layout="responsive" width=300 height=950></amp-img></div>

carousel1 has the width of 400, while the image's given ratio is 300:950. As the result, the image will be resized to have width 400 and height 950 * 400 / 300 = 1266. Then you apply object-fit: contain that will center the image within the 400x1266 rect.

I think in your case you want to make images to have the same size as the carousel (400x400 or layout=fill) and use object-fit: contain to size/center the images.

@Flowgram

This comment has been minimized.

Copy link

commented Feb 2, 2017

@dvoytenko Thanks for getting back to me.

Thing is, it works perfectly without the wrapping div. Height of each image is restricted to the carousel height regardless of the height passed in for each image, see carousel2 here: http://jsbin.com/fagigajoda/edit?html,css,output
That is the behaviour I'm looking for.

Eventually this is the markup that I'm aiming for:

<amp-carousel class="carousel1" height=400 width=400 type=slides>
<div class="slide">
<amp-img src="https://placehold.it/300x950" layout="responsive" width=300 height=950></amp-img>
<div class="caption">Caption</div>
</div>
@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Feb 2, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.