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.

Sign up for GitHub

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

✨Add img-sizes attribute to amp-img. #24223

Closed
wants to merge 4 commits into from
Closed

✨Add img-sizes attribute to amp-img. #24223

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e8c3e59
Add `img-sizes` attribute to `amp-img`.
Aug 27, 2019
31ddaf4
Add `img-sizes` documentation.
Aug 27, 2019
b5fcd3d
Review feedback.
Aug 27, 2019
464f260
Fix lint.
Aug 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions builtins/amp-img.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ import {registerElement} from '../src/service/custom-element-registry';
const TAG = 'amp-img';

/**
* Attributes to propagate to internal image when changed externally.
* Attributes to propagate to internal image when changed externally. The
* `sizes` attribute is handled separately to prioritize `img-sizes`.
* @type {!Array<string>}
*/
const ATTRIBUTES_TO_PROPAGATE = [
Expand All @@ -39,7 +40,6 @@ const ATTRIBUTES_TO_PROPAGATE = [
'aria-labelledby',
'srcset',
'src',
'sizes',
];

export class AmpImg extends BaseElement {
Expand Down Expand Up @@ -69,6 +69,30 @@ export class AmpImg extends BaseElement {
this.sizesWidth_ = 0;
}

/**
* @return {?string} The sizes value to use for the img, preferring
* `img-sizes` and falling back to `sizes`.
*/
getSizesValueToUse_() {
return (
this.element.getAttribute('img-sizes') ||
this.element.getAttribute('sizes')
);
}

/**
* If present, propagates the `img-sizes` or `sizes` value to the internal
* img.
*/
propagateSizes() {
const sizes = this.getSizesValueToUse_();
if (sizes === null) {
this.img_.removeAttribute('sizes', sizes);
} else {
this.img_.setAttribute('sizes', sizes);
}
}

/** @override */
mutatedAttributesCallback(mutations) {
if (this.img_) {
Expand Down Expand Up @@ -97,6 +121,9 @@ export class AmpImg extends BaseElement {
this.img_,
/* opt_removeMissingAttrs */ true
);
if (mutations['img-sizes'] || mutations['sizes']) {
sparhami marked this conversation as resolved.
Show resolved Hide resolved
this.propagateSizes();
}
guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_);
}
}
Expand Down Expand Up @@ -176,6 +203,7 @@ export class AmpImg extends BaseElement {
}

this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.img_);
this.propagateSizes();
guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_);
this.maybeGenerateSizes_();
this.applyFillContent(this.img_, true);
Expand All @@ -194,7 +222,7 @@ export class AmpImg extends BaseElement {
return;
}
// No need to generate sizes if already present.
const sizes = this.element.getAttribute('sizes');
const sizes = this.getSizesValueToUse_();
if (sizes) {
return;
}
Expand Down
4 changes: 4 additions & 0 deletions builtins/amp-img.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,10 @@ For the `<img>` tag in `HTML`, the `sizes` attribute is used in conjunction with

See [Responsive images with srcset, sizes & heights](https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/art_direction) for usage of `sizes` and `srcset`.

**img-sizes**

Like the `sizes` attribute, the value is applied to the underlying `<img>` as `sizes`. In contrast, this does not affect the width of the `<amp-img>` element and is only used to select one of the sources from the `srcset`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this does.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded, see updated text

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand why this is necessary. Why wouldn't they just use sizes? How does having <amp-img img-sizes="..."><img sizes="..."></amp-img> have a different behavior than <amp-img sizes="..."><img sizes="..."></amp-img>?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of what sizes does in AMP: https://amp.dev/documentation/guides-and-tutorials/learn/common_attributes/#sizes

Unfortunately, AMP sets the width of an element based on the value that the sizes attribute resolves to. This prevents developers from using sizes, since they want it for picking the source, but do not want to actually set the <amp-img> to that width.

