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

Jump to bottom

Remove transitions for cropped and object-fit contain images from Lightbox #13040

Merged
merged 4 commits into from
Jan 26, 2018
Merged

Remove transitions for cropped and object-fit contain images from Lightbox #13040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a23e131
Get natural image width
cathyxz Jan 25, 2018
8da28b8
No transition for images whose aspect ratios have changed
cathyxz Jan 25, 2018
38bfa18
Merge branch 'master' into bugfix/cropped-images
cathyxz Jan 26, 2018
dc812b6
Address PR comments
cathyxz Jan 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
59 changes: 44 additions & 15 deletions extensions/amp-image-viewer/0.1/amp-image-viewer.js
View file
Open in desktop
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}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it nullable? Looks like it asserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention was to remove the assertElement. Oops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reasoning here is that we're technically not guaranteed that this.image_ will be initialized until we return from layout callback. So there's the option between either returning a promise here and forcing the lightbox exit to wait for this to layout, or just to check in lightbox exit and skip the animation if no image has been laid out yet. Since the entire exit animation is dependent on the image actually loading, it is pointless to wait for this image to layout for just an exit animation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, that makes a lot of sense

*/
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
Open in desktop
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