Skip to content

Commit

Permalink
✨auto-lightbox carousels under experiment (ampproject#20910)
Browse files Browse the repository at this point in the history
* Introduces criteria to accept any carousel that:
    1. Has one image on every slide
    2. Has no valid `on=tap` actions.
* Since `amp-img` has to be measured against the slide element, some APIs are now async.
* Introduces a `mixed` manual test for several cases.
  • Loading branch information
alanorozco authored and bramanudom committed Mar 22, 2019
1 parent 8d8252f commit 81b870f
Show file tree
Hide file tree
Showing 9 changed files with 608 additions and 78 deletions.
119 changes: 91 additions & 28 deletions extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js
Expand Up @@ -24,14 +24,17 @@

import {AmpEvents} from '../../../src/amp-events';
import {AutoLightboxEvents} from '../../../src/auto-lightbox';
import {CarouselCriteria} from './carousel-criteria';
import {CommonSignals} from '../../../src/common-signals';
import {Services} from '../../../src/services';
import {
closestAncestorElementBySelector,
matches,
whenUpgradedToCustomElement,
} from '../../../src/dom';
import {dev} from '../../../src/log';
import {getMode} from '../../../src/mode';
import {resolveFalse, resolveTrue} from './utils/promise';
import {toArray} from '../../../src/types';
import {tryParseJson} from '../../../src/json';

Expand Down Expand Up @@ -70,21 +73,34 @@ export const RENDER_AREA_RATIO = 1.2;
/** Factor of renderArea vs viewportArea to lightbox. */
export const VIEWPORT_AREA_RATIO = 0.25;

/** @const {!Array<string>} */
const CANDIDATES = ['amp-img', 'amp-carousel'];

/**
* Selector for subnodes for which the auto-lightbox treatment does not apply.
* Selector for subnodes by attribute for which the auto-lightbox treatment
* does not apply. These can be set directly on the candidate or on an ancestor.
*/
const DISABLED_ANCESTORS = [
const DISABLED_BY_ATTR = [
// Runtime-specific.
'[placeholder]',

// Explicitly opted out.
'[data-amp-auto-lightbox-disable]',

// Considered "actionable", i.e. that are bound to a default
// onclick action(e.g. `button`) or where it cannot be determined whether
// they're actionable or not (e.g. `amp-script`).
'amp-selector [option]',
].join(',');

/**
* Selector for subnodes for which the auto-lightbox treatment does not apply.
*/
const DISABLED_ANCESTORS = [
// Ancestors considered "actionable", i.e. that are bound to a default
// onclick action(e.g. `button`) or where it cannot be determined whether
// they're actionable or not (e.g. `amp-script`).
'a[href]',
'amp-selector [option]',
'amp-script',
'amp-story',
'button',
Expand All @@ -93,8 +109,6 @@ const DISABLED_ANCESTORS = [
'amp-lightbox',

// Special treatment.
// TODO(alanorozco): Allow and possibly group carousels where images are the
// only content.
'amp-carousel',
].join(',');

Expand All @@ -117,13 +131,65 @@ export class Criteria {

/**
* @param {!Element} element
* @return {boolean}
* @return {!Promise<boolean>}
*/
static meetsAll(element) {
return Criteria.meetsSizingCriteria(element) &&
Criteria.meetsTreeShapeCriteria(element);
if (!Criteria.meetsSimpleCriteria(element) ||
!Criteria.meetsTreeShapeCriteria(element)) {
return resolveFalse();
}
return Criteria.meetsComplexCriteria(element);
}

/**
* Criteria that is "simple", ie runs quickly and discards elements in order
* to shortcircuit.
* @param {!Element} element
* @return {boolean}
*/
static meetsSimpleCriteria(element) {
if (element.tagName.toUpperCase() == 'AMP-IMG') {
return ImageCriteria.meetsSizingCriteria(element);
}
return true;
}

/**
* Criteria that is "complex", ie takes longer to run and discards elements
* after they're likely to be good candidates per previous conditions.
* @param {!Element} element
* @return {!Promise<boolean>}
*/
static meetsComplexCriteria(element) {
if (element.tagName.toUpperCase() == 'AMP-CAROUSEL') {
return CarouselCriteria.meetsAll(element);
}
return resolveTrue();
}

/**
* @param {!Element} element
* @return {boolean}
*/
static meetsTreeShapeCriteria(element) {
const disabledSelector = `${DISABLED_ANCESTORS},${DISABLED_BY_ATTR}`;
const disabledAncestor =
closestAncestorElementBySelector(element, disabledSelector);
// since we lookup both amp-img and amp-carousel at the same level, and
// we'd like to give amp-carousel special treatment by containing amp-img's,
// we need to filter out images inside carousels, but not carousels
// themselves.
if (disabledAncestor &&
(disabledAncestor != element ||
matches(disabledAncestor, DISABLED_BY_ATTR))) {
return false;
}
const actions = Services.actionServiceForDoc(element);
return !actions.hasResolvableAction(element, 'tap');
}
}

class ImageCriteria {
/**
* @param {!Element} element
* @return {boolean}
Expand All @@ -145,18 +211,6 @@ export class Criteria {
vw,
vh);
}

/**
* @param {!Element} element
* @return {boolean}
*/
static meetsTreeShapeCriteria(element) {
if (closestAncestorElementBySelector(element, DISABLED_ANCESTORS)) {
return false;
}
const actions = Services.actionServiceForDoc(element);
return !actions.hasResolvableAction(element, 'tap');
}
}


Expand Down Expand Up @@ -208,6 +262,15 @@ function markAsVisited(candidate) {
}


/**
* @param {string} tagName
* @return {string}
*/
function candidateSelector(tagName) {
return `${tagName}:not([${LIGHTBOXABLE_ATTR}]):not([${VISITED_ATTR}])`;
}


/**
* @param {!Element} element
* @return {!Promise<!Element>}
Expand All @@ -226,8 +289,7 @@ export class Scanner {
* @return {!Array<!Element>}
*/
static getCandidates(root) {
const selector =
`amp-img:not([${LIGHTBOXABLE_ATTR}]):not([${VISITED_ATTR}])`;
const selector = CANDIDATES.map(candidateSelector).join(',');
const candidates = toArray(root.querySelectorAll(selector));
candidates.forEach(markAsVisited);
return candidates;
Expand Down Expand Up @@ -410,12 +472,13 @@ export function apply(ampdoc, element) {
export function runCandidates(ampdoc, candidates) {
return candidates.map(candidate =>
whenLoaded(candidate).then(() => {
if (!Criteria.meetsAll(candidate)) {
dev().info(TAG, 'discarded', candidate);
return;
}
dev().info(TAG, 'apply', candidate);
return apply(ampdoc, candidate);
return Criteria.meetsAll(candidate).then(meetsAll => {
if (!meetsAll) {
return;
}
dev().info(TAG, 'apply', candidate);
return apply(ampdoc, candidate);
});
}, NOOP));
}

Expand Down
118 changes: 118 additions & 0 deletions extensions/amp-auto-lightbox/0.1/carousel-criteria.js
@@ -0,0 +1,118 @@
/**
* Copyright 2019 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import {devAssert} from '../../../src/log';
import {isActionableByTap} from '../../../src/auto-lightbox';
import {isExperimentOn} from '../../../src/experiments';
import {iterateCursor} from '../../../src/dom';
import {resolveFalse, resolveTrue} from './utils/promise';
import {toWin} from '../../../src/types';
import {tryResolve} from '../../../src/utils/promise';

const MIN_IMG_SLIDE_AREA_RATIO = 0.5;

export class CarouselCriteria {
/**
* @param {!Element} element
* @return {!Promise<boolean>}
*/
static meetsAll(element) {
const win = toWin(element.ownerDocument.defaultView);

if (!isExperimentOn(win, 'amp-auto-lightbox-carousel')) {
return resolveFalse();
}

const slides = element.querySelectorAll('.amp-carousel-slide');
const images = element.querySelectorAll('amp-img');

if (images.length < 1) {
return resolveFalse();
}

if (slides.length != images.length) {
return resolveFalse();
}

let promise = resolveTrue();

iterateCursor(slides, slide => {
promise = promise.then(previousWasAccepted => {
if (!previousWasAccepted) {
return false;
}
return SlideCriteria.meetsAll(slide);
});
});

return promise;
}
}

class SlideCriteria {

/**
* @param {!Element} element
* @return {!Promise<boolean>}
*/
static meetsAll(element) {
if (element.tagName == 'AMP-IMG') {
return tryResolve(() => !isActionableByTap(element));
}

const img = element.querySelector('amp-img');
if (!img) {
return resolveFalse();
}

const slideMeetsSizingPromise =
SlideCriteria.meetsSizingCriteria(img, element);

return slideMeetsSizingPromise.then(slideMeetsSizingCriteria => {
if (!slideMeetsSizingCriteria) {
return false;
}
return !isActionableByTap(element);
});
}

/**
* @param {!AmpElement} img
* @param {!Element} slide
* @return {!Promise<boolean>}
*/
static meetsSizingCriteria(img, slide) {
devAssert(img.tagName == 'AMP-IMG');

return img.getImpl().then(impl => new Promise(resolve => {
impl.measureElement(() => {
const {
width: imgWidth,
height: imgHeight,
} = img.getLayoutBox();

const {
width: slideWidth,
height: slideHeight,
} = slide./*OK*/getBoundingClientRect();

const imgArea = imgWidth * imgHeight;
const slideArea = slideWidth * slideHeight;

resolve((imgArea / slideArea) >= MIN_IMG_SLIDE_AREA_RATIO);
});
}));
}
}
Expand Up @@ -67,7 +67,8 @@ describes.realWin(TAG, {
}

const stubAllCriteriaMet = () => env.sandbox.stub(Criteria, 'meetsAll');
const mockAllCriteriaMet = isMet => stubAllCriteriaMet().returns(isMet);
const mockAllCriteriaMet = isMet =>
stubAllCriteriaMet().returns(tryResolve(() => isMet));

function mockCandidates(candidates) {
env.sandbox.stub(Scanner, 'getCandidates').returns(candidates);
Expand Down Expand Up @@ -434,9 +435,9 @@ describes.realWin(TAG, {

const allCriteriaMet = stubAllCriteriaMet();

allCriteriaMet.withArgs(matchEquals(a)).returns(true);
allCriteriaMet.withArgs(matchEquals(b)).returns(false);
allCriteriaMet.withArgs(matchEquals(c)).returns(true);
allCriteriaMet.withArgs(matchEquals(a)).returns(tryResolve(() => true));
allCriteriaMet.withArgs(matchEquals(b)).returns(tryResolve(() => false));
allCriteriaMet.withArgs(matchEquals(c)).returns(tryResolve(() => true));

mockCandidates([a, b, c]);
mockIsProxyOrigin(true);
Expand Down

0 comments on commit 81b870f

Please sign in to comment.