Bug 1480717 - Fix credit card form billing address and persist checkbox layout. r=sfoster
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Mon, 24 Sep 2018 19:51:39 +0000
changeset 437954 32d9dbdfae6b
parent 437953 abff79205da2
child 437955 8bf660646ca6
push id69851
push usermozilla@noorenberghe.ca
push dateMon, 24 Sep 2018 20:06:48 +0000
treeherderautoland@32d9dbdfae6b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfoster
bugs1480717
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1480717 - Fix credit card form billing address and persist checkbox layout. r=sfoster Differential Revision: https://phabricator.services.mozilla.com/D4175
browser/components/payments/content/paymentDialogWrapper.xul
browser/components/payments/res/components/accepted-cards.css
browser/components/payments/res/components/card-icon.svg
browser/components/payments/res/components/labelled-checkbox.js
browser/components/payments/res/containers/basic-card-form.css
browser/components/payments/res/containers/basic-card-form.js
browser/components/payments/test/browser/browser_card_edit.js
browser/components/payments/test/mochitest/test_basic_card_form.html
browser/extensions/formautofill/content/editCreditCard.xhtml
browser/extensions/formautofill/skin/shared/editCreditCard.css
--- a/browser/components/payments/content/paymentDialogWrapper.xul
+++ b/browser/components/payments/content/paymentDialogWrapper.xul
@@ -33,12 +33,12 @@
   </popupset>
 
   <browser type="content"
            id="paymentRequestFrame"
            disablehistory="true"
            nodefaultsrc="true"
            remote="true"
            selectmenulist="ContentSelectDropdown"
-           style="height:450px;width:700px"
+           style="height:500px;width:600px"
            transparent="true"></browser>
   <script type="application/javascript" src="chrome://payments/content/paymentDialogWrapper.js"></script>
 </window>
--- a/browser/components/payments/res/components/accepted-cards.css
+++ b/browser/components/payments/res/components/accepted-cards.css
@@ -13,30 +13,32 @@ accepted-cards {
   display: inline-block;
   list-style-type: none;
   margin: 0;
   padding: 0;
 }
 
 .accepted-cards-list > .accepted-cards-item {
   display: inline-block;
-  width: 47px;
-  height: 30px;
+  width: 50px;
+  height: 22px;
   padding: 0;
   text-align: center;
   margin-inline-end: 8px;
   background-repeat: no-repeat;
   background-position: center;
+  vertical-align: middle;
 }
 
 /* placeholders for specific card icons we don't yet have assets for */
 .accepted-cards-item[data-network-id] {
   background-image: url("./card-icon.svg");
 }
 .accepted-cards-item[data-network-id]::after {
+  box-sizing: border-box;
   content: attr(data-network-id);
   padding: 4px;
   text-align: center;
   font-size: 0.7rem;
   display: block;
   overflow: hidden;
-  width: 36px;
+  width: 100%;
 }
--- a/browser/components/payments/res/components/card-icon.svg
+++ b/browser/components/payments/res/components/card-icon.svg
@@ -1,6 +1,6 @@
-<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 47 30">
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 47 22">
   <rect x="0" y="0" width="47" height="22" rx="4" ry="4" fill="#000" fill-opacity="0.2">
   </rect>
   <rect x="0" y="5" width="47" height="12" fill="#fff" fill-opacity="1">
   </rect>
 </svg>