Since this behavior already exists, we cannot remove it (from amp-img or any other element).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, we discussed this in design review some time ago: #11575 (comment). I did not implement img-sizes because we thought that auto-sizes would suffice for most cases (and it did for WP), but from #21736, it seems like we do need a way to explicitly set <img> sizes without accidentally also setting AMP sizes. Sizes always has a side effect in AMP, and we could not remove that side effect due to backwards compatibility issues, hence the unfortunate introduction of a new attribute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a few demos:

Given the img-sizes and manual sizes are incompatible together, I think we need to ban using both at the same time. And if we do that, we could probably use a boolean attribute instead, saying, "behave like regular sizes".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it comes down to which one of these is syntactically less confusing:

so we will either do 2 or 3. Alternative name suggestions:
for (2): sizes-behavior=<native, layout>, use-native-sizes , ??
for (3): native-sizes

(Src: #11575 (comment))

Either will achieve the intended purpose.


**alt**

A string of alternate text, similar to the `alt` attribute on `img`.
Expand Down
94 changes: 94 additions & 0 deletions test/unit/test-amp-img.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,83 @@ describe('amp-img', () => {
});
});

it('should propagate srcset and img-sizes', async () => {
const ampImg = await getImg({
src: '/examples/img/sample.jpg',
srcset: SRCSET_STRING,
'img-sizes': '(max-width: 320px) 640px, 100vw',
width: 320,
height: 240,
});
const img = ampImg.querySelector('img');
expect(img.getAttribute('srcset')).to.equal(SRCSET_STRING);
expect(img.getAttribute('sizes')).to.equal(
'(max-width: 320px) 640px, 100vw'
);
});

it('should prefer img-sizes to sizes', async () => {
const ampImg = await getImg({
src: '/examples/img/sample.jpg',
srcset: SRCSET_STRING,
sizes: '(max-width: 320px) 640px, 100vw',
'img-sizes': '640px',
width: 320,
height: 240,
});
const img = ampImg.querySelector('img');
expect(img.getAttribute('srcset')).to.equal(SRCSET_STRING);
expect(img.getAttribute('sizes')).to.equal('640px');
});

it('should update sizes on mutation', async () => {
const ampImg = await getImg({
src: '/examples/img/sample.jpg',
srcset: SRCSET_STRING,
sizes: '640px',
width: 300,
height: 200,
});
const impl = ampImg.implementation_;

ampImg.setAttribute('sizes', '50vw');
impl.mutatedAttributesCallback({sizes: '50vw'});

expect(impl.img_.getAttribute('sizes')).to.equal('50vw');
});

it('should update sizes on img-sizes mutation', async () => {
const ampImg = await getImg({
src: '/examples/img/sample.jpg',
srcset: SRCSET_STRING,
'img-sizes': '640px',
width: 300,
height: 200,
});
const impl = ampImg.implementation_;

ampImg.setAttribute('img-sizes', '50vw');
impl.mutatedAttributesCallback({'img-sizes': '50vw'});

expect(impl.img_.getAttribute('sizes')).to.equal('50vw');
});

it('should ignore sizes mutations when img-sizes is used', async () => {
const ampImg = await getImg({
src: '/examples/img/sample.jpg',
srcset: SRCSET_STRING,
'img-sizes': '640px',
width: 300,
height: 200,
});
const impl = ampImg.implementation_;

ampImg.setAttribute('sizes', '50vw');
impl.mutatedAttributesCallback({sizes: '50vw'});

expect(impl.img_.getAttribute('sizes')).to.equal('640px');
});

describe('#fallback on initial load', () => {
let el;
let impl;
Expand Down Expand Up @@ -530,6 +607,23 @@ describe('amp-img', () => {
});
});

it('should not generate sizes for amp-imgs that already have img-sizes', async () => {
const ampImg = await getImg({
src: '/examples/img/sample.jpg',
srcset: SRCSET_STRING,
'img-sizes': '50vw',
width: 300,
height: 200,
});

const impl = ampImg.implementation_;
impl.buildCallback();
await impl.layoutCallback();

const img = impl.img_;
expect(img.getAttribute('sizes')).to.equal('50vw');
});

it('should not generate sizes for amp-imgs with layout intrinsic', () => {
let impl;
return getImg({
Expand Down