Skip to content

Commit

Permalink
Remove transitions for cropped and object-fit contain images from Lig…
Browse files Browse the repository at this point in the history 
…htbox (ampproject#13040)

* Get natural image width

* No transition for images whose aspect ratios have changed

* Address PR comments
  • Loading branch information
@cathyxz
cathyxz authored and protonate committed Mar 15, 2018
1 parent c47e924 commit 6acfbf0
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 25 deletions.
59 changes: 44 additions & 15 deletions extensions/amp-image-viewer/0.1/amp-image-viewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import {CSS} from '../../../build/amp-image-viewer-0.1.css';
import {Animation} from '../../../src/animation';
import {bezierCurve} from '../../../src/curve';
import {elementByTag} from '../../../src/dom';
import {listen} from '../../../src/event-helper';
import {Gestures} from '../../../src/gesture';
import {dev, user} from '../../../src/log';
Expand Down Expand Up @@ -118,7 +119,7 @@ export class AmpImageViewer extends AMP.BaseElement {
this.motion_ = null;

/** @private {?Element} */
this.sourceElement_ = null;
this.sourceAmpImage_ = null;
}

/** @override */
Expand All @@ -130,16 +131,16 @@ export class AmpImageViewer extends AMP.BaseElement {
children.length == 1 && children[0].tagName == 'AMP-IMG',
'amp-image-viewer should have an amp-img as its one and only child'
);
this.sourceElement_ = children[0];
this.setAsOwner(this.sourceElement_);
this.sourceAmpImage_ = children[0];
this.setAsOwner(this.sourceAmpImage_);
}

