Remove amp-autocomplete form ancestor requirement (#26705)

* Remove <form> tags from some examples * Remove form ancestor requirement * Update test regarding form ancestor * Use getForm method * Remove form element from unit tests * Rename method to be more generic * Additional rename
ampproject · Feb 11, 2020 · 9ef9811 · 9ef9811
1 parent deed39a
commit 9ef9811
Show file tree

Hide file tree

Showing 8 changed files with 41 additions and 60 deletions.
diff --git a/examples/autocomplete.amp.html b/examples/autocomplete.amp.html
@@ -69,7 +69,6 @@
   <body>
     <h1>Autocomplete</h1>
     <h2>Inline</h2>
-    <form method="post" action-xhr="/form/echo-json/post" target="_blank">
       <amp-autocomplete
         inline="@"
         min-characters="0"
@@ -161,18 +160,10 @@ <h2>Inline</h2>
           }
         </script>
       </amp-autocomplete>
-      <input name="submit-button" type="submit" value="Submit" />
-      <div submit-success>
-        <template type="amp-mustache">
-          Success! {{inlineInput2}}
-        </template>
-      </div>
-    </form>
     <p [text]="'Hello ' + inlineInputSelect">Hello World</p>
 
     <h2>Select Event</h2>
     <p>min-characters = 0, partial red underline</p>
-    <form method="post" action-xhr="/form/echo-json/post" target="_blank">
       <amp-autocomplete
         id="select-example"
         filter="substring"
@@ -183,13 +174,6 @@ <h2>Select Event</h2>
       >
         <input name="input2" required />
       </amp-autocomplete>
-      <input name="submit-button" type="submit" value="Submit" />
-      <div submit-success>
-        <template type="amp-mustache">
-          Success! {{input2}}
-        </template>
-      </div>
-    </form>
     <p [text]="'Hello ' + inputSelect">Hello World</p>
 
     <h2>Disable browser autofill</h2>

diff --git a/extensions/amp-autocomplete/0.1/amp-autocomplete.js b/extensions/amp-autocomplete/0.1/amp-autocomplete.js
@@ -249,16 +249,9 @@ export class AmpAutocomplete extends AMP.BaseElement {
     this.inputElement_.setAttribute('aria-autocomplete', 'both');
     this.inputElement_.setAttribute('role', 'combobox');
 
-    userAssert(
-      this.inputElement_.form,
-      '%s should be inside a <form> tag. %s',
-      TAG,
-      this.element
-    );
-    if (this.inputElement_.form.hasAttribute('autocomplete')) {
-      this.initialAutocompleteAttr_ = this.inputElement_.form.getAttribute(
-        'autocomplete'
-      );
+    const form = this.getFormOrNull_();
+    if (form && form.hasAttribute('autocomplete')) {
+      this.initialAutocompleteAttr_ = form.getAttribute('autocomplete');
     }
 
     // When SSR is supported, it is required.
@@ -333,6 +326,13 @@ export class AmpAutocomplete extends AMP.BaseElement {
     return /** @type {!HTMLInputElement} */ (possibleElements[0]);
   }
 
+  /**
+   * @return {?HTMLFormElement}
+   */
+  getFormOrNull_() {
+    return this.inputElement_.form || null;
+  }
+
   /**
    * Creates a binding associated with singular autocomplete or
    * inline autocomplete depending on the presence of the given element's "inline" attribute.
@@ -903,16 +903,16 @@ export class AmpAutocomplete extends AMP.BaseElement {
    * @private
    */
   toggleResultsHandler_(display) {
-    // Set/reset "autocomplete" attribute on the <form> ancestor.
-    if (display) {
-      this.inputElement_.form.setAttribute('autocomplete', 'off');
-    } else if (this.initialAutocompleteAttr_) {
-      this.inputElement_.form.setAttribute(
-        'autocomplete',
-        this.initialAutocompleteAttr_
-      );
-    } else {
-      this.inputElement_.form.removeAttribute('autocomplete');
+    // Set/reset "autocomplete" attribute on <form> ancestor if present.
+    const form = this.getFormOrNull_();
+    if (form) {
+      if (display) {
+        form.setAttribute('autocomplete', 'off');
+      } else if (this.initialAutocompleteAttr_) {
+        form.setAttribute('autocomplete', this.initialAutocompleteAttr_);
+      } else {
+        form.removeAttribute('autocomplete');
+      }
     }
 
     // Toggle results.
@@ -1203,10 +1203,10 @@ export class AmpAutocomplete extends AMP.BaseElement {
         }
         return this.updateActiveItem_(-1);
       case Keys.ENTER:
-        const shouldPreventSubmit = this.binding_.shouldPreventFormSubmissionOnEnter(
+        const shouldPreventDefault = this.binding_.shouldPreventDefaultOnEnter(
           !!this.activeElement_
         );
-        if (this.areResultsDisplayed_() && shouldPreventSubmit) {
+        if (this.areResultsDisplayed_() && shouldPreventDefault) {
           event.preventDefault();
         }
         this.binding_.removeSelectionHighlighting(this.inputElement_);

diff --git a/extensions/amp-autocomplete/0.1/autocomplete-binding-def.js b/extensions/amp-autocomplete/0.1/autocomplete-binding-def.js
@@ -87,5 +87,5 @@ export class AutocompleteBindingDef {
    * @param {boolean} unusedActiveElement
    * @return {boolean}
    */
-  shouldPreventFormSubmissionOnEnter(unusedActiveElement) {}
+  shouldPreventDefaultOnEnter(unusedActiveElement) {}
 }
diff --git a/extensions/amp-autocomplete/0.1/autocomplete-binding-inline.js b/extensions/amp-autocomplete/0.1/autocomplete-binding-inline.js
@@ -190,7 +190,7 @@ export class AutocompleteBindingInline {
    * @param {boolean} activeElement
    * @return {boolean}
    */
-  shouldPreventFormSubmissionOnEnter(activeElement) {
+  shouldPreventDefaultOnEnter(activeElement) {
     return activeElement;
   }
 }
diff --git a/extensions/amp-autocomplete/0.1/autocomplete-binding-single.js b/extensions/amp-autocomplete/0.1/autocomplete-binding-single.js
@@ -141,7 +141,7 @@ export class AutocompleteBindingSingle {
    * @param {boolean} unusedActiveElement
    * @return {boolean}
    */
-  shouldPreventFormSubmissionOnEnter(unusedActiveElement) {
+  shouldPreventDefaultOnEnter(unusedActiveElement) {
     return !this.submitOnEnter_;
   }
 }
diff --git a/extensions/amp-autocomplete/0.1/test/test-amp-autocomplete-bindings.js b/extensions/amp-autocomplete/0.1/test/test-amp-autocomplete-bindings.js
@@ -117,14 +117,14 @@ describes.realWin(
       });
 
       it('should prevent submission when "submit-on-enter" is absent', () => {
-        expect(binding.shouldPreventFormSubmissionOnEnter(true)).to.be.true;
-        expect(binding.shouldPreventFormSubmissionOnEnter(false)).to.be.true;
+        expect(binding.shouldPreventDefaultOnEnter(true)).to.be.true;
+        expect(binding.shouldPreventDefaultOnEnter(false)).to.be.true;
       });
 
       it('should not prevent submission when "submit-on-enter" is true', () => {
         binding = getBindingSingle({'submit-on-enter': 'true'});
-        expect(binding.shouldPreventFormSubmissionOnEnter(true)).to.be.false;
-        expect(binding.shouldPreventFormSubmissionOnEnter(false)).to.be.false;
+        expect(binding.shouldPreventDefaultOnEnter(true)).to.be.false;
+        expect(binding.shouldPreventDefaultOnEnter(false)).to.be.false;
       });
     });
 
@@ -209,8 +209,8 @@ describes.realWin(
       });
 
       it('should prevent default whenever there are active suggestions shown', () => {
-        expect(binding.shouldPreventFormSubmissionOnEnter(true)).to.be.true;
-        expect(binding.shouldPreventFormSubmissionOnEnter(false)).to.be.false;
+        expect(binding.shouldPreventDefaultOnEnter(true)).to.be.true;
+        expect(binding.shouldPreventDefaultOnEnter(false)).to.be.false;
       });
     });
   }

diff --git a/extensions/amp-autocomplete/0.1/test/test-amp-autocomplete-init.js b/extensions/amp-autocomplete/0.1/test/test-amp-autocomplete-init.js
@@ -348,14 +348,10 @@ describes.realWin(
       });
     });
 
