Skip to content

Commit

Permalink
Remove native srcset experiment and update fallback logic (#16404)
Browse files Browse the repository at this point in the history 
* Remove experiment from amp-img

* Update use of updateImgSrc

* Add a image fallback test and optimize images

* Update tests

* Update tests per srcset experiment removal

Update tests

* Refactor the toggleFallback logic to load and error listeners instead of loadPromise

* Fix unit tests per refactor

* Update documentation for native srcset

* Uncomment manual tests
  • Loading branch information
@cathyxz
cathyxz authored Jul 9, 2018
1 parent 8897f83 commit 7d34e9e
Show file tree
Hide file tree
Showing 9 changed files with 172 additions and 244 deletions.
150 changes: 48 additions & 102 deletions builtins/amp-img.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,17 @@
*/

import {BaseElement} from '../src/base-element';
import {isExperimentOn} from '../src/experiments';
import {dev} from '../src/log';
import {isLayoutSizeDefined} from '../src/layout';
import {listen} from '../src/event-helper';
import {registerElement} from '../src/service/custom-element-registry';
import {srcsetFromElement, srcsetFromSrc} from '../src/srcset';

/**
* Attributes to propagate to internal image when changed externally.
* @type {!Array<string>}
*/
const ATTRIBUTES_TO_PROPAGATE = ['alt', 'title', 'referrerpolicy', 'aria-label',
'aria-describedby', 'aria-labelledby'];

const EXPERIMENTAL_ATTRIBUTES_TO_PROPAGATE = ATTRIBUTES_TO_PROPAGATE
.concat(['srcset', 'src', 'sizes']);
'aria-describedby', 'aria-labelledby','srcset', 'src', 'sizes'];

export class AmpImg extends BaseElement {

Expand All @@ -45,46 +42,21 @@ export class AmpImg extends BaseElement {
/** @private {?Element} */
this.img_ = null;

/** @private {?../src/srcset.Srcset} */
this.srcset_ = null;
/** @private {?UnlistenDef} */
this.unlistenLoad_ = null;

/** @private @const {boolean} */
this.useNativeSrcset_ = isExperimentOn(this.win, 'amp-img-native-srcset');
/** @private {?UnlistenDef} */
this.unlistenError_ = null;
}

/** @override */
mutatedAttributesCallback(mutations) {
let mutated = false;
if (!this.useNativeSrcset_) {
if (mutations['srcset'] !== undefined) {
// `srcset` mutations take precedence over `src` mutations.
this.srcset_ = srcsetFromElement(this.element);
mutated = true;
} else if (mutations['src'] !== undefined) {
// If only `src` is mutated, then ignore the existing `srcset` attribute
// value (may be set automatically as cache optimization).
this.srcset_ = srcsetFromSrc(this.element.getAttribute('src'));
mutated = true;
}
// This element may not have been laid out yet.
if (mutated && this.img_) {
this.updateImageSrc_();
}
}

if (this.img_) {
const propAttrs = this.useNativeSrcset_ ?
EXPERIMENTAL_ATTRIBUTES_TO_PROPAGATE :
ATTRIBUTES_TO_PROPAGATE;
const attrs = propAttrs.filter(
const attrs = ATTRIBUTES_TO_PROPAGATE.filter(
value => mutations[value] !== undefined);
this.propagateAttributes(
attrs, this.img_, /* opt_removeMissingAttrs */ true);

if (this.useNativeSrcset_) {
this.guaranteeSrcForSrcsetUnsupportedBrowsers_();
}

this.guaranteeSrcForSrcsetUnsupportedBrowsers_();
}
}

Expand All @@ -102,7 +74,7 @@ export class AmpImg extends BaseElement {
return;
}
// We try to find the first url in the srcset
const srcseturl = /https?:\/\/\S+/.exec(srcset);
const srcseturl = /\S+/.exec(srcset);
// Connect to the first url if it exists
if (srcseturl) {
this.preconnect.url(srcseturl[0], onLayout);
Expand All @@ -128,9 +100,6 @@ export class AmpImg extends BaseElement {
if (this.img_) {
return;
}
if (!this.useNativeSrcset_ && !this.srcset_) {
this.srcset_ = srcsetFromElement(this.element);
}
// If this amp-img IS the fallback then don't allow it to have its own
// fallback to stop from nested fallback abuse.
this.allowImgLoadFallback_ = !this.element.hasAttribute('fallback');
Expand All @@ -156,17 +125,9 @@ export class AmpImg extends BaseElement {
'be correctly propagated for the underlying <img> element.');
}


if (this.useNativeSrcset_) {
this.propagateAttributes(EXPERIMENTAL_ATTRIBUTES_TO_PROPAGATE,
this.img_);
this.guaranteeSrcForSrcsetUnsupportedBrowsers_();
} else {
this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.img_);
}

this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.img_);
this.guaranteeSrcForSrcsetUnsupportedBrowsers_();
this.applyFillContent(this.img_, true);

this.element.appendChild(this.img_);
}

Expand All @@ -175,11 +136,6 @@ export class AmpImg extends BaseElement {
return this.isPrerenderAllowed_;
}

/** @override */
isRelayoutNeeded() {
return true;
}

