There are various improvements to make to the basic card add/edit page. See the user story.

Comment on attachment 8993821 [details] Bug 1476204 - Check Luhn algorithm in the basic-card-form and in storage and disable save button when invalid. https://reviewboard.mozilla.org/r/258498/#review265542 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/extensions/formautofill/content/autofillEditForms.js:25 (Diff revision 1) > for (let field of this._elements.form.elements) { > let value = record[field.id]; > - field.value = typeof(value) == "undefined" ? "" : value; > + field.setAttribute("value", typeof(value) == "undefined" ? "" : value); > } > + // Reset the dirty value flag and validity state. > + //this._elements.form.reset(); Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

Comment on attachment 8993820 [details] Bug 1476204 - Make the basic-card-option field order and alignment match the <option>. https://reviewboard.mozilla.org/r/258496/#review265558 ::: browser/components/payments/res/components/basic-card-option.css:7 (Diff revision 1) > * 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-option { > - grid-row-gap: 5px; > - grid-column-gap: 10px; > + grid-column-gap: 1ch; > + grid-template-areas: "type cc-number cc-exp cc-name"; I'm not sure I understand the intention here. I read the commit message to mean the order of the values in the <option> should match the order of the fields - in the add/edit form. But those are number, name, expiry. Whereas this orders the values as number, expiry name. I'll call this a nit as its easy to address either way but color me confused :)

Comment on attachment 8993820 [details] Bug 1476204 - Make the basic-card-option field order and alignment match the <option>. https://reviewboard.mozilla.org/r/258496/#review265558 > I'm not sure I understand the intention here. I read the commit message to mean the order of the values in the <option> should match the order of the fields - in the add/edit form. But those are number, name, expiry. Whereas this orders the values as number, expiry name. > > I'll call this a nit as its easy to address either way but color me confused :) The commit message is talking about <basic-card-option> (closed state) vs. <option> (open state).

Comment on attachment 8993821 [details] Bug 1476204 - Check Luhn algorithm in the basic-card-form and in storage and disable save button when invalid. https://reviewboard.mozilla.org/r/258498/#review265676

Comment on attachment 8994103 [details] Bug 1476204 - Replace duplicate #error-text with .page-error. https://reviewboard.mozilla.org/r/258726/#review265680 ::: browser/components/payments/res/paymentRequest.xhtml:131 (Diff revision 1) > <address-picker class="payer-related" > label="&payerLabel;" > data-add-link-label="&payer.addLink.label;" > data-edit-link-label="&payer.editLink.label;" > selected-state-key="selectedPayerAddress"></address-picker> > - <div id="error-text"></div> > + <div class="page-error" aria-live="polite"></div> I didn't realize that we had two #error-text before, but now we would still have two .page-error. Our code just gets the first reference and sets/gets the textContent of that first reference. We should either remove this second instance or update the code to set the textContent on each instance.

Comment on attachment 8994103 [details] Bug 1476204 - Replace duplicate #error-text with .page-error. https://reviewboard.mozilla.org/r/258726/#review265878 ::: browser/components/payments/res/paymentRequest.xhtml:131 (Diff revision 1) > <address-picker class="payer-related" > label="&payerLabel;" > data-add-link-label="&payer.addLink.label;" > data-edit-link-label="&payer.editLink.label;" > selected-state-key="selectedPayerAddress"></address-picker> > - <div id="error-text"></div> > + <div class="page-error" aria-live="polite"></div> This one looks like its supposed to be the inline (field-specific) error? The spec shows the page error at the top of the page. If its for inline error messages it should probably get a different class. But if that's true then shouldn't we have containers for error messages associated with the other fields? So I'm not sure what's going on here.

Comment on attachment 8994103 [details] Bug 1476204 - Replace duplicate #error-text with .page-error. https://reviewboard.mozilla.org/r/258726/#review265680 > I didn't realize that we had two #error-text before, but now we would still have two .page-error. Our code just gets the first reference and sets/gets the textContent of that first reference. > > We should either remove this second instance or update the code to set the textContent on each instance. Oops, good catch, somehow I thought the one was on a different page. I split moving this error to a separate commit.

Comment on attachment 8994103 [details] Bug 1476204 - Replace duplicate #error-text with .page-error. https://reviewboard.mozilla.org/r/258726/#review265878 > This one looks like its supposed to be the inline (field-specific) error? The spec shows the page error at the top of the page. If its for inline error messages it should probably get a different class. > > But if that's true then shouldn't we have containers for error messages associated with the other fields? > > So I'm not sure what's going on here. This wasn't a fields-specific one but I can see why you may think that. I've moved it to the header in a new commit.

I moved "Saving an invalid form reverts values to the original, losing changes." to bug 1476345 since that is complicated enough and needs more testing.

Comment on attachment 8994103 [details] Bug 1476204 - Replace duplicate #error-text with .page-error. https://reviewboard.mozilla.org/r/258726/#review266096

