Bug 1427950 - Make <rich-select>'s .selectedOption setter the only supported way to change selection. r=jaws draft
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Mon, 29 Jan 2018 20:10:59 -0800
changeset 749033 45613b70daa2757f83de5c2fdeaacc596d6ce012
parent 749032 7d3385f47c47c668d19e795499f468204446d692
child 749034 49a69af9a49107ea63c308ce9bd945b4868d7485
push id97299
push usermozilla@noorenberghe.ca
push dateTue, 30 Jan 2018 21:24:25 +0000
reviewersjaws
bugs1427950
milestone60.0a1
Bug 1427950 - Make <rich-select>'s .selectedOption setter the only supported way to change selection. r=jaws It's no longer supported to change the selectedness of options directly since that state causes conflicts with the application's own state. MozReview-Commit-ID: kioot4BXoX
toolkit/components/payments/res/components/rich-option.js
toolkit/components/payments/res/components/rich-select.js
toolkit/components/payments/res/containers/address-picker.js
toolkit/components/payments/res/paymentRequest.xhtml
toolkit/components/payments/test/mochitest/test_rich_select.html
--- a/toolkit/components/payments/res/components/rich-option.js
+++ b/toolkit/components/payments/res/components/rich-option.js
@@ -7,17 +7,22 @@
  *  <rich-option></rich-option>
  * </rich-select>
  */
 
 /* global ObservedPropertiesMixin */
 /* exported RichOption */
 
 class RichOption extends ObservedPropertiesMixin(HTMLElement) {
-  static get observedAttributes() { return ["selected", "hidden"]; }
+  static get observedAttributes() {
+    return [
+      "selected",
+      "value",
+    ];
+  }
 
   connectedCallback() {
     this.render();
     let richSelect = this.closest("rich-select");
     if (richSelect && richSelect.render) {
       richSelect.render();
     }
 
@@ -52,36 +57,15 @@ class RichOption extends ObservedPropert
     if (!this.disabled &&
         event.which == 13 /* Enter */) {
       for (let option of this.parentNode.children) {
         option.selected = option == this;
       }
     }
   }
 
-  get selected() {
-    return this.hasAttribute("selected");
-  }
-
-  set selected(value) {
-    if (value) {
-      let oldSelectedOptions = this.parentNode.querySelectorAll("[selected]");
-      for (let option of oldSelectedOptions) {
-        option.removeAttribute("selected");
-      }
-      this.setAttribute("selected", value);
-    } else {
-      this.removeAttribute("selected");
-    }
-    let richSelect = this.closest("rich-select");
-    if (richSelect && richSelect.render) {
-      richSelect.render();
-    }
-    return value;
-  }
-
   static _createElement(fragment, className) {
     let element = document.createElement("span");
     element.classList.add(className);
     fragment.appendChild(element);
     return element;
   }
 }
--- a/toolkit/components/payments/res/components/rich-select.js
+++ b/toolkit/components/payments/res/components/rich-select.js
@@ -1,20 +1,22 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+/* global ObservedPropertiesMixin */
+
 /**
  * <rich-select>
  *  <rich-option></rich-option>
  * </rich-select>
+ *
+ * Note: The only supported way to change the selected option is via the
+ *       `selectedOption` setter.
  */
