Closed Bug 1476204 Opened 6 years ago Closed 6 years ago

Credit card add/edit page error handling fixes

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webpayments])

User Story

Problems to fix:
* Can save with a blank credit card
* No empty year option (inconsistent with month)
* Save/Add button not disabled initially on an empty form
* Error message is in normal black text
* The field validation should trigger a red border on the cc-number field upon change if invalid

Attachments

(5 files)

Bug 1476204 - Make the basic-card-option field order and alignment match the <option>.
59 bytes, text/x-review-board-request
sfoster
: review+
Details
Bug 1476204 - Check Luhn algorithm in the basic-card-form and in storage and disable save button when invalid.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1476204 - Replace duplicate #error-text with .page-error.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1476204 - Implement the payment summary error bar.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1476204 - Handle autofill record update state changes in the unpriv. PR forms.
59 bytes, text/x-review-board-request
jaws
: review+
Details
There are various improvements to make to the basic card add/edit page. See the user story.
Blocks: 1476345
User Story: (updated)
User Story: (updated)
Flags: qe-verify+
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 :)
Attachment #8993820 - Flags: review?(sfoster) → review+
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
Attachment #8993821 - Flags: review?(jaws) → review+
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.
Attachment #8994103 - Flags: review-
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.
Attachment #8994103 - Flags: review?(sfoster)
User Story: (updated)
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
Attachment #8994103 - Flags: review?(jaws) → review+
Comment on attachment 8994393 [details] Bug 1476204 - Implement the payment summary error bar. https://reviewboard.mozilla.org/r/258960/#review266194
Attachment #8994393 - Flags: review?(jaws) → review+
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?
Attachment #8994394 - Flags: review?(jaws) → review+
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
Flags: needinfo?(MattN+bmo)
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
QA Contact: hani.yacoub
Depends on: 1478880
Verified - Fixed on the latest Nightly 63.0a1 (2018-08-16) on Windows 7/10 x64, Ubuntu 16.04 and Mac OS 10.13.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: