Put things where they belong

ampproject · Jul 19, 2018 · 7426f1b · 7426f1b
1 parent 5b57fcf
commit 7426f1b
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 86 deletions.
diff --git a/extensions/amp-lightbox/0.1/amp-lightbox.js b/extensions/amp-lightbox/0.1/amp-lightbox.js
@@ -28,12 +28,9 @@ 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 {
-  renderCloseButtonHeader,
-  showCloseButtonHeader,
-} from '../../../src/full-overlay-frame-helper';
 import {toArray} from '../../../src/types';
 
 /** @const {string} */
@@ -76,6 +73,28 @@ const AnimationPresets = {
 /** @private @const {string} */
 const DEFAULT_ANIMATION = 'fade-in';
 
+/**
+ * @param {!Element} ctx
+ * @return {!Element}
+ * @visibleForTesting
+ */
+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 */

diff --git a/src/full-overlay-frame-helper.js b/src/full-overlay-frame-helper.js
@@ -93,25 +93,3 @@ export function collapseFrameUnderVsyncMutate(iframe) {
     'border',
   ]);
 }
-
-
-/**
- * @param {!Element} ctx
- * @return {!Element}
- * @visibleForTesting
- */
-export 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
- */
-export function showCloseButtonHeader(header) {
-  header.classList.add('amp-ad-close-header');
-}
diff --git a/test/functional/test-friendly-iframe-embed.js b/test/functional/test-friendly-iframe-embed.js
@@ -16,13 +16,11 @@
 
 import * as sinon from 'sinon';
 
-import {ActionTrust} from '../../src/action-constants';
 import {
   FriendlyIframeEmbed,
   getFriendlyIframeEmbedOptional,
   installFriendlyIframeEmbed,
   mergeHtmlForTesting,
-  renderClickableCloseButtonHeader,
   setFriendlyIframeEmbedVisible,
   setSrcdocSupportedForTesting,
   whenContentIniLoad,
@@ -771,17 +769,14 @@ describe('friendly-iframe-embed', () => {
     it('resizes body and fixed container when entering', function* () {
       const bodyElementMock = document.createElement('div');
       const fie = createFie(bodyElementMock);
-      const headerHeight = 60;
-
-      sandbox.stub(fie, 'getHeaderHeight_').returns(headerHeight);
 
       yield fie.enterFullOverlayMode();
 
       expect(bodyElementMock.style.background).to.equal('transparent');
       expect(bodyElementMock.style.position).to.equal('absolute');
       expect(bodyElementMock.style.width).to.equal(`${w}px`);
-      expect(bodyElementMock.style.height).to.equal(`${h - headerHeight}px`);
-      expect(bodyElementMock.style.top).to.equal(`${y - headerHeight}px`);
+      expect(bodyElementMock.style.height).to.equal(`${h}px`);
+      expect(bodyElementMock.style.top).to.equal(`${y}px`);
       expect(bodyElementMock.style.left).to.equal(`${x}px`);
       expect(bodyElementMock.style.right).to.equal('auto');
       expect(bodyElementMock.style.bottom).to.equal('auto');
@@ -791,10 +786,10 @@ describe('friendly-iframe-embed', () => {
       expect(iframe.style.position).to.equal('fixed');
       expect(iframe.style.left).to.equal('0px');
       expect(iframe.style.right).to.equal('0px');
-      expect(iframe.style.top).to.equal(`${headerHeight}px`);
+      expect(iframe.style.top).to.equal('0px');
       expect(iframe.style.bottom).to.equal('0px');
       expect(iframe.style.width).to.equal('100vw');
-      expect(iframe.style.height).to.equal(`calc(100vh - ${headerHeight}px)`);
+      expect(iframe.style.height).to.equal('100vh');
     });
 
     it('should reset body and fixed container when leaving', function* () {
@@ -823,55 +818,4 @@ describe('friendly-iframe-embed', () => {
       expect(iframe.style.height).to.be.empty;
     });
   });
-
-  describe('#renderClickableCloseButtonHeader', () => {
-
-    const win = document.defaultView;
-
-    it('renders', () => {
-      const ampAdParentMock = document.createElement('div');
-      const ampLightboxMock = document.createElement('div');
-
-      const el = renderClickableCloseButtonHeader(
-          win, ampAdParentMock, ampLightboxMock);
-
-      expect(el.tagName.toLowerCase()).to.equal('i-amphtml-ad-close-header');
-      expect(el.getAttribute('role')).to.equal('button');
-      expect(el.getAttribute('aria-label')).to.equal('Close Ad');
-      expect(el.getAttribute('tabindex')).to.equal('0');
-
-      expect(el.firstElementChild.textContent).to.equal('Ad');
-
-      const closeButton = el.lastElementChild;
-
-      expect(closeButton.tagName.toLowerCase())
-          .to.equal('i-amphtml-ad-close-button');
-
-      expect(closeButton).to.have.class('amp-ad-close-button');
-    });
-
-    it('triggers action', () => {
-      const ampAdParentMock = document.createElement('div');
-      const ampLightboxMock = document.createElement('div');
-
-      const el = renderClickableCloseButtonHeader(
-          win, ampAdParentMock, ampLightboxMock);
-
-      const execute = sandbox.spy();
-      const actionServiceMock = {execute};
-
-      sandbox.stub(Services, 'actionServiceForDoc').returns(actionServiceMock);
-
-      el.click();
-
-      expect(execute.withArgs(
-          /* target */ ampLightboxMock,
-          /* method */ 'close',
-          /* args   */ sinon.match.any,
-          /* source */ ampAdParentMock,
-          /* caller */ ampAdParentMock,
-          /* event  */ sinon.match.any,
-          /* trust  */ ActionTrust.HIGH)).to.be.calledOnce;
-    });
-  });
 });
diff --git a/test/manual/amp-lightbox-carousel-amp-youtube.html b/test/manual/amp-lightbox-carousel-amp-youtube.html
@@ -0,0 +1,46 @@
+<!doctype html>
+<html amp lang="en">
+<head>
+  <meta charset="utf-8">
+    <script async custom-element="amp-lightbox" src="https://cdn.ampproject.org/v0/amp-lightbox-0.1.js"></script>
+    <script async custom-element="amp-youtube" src="https://cdn.ampproject.org/v0/amp-youtube-0.1.js"></script>
+    <script async custom-element="amp-carousel" src="https://cdn.ampproject.org/v0/amp-carousel-0.1.js"></script>
+
+    <title>AMP Lightbox</title>
+    <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
+
+    <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+    <script async src="https://cdn.ampproject.org/v0.js"></script>
+  <style amp-custom>
+    #lightbox {
+      padding: 100px;
+    }
+
+    .youtube-wrapper {
+      position: relative;
+      width:300px;
+      height: 250px;
+    }
+  </style>
+ </head>
+<body>
+  <div on="tap:lightbox">
+    Open Lightbox
+  </div>
+  <amp-lightbox layout="nodisplay" id="lightbox">
+    <amp-carousel type="slides" width=500 height=300 layout="fixed">
+      <div class="youtube-wrapper">
+        <amp-youtube
+                     data-videoid="IpT1787EjbE"
+                     data-param-controls="1"
+                     data-param-autohide="1"
+                     data-param-wmode="transparent"
+                     layout="fill"></amp-youtube>
+      </div>
+    </amp-carousel>
+      <div on="tap:lightbox.close">
+        Close Lightbox
+      </div>
+  </amp-lightbox>
+</body>
+</html>