Amphtml: Add a max-width attribute for amp-img elements with layout=responsive.

[plegner]

Small AMP images that have layout=responsive are upscaled if the available space is larger. This can be fixed by adding a special ID and a max-width style in style amp-custom, but this is cumbersome if there are lots of images, and if the page styles are generated independently of the page content. You

You could also use the sizes attribute to achieve this, but that is difficult if there is no simple relationship between the viewport width and the available width for the image (e.g. multiple columns).

It would be great to have an additional max-width attribute that acts the same as an inline style (style="max-width: 92px"):

<amp-img src="..." alt="..." width="92" height="64" max-width="92" layout="responsive"></amp-img>

[ericlindley-g]

Thanks @plegner!
@aghassemi — did you have any thoughts about this?

The feature seems useful, but I have some reservations about the slippery slope of what styles we support as attributes versus only in amp-custom CSS

[aghassemi]

@ericlindley-g https://github.com/ampproject/amphtml/issues/4988 is also asking to expose a style property as attribute. Considering we are exposing width and height as attributes, I am not opposed to exposing min and max too. But as you mentioned we need to be careful on how far we take this.

[elbaulp]

I think this is somehow related to the problem I'm having http://stackoverflow.com/questions/42586485/styling-responsive-layout-in-amp-img

If it isn't, I could open a new issue.

[SchnWalter]

This would break existing websites. Instead I would like to propose a new layout. Maybe it could be called "shrink-only".

Here's a quick patch, probably it's incomplete:
7009-amphtml_layout_shrink.patch.txt

I'm not sure if for adding a new layout you need to add something to the Calculate effective layout. section.

[aghassemi]

@SchnWalter I don't see how a new attribute (which will be opt-in) would break existing websites. Am I missing something?

Is there a difference between <amp-img width=400 height=200 layout=shrink-only> and <amp-img width=400 height=200 max-width=400 max-height=200 layout=responsive> in terms of the behaviour you had in mind for layout=shrink-only?

[SchnWalter]

The layout attribute is an attribute that it is already used by various elements, and introducing a new value, or set of values (for height and width), feels like the right thing to do and this way you don't introduce a new set of AMP specific attributes.

Besides that, you would need to make a lot more code changes if you want to introduce a new attributes. So from a technical dept point of view, having a new value make more sense.

EDIT: And besides that, the layout attribute is already used to alter the behavior of the "width" and "height" attributes, so it would make sense to limit them using this attribute.

[aghassemi]

Thanks for clarifying @SchnWalter. I see your points, layout=shrink-only is certainly an option to consider. Note that supporting max and min would also apply to other layouts (fixed, fixed-height) etc.. and at the end of the day both approaches are simply a convenience API since one can already use CSS rules to achieve them.

[SchnWalter]

Oh... yeah... thanks for the explanation! I was only thinking about the responsive layout. The question is: where does one draw the line? There are many things that people would like, but you can't really accommodate all the feature requests without removing "Accelerated" from the project name.

Here's an issue that I encountered, which made me believe that a new "shrink-only" (or whatever name it would have) layout would be a good companion for the existing layouts. And I believe people would expect something like this to exist in AMP:

I've noticed issues with very small images (or narrow images) when I've started preparing a Drupal website for AMP; one that has a lot of images of various shapes and sizes. I've eventually fixed the issues by processing the markup and adding a style of max-width:XX attribute to all AMP image tags on render (besides the layout="reponsive" tag).

It is reasonable to expect people to want to include both images that are large (for example a normal 4:3 photo) and small images that are just 240 pixels wide (maybe a logo) and expect the logo to stay small and not grow to be the same size as the large photos. And yes, you can control everything by manually adding or removing layout="responsive", but I think that there are many cases where you might want to have a layout="shrink-only" and not worry about stretching small or narrow images.

Have a great day,
Walter S.

[SchnWalter]

Looking back on this, I think that @aghassemi is right, min and max attributes aren't really that confusing, but we need to be clear in the documentation to what layouts they apply.

From what I can tell a table like this one would help:

| layout | min-height/max-height | min-width/max-width |
|--------|----------------|---------------|
| container | :x: | :x: |
| fill | :x: * | :x: * |
| fixed | :x: | :x: |
| fixed-height | :x: | :white_check_mark: |
| flex-item | :x: * | :x: * |
| nodisplay | :x: | :x: |
| responsive | :white_check_mark: | :white_check_mark: |

* maybe, the fill layout could also accept all min/max attributes.
** also, the flex-item layout could use individual, inline min/max limits, but I'm not so sure about these.

[SchnWalter]

This can now be closed. We have layout="intrinsic"

[aghassemi]

We recently added object-fit to amp-img so there is precedence for this.

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