/** @override */
reconstructWhenReparented() {
return false;
Expand All @@ -188,18 +144,26 @@ export class AmpImg extends BaseElement {
/** @override */
layoutCallback() {
this.initialize_();
let promise = this.updateImageSrc_();
const img = dev().assertElement(this.img_);
this.unlistenLoad_ = listen(img, 'load', () => this.hideFallbackImg_());
this.unlistenError_ = listen(img, 'error', () => this.onImgLoadingError_());
if (this.getLayoutWidth() <= 0) {
return Promise.resolve();
}
return this.loadPromise(img);
}

// We only allow to fallback on error on the initial layoutCallback
// or else this would be pretty expensive.
if (this.allowImgLoadFallback_) {
promise = promise.catch(e => {
this.onImgLoadingError_();
throw e;
});
this.allowImgLoadFallback_ = false;
/** @override */
unlayoutCallback() {
if (this.unlistenError_) {
this.unlistenError_();
this.unlistenError_ = null;
}
return promise;
if (this.unlistenLoad_) {
this.unlistenLoad_();
this.unlistenLoad_ = null;
}
return true;
}

/**
Expand All @@ -221,51 +185,33 @@ export class AmpImg extends BaseElement {
}

/**
* @return {!Promise}
* @private
*/
updateImageSrc_() {
if (this.getLayoutWidth() <= 0) {
return Promise.resolve();
}

if (!this.useNativeSrcset_) {
const src = this.srcset_.select(
// The width should never be 0, but we fall back to the screen width
// just in case.
this.getViewport().getWidth() || this.win.screen.width,
this.getDpr());
if (src == this.img_.getAttribute('src')) {
return Promise.resolve();
}

this.img_.setAttribute('src', src);
hideFallbackImg_() {
if (!this.allowImgLoadFallback_
&& this.img_.classList.contains('i-amphtml-ghost')) {
this.getVsync().mutate(() => {
this.img_.classList.remove('i-amphtml-ghost');
this.toggleFallback(false);
});
}

return this.loadPromise(this.img_).then(() => {
// Clean up the fallback if the src has changed.
if (!this.allowImgLoadFallback_ &&
this.img_.classList.contains('i-amphtml-ghost')) {
this.getVsync().mutate(() => {
this.img_.classList.remove('i-amphtml-ghost');
this.toggleFallback(false);
});
}
});
}

/**
* If the image fails to load, show a placeholder instead.
* If the image fails to load, show a fallback or placeholder instead.
* @private
*/
onImgLoadingError_() {
this.getVsync().mutate(() => {
this.img_.classList.add('i-amphtml-ghost');
this.toggleFallback(true);
// Hide placeholders, as browsers that don't support webp
// Would show the placeholder underneath a transparent fallback
this.togglePlaceholder(false);
});
if (this.allowImgLoadFallback_) {
this.getVsync().mutate(() => {
this.img_.classList.add('i-amphtml-ghost');
this.toggleFallback(true);
// Hide placeholders, as browsers that don't support webp
// Would show the placeholder underneath a transparent fallback
this.togglePlaceholder(false);
});
this.allowImgLoadFallback_ = false;
}
}
}

Expand Down
12 changes: 6 additions & 6 deletions builtins/amp-img.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ In the following example, we display an image that responds to the size of the v
resizable
src="https://ampproject-b5f4c.firebaseapp.com/examples/ampimg.basic.embed.html">
<div overflow tabindex="0" role="button" aria-label="Show more">Show full code</div>
<div placeholder></div>
<div placeholder></div>
</amp-iframe>
</div>

Expand All @@ -74,7 +74,7 @@ In the following example, if the browser doesn't support WebP, the fallback JPG
resizable
src="https://ampproject-b5f4c.firebaseapp.com/examples/ampimg.fallback.embed.html">
<div overflow tabindex="0" role="button" aria-label="Show more">Show full code</div>
<div placeholder></div>
<div placeholder></div>
</amp-iframe>
</div>

Expand All @@ -97,11 +97,11 @@ This attribute is similar to the `src` attribute on the `img` tag. The value mus

**srcset**

Same as `srcset` attribute on the `img` tag. The behavior will be polyfilled where not natively supported.
Same as `srcset` attribute on the `img` tag. For browsers that do not support `srcset`, `<amp-img>` will default to using `src`. If only `srcset` and no `src` is provided, the first url in the `srcset` will be selected.

**sizes**

Same as `sizes` attribute on the `img` tag.
Same as `sizes` attribute on the `img` tag.

{% call callout('Read on', type='read') %}
See [Responsive images with srcset, sizes & heights](https://www.ampproject.org/docs/design/responsive/art_direction) for usage of `sizes` and `srcset`.
Expand Down Expand Up @@ -173,13 +173,13 @@ For example, instead of specifying `width="900"` and `height="675"`, you can jus
resizable
src="https://ampproject-b5f4c.firebaseapp.com/examples/ampimg.aspectratio.embed.html">
<div overflow tabindex="0" role="button" aria-label="Show more">Show full code</div>
<div placeholder></div>
<div placeholder></div>
</amp-iframe>
</div>

#### Setting multiple source files for different screen resolutions

The [`srcset`](#attributes) attribute should be used to provide different resolutions of the same image, that all have the same aspect ratio. The AMP runtime will automatically choose the most appropriate file from `srcset` based on the screen resolution and width of the user's device.
The [`srcset`](#attributes) attribute should be used to provide different resolutions of the same image, that all have the same aspect ratio. The browser will automatically choose the most appropriate file from `srcset` based on the screen resolution and width of the user's device.

In contrast, the [`media`](https://www.ampproject.org/docs/reference/common_attributes#media) attribute shows or hides AMP components, and should be used when designing responsive layouts. The appropriate way to display images with differing aspect ratios is to use multiple `<amp-img>` components, each with a `media` attribute that matches the screen widths in which to show each instance.

Expand Down
Binary file modified examples/img/hero@1x.jpg
View file
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added examples/img/hero@1x.webp
View file
Binary file not shown.
Binary file modified examples/img/hero@2x.jpg
View file
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added examples/img/hero@2x.webp
View file
Binary file not shown.
Loading

0 comments on commit 7d34e9e

Please sign in to comment.