Bug 1477114 - Add an asterisk to the required fields of the credit card form as well as the CVV placeholder. r?sfoster draft
authorJared Wein <jwein@mozilla.com>
Mon, 30 Jul 2018 17:52:24 -0400
changeset 826007 01944756583efe10b5d43e015f4ba0dc0f52b3f9
parent 825858 a2d65d03e46a9a42b5bee5c2a7864d3f987a8ca7
child 826027 bfeebe1b8b8309c0c73bc67b480c59784d3824b6
push id118230
push userbmo:jaws@mozilla.com
push dateThu, 02 Aug 2018 20:26:53 +0000
reviewerssfoster
bugs1477114
milestone63.0a1
Bug 1477114 - Add an asterisk to the required fields of the credit card form as well as the CVV placeholder. r?sfoster MozReview-Commit-ID: 2zg5HOZVtxN
browser/components/payments/res/containers/address-form.css
browser/components/payments/res/containers/address-form.js
browser/components/payments/res/containers/basic-card-form.js
browser/components/payments/res/containers/form.css
browser/components/payments/res/containers/payment-method-picker.js
browser/components/payments/res/paymentRequest.xhtml
browser/components/payments/test/mochitest/test_address_form.html
browser/components/payments/test/mochitest/test_basic_card_form.html
--- a/browser/components/payments/res/containers/address-form.css
+++ b/browser/components/payments/res/containers/address-form.css
@@ -19,21 +19,16 @@ address-form[address-fields] #postal-cod
 address-form[address-fields] #country-container,
 address-form[address-fields]:not([address-fields~='email']) #email-container,
 address-form[address-fields]:not([address-fields~='tel']) #tel-container {
   /* !important is needed because autofillEditForms.js sets
      inline styles on the form fields with display: flex; */
   display: none !important;
 }
 
-label[required] > span:first-of-type::after {
-  /* The asterisk should be localized, bug 1472278 */
-  content: "*";
-}
-
 .error-text:not(:empty) {
   color: #fff;
   background-color: #d70022;
   border-radius: 2px;
   /* The padding-top and padding-bottom are referenced by address-form.js */
   padding: 5px 12px;
   position: absolute;
   z-index: 1;
--- a/browser/components/payments/res/containers/address-form.js
+++ b/browser/components/payments/res/containers/address-form.js
@@ -148,28 +148,17 @@ export default class AddressForm extends
       this.persistCheckbox.hidden = false;
       this.persistCheckbox.checked = state.isPrivate ? false :
                                                        defaults.saveAddressDefaultChecked;
     }
 
     this.formHandler.loadRecord(record);
 
     // Add validation to some address fields
-    for (let formElement of this.form.elements) {
-      let container = formElement.closest(`#${formElement.id}-container`);
-      if (formElement.localName == "button" || !container) {
-        continue;
-      }
-      let required = formElement.required && !formElement.disabled;
-      if (required) {
-        container.setAttribute("required", "true");
-      } else {
-        container.removeAttribute("required");
-      }
-    }
+    this.updateRequiredState();
 
     let shippingAddressErrors = request.paymentDetails.shippingAddressErrors;
     for (let [errorName, errorSelector] of Object.entries(this._errorFieldMap)) {
       let container = this.form.querySelector(errorSelector + "-container");
       let field = this.form.querySelector(errorSelector);
       let errorText = (shippingAddressErrors && shippingAddressErrors[errorName]) || "";
       container.classList.toggle("error", !!errorText);
       field.setCustomValidity(errorText);
@@ -200,16 +189,17 @@ export default class AddressForm extends
       data.span.style.top = (data.top - 10) + "px";
       if (isRTL) {
         data.span.style.right = data.right + "px";
       } else {
         data.span.style.left = data.left + "px";
       }
     }
   }
+
   handleEvent(event) {
     switch (event.type) {
       case "click": {
         this.onClick(event);
         break;
       }
     }
   }
@@ -241,16 +231,31 @@ export default class AddressForm extends
         break;
       }
       default: {
         throw new Error("Unexpected click target");
       }
     }
   }
 
