Fix amp-bind in PWA (#10131)

* fix bind in pwa

* add element service tests

* add bind test

* remove large comment

* lint and type fixes

* fix service tests

* justin's comments

* oops promises

examples/pwa/pwa.html

@@ -104,6 +104,12 @@ <h4>A4A</h4>
           <div class="detail">Fusce pretium tempor justo, vitae consequat dolor maximus eget.</div>
         </a>
       </article>
+      <article class="card">
+        <a href="/pwa/examples/bind/basic.amp.html">
+          <h4>amp-bind</h4>
+          <div class="detail">Fusce pretium tempor justo, vitae consequat dolor maximus eget.</div>
+        </a>
+      </article>
     </section>
 
     <div id="doc-container" class="doc-container">

extensions/amp-bind/0.1/bind-impl.js

@@ -105,7 +105,8 @@ export class Bind {
     /**
      * The window containing the document to scan.
      * May differ from the `ampdoc`'s window e.g. in FIE.
-     * @const @private {!Window} */
+     * @const @private {!Window}
+     */
     this.localWin_ = opt_win || ampdoc.win;
 
     /** @private {!Array<BoundElementDef>} */
@@ -135,17 +136,19 @@ export class Bind {
     /** @const @private {!../../../src/service/viewer-impl.Viewer} */
     this.viewer_ = viewerForDoc(this.ampdoc);
 
-    /**
+    const bodyPromise = (opt_win)
+        ? waitForBodyPromise(opt_win.document)
+            .then(() => dev().assertElement(opt_win.document.body))
+        : ampdoc.whenBodyAvailable();
+
+    /**c.
      * Resolved when the service finishes scanning the document for bindings.
      * @const @private {Promise}
      */
-    this.initializePromise_ = Promise.all([
-      waitForBodyPromise(this.localWin_.document), // Wait for body.
-      this.viewer_.whenFirstVisible(), // Don't initialize in prerender mode.
-    ]).then(() => {
-      const rootNode = dev().assertElement(this.localWin_.document.body);
-      return this.initialize_(rootNode);
-    });
+    this.initializePromise_ =
+        this.viewer_.whenFirstVisible().then(() => bodyPromise).then(body => {
+          return this.initialize_(body);
+        });
 
     /** @const @private {!Function} */
     this.boundOnTemplateRendered_ = this.onTemplateRendered_.bind(this);
@@ -241,7 +244,6 @@ export class Bind {
       rootNode.addEventListener(
           AmpEvents.TEMPLATE_RENDERED, this.boundOnTemplateRendered_);
     });
-    // Check default values against initial expression results in development.
     if (getMode().development) {
       // Check default values against initial expression results.
       promise = promise.then(() =>

extensions/amp-bind/0.1/test/test-bind-impl.js

@@ -19,7 +19,6 @@ import {AmpEvents} from '../../../../src/amp-events';
 import {Bind} from '../bind-impl';
 import {BindEvents} from '../bind-events';
 import {chunkInstanceForTesting} from '../../../../src/chunk';
-import {installTimerService} from '../../../../src/service/timer-impl';
 import {toArray} from '../../../../src/types';
 import {user} from '../../../../src/log';
 
@@ -109,8 +108,41 @@ function waitForEvent_(bind, env) {
 // Unit tests
 ////////////////////////////////////////////////////////////////////////////////
 
-describes.realWin('Bind', {
+describes.realWin('Bind in shadow ampdoc', {
   amp: {
+    ampdoc: 'shadow',
+    runtimeOn: false,
+  },
+}, env => {
+  let bind;
+  let parent;
+
+  let createElementWithBinding;
+  let onBindReady;
+
+  beforeEach(() => {
+    // Make sure we have a chunk instance for testing.
+    chunkInstanceForTesting(env.ampdoc);
+
+    bind = new Bind(env.ampdoc);
+    parent = env.ampdoc.getBody();
+
+    createElementWithBinding = createElementWithBinding_(parent, env);
+    onBindReady = onBindReady_(bind, env);
+  });
+
+  it('should scan for bindings when ampdoc is ready', () => {
+    createElementWithBinding('[text]="1+1"');
+    expect(bind.boundElements_.length).to.equal(0);
+    return onBindReady().then(() => {
+      expect(bind.boundElements_.length).to.equal(1);
+    });
+  });
+});
+
+describes.realWin('Bind in single ampdoc', {
+  amp: {
+    ampdoc: 'single',
     runtimeOn: false,
   },
 }, env => {
@@ -124,8 +156,6 @@ describes.realWin('Bind', {
   let waitForEvent;
 
   beforeEach(() => {
-    installTimerService(env.win);
-
     // Make sure we have a chunk instance for testing.
     chunkInstanceForTesting(env.ampdoc);
 

src/element-service.js

@@ -21,6 +21,7 @@ import {
   getAmpdoc,
   getServicePromiseForDoc,
   getServicePromiseOrNullForDoc,
+  getTopWindow,
 } from './service';
 import {user} from './log';
 import * as dom from './dom';
@@ -61,7 +62,7 @@ export function getElementServiceIfAvailable(win, id, extension, opt_element) {
   if (s) {
     return /** @type {!Promise<?Object>} */ (s);
   }
-  return elementServicePromiseOrNull(win, id, extension, opt_element);
+  return getElementServicePromiseOrNull(win, id, extension, opt_element);
 }
 
 /**
@@ -173,10 +174,17 @@ export function getElementServiceIfAvailableForDocInEmbedScope(
   if (s) {
     return /** @type {!Promise<?Object>} */ (Promise.resolve(s));
   }
+  // Return embed-scope element service promise if scheduled.
   if (nodeOrDoc.nodeType) {
     const win = /** @type {!Document} */ (
         nodeOrDoc.ownerDocument || nodeOrDoc).defaultView;
-    return elementServicePromiseOrNull(win, id, extension);
+    const topWin = getTopWindow(win);
+    // In embeds, doc-scope services are window-scope. But make sure to
+    // only do this for embeds (not the top window), otherwise we'd grab
+    // a promise from the wrong service holder which would never resolve.
+    if (win !== topWin) {
+      return getElementServicePromiseOrNull(win, id, extension);
+    }
   }
   return /** @type {!Promise<?Object>} */ (Promise.resolve(null));
 }
@@ -207,7 +215,7 @@ function assertService(service, id, extension) {
  * @return {!Promise<Object>}
  * @private
  */
-function elementServicePromiseOrNull(win, id, extension, opt_element) {
+function getElementServicePromiseOrNull(win, id, extension, opt_element) {
   // Microtask is necessary to ensure that window.ampExtendedElements has been
   // initialized.
   return Promise.resolve().then(() => {

src/service.js

@@ -14,6 +14,12 @@
  * limitations under the License.
  */
 
+/**
+ * @fileoverview Registration and getter functions for AMP services.
+ *
+ * Invariant: Service getters never return null for registered services.
+ */
+
 // Requires polyfills in immediate side effect.
 import './polyfills';
 import {dev} from './log';
@@ -68,46 +74,37 @@ export class EmbeddableService {
   adoptEmbedWindow(unusedEmbedWin) {}
 }
 
-/**
- * Returns a service or null with the given id.
- * @param {!Window} win
- * @param {string} id
- * @return {?Object} The service.
- */
-export function getExistingServiceOrNull(win, id) {
-  win = getTopWindow(win);
-  if (isServiceRegistered(win, id)) {
-    return getServiceInternal(win, id);
-  } else {
-    return null;
-  }
-}
 
 /**
  * Returns a service with the given id. Assumes that it has been registered
  * already.
  * @param {!Window} win
  * @param {string} id
- * @return {!Object} The service.
+ * @param {boolean=} opt_fallbackToTopWin
+ * @return {Object} The service.
  */
-export function getExistingServiceInEmbedScope(win, id) {
+export function getExistingServiceInEmbedScope(win, id, opt_fallbackToTopWin) {
   // First, try to resolve via local (embed) window.
   const local = getLocalExistingServiceForEmbedWinOrNull(win, id);
   if (local) {
     return local;
   }
-  // Fallback to top-level window.
-  return getService(win, id);
+  if (opt_fallbackToTopWin) {
+    return getService(win, id);
+  }
+  return null;
 }
 
 /**
  * Returns a service with the given id. Assumes that it has been constructed
  * already.
  * @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc
  * @param {string} id
- * @return {!Object} The service.
+ * @param {boolean=} opt_fallbackToTopWin
+ * @return {Object} The service.
  */
-export function getExistingServiceForDocInEmbedScope(nodeOrDoc, id) {
+export function getExistingServiceForDocInEmbedScope(
+    nodeOrDoc, id, opt_fallbackToTopWin) {
   // First, try to resolve via local (embed) window.
   if (nodeOrDoc.nodeType) {
     // If a node is passed, try to resolve via this node.
@@ -118,8 +115,11 @@ export function getExistingServiceForDocInEmbedScope(nodeOrDoc, id) {
       return local;
     }
   }
-  // Fallback to ampdoc.
-  return getServiceForDoc(nodeOrDoc, id);
+  // If an ampdoc is passed or fallback is allowed, continue resolving.
+  if (!nodeOrDoc.nodeType || opt_fallbackToTopWin) {
+    return getServiceForDoc(nodeOrDoc, id);
+  }
+  return null;
 }
 
 /**
@@ -134,13 +134,8 @@ export function installServiceInEmbedScope(embedWin, id, service) {
       'Service override can only be installed in embed window: %s', id);
   dev().assert(!getLocalExistingServiceForEmbedWinOrNull(embedWin, id),
       'Service override has already been installed: %s', id);
-  registerServiceInternal(
-      embedWin,
-      embedWin,
-      id,
-      () => service);
-  // Force service to build
-  getServiceInternal(embedWin, id);
+  registerServiceInternal(embedWin, embedWin, id, () => service);
+  getServiceInternal(embedWin, id); // Force service to build.
 }
 
 /**
@@ -160,21 +155,6 @@ function getLocalExistingServiceForEmbedWinOrNull(embedWin, id) {
   }
 }
 
-/**
- * Returns a service for the given id and window (a per-window singleton).
- * Users should typically wrap this as a special purpose function (e.g.
- * `vsyncFor(win)`) for type safety and because the factory should not be
- * passed around.
- * @param {!Window} win
- * @param {string} id of the service.
- * @template T
- * @return {T}
- */
-export function getService(win, id) {
-  win = getTopWindow(win);
-  return getServiceInternal(win, id);
-}
-
 /**
  * Registers a service given a class to be used as implementation.
  * @param {!Window} win
@@ -193,6 +173,7 @@ export function registerServiceBuilder(win,
   }
 }
 
+
 /**
  * Returns a service and registers it given a class to be used as
  * implementation.
@@ -213,6 +194,23 @@ export function registerServiceBuilderForDoc(nodeOrDoc,
   }
 }
 
+
+/**
+ * Returns a service for the given id and window (a per-window singleton).
+ * Users should typically wrap this as a special purpose function (e.g.
+ * `vsyncFor(win)`) for type safety and because the factory should not be
+ * passed around.
+ * @param {!Window} win
+ * @param {string} id of the service.
+ * @template T
+ * @return {T}
+ */
+export function getService(win, id) {
+  win = getTopWindow(win);
+  return getServiceInternal(win, id);
+}
+
+
 /**
  * Returns a promise for a service for the given id and window. Also expects
  * an element that has the actual implementation. The promise resolves when
@@ -229,6 +227,22 @@ export function getServicePromise(win, id) {
 }
 
 
+/**
+ * Returns a service or null with the given id.
+ * @param {!Window} win
+ * @param {string} id
+ * @return {?Object} The service.
+ */
+export function getExistingServiceOrNull(win, id) {
+  win = getTopWindow(win);
+  if (isServiceRegistered(win, id)) {
+    return getServiceInternal(win, id);
+  } else {
+    return null;
+  }
+}
+
+
 /**
  * Like getServicePromise but returns null if the service was never registered.
  * @param {!Window} win
@@ -239,6 +253,7 @@ export function getServicePromiseOrNull(win, id) {
   return getServicePromiseOrNullInternal(win, id);
 }
 
+
 /**
  * Returns a service for the given id and ampdoc (a per-ampdoc singleton).
  * Expects service `id` to be registered.
@@ -253,6 +268,7 @@ export function getServiceForDoc(nodeOrDoc, id) {
   return getServiceInternal(holder, id);
 }
 
+
 /**
  * Returns a promise for a service for the given id and ampdoc. Also expects
  * a service that has the actual implementation. The promise resolves when
@@ -263,8 +279,7 @@ export function getServiceForDoc(nodeOrDoc, id) {
  */
 export function getServicePromiseForDoc(nodeOrDoc, id) {
   return getServicePromiseInternal(
-      getAmpdocServiceHolder(nodeOrDoc),
-      id);
+      getAmpdocServiceHolder(nodeOrDoc), id);
 }
 
 
@@ -277,8 +292,7 @@ export function getServicePromiseForDoc(nodeOrDoc, id) {
  */
 export function getServicePromiseOrNullForDoc(nodeOrDoc, id) {
   return getServicePromiseOrNullInternal(
-      getAmpdocServiceHolder(nodeOrDoc),
-      id);
+      getAmpdocServiceHolder(nodeOrDoc), id);
 }
 
 /**

src/services.js

@@ -57,7 +57,8 @@ export function accessServiceForDocOrNull(nodeOrDoc) {
  */
 export function actionServiceForDoc(nodeOrDoc) {
   return /** @type {!./service/action-impl.ActionService} */ (
-      getExistingServiceForDocInEmbedScope(nodeOrDoc, 'action'));
+      getExistingServiceForDocInEmbedScope(
+          nodeOrDoc, 'action', /* opt_fallbackToTopWin */ true));
 }
 
 /**
@@ -232,7 +233,8 @@ export function timerFor(window) {
  */
 export function urlReplacementsForDoc(nodeOrDoc) {
   return /** @type {!./service/url-replacements-impl.UrlReplacements} */ (
-      getExistingServiceForDocInEmbedScope(nodeOrDoc, 'url-replace'));
+      getExistingServiceForDocInEmbedScope(
+          nodeOrDoc, 'url-replace', /* opt_fallbackToTopWin */ true));
 }
 
 /**