--- a/browser/components/payments/res/components/labelled-checkbox.js
+++ b/browser/components/payments/res/components/labelled-checkbox.js
@@ -6,16 +6,17 @@ import ObservedPropertiesMixin from "../
 
 /**
  *  <labelled-checkbox label="Some label" value="The value"></labelled-checkbox>
  */
 
 export default class LabelledCheckbox extends ObservedPropertiesMixin(HTMLElement) {
   static get observedAttributes() {
     return [
+      "form",
       "label",
       "value",
     ];
   }
   constructor() {
     super();
 
     this._label = document.createElement("label");
@@ -28,16 +29,23 @@ export default class LabelledCheckbox ex
     this.appendChild(this._label);
     this._label.appendChild(this._checkbox);
     this._label.appendChild(this._labelSpan);
     this.render();
   }
 
   render() {
     this._labelSpan.textContent = this.label;
+    // We don't use the ObservedPropertiesMixin behaviour because we want to be able to mirror
+    // form="" but ObservedPropertiesMixin removes attributes when "".
+    if (this.hasAttribute("form")) {
+      this._checkbox.setAttribute("form", this.getAttribute("form"));
+    } else {
+      this._checkbox.removeAttribute("form");
+    }
   }
 
   get checked() {
     return this._checkbox.checked;
   }
 
   set checked(value) {
     return this._checkbox.checked = value;
--- a/browser/components/payments/res/containers/basic-card-form.css
+++ b/browser/components/payments/res/containers/basic-card-form.css
@@ -1,12 +1,41 @@
 /* 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/. */
 
+basic-card-form .editCreditCardForm {
+  /* Add the persist-checkbox row to the grid */
+  grid-template-areas:
+    "cc-number cc-exp-month cc-exp-year"
+    "cc-name   cc-type      cc-csc"
+    "accepted  accepted     accepted"
+    "persist-checkbox persist-checkbox persist-checkbox"
+    "billingAddressGUID billingAddressGUID billingAddressGUID";
+}
+
+basic-card-form .editCreditCardForm > accepted-cards {
+  grid-area: accepted;
+  margin: 0;
+}
+
+basic-card-form .editCreditCardForm .persist-checkbox {
+  display: flex;
+  grid-area: persist-checkbox;
+}
+
+#billingAddressGUID-container {
+  display: grid;
+}
+
+#billingAddressGUID {
+  /* XXX: temporary until converted to a rich-picker in bug 1482689 */
+  margin: 14px 0;
+}
+
 basic-card-form > footer > .cancel-button {
   /* When cancel is shown (during onboarding), it should always be on the left with a space after it */
   margin-right: auto;
 }
 
 basic-card-form > footer > .cancel-button[hidden] ~ .back-button {
   /* When back is shown (outside onboarding) we want "Back <space> Add/Save" */
   /* Bug 1468153 may change the button ordering to match platform conventions */
--- a/browser/components/payments/res/containers/basic-card-form.js
+++ b/browser/components/payments/res/containers/basic-card-form.js
@@ -32,16 +32,19 @@ export default class BasicCardForm exten
     this.addressAddLink.href = "javascript:void(0)";
     this.addressAddLink.addEventListener("click", this);
     this.addressEditLink = document.createElement("a");
     this.addressEditLink.className = "edit-link";
     this.addressEditLink.href = "javascript:void(0)";
     this.addressEditLink.addEventListener("click", this);
 
     this.persistCheckbox = new LabelledCheckbox();
+    // The persist checkbox shouldn't be part of the record which gets saved so
+    // exclude it from the form.
+    this.persistCheckbox.form = "";
     this.persistCheckbox.className = "persist-checkbox";
 
     this.acceptedCardsList = new AcceptedCards();
 
     // page footer
     this.cancelButton = document.createElement("button");
     this.cancelButton.className = "cancel-button";
     this.cancelButton.addEventListener("click", this);
@@ -100,25 +103,30 @@ export default class BasicCardForm exten
 
       // The "invalid" event does not bubble and needs to be listened for on each
       // form element.
       for (let field of this.form.elements) {
         field.addEventListener("invalid", this);
       }
 
       let fragment = document.createDocumentFragment();
-      fragment.append(this.addressAddLink);
       fragment.append(" ");
       fragment.append(this.addressEditLink);
+      fragment.append(this.addressAddLink);
       let billingAddressRow = this.form.querySelector(".billingAddressRow");
+
+      // XXX: Bug 1482689 - Remove the label-text class from the billing field
+      // which will be removed when switching to <rich-select>.
+      billingAddressRow.querySelector(".label-text").classList.remove("label-text");
+
       billingAddressRow.appendChild(fragment);
 
-      this.body.appendChild(this.persistCheckbox);
+      form.insertBefore(this.persistCheckbox, billingAddressRow);
+      form.insertBefore(this.acceptedCardsList, billingAddressRow);
       this.body.appendChild(this.genericErrorText);
-      this.body.appendChild(this.acceptedCardsList);
       // Only call the connected super callback(s) once our markup is fully
       // connected, including the shared form fetched asynchronously.
       super.connectedCallback();
     });
   }
 
   render(state) {
     let {
@@ -140,16 +148,17 @@ export default class BasicCardForm exten
     } else {
       this.saveButton.textContent = editing ? this.dataset.updateButtonLabel :
                                               this.dataset.addButtonLabel;
     }
     this.persistCheckbox.label = this.dataset.persistCheckboxLabel;
     this.addressAddLink.textContent = this.dataset.addressAddLinkLabel;
     this.addressEditLink.textContent = this.dataset.addressEditLinkLabel;
     this.acceptedCardsList.label = this.dataset.acceptedCardsLabel;
+    this.acceptedCardsList.hidden = editing;
 
     // The next line needs an onboarding check since we don't set previousId
     // when navigating to add/edit directly from the summary page.
     this.backButton.hidden = !page.previousId && page.onboardingWizard;
     this.cancelButton.hidden = !page.onboardingWizard;
 
     let record = {};
     let basicCards = paymentRequest.getBasicCards(state);
@@ -198,17 +207,22 @@ export default class BasicCardForm exten
 
     let billingAddressSelect = this.form.querySelector("#billingAddressGUID");
     if (basicCardPage.billingAddressGUID) {
       billingAddressSelect.value = basicCardPage.billingAddressGUID;
     } else if (!editing) {
       if (paymentRequest.getAddresses(state)[selectedShippingAddress]) {
         billingAddressSelect.value = selectedShippingAddress;
       } else {
-        billingAddressSelect.value = Object.keys(addresses)[0];
+        let firstAddressGUID = Object.keys(addresses)[0];
+        if (firstAddressGUID) {
+          // Only set the value if we have a saved address to not mark the field
+          // dirty and invalid on an add form with no saved addresses.
+          billingAddressSelect.value = firstAddressGUID;
+        }
       }
     }
     // Need to recalculate the populated state since
     // billingAddressSelect is updated after loadRecord.
     this.formHandler.updatePopulatedState(billingAddressSelect);
 
     this.updateRequiredState();
     this.updateSaveButtonState();
@@ -353,16 +367,20 @@ export default class BasicCardForm exten
   updateSaveButtonState() {
     this.saveButton.disabled = !this.form.checkValidity();
   }
 
   updateRequiredState() {
     for (let field of this.form.elements) {
       let container = field.closest(".container");
       let span = container.querySelector(".label-text");
+      if (!span) {
+        // The billing address field doesn't use a label inside the field.
+        continue;
+      }
       span.setAttribute("fieldRequiredSymbol", this.dataset.fieldRequiredSymbol);
       let required = field.required && !field.disabled;
       if (required) {
         container.setAttribute("required", "true");
       } else {
         container.removeAttribute("required");
       }
     }
--- a/browser/components/payments/test/browser/browser_card_edit.js
+++ b/browser/components/payments/test/browser/browser_card_edit.js
@@ -582,35 +582,42 @@ add_task(async function test_invalid_net
     }, "Check edit page state");
 
     state = await PTU.DialogContentUtils.waitForState(content, (state) => {
       return Object.keys(state.savedBasicCards).length == 1 &&
              Object.keys(state.savedAddresses).length == 1;
     }, "Check card and address present at beginning of test");
 
     let networkSelector = content.document.querySelector("basic-card-form #cc-type");
-    todo_is(Cu.waiveXrays(networkSelector).selectedIndex, 0,
-            "An invalid cc-type should result in the first option being selected");
+    is(Cu.waiveXrays(networkSelector).selectedIndex, -1,
+       "An invalid cc-type should result in no selection");
     is(Cu.waiveXrays(networkSelector).value, "",
        "An invalid cc-type should result in an empty string as value");
 
+    ok(content.document.querySelector("basic-card-form button.save-button").disabled,
+       "Save button should be disabled due to a missing cc-type");
+
+    content.fillField(Cu.waiveXrays(networkSelector), "visa");
+
+    ok(!content.document.querySelector("basic-card-form button.save-button").disabled,
+       "Save button should be enabled after fixing cc-type");
     content.document.querySelector("basic-card-form button.save-button").click();
 
-    // we expect that saving a card with an invalid network will result in the
-    // cc-type property being changed to undefined
+    // We expect that saving a card with a fixed network will result in the
+    // cc-type property being changed to the new value.
     state = await PTU.DialogContentUtils.waitForState(content, (state) => {
       let cards = Object.entries(state.savedBasicCards);
       return cards.length == 1 &&
-             cards[0][1]["cc-type"] == undefined;
+             cards[0][1]["cc-type"] == "visa";
     }, "Check card was edited");
 
     let cardGUIDs = Object.keys(state.savedBasicCards);
     is(cardGUIDs.length, 1, "Check there is still one card");
     let savedCard = state.savedBasicCards[cardGUIDs[0]];
-    ok(!savedCard["cc-type"], "We expect the cc-type value to be updated");
+    is(savedCard["cc-type"], "visa", "We expect the cc-type value to be updated");
 
     state = await PTU.DialogContentUtils.waitForState(content, (state) => {
       return state.page.id == "payment-summary";
     }, "Switched back to payment-summary");
   }, args);
 });
 
 add_task(async function test_private_card_adding() {
--- a/browser/components/payments/test/mochitest/test_basic_card_form.html
+++ b/browser/components/payments/test/mochitest/test_basic_card_form.html
@@ -106,28 +106,48 @@ add_task(async function test_backButton(
 });
 
 add_task(async function test_saveButton() {
   let form = new BasicCardForm();
   form.dataset.addButtonLabel = "Add";
   form.dataset.errorGenericSave = "Generic error";
   await form.promiseReady;
   display.appendChild(form);
+
+  let address1 = deepClone(PTU.Addresses.TimBL);
+  address1.guid = "TimBLGUID";
+  let address2 = deepClone(PTU.Addresses.TimBL2);
+  address2.guid = "TimBL2GUID";
+
+  await form.requestStore.setState({
+    savedAddresses: {
+      [address1.guid]: deepClone(address1),
+      [address2.guid]: deepClone(address2),
+    },
+  });
+
   await asyncElementRendered();
 
+  ok(!form.acceptedCardsList.hidden, "Accepted card list should be visible when adding a card");
+
   ok(form.saveButton.disabled, "Save button should initially be disabled");
   fillField(form.form.querySelector("#cc-number"), "4111 1111-1111 1111");
   form.form.querySelector("#cc-name").focus();
   // Check .disabled after .focus() so that it's after both "input" and "change" events.
   ok(form.saveButton.disabled, "Save button should still be disabled without a name");
   sendString("J. Smith");
   fillField(form.form.querySelector("#cc-exp-month"), "11");
   let year = (new Date()).getFullYear().toString();
   fillField(form.form.querySelector("#cc-exp-year"), year);
   fillField(form.form.querySelector("#cc-type"), "visa");
+  isnot(form.form.querySelector("#billingAddressGUID").value, address2.guid,
+        "Check initial billing address");
+  fillField(form.form.querySelector("#billingAddressGUID"), address2.guid);
+  is(form.form.querySelector("#billingAddressGUID").value, address2.guid,
+     "Check selected billing address");
   form.saveButton.focus();
   ok(!form.saveButton.disabled,
      "Save button should be enabled since the required fields are filled");
 
   fillField(form.form.querySelector("#cc-exp-month"), "");
   fillField(form.form.querySelector("#cc-exp-year"), "");
   form.saveButton.focus();
   ok(form.saveButton.disabled,
@@ -164,32 +184,36 @@ add_task(async function test_saveButton(
     guid: undefined,
     messageType: "updateAutofillRecord",
     record: {
       "cc-exp-month": "11",
       "cc-exp-year": year,
       "cc-name": "J. Smith",
       "cc-number": "4111 1111-1111 1111",
       "cc-type": "visa",
-      "billingAddressGUID": "",
+      "billingAddressGUID": address2.guid,
       "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");
+  is(requiredElements.length, 6, "Number of required elements");
   for (let element of requiredElements) {
+    if (element.id == "billingAddressGUID") {
+      // The billing address has a different layout.
+      continue;
+    }
     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");
@@ -309,42 +333,56 @@ add_task(async function test_add_noSelec
 
 add_task(async function test_edit() {
   let form = new BasicCardForm();
   form.dataset.updateButtonLabel = "Update";
   await form.promiseReady;
   display.appendChild(form);
   await asyncElementRendered();
 
+  let address1 = deepClone(PTU.Addresses.TimBL);
+  address1.guid = "TimBLGUID";
+
   info("test year before current");
   let card1 = deepClone(PTU.BasicCards.JohnDoe);
   card1.guid = "9864798564";
   card1["cc-exp-year"] = 2011;
+  card1.billingAddressGUID = address1.guid;
 
   await form.requestStore.setState({
     page: {
       id: "basic-card-page",
     },
     "basic-card-page": {
       guid: card1.guid,
     },
+    savedAddresses: {
+      [address1.guid]: deepClone(address1),
+    },
     savedBasicCards: {
       [card1.guid]: deepClone(card1),
     },
   });
   await asyncElementRendered();
   is(form.saveButton.textContent, "Update", "Check label");
   is(form.querySelectorAll(":-moz-ui-invalid").length, 0,
      "Check no fields are visibly invalid on an 'edit' form with a complete card");
   checkCCForm(form, card1);
   ok(!form.saveButton.disabled, "Save button should be enabled upon edit for a valid card");
+  ok(form.acceptedCardsList.hidden, "Accepted card list should be hidden when editing a card");
 
   let requiredElements = [...form.form.elements].filter(e => e.required && !e.disabled);
   ok(requiredElements.length, "There should be at least one required element");
+  is(requiredElements.length, 5, "Number of required elements");
   for (let element of requiredElements) {
+    if (element.id == "billingAddressGUID") {
+      // The billing address has a different layout.
+      continue;
+    }
+
     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;
 
@@ -385,48 +423,53 @@ add_task(async function test_edit() {
     },
     "basic-card-page": {
       guid: null,
     },
   });
   await asyncElementRendered();
   is(form.querySelectorAll(":-moz-ui-invalid").length, 0,
      "Check no fields are visibly invalid after converting to an 'add' form");
-  checkCCForm(form, {});
+  checkCCForm(form, {
+    billingAddressGUID: address1.guid, // Default selected
+  });
 
   form.remove();
 });
 
 add_task(async function test_field_validity_updates() {
   let form = new BasicCardForm();
   form.dataset.updateButtonLabel = "Add";
   await form.promiseReady;
   display.appendChild(form);
   await asyncElementRendered();
 
   let ccNumber = form.form.querySelector("#cc-number");
   let nameInput = form.form.querySelector("#cc-name");
+  let typeInput = form.form.querySelector("#cc-type");
   let monthInput = form.form.querySelector("#cc-exp-month");
   let yearInput = form.form.querySelector("#cc-exp-year");
 
   info("test with valid cc-number but missing cc-name");
   fillField(ccNumber, "4111111111111111");
   ok(ccNumber.checkValidity(), "cc-number field is valid with good input");
   ok(!nameInput.checkValidity(), "cc-name field is invalid when empty");
   ok(form.saveButton.disabled, "Save button should be disabled with incomplete input");
 
   info("correct by adding cc-name and expiration values");
   fillField(nameInput, "First");
   fillField(monthInput, "11");
   let year = (new Date()).getFullYear().toString();
   fillField(yearInput, year);
+  fillField(typeInput, "visa");
   ok(ccNumber.checkValidity(), "cc-number field is valid with good input");
   ok(nameInput.checkValidity(), "cc-name field is valid with a value");
   ok(monthInput.checkValidity(), "cc-exp-month field is valid with a value");
   ok(yearInput.checkValidity(), "cc-exp-year field is valid with a value");
+  ok(typeInput.checkValidity(), "cc-type field is valid with a value");
   ok(!form.saveButton.disabled, "Save button should not be disabled with good input");
 
   info("edit to make the cc-number invalid");
   ccNumber.focus();
   sendString("aa");
   nameInput.focus();
   sendString("Surname");
 
--- a/browser/extensions/formautofill/content/editCreditCard.xhtml
+++ b/browser/extensions/formautofill/content/editCreditCard.xhtml
@@ -51,25 +51,25 @@
       </select>
       <span data-localization="cardExpiresYear" class="label-text"/>
     </label>
     <label id="cc-name-container" class="container">
       <input id="cc-name" type="text" required="required"/>
       <span data-localization="nameOnCard" class="label-text"/>
     </label>
     <label id="cc-type-container" class="container">
-      <select id="cc-type">
+      <select id="cc-type" required="required">
       </select>
       <span data-localization="cardNetwork" class="label-text"/>
     </label>
-    <label id="billingAddressGUID-container" class="billingAddressRow container">
-      <select id="billingAddressGUID">
+    <div id="billingAddressGUID-container" class="billingAddressRow container rich-picker">
+      <select id="billingAddressGUID" required="required">
       </select>
-      <span data-localization="billingAddress" class="label-text"/>
-    </label>
+      <label for="billingAddressGUID" data-localization="billingAddress" class="label-text"/>
+    </div>
   </form>
   <div id="controls-container">
     <button id="cancel" data-localization="cancelBtnLabel"/>
     <button id="save" disabled="disabled" data-localization="saveBtnLabel"/>
   </div>
   <script type="application/javascript"><![CDATA[
     "use strict";
 
--- a/browser/extensions/formautofill/skin/shared/editCreditCard.css
+++ b/browser/extensions/formautofill/skin/shared/editCreditCard.css
@@ -3,16 +3,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 .editCreditCardForm {
   display: grid;
   grid-template-areas:
     "cc-number          cc-exp-month       cc-exp-year"
     "cc-name            cc-type            cc-csc"
     "billingAddressGUID billingAddressGUID billingAddressGUID";
+  grid-template-columns: 4fr 2fr 2fr;
   grid-row-gap: var(--grid-column-row-gap);
   grid-column-gap: var(--grid-column-row-gap);
 }
 
 .editCreditCardForm label {
   /* Remove the margin on these labels since they are styled on top of
      the input/select element. */
   margin-inline-start: 0;
@@ -41,8 +42,12 @@
 
 #cc-type-container {
   grid-area: cc-type;
 }
 
 #billingAddressGUID-container {
   grid-area: billingAddressGUID;
 }
+
+#billingAddressGUID {
+  grid-area: dropdown;
+}