+  updateRequiredState() {
+    for (let formElement of this.form.elements) {
+      let container = formElement.closest(`#${formElement.id}-container`);
+      if (formElement.localName == "button" || !container) {
+        continue;
+      }
+      let required = formElement.required && !formElement.disabled;
+      if (required) {
+        container.setAttribute("required", "true");
+      } else {
+        container.removeAttribute("required");
+      }
+    }
+  }
+
   async saveRecord() {
     let record = this.formHandler.buildFormObject();
     let currentState = this.requestStore.getState();
     let {
       page,
       tempAddresses,
       savedBasicCards,
       "address-page": addressPage,
--- a/browser/components/payments/res/containers/basic-card-form.js
+++ b/browser/components/payments/res/containers/basic-card-form.js
@@ -176,16 +176,17 @@ export default class BasicCardForm exten
     } else if (!editing) {
       if (paymentRequest.getAddresses(state)[selectedShippingAddress]) {
         billingAddressSelect.value = selectedShippingAddress;
       } else {
         billingAddressSelect.value = Object.keys(addresses)[0];
       }
     }
 
+    this.updateRequiredState();
     this.updateSaveButtonState();
   }
 
   handleEvent(event) {
     switch (event.type) {
       case "click": {
         this.onClick(event);
         break;
@@ -294,16 +295,28 @@ export default class BasicCardForm exten
   onInvalid(event) {
     this.saveButton.disabled = true;
   }
 
   updateSaveButtonState() {
     this.saveButton.disabled = !this.form.checkValidity();
   }
 
+  updateRequiredState() {
+    for (let formElement of this.form.elements) {
+      let container = formElement.closest("label") || formElement.closest("div");
+      let required = formElement.required && !formElement.disabled;
+      if (required) {
+        container.setAttribute("required", "true");
+      } else {
+        container.removeAttribute("required");
+      }
+    }
+  }
+
   async saveRecord() {
     let record = this.formHandler.buildFormObject();
     let currentState = this.requestStore.getState();
     let {
       tempBasicCards,
       "basic-card-page": basicCardPage,
     } = currentState;
     let editing = !!basicCardPage.guid;
new file mode 100644
--- /dev/null
+++ b/browser/components/payments/res/containers/form.css
@@ -0,0 +1,8 @@
+/* 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/. */
+
+:-moz-any(label, div)[required] > span:first-of-type::after {
+  /* The asterisk should be localized, bug 1472278 */
+  content: "*";
+}
--- a/browser/components/payments/res/containers/payment-method-picker.js
+++ b/browser/components/payments/res/containers/payment-method-picker.js
@@ -13,17 +13,17 @@ import paymentRequest from "../paymentRe
  */
 
 export default class PaymentMethodPicker extends RichPicker {
   constructor() {
     super();
     this.dropdown.setAttribute("option-type", "basic-card-option");
     this.securityCodeInput = document.createElement("input");
     this.securityCodeInput.autocomplete = "off";
-    this.securityCodeInput.placeholder = "CVV"; /* XXX Bug 1473772 */
+    this.securityCodeInput.placeholder = "CVV*"; /* XXX Bug 1473772 */
     this.securityCodeInput.size = 3;
     this.securityCodeInput.classList.add("security-code");
     this.securityCodeInput.addEventListener("change", this);
   }
 
   connectedCallback() {
     super.connectedCallback();
     this.dropdown.after(this.securityCodeInput);
--- a/browser/components/payments/res/paymentRequest.xhtml
+++ b/browser/components/payments/res/paymentRequest.xhtml
@@ -77,16 +77,17 @@
 
   <link rel="stylesheet" href="chrome://global/skin/in-content/common.css"/>
   <link rel="stylesheet" href="paymentRequest.css"/>
   <link rel="stylesheet" href="components/rich-select.css"/>
   <link rel="stylesheet" href="components/address-option.css"/>
   <link rel="stylesheet" href="components/basic-card-option.css"/>
   <link rel="stylesheet" href="components/shipping-option.css"/>
   <link rel="stylesheet" href="components/payment-details-item.css"/>
+  <link rel="stylesheet" href="containers/form.css"/>
   <link rel="stylesheet" href="containers/address-form.css"/>
   <link rel="stylesheet" href="containers/basic-card-form.css"/>
   <link rel="stylesheet" href="containers/order-details.css"/>
   <link rel="stylesheet" href="containers/rich-picker.css"/>
   <link rel="stylesheet" href="containers/error-page.css"/>
 
   <script src="unprivileged-fallbacks.js"></script>
 
--- a/browser/components/payments/test/mochitest/test_address_form.html
+++ b/browser/components/payments/test/mochitest/test_address_form.html
@@ -12,16 +12,17 @@ Test the address-form element
   <script src="sinon-2.3.2.js"></script>
   <script src="payments_common.js"></script>
   <script src="../../res/vendor/custom-elements.min.js"></script>
   <script src="../../res/unprivileged-fallbacks.js"></script>
   <script src="autofillEditForms.js"></script>
 
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
   <link rel="stylesheet" type="text/css" href="../../res/paymentRequest.css"/>
+  <link rel="stylesheet" type="text/css" href="../../res/containers/form.css"/>
   <link rel="stylesheet" type="text/css" href="../../res/containers/address-form.css"/>
 </head>
 <body>
   <p id="display">
   </p>
 <div id="content" style="display: none">
 
 </div>
@@ -293,18 +294,18 @@ add_task(async function test_field_valid
     form.form.querySelector("#address-level2"),
     postalCodeInput,
     addressLevel1Input,
     countrySelect,
   ];
   for (let field of requiredFields) {
     let container = field.closest("label");
     ok(container.hasAttribute("required"), "Container should have required attribute");
-    let label = field.closest("label").querySelector("span");
-    is(getComputedStyle(label, "::after").content, "\"*\"", "Asterisk should be on " + field.id);
+    let span = container.querySelector("span");
+    is(getComputedStyle(span, "::after").content, "\"*\"", "Asterisk should be on " + field.id);
   }
 
   countrySelect.selectedIndex = [...countrySelect.options].findIndex(o => o.value == "US");
   countrySelect.dispatchEvent(new Event("change"));
 
   sendStringAndCheckValidity(addressLevel1Input, "MI", true);
   sendStringAndCheckValidity(addressLevel1Input, "", false);
   sendStringAndCheckValidity(postalCodeInput, "B4N4N4", false);
--- a/browser/components/payments/test/mochitest/test_basic_card_form.html
+++ b/browser/components/payments/test/mochitest/test_basic_card_form.html
@@ -131,16 +131,48 @@ add_task(async function test_saveButton(
       "cc-number": "4111 1111-1111 1111",
       "billingAddressGUID": "",
       "isTemporary": true,
     },
   }, "Check event details for the message to chrome");
   form.remove();
 });
 
+add_task(async function test_requiredAttributePropagated() {
+  let form = new BasicCardForm();
+  await form.promiseReady;
+  display.appendChild(form);
+  await asyncElementRendered();
+
+  let requiredElements = [...form.form.elements].filter(e => e.required && !e.disabled);
+  ok(requiredElements.length, "There should be at least one required element");
+  for (let element of requiredElements) {
+    let container = element.closest("label") || element.closest("div");
+    ok(container.hasAttribute("required"), "Container should also be marked as required");
+  }
+  // Now test that toggling the `required` attribute will affect the container.
+  let sampleRequiredElement = requiredElements[0];
+  let sampleRequiredContainer = sampleRequiredElement.closest("label") ||
+                                sampleRequiredElement.closest("div");
+  sampleRequiredElement.removeAttribute("required");
+  await form.requestStore.setState({});
+  await asyncElementRendered();
+  ok(!sampleRequiredElement.hasAttribute("required"),
+     `"required" attribute should still be removed from element (${sampleRequiredElement.id})`);
+  ok(!sampleRequiredContainer.hasAttribute("required"),
+     `"required" attribute should be removed from container`);
+  sampleRequiredElement.setAttribute("required", "true");
+  await form.requestStore.setState({});
+  await asyncElementRendered();
+  ok(sampleRequiredContainer.hasAttribute("required"),
+     "`required` attribute is re-added to container");
+
+  form.remove();
+});
+
 add_task(async function test_genericError() {
   let form = new BasicCardForm();
   await form.requestStore.setState({
     page: {
       id: "test-page",
       error: "Generic Error",
     },
   });
@@ -257,16 +289,24 @@ add_task(async function test_edit() {
     savedBasicCards: {
       [card1.guid]: deepClone(card1),
     },
   });
   await asyncElementRendered();
   is(form.saveButton.textContent, "Update", "Check label");
   checkCCForm(form, card1);
 
+  let requiredElements = [...form.form.elements].filter(e => e.required && !e.disabled);
+  ok(requiredElements.length, "There should be at least one required element");
+  for (let element of requiredElements) {
+    let container = element.closest("label") || element.closest("div");
+    ok(element.hasAttribute("required"), "Element should be marked as required");
+    ok(container.hasAttribute("required"), "Container should also be marked as required");
+  }
+
   info("test future year");
   card1["cc-exp-year"] = 2100;
 
   await form.requestStore.setState({
     savedBasicCards: {
       [card1.guid]: deepClone(card1),
     },
   });