-
-/* global ObservedPropertiesMixin */
-
 class RichSelect extends ObservedPropertiesMixin(HTMLElement) {
   static get observedAttributes() {
     return [
       "open",
       "disabled",
       "hidden",
     ];
   }
@@ -43,18 +45,32 @@ class RichSelect extends ObservedPropert
   get popupBox() {
     return this.querySelector(":scope > .rich-select-popup-box");
   }
 
   get selectedOption() {
     return this.popupBox.querySelector(":scope > [selected]");
   }
 
-  namedItem(name) {
-    return this.popupBox.querySelector(`:scope > [name="${CSS.escape(name)}"]`);
+
+  /**
+   * This is the only supported method of changing the selected option. Do not
+   * manipulate the `selected` property or attribute on options directly.
+   * @param {HTMLOptionElement} option
+   */
+  set selectedOption(option) {
+    for (let child of this.popupBox.children) {
+      child.selected = child == option;
+    }
+
+    this.render();
+  }
+
+  getOptionByValue(value) {
+    return this.popupBox.querySelector(`:scope > [value="${CSS.escape(value)}"]`);
   }
 
   handleEvent(event) {
     switch (event.type) {
       case "blur": {
         this.onBlur(event);
         break;
       }
@@ -112,16 +128,19 @@ class RichSelect extends ObservedPropert
 
     let aAttrs = a.constructor.observedAttributes;
     let bAttrs = b.constructor.observedAttributes;
     if (aAttrs.length != bAttrs.length) {
       return false;
     }
 
     for (let aAttr of aAttrs) {
+      if (aAttr == "selected") {
+        continue;
+      }
       if (a.getAttribute(aAttr) != b.getAttribute(aAttr)) {
         return false;
       }
     }
 
     return true;
   }
 
@@ -140,32 +159,34 @@ class RichSelect extends ObservedPropert
     for (let option of options) {
       popupBox.appendChild(option);
     }
 
     let selectedChild;
     for (let child of popupBox.children) {
       if (child.selected) {
         selectedChild = child;
+        break;
       }
     }
     if (!selectedChild && popupBox.children.length) {
       selectedChild = popupBox.children[0];
       selectedChild.selected = true;
     }
 
     let selectedClone = this.querySelector(":scope > .rich-select-selected-clone");
     if (!this._optionsAreEquivalent(selectedClone, selectedChild)) {
       if (selectedClone) {
         selectedClone.remove();
       }
 
       if (selectedChild) {
         selectedClone = selectedChild.cloneNode(false);
         selectedClone.removeAttribute("id");
+        selectedClone.removeAttribute("selected");
         selectedClone.classList.add("rich-select-selected-clone");
         selectedClone = this.appendChild(selectedClone);
       }
     }
   }
 }
 
 customElements.define("rich-select", RichSelect);
--- a/toolkit/components/payments/res/containers/address-picker.js
+++ b/toolkit/components/payments/res/containers/address-picker.js
@@ -22,28 +22,43 @@ class AddressPicker extends PaymentState
     this.appendChild(this.dropdown);
     super.connectedCallback();
   }
 
   render(state) {
     let {savedAddresses} = state;
     let desiredOptions = [];
     for (let [guid, address] of Object.entries(savedAddresses)) {
-      let optionEl = this.dropdown.namedItem(guid);
+      let optionEl = this.dropdown.getOptionByValue(guid);
       if (!optionEl) {
         optionEl = document.createElement("address-option");
-        optionEl.name = guid;
-        optionEl.guid = guid;
+        optionEl.value = guid;
       }
       for (let [key, val] of Object.entries(address)) {
         optionEl.setAttribute(key, val);
       }
       desiredOptions.push(optionEl);
     }
     let el = null;
     while ((el = this.dropdown.popupBox.querySelector(":scope > address-option"))) {
       el.remove();
     }
-    this.dropdown.popupBox.append(...desiredOptions);
+    for (let option of desiredOptions) {
+      this.dropdown.popupBox.appendChild(option);
+    }
+
+    // Update selectedness after the options are updated
+    let selectedAddressGUID = state[this.selectedStateKey];
+    let optionWithGUID = this.dropdown.getOptionByValue(selectedAddressGUID);
+    this.dropdown.selectedOption = optionWithGUID;
+
+    if (selectedAddressGUID && !optionWithGUID) {
+      throw new Error(`${this.selectedStateKey} option ${selectedAddressGUID}` +
+                      `does not exist in options`);
+    }
+  }
+
+  get selectedStateKey() {
+    return this.getAttribute("selected-state-key");
   }
 }
 
 customElements.define("address-picker", AddressPicker);
--- a/toolkit/components/payments/res/paymentRequest.xhtml
+++ b/toolkit/components/payments/res/paymentRequest.xhtml
@@ -40,18 +40,17 @@
     </header>
 
     <div id="main-container">
       <section id="payment-summary">
         <h1>Your Payment</h1>
 
         <section>
           <div><label>Shipping Address</label></div>
-          <address-picker>
-          </address-picker>
+          <address-picker selected-state-key="selectedShippingAddress"></address-picker>
         </section>
 
         <footer id="controls-container">
           <button id="cancel">Cancel</button>
           <button id="pay">Pay</button>
         </footer>
       </section>
       <section id="order-details-overlay" hidden="hidden">
--- a/toolkit/components/payments/test/mochitest/test_rich_select.html
+++ b/toolkit/components/payments/test/mochitest/test_rich_select.html
@@ -159,17 +159,17 @@ add_task(async function test_clicking_on
      "The selected clone email should be equivalent to the selected option 2");
   is(selectedClone.getAttribute("name"), option2.getAttribute("name"),
      "The selected clone name should be equivalent to the selected option 2");
 });
 
 add_task(async function test_changing_option_selected_affects_other_options() {
   ok(option2.selected, "Option 2 should be selected from prior test");
 
-  option1.selected = true;
+  select1.selectedOption = option1;
   ok(!option2.selected, "Option 2 should no longer be selected after making option 1 selected");
   ok(option1.hasAttribute("selected"), "Option 1 should now have selected attribute");
 });
 
 add_task(async function test_up_down_keys_change_selected_item() {
   let openObserver = new MutationObserver(mutations => {
     for (let mutation of mutations) {
       ok(mutation.attributeName != "open", "the select should not open/close during this test");
@@ -275,17 +275,17 @@ add_task(async function test_selected_cl
 
   let clonedOption = clonedOptions[0];
   for (let attrName of AddressOption.observedAttributes) {
     is(clonedOption.attributes[attrName] && clonedOption.attributes[attrName].value,
        option1.attributes[attrName] && option1.attributes[attrName].value,
        "attributes should have matching value; name=" + attrName);
   }
 
-  option2.selected = true;
+  select1.selectedOption = option2;
   await asyncElementRendered();
 
   clonedOptions = select1.querySelectorAll(".rich-select-selected-clone");
   is(clonedOptions.length, 1, "there should only be one cloned option");
 
   clonedOption = clonedOptions[0];
   for (let attrName of AddressOption.observedAttributes) {
     is(clonedOption.attributes[attrName] && clonedOption.attributes[attrName].value,