-    it('should error without the form ancestor', () => {
-      return allowConsoleError(() => {
-        const autocomplete = setupAutocomplete({'filter': 'substring'});
-        doc.body.appendChild(autocomplete);
-        return expect(autocomplete.build()).to.be.rejectedWith(
-          'amp-autocomplete should be inside a <form> tag'
-        );
-      });
+    it('should not require a form ancestor', () => {
+      const autocomplete = setupAutocomplete({'filter': 'substring'});
+      doc.body.appendChild(autocomplete);
+      return expect(autocomplete.build()).to.be.fulfilled;
     });
 
     it('should read the autocomplete attribute on the form as null', () => {

diff --git a/extensions/amp-autocomplete/0.1/test/test-amp-autocomplete.js b/extensions/amp-autocomplete/0.1/test/test-amp-autocomplete.js
@@ -39,7 +39,6 @@ describes.realWin(
     });
 
     function buildAmpAutocomplete(wantSsr) {
-      const form = doc.createElement('form');
       const element = createElementWithAttributes(doc, 'amp-autocomplete', {
         layout: 'container',
         filter: 'substring',
@@ -54,8 +53,7 @@ describes.realWin(
       script.innerHTML = '{ "items" : ["apple", "banana", "orange"] }';
       element.appendChild(script);
 
-      form.appendChild(element);
-      doc.body.appendChild(form);
+      doc.body.appendChild(element);
 
       if (wantSsr) {
         element.removeAttribute('filter');
@@ -678,7 +676,7 @@ describes.realWin(
             impl.activeElement_ = impl.createElementFromItem_('abc');
             env.sandbox.stub(impl, 'areResultsDisplayed_').returns(true);
             env.sandbox
-              .stub(impl.binding_, 'shouldPreventFormSubmissionOnEnter')
+              .stub(impl.binding_, 'shouldPreventDefaultOnEnter')
               .returns(true);
             return impl.keyDownHandler_(event);
           })
@@ -777,14 +775,17 @@ describes.realWin(
     it('should call toggleResultsHandler_()', () => {
       const toggleResultsSpy = env.sandbox.spy(impl, 'toggleResults_');
       const resetSpy = env.sandbox.spy(impl, 'resetActiveElement_');
+      const form = doc.createElement('form');
+      form.appendChild(impl.element);
+      doc.body.appendChild(form);
       return impl
         .layoutCallback()
         .then(() => {
           return impl.toggleResultsHandler_(true);
         })
         .then(() => {
           expect(toggleResultsSpy).to.have.been.calledOnce;
-          expect(impl.inputElement_.form.getAttribute('autocomplete')).to.equal(
+          expect(impl.getFormOrNull_().getAttribute('autocomplete')).to.equal(
             'off'
           );
           expect(resetSpy).not.to.have.been.called;