amp-autocomplete: Revise order of selection events (#26826)

* amp-autocomplete: Revise order of selection events - When selecting an auto-complete item, if .mutateElement() is called before the input element's value is changed, then input and change events are fired in amp-script with the old input element's value. Split up selectItem_() into two functions, one which sets input values early and the other which fires events specific to amp-autocomplete at the end. - When Enter or Tab is used to make a selection, .mutateElement() does not automatically trigger a change event. Do this manually. * Fix types * Fire additional events and update examples * Unset dev mode on amp-script * amp-autocomplete: Revisions from review - Rename event method - More specific unit test - Minor type safety - Match requested variable assignment preferences - Restore original Tab behavior when popup not active * Revisions based on review
ampproject · Mar 16, 2020 · 20dbb82 · 20dbb82
1 parent 8be63bf
commit 20dbb82
Show file tree

Hide file tree

Showing 4 changed files with 223 additions and 29 deletions.
diff --git a/examples/amp-autocomplete-bind.html b/examples/amp-autocomplete-bind.html
@@ -0,0 +1,55 @@
+<!DOCTYPE html>
+<html amp>
+  <head>
+    <meta charset="utf-8">
+    <title>autocomplete and bind testing</title>
+    <link rel="canonical" href="autocomplete-testing.html" >
+    <meta name="viewport" content="width=device-width,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>
+    <script async custom-element="amp-bind" src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>
+    <script async custom-element="amp-form" src="https://cdn.ampproject.org/v0/amp-form-0.1.js"></script>
+    <script async custom-element="amp-autocomplete" src="https://cdn.ampproject.org/v0/amp-autocomplete-0.1.js"></script>
+    <style amp-custom>
+      body {
+        margin: 0;
+        padding: 0.5em;
+        font-family: sans-serif;
+      }
+      p {
+        padding-top: 150px;
+      }
+      input[readonly],
+      output {
+        background-color: #eee;
+        border: none;
+        padding: 2px 6px;
+        display: inline-block;
+        width: 150px;
+        height: 20px;
+        line-height: 1;
+        box-sizing: border-box;
+        font-size: 16px;
+        font-family: monospace;
+        font-weight: 400;
+      }
+      span {
+        color: black;
+      }
+    </style>
+  </head>
+  <body>
+    <h3>amp-autocomplete + amp-bind testing</h3>
+    <amp-autocomplete filter="substring">
+      <div>
+        <span>Search <input type="text" name="search" id="search" on="input-debounced:AMP.setState({search: event.value});change:AMP.setState({search: event.value})"></span>
+      </div>
+      <script type="application/json">
+        {
+          "items": ["apple", "orange", "banana"]
+        }
+      </script>
+    </amp-autocomplete>
+    <p>Value set by amp-bind: <input readonly type="text" value="" [value]="search"></p>
+  </body>
+</html>
diff --git a/examples/amp-autocomplete-testing.html b/examples/amp-autocomplete-testing.html
@@ -0,0 +1,84 @@
+<!DOCTYPE html>
+<html amp>
+  <head>
+    <meta charset="utf-8">
+    <title>autocomplete testing</title>
+    <link rel="canonical" href="autocomplete-testing.html" >
+    <meta name="viewport" content="width=device-width,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>
+    <script async custom-element="amp-bind" src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>
+    <script async custom-element="amp-form" src="https://cdn.ampproject.org/v0/amp-form-0.1.js"></script>
+    <script async custom-element="amp-autocomplete" src="https://cdn.ampproject.org/v0/amp-autocomplete-0.1.js"></script>
+    <script async custom-element="amp-script" src="https://cdn.ampproject.org/v0/amp-script-0.1.js"></script>
+    <meta name="amp-script-src" content="sha384-lkZe5hu5T1z7PF2HQkxeneF_vgDxtjTso8jfxHdHe6obPTTZoYiBhm9tjU9Qsre1"></head>
+    <style amp-custom>
+      body {
+        margin: 0;
+        padding: 0.5em;
+        font-family: sans-serif;
+      }
+      p {
+        padding-top: 150px;
+      }
+      input[readonly],
+      output {
+        background-color: #eee;
+        border: none;
+        padding: 2px 6px;
+        display: inline-block;
+        width: 150px;
+        height: 20px;
+        line-height: 1;
+        box-sizing: border-box;
+        font-size: 16px;
+        font-family: monospace;
+        font-weight: 400;
+      }
+      span {
+        color: black;
+      }
+    </style>
+  </head>
+  <body>
+    <h3>amp-autocomplete + amp-bind + amp-script testing</h3>
+    <amp-autocomplete filter="substring">
+      <amp-script script="search-script" sandbox="allow-forms">
+        <div>
+          <span>Search <input type="text" name="search" id="search" on="input-debounced:AMP.setState({search: event.value});change:AMP.setState({search: event.value})"></span> <span>Value set by amp-script: <output id="asout"></output></span>
+        </div>
+      </amp-script>
+      <script type="application/json">
+        {
+          "items": ["apple", "orange", "banana"]
+        }
+      </script>
+    </amp-autocomplete>
+    <p>Value set by amp-bind: <input readonly type="text" value="" [value]="search"></p>
+
+    <script id="search-script" type="text/plain" target="amp-script">
+      const searchOutput = document.getElementById('asout');
+      const searchInput = document.getElementById('search');
+
+      searchInput.addEventListener('input', function (e) {
+        try {
+          console.log('input event; value: ' + searchInput.value);
+          searchOutput.setAttribute('data-value', searchInput.value);
+          searchOutput.textContent = searchInput.value;
+        } catch (ex) {
+          console.error('Unable to execute "input" event callback\n', ex);
+        }
+      });
+
+      searchInput.addEventListener('change', function (e) {
+        try {
+          console.log('change event; value: ' + searchInput.value);
+          searchOutput.setAttribute('data-value', searchInput.value);
+          searchOutput.textContent = searchInput.value;
+        } catch (ex) {
+          console.error('Unable to execute "change" event callback\n', ex);
+        }
+      });
+    </script>
+  </body>
+</html>
diff --git a/extensions/amp-autocomplete/0.1/amp-autocomplete.js b/extensions/amp-autocomplete/0.1/amp-autocomplete.js
@@ -628,9 +628,10 @@ export class AmpAutocomplete extends AMP.BaseElement {
    * @private
    */
   selectHandler_(event) {
+    const element = dev().assertElement(event.target);
+    const selectedValue = this.setInputValue_(this.getItemElement_(element));
     return this.mutateElement(() => {
-      const element = dev().assertElement(event.target);
-      this.selectItem_(this.getItemElement_(element));
+      this.selectItem_(selectedValue);
     });
   }
 
@@ -1010,40 +1011,85 @@ export class AmpAutocomplete extends AMP.BaseElement {
   /**
    * Writes the selected value into the input field.
    * @param {?Element} element
+   * @return {?string}
    * @private
    */
-  selectItem_(element) {
+  setInputValue_(element) {
     if (element === null || element.hasAttribute('data-disabled')) {
-      return;
+      return null;
     }
-    const selectedValue =
-      element.getAttribute('data-value') || element.textContent;
 
-    this.fireSelectEvent_(selectedValue);
-    this.clearAllItems_();
-    this.toggleResults_(false);
+    const selectedValue =
+      element.getAttribute('data-value') || element.textContent || '';
 
     this.inputElement_.value = this.binding_.getUserInputForUpdateWithSelection(
       selectedValue,
       this.inputElement_,
       this.userInput_
     );
     this.userInput_ = this.binding_.getUserInputForUpdate(this.inputElement_);
+
+    return selectedValue;
   }
 
   /**
-   * Triggers a 'select' event with the given value as the value emitted.
+   * Finish item selection with the selected value.
+   * @param {?string} value
+   * @private
+   */
+  selectItem_(value) {
+    if (value === null) {
+      return;
+    }
+    this.fireSelectAndChangeEvents_(value);
+    this.clearAllItems_();
+    this.toggleResults_(false);
+  }
+
+  /**
+   * Triggers select event on amp-autocomplete element and change events (bind
+   * and native, for amp-script) on child input/textarea with the given value
+   * as the value emitted.
    * @param {string} value
    * @private
    */
-  fireSelectEvent_(value) {
-    const name = 'select';
+  fireSelectAndChangeEvents_(value) {
+    const selectName = 'select';
     const selectEvent = createCustomEvent(
       this.win,
-      `amp-autocomplete.${name}`,
+      `amp-autocomplete.${selectName}`,
       /** @type {!JsonObject} */ ({value})
     );
-    this.action_.trigger(this.element, name, selectEvent, ActionTrust.HIGH);
+    this.action_.trigger(
+      this.element,
+      selectName,
+      selectEvent,
+      ActionTrust.HIGH
+    );
+
+    dev().assertElement(this.inputElement_);
+
+    // Ensure on="change" is triggered for input
+    const changeName = 'change';
+    const changeEvent = createCustomEvent(
+      this.win,
+      `amp-autocomplete.${changeName}`,
+      /** @type {!JsonObject} */ ({value})
+    );
+    this.action_.trigger(
+      this.inputElement_,
+      changeName,
+      changeEvent,
+      ActionTrust.HIGH
+    );
+
+    // Ensure native change listeners are triggered
+    const nativeChangeEvent = createCustomEvent(
+      this.win,
+      'change',
+      /** @type {!JsonObject} */ ({value})
+    );
+    this.inputElement_.dispatchEvent(nativeChangeEvent);
   }
 
   /**
@@ -1209,12 +1255,14 @@ export class AmpAutocomplete extends AMP.BaseElement {
           event.preventDefault();
         }
         this.binding_.removeSelectionHighlighting(this.inputElement_);
-        return this.mutateElement(() => {
-          if (this.areResultsDisplayed_() && !!this.activeElement_) {
-            this.selectItem_(this.activeElement_);
+        if (this.areResultsDisplayed_() && this.activeElement_) {
+          const selectedValue = this.setInputValue_(this.activeElement_);
+          return this.mutateElement(() => {
+            this.selectItem_(selectedValue);
             this.resetActiveElement_();
-            return Promise.resolve();
-          }
+          });
+        }
+        return this.mutateElement(() => {
           this.toggleResults_(false);
         });
       case Keys.ESCAPE:
@@ -1229,7 +1277,10 @@ export class AmpAutocomplete extends AMP.BaseElement {
       case Keys.TAB:
         if (this.areResultsDisplayed_() && this.activeElement_) {
           event.preventDefault();
-          this.selectItem_(this.activeElement_);
+          const selectedValue = this.setInputValue_(this.activeElement_);
+          return this.mutateElement(() => {
+            this.selectItem_(selectedValue);
+          });
         }
         return Promise.resolve();
       case Keys.BACKSPACE:

diff --git a/extensions/amp-autocomplete/0.1/test/test-amp-autocomplete.js b/extensions/amp-autocomplete/0.1/test/test-amp-autocomplete.js
@@ -722,7 +722,7 @@ describes.realWin(
       impl.activeElement_ = doc.createElement('div');
       expect(impl.userInput_).not.to.equal(impl.inputElement_.value);
       env.sandbox.stub(impl, 'areResultsDisplayed_').returns(true);
-      const fireEventSpy = env.sandbox.spy(impl, 'fireSelectEvent_');
+      const fireEventSpy = env.sandbox.spy(impl, 'fireSelectAndChangeEvents_');
       return impl
         .layoutCallback()
         .then(() => {
@@ -812,29 +812,33 @@ describes.realWin(
         })
         .then(() => {
           expect(getItemSpy).to.have.been.calledTwice;
-          expect(selectItemSpy).to.have.been.called;
+          expect(selectItemSpy).to.have.been.calledWith(null);
           expect(impl.inputElement_.value).to.equal('');
           mockEl = impl.createElementFromItem_('abc');
           return impl.selectHandler_({target: mockEl});
         })
         .then(() => {
           expect(getItemSpy).to.have.been.calledWith(mockEl);
-          expect(selectItemSpy).to.have.been.calledWith(mockEl);
+          expect(selectItemSpy).to.have.been.calledWith('abc');
           expect(impl.inputElement_.value).to.equal('abc');
         });
     });
 
-    it('should fire select event from selectItem_', () => {
-      const fireEventSpy = env.sandbox.spy(impl, 'fireSelectEvent_');
+    it('should fire events from selectItem_', () => {
+      const fireEventSpy = env.sandbox.spy(impl, 'fireSelectAndChangeEvents_');
       const triggerSpy = env.sandbox.spy(impl.action_, 'trigger');
-      const mockEl = doc.createElement('div');
+      const dispatchSpy = env.sandbox.spy(impl.inputElement_, 'dispatchEvent');
       return impl.layoutCallback().then(() => {
         impl.toggleResults_(true);
-        mockEl.setAttribute('data-value', 'test');
-        impl.selectItem_(mockEl);
+        impl.selectItem_('test');
         expect(fireEventSpy).to.have.been.calledOnce;
         expect(fireEventSpy).to.have.been.calledWith('test');
-        expect(triggerSpy).to.have.been.calledOnce;
+        expect(triggerSpy).to.have.been.calledWith(impl.element, 'select');
+        expect(triggerSpy).to.have.been.calledWith(
+          impl.inputElement_,
+          'change'
+        );
+        expect(dispatchSpy).to.have.been.calledOnce;
       });
     });