diff --git a/build-system/tasks/compile.js b/build-system/tasks/compile.js index 4870d5665e993..d3fdff06d7e02 100644 --- a/build-system/tasks/compile.js +++ b/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', diff --git a/examples/bind/forms.amp.html b/examples/bind/forms.amp.html index 704af2d3cbf9f..101cfeb088119 100644 --- a/examples/bind/forms.amp.html +++ b/examples/bind/forms.amp.html @@ -39,7 +39,14 @@

Enter your name and email.

+ +
+
diff --git a/extensions/amp-bind/0.1/bind-impl.js b/extensions/amp-bind/0.1/bind-impl.js index b445c685b981d..377220de86142 100644 --- a/extensions/amp-bind/0.1/bind-impl.js +++ b/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} - * @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
with an - // `items` attribute. - const itemsDiv = childElementByAttr(element, 'items'); - // Should not happen on any page that passes the AMP validator - // as
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 diff --git a/extensions/amp-bind/0.1/test/test-bind-impl.js b/extensions/amp-bind/0.1/test/test-bind-impl.js index 9a0209fc89cc0..66565595dbe3e 100644 --- a/extensions/amp-bind/0.1/test/test-bind-impl.js +++ b/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); diff --git a/extensions/amp-bind/0.1/test/test-bind-integration.js b/extensions/amp-bind/0.1/test/test-bind-integration.js index 6fb3c5b4495ac..4028be85e2897 100644 --- a/extensions/amp-bind/0.1/test/test-bind-integration.js +++ b/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 = '

'; const textElement = div.firstElementChild; - template.parentElement.appendChild(textElement); + // for amp-live-list, dynamic element is
, 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 = ''; 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; }); }); diff --git a/extensions/amp-form/0.1/amp-form.js b/extensions/amp-form/0.1/amp-form.js index 75a9bf8afa50c..2ed65eac6fb2c 100644 --- a/extensions/amp-form/0.1/amp-form.js +++ b/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} + * @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. diff --git a/extensions/amp-live-list/0.1/amp-live-list.js b/extensions/amp-live-list/0.1/amp-live-list.js index 8efdad99b742a..9881761d87b9e 100644 --- a/extensions/amp-live-list/0.1/amp-live-list.js +++ b/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); diff --git a/src/base-element.js b/src/base-element.js index 2c48cc032ced4..e85a073e1c2b9 100644 --- a/src/base-element.js +++ b/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} + * @public + */ + getDynamicElementContainers() { + return []; + } + /** * Called when we just measured the layout rect of this element. Doing * more expensive style reads should now be cheap. diff --git a/src/custom-element.js b/src/custom-element.js index 6dc1714ab04cf..306c4ccb21ab4 100644 --- a/src/custom-element.js +++ b/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} + * @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. diff --git a/src/form.js b/src/form.js new file mode 100644 index 0000000000000..fda0a7066d3de --- /dev/null +++ b/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; +} diff --git a/src/service/resource.js b/src/service/resource.js index 604e949631229..d3a5e240cafc7 100644 --- a/src/service/resource.js +++ b/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_]); } /** diff --git a/test/fixtures/amp-bind-integrations.html b/test/fixtures/amp-bind-integrations.html index 602e0ab8546f9..a12f0b9e01825 100644 --- a/test/fixtures/amp-bind-integrations.html +++ b/test/fixtures/amp-bind-integrations.html @@ -26,12 +26,17 @@ + + +

DYNAMIC TAGS

- -
- - -
+ + +
+ +
+ +

P TAG