/** @override */
layoutCallback() {
let elementLayoutPromise = Promise.resolve();
if (this.sourceElement_) {
this.scheduleLayout(this.sourceElement_);
elementLayoutPromise = this.sourceElement_.signals()
if (this.sourceAmpImage_) {
this.scheduleLayout(this.sourceAmpImage_);
elementLayoutPromise = this.sourceAmpImage_.signals()
.whenSignal(CommonSignals.LOAD_END);
}
return elementLayoutPromise
Expand All @@ -151,8 +152,8 @@ export class AmpImageViewer extends AMP.BaseElement {

this.init_();
this.element.appendChild(this.image_);
this.element.removeChild(this.sourceElement_);
this.sourceElement_ = null;
this.element.removeChild(this.sourceAmpImage_);
this.sourceAmpImage_ = null;
}
});
}).then(() => {
Expand Down Expand Up @@ -208,10 +209,10 @@ export class AmpImageViewer extends AMP.BaseElement {

/**
* Returns the boundaries of the image element.
* @return {!Element}
* @return {?Element}
*/
getImage() {
return dev().assertElement(this.image_);
return this.image_;
}

/**
Expand Down Expand Up @@ -275,19 +276,47 @@ export class AmpImageViewer extends AMP.BaseElement {
}
}

/**
* @return {number}
* @private
*/
getSourceWidth_() {
if (this.sourceAmpImage_.hasAttribute('width')) {
return parseInt(this.sourceAmpImage_.getAttribute('width'), 10);
} else {
const img = elementByTag(dev().assertElement(this.sourceAmpImage_),
'img');
return img ? img.naturalWidth : this.sourceAmpImage_./*OK*/offsetWidth;
}
}

/**
* @return {number}
* @private
*/
getSourceHeight_() {
if (this.sourceAmpImage_.hasAttribute('height')) {
return parseInt(this.sourceAmpImage_.getAttribute('height'), 10);
} else {
const img = elementByTag(dev().assertElement(this.sourceAmpImage_),
'img');
return img ? img.naturalHeight : this.sourceAmpImage_./*OK*/offsetHeight;
}
}

/**
* Initializes the image viewer to the target image element such as
* "amp-img". The target image element may or may not yet have the img
* element initialized.
*/
init_() {
this.sourceWidth_ = this.sourceElement_./*OK*/offsetWidth;
this.sourceHeight_ = this.sourceElement_./*OK*/offsetHeight;
this.srcset_ = srcsetFromElement(this.sourceElement_);
this.sourceWidth_ = this.getSourceWidth_();
this.sourceHeight_ = this.getSourceHeight_();
this.srcset_ = srcsetFromElement(dev().assertElement(this.sourceAmpImage_));

ARIA_ATTRIBUTES.forEach(key => {
if (this.sourceElement_.hasAttribute(key)) {
this.image_.setAttribute(key, this.sourceElement_.getAttribute(key));
if (this.sourceAmpImage_.hasAttribute(key)) {
this.image_.setAttribute(key, this.sourceAmpImage_.getAttribute(key));
}
});
st.setStyles(dev().assertElement(this.image_), {
Expand Down
43 changes: 33 additions & 10 deletions extensions/amp-lightbox-viewer/0.1/amp-lightbox-viewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {bezierCurve} from '../../../src/curve';
import {CSS} from '../../../build/amp-lightbox-viewer-0.1.css';
import {Gestures} from '../../../src/gesture';
import {KeyCodes} from '../../../src/utils/key-codes';
import {clamp} from '../../../src/utils/math';
import {Services} from '../../../src/services';
import {isExperimentOn} from '../../../src/experiments';
import {isLoaded} from '../../../src/event-helper';
Expand Down Expand Up @@ -59,6 +60,7 @@ const MAX_TRANSITION_DURATION = 1000; // ms
const MIN_TRANSITION_DURATION = 300; // ms
const MAX_DISTANCE_APPROXIMATION = 250; // px
const MOTION_DURATION_RATIO = 0.8; // fraction of animation
const EPSILON = 0.001; // precision for approx equals

/**
* TODO(aghassemi): Make lightbox-manager into a doc-level service.
Expand Down Expand Up @@ -667,6 +669,20 @@ export class AmpLightboxViewer extends AMP.BaseElement {
this.updateDescriptionBox_();
}

/**
* @param {!Element} ampImage
* @return {boolean}
* @private
*/
aspectRatioChanged_(ampImage) {
const img = elementByTag(dev().assertElement(ampImage), 'img');
const naturalAspectRatio = img.naturalWidth / img.naturalHeight;
const elementHeight = ampImage./*OK*/offsetHeight;
const elementWidth = ampImage./*OK*/offsetWidth;
const ampImageAspectRatio = elementWidth / elementHeight;
return Math.abs(naturalAspectRatio - ampImageAspectRatio) > EPSILON;
}

/**
* Entry animation to transition in a lightboxable image
* @return {!Promise}
Expand All @@ -675,7 +691,7 @@ export class AmpLightboxViewer extends AMP.BaseElement {
// TODO (cathyxz): make this generalizable to more than just images
enter_() {
const anim = new Animation(this.element);
let duration = MAX_TRANSITION_DURATION;
let duration = MIN_TRANSITION_DURATION;
let transLayer = null;
return this.vsync_.measurePromise(() => {
// Lightbox background fades in.
Expand All @@ -684,7 +700,10 @@ export class AmpLightboxViewer extends AMP.BaseElement {
}), MOTION_DURATION_RATIO, ENTER_CURVE_);

// Try to transition from the source image.
if (this.sourceElement_ && isLoaded(this.sourceElement_)) {
if (this.sourceElement_ && isLoaded(this.sourceElement_)
&& !this.aspectRatioChanged_(this.sourceElement_)) {

// TODO (#13039): implement crop and object fit contain transitions
transLayer = this.element.ownerDocument.createElement('div');
transLayer.classList.add('i-amphtml-lightbox-viewer-trans');
this.element.ownerDocument.body.appendChild(transLayer);
Expand All @@ -695,6 +714,7 @@ export class AmpLightboxViewer extends AMP.BaseElement {
.implementation_.getImageBoxWithOffset();

const clone = this.sourceElement_.cloneNode(true);

clone.className = '';
st.setStyles(clone, {
position: 'absolute',
Expand All @@ -719,7 +739,9 @@ export class AmpLightboxViewer extends AMP.BaseElement {
anim.add(MOTION_DURATION_RATIO - 0.01,
tr.setStyles(dev().assertElement(this.carousel_), {
opacity: tr.numeric(0, 1),
}), 0.01);
}),
0.01
);

anim.add(0, tr.setStyles(clone, {
transform: tr.concat([
Expand Down Expand Up @@ -753,17 +775,18 @@ export class AmpLightboxViewer extends AMP.BaseElement {
* @private
*/
exit_() {
// TODO (cathyxz): settle on a real animation
const anim = new Animation(this.element);
let duration = MAX_TRANSITION_DURATION;
let duration = MIN_TRANSITION_DURATION;
const imageBox = /**@type {?}*/ (this.getCurrentElement_().imageViewer)
.implementation_.getImageBoxWithOffset();
const image = /**@type {?}*/ (this.getCurrentElement_().imageViewer)
.implementation_.getImage();
// Try to transition to the source image.
let transLayer = null;
return this.vsync_.measurePromise(() => {
if (this.sourceElement_) {
if (this.sourceElement_ && image
&& !this.aspectRatioChanged_(this.sourceElement_)) {
// TODO (#13013): if current image is not the original image, don't transition
transLayer = this.element.ownerDocument.createElement('div');
transLayer.classList.add('i-amphtml-lightbox-viewer-trans');
this.element.ownerDocument.body.appendChild(transLayer);
Expand Down Expand Up @@ -846,10 +869,10 @@ export class AmpLightboxViewer extends AMP.BaseElement {
getTransitionDuration_(dy) {
const distanceAdjustedDuration =
Math.abs(dy) / MAX_DISTANCE_APPROXIMATION * MAX_TRANSITION_DURATION;
// clamp duration to MIN and MAX duration constants
return Math.max(
Math.min(distanceAdjustedDuration, MAX_TRANSITION_DURATION),
MIN_TRANSITION_DURATION
return clamp(
distanceAdjustedDuration,
MIN_TRANSITION_DURATION,
MAX_TRANSITION_DURATION
);
}
/**
Expand Down

0 comments on commit 6acfbf0

Please sign in to comment.