amp-selector: Support event.selectedOptions in on="select:..." (amppr…

…oject#18641)

* Fix event.selectedOptions from amp-selector.

* Tests.

* Update documentation.

extensions/amp-selector/0.1/amp-selector.js

@@ -324,38 +324,32 @@ export class AmpSelector extends AMP.BaseElement {
     if (el.hasAttribute('disabled')) {
       return;
     }
-
     this.mutateElement(() => {
-      /** @type {?Array<string>} */
-      let selectedValues;
       if (el.hasAttribute('selected')) {
         if (this.isMultiple_) {
           this.clearSelection_(el);
-          selectedValues = this.setInputs_();
+          this.setInputs_();
         }
       } else {
         this.setSelection_(el);
-        selectedValues = this.setInputs_();
-      }
-
-      // Don't trigger action or update focus if
-      // selected values haven't changed.
-      if (selectedValues) {
-        // Newly picked option should always have focus.
-        this.updateFocus_(el);
-
-        // Trigger 'select' event with two data params:
-        // 'targetOption' - option value of the selected or deselected element.
-        // 'selectedOptions' - array of option values of selected elements.
-        const name = 'select';
-        const selectEvent =
-            createCustomEvent(this.win, `amp-selector.${name}`, dict({
-              'targetOption': el.getAttribute('option'),
-              'selectedOptions': selectedValues,
-            }));
-        this.action_.trigger(this.element, name, selectEvent,
-            ActionTrust.HIGH);
+        this.setInputs_();
       }
+      // Newly picked option should always have focus.
+      this.updateFocus_(el);
+
+      // Trigger 'select' event with two data params:
+      // 'targetOption' - option value of the selected or deselected element.
+      // 'selectedOptions' - array of option values of selected elements.
+      const name = 'select';
+      const selections =
+          this.selectedOptions_.map(el => el.getAttribute('option'));
+      const selectEvent =
+          createCustomEvent(this.win, `amp-selector.${name}`, dict({
+            'targetOption': el.getAttribute('option'),
+            'selectedOptions': selections,
+          }));
+      this.action_.trigger(this.element, name, selectEvent,
+          ActionTrust.HIGH);
     });
   }
 

extensions/amp-selector/0.1/test/test-amp-selector.js

@@ -767,7 +767,7 @@ describes.realWin('amp-selector', {
       expect(ampSelector.children[2].hasAttribute('selected')).to.be.false;
     });
 
-    it('should trigger `select` action when user selects an option', () => {
+    it('should trigger "select" event when user selects an option', () => {
       const ampSelector = getSelector({
         config: {
           count: 5,
@@ -777,14 +777,46 @@ describes.realWin('amp-selector', {
       ampSelector.build();
       const impl = ampSelector.implementation_;
       impl.mutateElement = fn => fn();
-      const triggerSpy = sandbox.spy(impl.action_, 'trigger');
 
+      const triggerSpy = sandbox.spy(impl.action_, 'trigger');
       impl.clickHandler_({target: impl.options_[3]});
 
-      const eventMatcher =
-          sandbox.match.has('detail', sandbox.match.has('targetOption', '3'));
-      expect(triggerSpy).to.have.been.calledWith(
-          ampSelector, 'select', /* CustomEvent */ eventMatcher);
+      expect(triggerSpy).to.be.calledOnce;
+      expect(triggerSpy).to.have.been.calledWith(ampSelector, 'select');
+
+      const event = triggerSpy.firstCall.args[2];
+      expect(event).to.have.property('detail');
+      expect(event.detail).to.have.property('targetOption', '3');
+      expect(event.detail).to.have.deep.property('selectedOptions', ['3']);
+    });
+
+    it('should trigger "select" event for multiple selections', function* () {
+      const ampSelector = getSelector({
+        attributes: {
+          multiple: true,
+        },
+        config: {
+          count: 6,
+        },
+      });
+      ampSelector.children[0].setAttribute('selected', '');
+      ampSelector.children[1].setAttribute('selected', '');
+
+      ampSelector.build();
+      const impl = ampSelector.implementation_;
+      impl.mutateElement = fn => fn();
+
+      const triggerSpy = sandbox.spy(impl.action_, 'trigger');
+      impl.clickHandler_({target: impl.options_[2]});
+
+      expect(triggerSpy).to.be.calledOnce;
+      expect(triggerSpy).to.have.been.calledWith(ampSelector, 'select');
+
+      const event = triggerSpy.firstCall.args[2];
+      expect(event).to.have.property('detail');
+      expect(event.detail).to.have.property('targetOption', '2');
+      expect(event.detail).to.have.deep.property('selectedOptions',
+          ['0', '1', '2']);
     });
 
     it('should trigger `select` action when user uses ' +

extensions/amp-selector/amp-selector.md

@@ -134,6 +134,9 @@ Read more about [AMP Actions and Events](../../spec/amp-actions-and-events.md).
 Multi-selectors and single-selectors fire this when selecting or unselecting options.
 Tapping disabled options does not trigger the `select` event.
 
+- `event.targetOption` contains the `option` attribute value of the selected element.
+- `event.selectedOptions` contains an array of the `option` attribute values of all selected elements.
+
 ## Validation
 
 See [amp-selector rules](https://github.com/ampproject/amphtml/blob/master/extensions/amp-selector/validator-amp-selector.protoascii) in the AMP validator specification.

spec/amp-actions-and-events.md

@@ -205,6 +205,24 @@ event.index</pre></td>
   </tr>
 </table>
 
+### amp-selector
+<table>
+  <tr>
+    <th width="25%">Event</th>
+    <th width="35%">Description</th>
+    <th width="40%">Data</th>
+  </tr>
+  <tr>
+    <td><code>select</code></td>
+    <td>Fired when an option is selected or deselected.</td>
+    <td><pre>// Target element's "option" attribute value.
+event.targetOption
+
+// Array of "option" attribute values of all selected elements.
+event.selectedOptions</pre></td>
+  </tr>
+</table>
+
 ### amp-sidebar
 <table>
   <tr>