Comment on attachment 8994393 [details] Bug 1476204 - Implement the payment summary error bar. https://reviewboard.mozilla.org/r/258960/#review266194

After talking with Jacqueline on Friday, we agreed to do: * Fields shouldn't be marked invalid immediately for an add form, only an edit form. I will also do this in bug 1476345.

Comment on attachment 8994394 [details] Bug 1476204 - Handle autofill record update state changes in the unpriv. PR forms. https://reviewboard.mozilla.org/r/258962/#review266448 ::: browser/components/payments/content/paymentDialogWrapper.js:582 (Diff revision 2) > - } > + } > - } > + } > + > - let isTemporary = record.isTemporary; > + let isTemporary = record.isTemporary; > - let collection = isTemporary ? this.temporaryStore[collectionName] : > + let collection = isTemporary ? this.temporaryStore[collectionName] : > - formAutofillStorage[collectionName]; > + formAutofillStorage[collectionName]; Four-space indent seems odd. Shouldn't this either be using two-space indent or have this align with the `this.temporaryStore` above it? ::: browser/components/payments/content/paymentDialogWrapper.js:609 (Diff revision 2) > - } > - > - this.sendMessageToContent("updateState", successStateChange); > } catch (ex) { > - this.sendMessageToContent("updateState", errorStateChange); > + responseMessage.error = true; > + this.sendMessageToContent("updateAutofillRecord:Response", responseMessage); This line is duplicated above at the end of the `try` block. Can you move it out of the `catch` block? Or put it in a `finally` block?

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/ed92000eca20 Make the basic-card-option field order and alignment match the <option>. r=sfoster https://hg.mozilla.org/integration/autoland/rev/f0f464e3c27c Check Luhn algorithm in the basic-card-form and in storage and disable save button when invalid. r=jaws https://hg.mozilla.org/integration/autoland/rev/001f13f2dd21 Replace duplicate #error-text with .page-error. r=jaws https://hg.mozilla.org/integration/autoland/rev/a88879ea32e8 Implement the payment summary error bar. r=jaws https://hg.mozilla.org/integration/autoland/rev/10f3d1014592 Handle autofill record update state changes in the unpriv. PR forms. r=jaws

Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/438769e48bce Backed out 5 changesets for browser chrome failures on browser_editCreditCardDialog. CLOSED TREE

Backed out for browser chrome failures on browser_editCreditCardDialog. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=10f3d10145925ae3048b859a7f0b6023f969fe89&tochange=438769e48bcee3d2ddfa57f0167342f50a4f4e18&selectedJob=190149325 Failures log: https://treeherder.mozilla.org/logviewer.html#?job_id=190149325&repo=autoland&lineNumber=3820 https://treeherder.mozilla.org/logviewer.html#?job_id=190141520&repo=autoland&lineNumber=13504 https://treeherder.mozilla.org/logviewer.html#?job_id=190149289&repo=autoland&lineNumber=2748 01:08:45 ERROR - 1139 INFO TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js | Unable to find a rejection expected by expectUncaughtRejection. - 1 == 0 - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoMoreExpectedRejections :: line 273 01:08:45 INFO - Stack trace: 01:08:45 INFO - resource://testing-common/PromiseTestUtils.jsm:assertNoMoreExpectedRejections:273 01:08:45 INFO - chrome://mochikit/content/browser-test.js:nextTest:746 01:08:45 INFO - chrome://mochikit/content/browser-test.js:testScope/test_finish/<:1397 01:08:45 INFO - chrome://mochikit/content/browser-test.js:run:1334 01:08:45 INFO - GECKO(5588) | MEMORY STAT | vsize 717MB | vsizeMaxContiguous 789MB | residentFast 248MB | heapAllocated 101MB

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2077e3828a70553515cfd57179546197fb6f1e38

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/d717ad76e616 Make the basic-card-option field order and alignment match the <option>. r=sfoster https://hg.mozilla.org/integration/autoland/rev/c63340378a52 Check Luhn algorithm in the basic-card-form and in storage and disable save button when invalid. r=jaws https://hg.mozilla.org/integration/autoland/rev/9ad00680f9aa Replace duplicate #error-text with .page-error. r=jaws https://hg.mozilla.org/integration/autoland/rev/d57443c3e4ba Implement the payment summary error bar. r=jaws https://hg.mozilla.org/integration/autoland/rev/45cf26c3c53c Handle autofill record update state changes in the unpriv. PR forms. r=jaws

https://hg.mozilla.org/mozilla-central/rev/d717ad76e616 https://hg.mozilla.org/mozilla-central/rev/c63340378a52 https://hg.mozilla.org/mozilla-central/rev/9ad00680f9aa https://hg.mozilla.org/mozilla-central/rev/d57443c3e4ba https://hg.mozilla.org/mozilla-central/rev/45cf26c3c53c

Verified - Fixed on the latest Nightly 63.0a1 (2018-08-16) on Windows 7/10 x64, Ubuntu 16.04 and Mac OS 10.13.