amp-bind: Don't depend on template location to locate dynamic elements (

ampproject#8367)

build-system/tasks/compile.js

@@ -149,6 +149,8 @@ function compile(entryModuleFilenames, outputDir,
       'extensions/amp-analytics/**/*.js',
       // For amp-bind in the web worker (ww.js).
       'extensions/amp-bind/**/*.js',
+      // Needed to access form impl from other extensions
+      'extensions/amp-form/**/*.js',
       'src/*.js',
       'src/!(inabox)*/**/*.js',
       '!third_party/babel/custom-babel-helpers.js',

examples/bind/forms.amp.html

@@ -39,7 +39,14 @@ <h4>Enter your name and email.</h4>
           <template type="amp-mustache">
               Success! Thanks {{name}} for entering your email: {{email}}
 
-              <p>You have selected: <span [text]="selected ? selected : 'No selection'">0</span></p>
+              <p>You have selected: <span [text]="selected ? selected : 'No selection'">No selection</span></p>
+          </template>
+      </div>
+      <div submit-error>
+        <template type="amp-mustache">
+              Error! Failure to register: {{name}} : {{email}}
+
+              <p>You have selected: <span [text]="selected ? selected : 'No selection'">No selection</span></p>
           </template>
       </div>
   </form>

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

@@ -15,17 +15,16 @@
  */
 
 import {BindExpressionResultDef} from './bind-expression';
-import {childElementByAttr} from '../../../src/dom';
 import {BindingDef, BindEvaluator} from './bind-evaluator';
 import {BindValidator} from './bind-validator';
 import {chunk, ChunkPriority} from '../../../src/chunk';
 import {dev, user} from '../../../src/log';
 import {getMode} from '../../../src/mode';
+import {formOrNullForElement} from '../../../src/form';
 import {isArray, toArray} from '../../../src/types';
 import {isExperimentOn} from '../../../src/experiments';
 import {invokeWebWorker} from '../../../src/web-worker/amp-worker';
 import {isFiniteNumber} from '../../../src/types';
-import {map} from '../../../src/utils/object';
 import {reportError} from '../../../src/error';
 import {resourcesForDoc} from '../../../src/resources';
 import {filterSplice} from '../../../src/utils/array';
@@ -40,17 +39,6 @@ const TAG = 'amp-bind';
  */
 const AMP_CSS_RE = /^(i?-)?amp(html)?-/;
 
-/**
- * Tags under which bind should observe mutaitons to detect added/removed
- * bindings.
- * @type {!Object<string, boolean>}
- * @private
- */
-const DYNAMIC_TAGS = map({
-  'TEMPLATE': true,
-  'AMP-LIVE-LIST': true,
-});
-
 /**
  * A bound property, e.g. [property]="expression".
  * `previousResult` is the result of this expression during the last digest.
@@ -392,9 +380,20 @@ export class Bind {
       }
       const element = dev().assertElement(node);
       const tagName = element.tagName;
-      if (DYNAMIC_TAGS[tagName]) {
-        this.observeElementForMutations_(element);
+
+      let dynamicElements = [];
+      if (typeof element.getDynamicElementContainers === 'function') {
+        dynamicElements = element.getDynamicElementContainers();
+      } else if (element.tagName === 'FORM') {
+        // FORM is not an amp element, so it doesn't have the getter directly.
+        const form = formOrNullForElement(element);
+        dev().assert(form, 'could not find form implementation');
+        dynamicElements = form.getDynamicElementContainers();
       }
+      dynamicElements.forEach(elementToObserve => {
+        this.mutationObserver_.observe(elementToObserve, {childList: true});
+      });
+
       let boundProperties = this.scanElement_(element);
       // Stop scanning once |limit| bindings are reached.
       if (bindings.length + boundProperties.length > limit) {
@@ -765,38 +764,6 @@ export class Bind {
     }
   }
 
-  /**
-   * Begin observing mutations to element. Presently, all supported elements
-   * that can add/remove bindings add new elements to their parent, so parent
-   * node should be observed for mutations.
-   * @param {!Element} element
-   * @private
-   */
-  observeElementForMutations_(element) {
-    const tagName = element.tagName;
-    let elementToObserve;
-    if (tagName === 'TEMPLATE') {
-      // Templates add templated elements as siblings of the template tag
-      // so the parent must be observed.
-      // TODO(kmh287): What if parent is the body tag?
-      elementToObserve = element.parentElement;
-    } else if (tagName === 'AMP-LIVE-LIST') {
-      // All elements in AMP-LIVE-LIST are children of a <div> with an
-      // `items` attribute.
-      const itemsDiv = childElementByAttr(element, 'items');
-      // Should not happen on any page that passes the AMP validator
-      // as <div items> is required.
-      elementToObserve = dev().assert(itemsDiv,
-          'Could not find items div in amp-live-list');
-    } else {
-      dev().assert(false,
-           `amp-bind asked to observe unexpected element ${tagName}`);
-    }
-    if (elementToObserve) {
-      this.mutationObserver_.observe(elementToObserve, {childList: true});
-    }
-  }
-
   /**
    * Respond to observed mutations. Adds all bindings for newly added elements
    * removes bindings for removed elements, then immediately applies the current

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

@@ -15,14 +15,15 @@
  */
 
 import * as sinon from 'sinon';
+import {AmpForm} from '../../../amp-form/0.1/amp-form';
 import {Bind} from '../bind-impl';
 import {BindExpression} from '../bind-expression';
 import {BindValidator} from '../bind-validator';
 import {chunkInstanceForTesting} from '../../../../src/chunk';
+import {installTimerService} from '../../../../src/service/timer-impl';
 import {toArray} from '../../../../src/types';
 import {toggleExperiment} from '../../../../src/experiments';
 import {user} from '../../../../src/log';
-import {installTimerService} from '../../../../src/service/timer-impl';
 
 describes.realWin('Bind', {
   amp: {
@@ -168,13 +169,17 @@ describes.realWin('Bind', {
 
   it('should dynamically detect new bindings under dynamic tags', () => {
     const doc = env.win.document;
-    const template = doc.createElement('template');
-    doc.getElementById('parent').appendChild(template);
+    const form = doc.createElement('form');
+    doc.getElementById('parent').appendChild(form);
+    const dynamicTag = doc.createElement('div');
+    dynamicTag.setAttribute('submit-success', null);
+    form.appendChild(dynamicTag);
+    // Wrap form in amp-form implementation so bind can access it
+    new AmpForm(form);
     return onBindReady().then(() => {
       expect(bind.boundElements_.length).to.equal(0);
-      // As a dynamic element, template adds rendered templates as siblings.
-      // Element is added as a sibling to the template
-      createElementWithBinding('[onePlusOne]="1+1"');
+      const elementWithBinding = createElementWithBinding('[onePlusOne]="1+1"');
+      dynamicTag.appendChild(elementWithBinding);
       return waitForEvent('amp:bind:mutated');
     }).then(() => {
       expect(bind.boundElements_.length).to.equal(1);

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

@@ -67,14 +67,20 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {
 
   describe('detecting bindings under dynamic tags', () => {
     it('should NOT bind blacklisted attributes', () => {
-      const template = fixture.doc.getElementById('dynamicTemplate');
+      const dynamicTag = fixture.doc.getElementById('dynamicTag');
       const div = fixture.doc.createElement('div');
       div.innerHTML = '<p [onclick]="javascript:alert(document.cookie)" ' +
                          '[onmouseover]="javascript:alert()" ' +
                          '[style]="background=color:black"></p>';
       const textElement = div.firstElementChild;
-      template.parentElement.appendChild(textElement);
+      // for amp-live-list, dynamic element is <div items>, which is a child
+      // of the list.
+      dynamicTag.firstElementChild.appendChild(textElement);
       return waitForAllMutations().then(() => {
+        // Force bind to apply bindings
+        fixture.doc.getElementById('triggerBindApplicationButton').click();
+        return waitForBindApplication();
+      }).then(() => {
         expect(textElement.getAttribute('onclick')).to.be.null;
         expect(textElement.getAttribute('onmouseover')).to.be.null;
         expect(textElement.getAttribute('style')).to.be.null;
@@ -85,9 +91,13 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {
       const div = fixture.doc.createElement('div');
       div.innerHTML = '<a [href]="javascript:alert(1)"></a>';
       const aElement = div.firstElementChild;
-      const template = fixture.doc.getElementById('dynamicTemplate');
-      template.parentElement.appendChild(aElement);
+      const dynamicTag = fixture.doc.getElementById('dynamicTag');
+      dynamicTag.firstElementChild.appendChild(aElement);
       return waitForAllMutations().then(() => {
+        // Force bind to apply bindings
+        fixture.doc.getElementById('triggerBindApplicationButton').click();
+        return waitForBindApplication();
+      }).then(() => {
         expect(aElement.getAttribute('href')).to.be.null;
       });
     });

extensions/amp-form/0.1/amp-form.js

@@ -18,6 +18,7 @@ import {installFormProxy} from './form-proxy';
 import {triggerAnalyticsEvent} from '../../../src/analytics';
 import {createCustomEvent} from '../../../src/event-helper';
 import {documentInfoForDoc} from '../../../src/document-info';
+import {setFormForElement} from '../../../src/form';
 import {getService} from '../../../src/service';
 import {
   assertAbsoluteHttpOrHttpsUrl,
@@ -102,6 +103,8 @@ export class AmpForm {
       dev().error(TAG, 'form proxy failed to install', e);
     }
 
+    setFormForElement(element, this);
+
     /** @private @const {string} */
     this.id_ = id;
 
@@ -587,6 +590,25 @@ export class AmpForm {
     }
   }
 
+  /**
+   * @return {Array<!Element>}
+   * @public
+   */
+  getDynamicElementContainers() {
+    const dynamicElements = [];
+    const successDiv =
+        this.form_./*OK*/querySelector(`[${FormState_.SUBMIT_SUCCESS}]`);
+    const errorDiv =
+        this.form_./*OK*/querySelector(`[${FormState_.SUBMIT_ERROR}]`);
+    if (successDiv) {
+      dynamicElements.push(successDiv);
+    }
+    if (errorDiv) {
+      dynamicElements.push(errorDiv);
+    }
+    return dynamicElements;
+  }
+
   /**
    * Returns a promise that resolves when xhr submit finishes. the promise
    * will be null if xhr submit has not started.

extensions/amp-live-list/0.1/amp-live-list.js

@@ -840,6 +840,11 @@ export class AmpLiveList extends AMP.BaseElement {
     return this.updateTime_;
   }
 
+  /** @override */
+  getDynamicElementContainers() {
+    return this.itemsSlot_ ? [this.itemsSlot_] : [];
+  }
+
   sendAmpDomUpdateEvent_() {
     const event = this.win.document.createEvent('Event');
     event.initEvent('amp:dom-update', true, true);

src/base-element.js

@@ -895,6 +895,18 @@ export class BaseElement {
     // Subclasses may override.
   }
 
+  /**
+   * Returns an array of elements in this element's subtree that this
+   * element owns that could have children added or removed dynamically.
+   * The array should not contain any ancestors of this element, but could
+   * contain this element itself.
+   * @return {!Array<!Element>}
+   * @public
+   */
+  getDynamicElementContainers() {
+    return [];
+  }
+
   /**
    * Called when we just measured the layout rect of this element. Doing
    * more expensive style reads should now be cheap.

src/custom-element.js

@@ -1353,6 +1353,18 @@ function createBaseCustomElementClass(win) {
       this.implementation_.mutatedAttributesCallback(mutations);
     }
 
+    /**
+     * Returns an array of elements in this element's subtree that this
+     * element owns that could have children added or removed dynamically.
+     * The array should not contain any ancestors of this element, but could
+     * contain this element itself.
+     * @return {Array<!Element>}
+     * @public
+     */
+    getDynamicElementContainers() {
+      return this.implementation_.getDynamicElementContainers();
+    }
+
     /**
      * Enqueues the action with the element. If element has been upgraded and
      * built, the action is dispatched to the implementation right away.

src/form.js

@@ -0,0 +1,34 @@
+/**
+ * Copyright 2017 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.
+ */
+
+ /** @const {string} */
+const FORM_PROP_ = '__AMP_FORM';
+
+/**
+ * @param {!Element} element
+ * @return {../extensions/amp-form/0.1/amp-form.AmpForm}
+ */
+export function formOrNullForElement(element) {
+  return element[FORM_PROP_] || null;
+}
+
+/**
+ * @param {!Element} element
+ * @param {!../extensions/amp-form/0.1/amp-form.AmpForm} form
+ */
+export function setFormForElement(element, form) {
+  element[FORM_PROP_] = form;
+}

src/service/resource.js

@@ -86,10 +86,10 @@ export class Resource {
 
   /**
    * @param {!Element} element
-   * @return {?Resource}
+   * @return {Resource}
    */
   static forElementOptional(element) {
-    return /** @type {!Resource} */ (element[RESOURCE_PROP_]);
+    return /** @type {Resource} */ (element[RESOURCE_PROP_]);
   }
 
   /**

test/fixtures/amp-bind-integrations.html

@@ -26,12 +26,17 @@
 </head>
 
 <body>
+
+  <button on="tap:AMP.setState({})" id="triggerBindApplicationButton"></button>
+
   <p>DYNAMIC TAGS</p>
-  <!-- For generic dynamic tag tests only. Template tag used as an example. -->
-  <div>
-    <template id="dynamicTemplate"></template>
-    <!-- Siblings added in test -->
-  </div>
+  <!-- Live list is used for generic dynamic tag testing  -->
+  <amp-live-list id="dynamicTag" data-poll-interval="15000" data-max-items-per-page="2">
+    <div items>
+      <!-- Children added in test -->
+    </div>
+    <button update></button>
+  </amp-live-list>
 
   <p>P TAG</p>
   <button on="tap:AMP.setState(boundText='hello world')" id="changeTextButton"></button>