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.

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

✨ Render close button for lightbox ads in inabox #16886

Merged
merged 24 commits into from
Jul 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
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
4 changes: 2 additions & 2 deletions ads/inabox/frame-overlay-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ import {
import {restrictedVsync, timer} from './util';


const CENTER_TRANSITION_TIME_MS = 500;
const CENTER_TRANSITION_END_WAIT_TIME_MS = 200;
const CENTER_TRANSITION_TIME_MS = 150;
const CENTER_TRANSITION_END_WAIT_TIME_MS = 50;


/**
Expand Down
2 changes: 1 addition & 1 deletion build-system/tasks/bundle-size.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const log = require('fancy-log');
const {getStdout} = require('../exec');

const runtimeFile = './dist/v0.js';
const maxSize = '79.24KB';
const maxSize = '79.25KB';

const {green, red, cyan, yellow} = colors;

Expand Down
65 changes: 0 additions & 65 deletions css/lightbox-ad.css

This file was deleted.

4 changes: 3 additions & 1 deletion examples/ad-lightbox.amp.html
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@

<amp-lightbox id="main-lightbox"
class="main-lightbox"
layout="nodisplay">
layout="nodisplay"
animate-in="fly-in-bottom"
close-button>
<amp-carousel type="slides"
height="300"
layout="fixed-height">
Expand Down
2 changes: 0 additions & 2 deletions extensions/amp-ad/0.1/amp-ad.css
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
* limitations under the License.
*/

@import '../../../css/lightbox-ad.css';

/**
* Force the layout box of the ad iframe to be exactly as big as the actual
* iframe. The `amp-ad` tag itself can be freely styled.
Expand Down
76 changes: 67 additions & 9 deletions extensions/amp-lightbox/0.1/amp-lightbox.css
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,74 @@
* limitations under the License.
*/
amp-lightbox {
display: none;
position: fixed !important;
z-index: 1000;
top: 0 !important;
left: 0 !important;
bottom: 0 !important;
right: 0 !important;
display: none;
position: fixed !important;
z-index: 1000;
top: 0 !important;
left: 0 !important;
bottom: 0 !important;
right: 0 !important;
}

amp-lightbox[scrollable] {
overflow-y: auto !important;
overflow-x: hidden !important;
overflow-y: auto !important;
overflow-x: hidden !important;
}

/* Ad lightbox styles */

i-amphtml-ad-close-header {
height: 60px !important;
display: block !important;
visibility: visible !important;
opacity: 0;
position: fixed !important;
top: 0 !important;
left: 0 !important;
right: 0 !important;
z-index: 1000 !important; /* same as amp-lightbox */
display: flex !important;
align-items: center !important;
justify-content: right !important;

/* same as <amp-lightbox [animate-in="fade-in"]> */
transition: 0.1s opacity ease-in;
}

[animate-in="fly-in-bottom"] > i-amphtml-ad-close-header,
[animate-in="fly-in-top"] > i-amphtml-ad-close-header {
/* show after transition has finished */
transition-delay: 0.2s;
}

.amp-ad-close-header {
opacity: 1 !important;
box-sizing: border-box;
padding: 5px;
line-height: 40px; /* 60px - 5px * 2 */
background-color: rgba(0, 0, 0, 1);
color: white;
font-family: Helvetica, sans-serif;
font-size: 12px;
cursor: pointer;
}

.amp-ad-close-header > *:first-child {
margin-left: auto !important; /* aligns right */
pointer-events: none !important; /* pass-thru */
}

.amp-ad-close-button {
display: block !important;
background: url("data:image/svg+xml;charset=utf-8,%3Csvg xmlns='http://www.w3.org/2000/svg' width='24px' height='24px' viewBox='0 0 24 24' fill='%23ffffff'%3E%3Cpath d='M19 6.41L17.59 5 12 10.59 6.41 5 5 6.41 10.59 12 5 17.59 6.41 19 12 13.41 17.59 19 19 17.59 13.41 12z'/%3E%3Cpath d='M0 0h24v24H0z' fill='none'/%3E%3C/svg%3E") no-repeat;
background-position: center center;
width: 40px;
height: 40px;
pointer-events: none !important; /* pass-thru */
border-radius: 40px;
margin-left: 5px;
}

.amp-ad-close-header:active>.amp-ad-close-button {
background-color: rgba(255, 255, 255, 0.3);
}
95 changes: 84 additions & 11 deletions extensions/amp-lightbox/0.1/amp-lightbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,16 @@ import {Gestures} from '../../../src/gesture';
import {KeyCodes} from '../../../src/utils/key-codes';
import {Services} from '../../../src/services';
import {SwipeXYRecognizer} from '../../../src/gesture-recognizers';
import {computedStyle, setImportantStyles} from '../../../src/style';
import {createCustomEvent} from '../../../src/event-helper';
import {computedStyle, px, setImportantStyles} from '../../../src/style';
import {createCustomEvent, listenOnce} from '../../../src/event-helper';
import {debounce} from '../../../src/utils/rate-limit';
import {dev, user} from '../../../src/log';
import {dict, hasOwn} from '../../../src/utils/object';
import {getMode} from '../../../src/mode';
import {htmlFor} from '../../../src/static-template';
import {isInFie} from '../../../src/friendly-iframe-embed';
import {removeElement, tryFocus} from '../../../src/dom';
import {toArray} from '../../../src/types';
import {tryFocus} from '../../../src/dom';

/** @const {string} */
const TAG = 'amp-lightbox';
Expand Down Expand Up @@ -60,18 +61,38 @@ const AnimationPresets = {
'fly-in-bottom': {
openStyle: dict({'transform': 'translate(0, 0)'}),
closedStyle: dict({'transform': 'translate(0, 100%)'}),
durationSeconds: 0.3,
durationSeconds: 0.2,
},
'fly-in-top': {
openStyle: dict({'transform': 'translate(0, 0)'}),
closedStyle: dict({'transform': 'translate(0, -100%)'}),
durationSeconds: 0.3,
durationSeconds: 0.2,
},
};

/** @private @const {string} */
const DEFAULT_ANIMATION = 'fade-in';

/**
* @param {!Element} ctx
* @return {!Element}
*/
function renderCloseButtonHeader(ctx) {
return htmlFor(ctx)`
<i-amphtml-ad-close-header role=button tabindex=0 aria-label="Close Ad">
<div>Ad</div>
<i-amphtml-ad-close-button class="amp-ad-close-button">
</i-amphtml-ad-close-button>
</i-amphtml-ad-close-header>`;
}

/**
* @param {!Element} header
*/
function showCloseButtonHeader(header) {
header.classList.add('amp-ad-close-header');
}

class AmpLightbox extends AMP.BaseElement {

/** @param {!AmpElement} element */
Expand Down Expand Up @@ -118,6 +139,9 @@ class AmpLightbox extends AMP.BaseElement {
this.animationPreset_ =
(element.getAttribute('animate-in') || DEFAULT_ANIMATION).toLowerCase();

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

/** @const {function()} */
this.boundReschedule_ = debounce(this.win, () => {
const container = dev().assertElement(this.container_);
Expand Down Expand Up @@ -281,6 +305,7 @@ class AmpLightbox extends AMP.BaseElement {
});

this.handleAutofocus_();
this.maybeRenderCloseButtonHeader_();

// TODO (jridgewell): expose an API accomodating this per PR #14676
this.mutateElement(() => {
Expand Down Expand Up @@ -309,6 +334,37 @@ class AmpLightbox extends AMP.BaseElement {
this.active_ = true;
}

/** @private */
maybeRenderCloseButtonHeader_() {
const {element} = this;

if (element.getAttribute('close-button') == null) {
return;
}

const header = renderCloseButtonHeader(element);

this.closeButtonHeader_ = header;

listenOnce(header, 'click', () => this.close());

element.insertBefore(header, this.container_);

let headerHeight;

this.measureMutateElement(() => {
headerHeight = header./*OK*/getBoundingClientRect().height;
}, () => {
// Done in vsync in order to apply transition.
showCloseButtonHeader(header);

setImportantStyles(dev().assertElement(this.container_), {
'margin-top': px(headerHeight),
'min-height': `calc(100vh - ${px(headerHeight)})`,
});
});
}

/**
* @private
* @return {!AnimationPresetDef}
Expand Down Expand Up @@ -338,6 +394,10 @@ class AmpLightbox extends AMP.BaseElement {
if (this.isScrollable_) {
st.setStyle(this.element, 'webkitOverflowScrolling', '');
}
if (this.closeButtonHeader_) {
removeElement(this.closeButtonHeader_);
this.closeButtonHeader_ = null;
}
this.getViewport().leaveLightboxMode(this.element)
.then(() => this.finalizeClose_());
}
Expand All @@ -347,22 +407,27 @@ class AmpLightbox extends AMP.BaseElement {
*/
finalizeClose_() {
const {element} = this;

st.setStyles(element, this.getAnimationPresetDef_().closedStyle);

const event = ++this.eventCounter_;

const collapseAndReschedule = () => {
// Don't collapse on transitionend if there was a previous event.
// Don't collapse on transitionend if there was a subsequent event.
if (event != this.eventCounter_) {
return;
}
this./*OK*/collapse();
this.boundReschedule_();
};

element.addEventListener('transitionend', collapseAndReschedule);
element.addEventListener('animationend', collapseAndReschedule);
// Disable transition for ads since the frame gets immediately collapsed.
if (this.isInAd_()) {
st.resetStyles(element, ['transition']);
collapseAndReschedule();
} else {
element.addEventListener('transitionend', collapseAndReschedule);
element.addEventListener('animationend', collapseAndReschedule);
}

st.setStyles(element, this.getAnimationPresetDef_().closedStyle);

if (this.historyId_ != -1) {
this.getHistory_().pop(this.historyId_);
Expand All @@ -375,6 +440,14 @@ class AmpLightbox extends AMP.BaseElement {
this.triggerEvent_(LightboxEvents.CLOSE);
}

/**
* @return {boolean}
* @private
*/
isInAd_() {
Copy link
Contributor

Choose a reason for hiding this comment

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

or, can we just check if the element is in an iframe? Is there a way to check if the owner document is the one AMP runtime is on?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted.

return getMode(this.win).runtime == 'inabox' || isInFie(this.element);
}

/**
* Handles scroll on the amp-lightbox.
* The scroll throttling and visibility calculation is similar to
Expand Down
1 change: 1 addition & 0 deletions extensions/amp-lightbox/OWNERS.yaml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
- aghassemi
- alanorozco
- cathyxz
Loading