✨ Render close button for lightbox ads in inabox (ampproject#16886)

* Render close button for lightbox ads in inabox * Refactor css entry points * Fix transitions * Decrease complexity by using `close-button` attribute * Put things where they belong * Types, lint * Bundle size * Change transition times * Remove unused * Add validation rules for A4A. * 👨🏻 Add self as owner of <amp-lightbox> * Clarification * Address review * Bundle size * Address review * Mock getComputedStyle
devunrulymedia · Aug 2, 2018 · 236a7e1 · 236a7e1
1 parent 85445ab
commit 236a7e1
Show file tree

Hide file tree

Showing 14 changed files with 257 additions and 255 deletions.
diff --git a/ads/inabox/frame-overlay-helper.js b/ads/inabox/frame-overlay-helper.js
@@ -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;
 
 
 /**

diff --git a/css/lightbox-ad.css b/css/lightbox-ad.css
diff --git a/examples/ad-lightbox.amp.html b/examples/ad-lightbox.amp.html
@@ -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">

diff --git a/extensions/amp-ad/0.1/amp-ad.css b/extensions/amp-ad/0.1/amp-ad.css
@@ -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.

diff --git a/extensions/amp-lightbox/0.1/amp-lightbox.css b/extensions/amp-lightbox/0.1/amp-lightbox.css
@@ -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);
 }
diff --git a/extensions/amp-lightbox/0.1/amp-lightbox.js b/extensions/amp-lightbox/0.1/amp-lightbox.js
@@ -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';
@@ -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 */
@@ -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_);
@@ -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(() => {
@@ -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}
@@ -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_());
   }
@@ -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_);
@@ -375,6 +440,14 @@ class AmpLightbox extends AMP.BaseElement {
     this.triggerEvent_(LightboxEvents.CLOSE);
   }
 
+  /**
+   * @return {boolean}
+   * @private
+   */
+  isInAd_() {
+    return getMode(this.win).runtime == 'inabox' || isInFie(this.element);
+  }
+
   /**
    * Handles scroll on the amp-lightbox.
    * The scroll throttling and visibility calculation is similar to

diff --git a/extensions/amp-lightbox/OWNERS.yaml b/extensions/amp-lightbox/OWNERS.yaml
@@ -1,2 +1,3 @@
 - aghassemi
+- alanorozco
 - cathyxz
diff --git a/extensions/amp-lightbox/validator-amp-lightbox.protoascii b/extensions/amp-lightbox/validator-amp-lightbox.protoascii
@@ -58,3 +58,32 @@ tags: {  # <amp-lightbox>
     supported_layouts: NODISPLAY
   }
 }
+tags: {   # <amp-lightbox close-button> in AMP4ADS.
+  html_format: AMP4ADS
+  tag_name: "AMP-LIGHTBOX"
+  spec_name: "amp-lightbox [AMP4ADS]"
+  requires_extension: "amp-lightbox"
+  attrs: {
+    name: "animate-in"
+    value_casei: "fade-in"
+    value_casei: "fly-in-bottom"
+    value_casei: "fly-in-top"
+  }
+  attrs: {
+    name: "close-button"
+    # Require `close-button` since we can't rely on the lightbox content
+    # containing a UI affordance to close it, effectively hijacking the
+    # document otherwise.
+    mandatory: true
+  }
+  attrs: { name: "controls" }
+  attrs: { name: "from" }
+
+  # Never allow `scrollable` as it would break layout with close button and
+  # async iframe interaction.
+
+  attr_lists: "extended-amp-global"
+  amp_layout: {
+    supported_layouts: NODISPLAY
+  }
+}