Share logic/UI with about:preferences (editCreditCard.xhtml[1]) from the management UI. [1] https://dxr.mozilla.org/mozilla-central/source/browser/extensions/formautofill/content/editCreditCard.xhtml

Comment on attachment 8959381 [details] Bug 1428414 - Use the autofill credit card form in the Payment dialog. https://reviewboard.mozilla.org/r/228208/#review234466 Code analysis found 2 defects in this patch: - 2 defects 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 ::: toolkit/components/payments/test/mochitest/payments_common.js:5 (Diff revision 2) > "use strict"; > > /* exported asyncElementRendered, promiseStateChange, deepClone */ > > +const PTU = SpecialPowers.Cu.import("resource://testing-common/PaymentTestUtils.jsm", {}) Error: 'ptu' is assigned a value but never used. [eslint: no-unused-vars] ::: toolkit/components/payments/test/mochitest/test_basic_card_form.html:71 (Diff revision 2) > + let form = document.createElement("basic-card-form"); > + form.dataset.backButtonLabel = "Back"; > + await form.requestStore.setState({ > + page: { > + id: "test-page", > + } Error: Missing trailing comma. [eslint: comma-dangle]

Comment on attachment 8959381 [details] Bug 1428414 - Use the autofill credit card form in the Payment dialog. https://reviewboard.mozilla.org/r/228208/#review235006 ::: commit-message-300eb:1 (Diff revision 4) > +Bug 1428414 - Use the autofill credit card form in the Payment dialog. r=jaws I'll try to split this up more when I get a chance but it'd be great to get at least a high-level review in the meantime.

Comment on attachment 8959929 [details] Bug 1428414 - Add a PaymentRequest local dev. server. https://reviewboard.mozilla.org/r/228688/#review235128

Comment on attachment 8959930 [details] Bug 1428414 - Support re-using autofill edit forms for different records. https://reviewboard.mozilla.org/r/228690/#review235140 ::: browser/extensions/formautofill/content/autofillEditForms.js:21 (Diff revision 2) > /** > * Fill the form with a record object. > * @param {object} record > */ > - loadInitialValues(record) { > - for (let field in record) { > + loadRecord(record) { > + this._record = record; I think we should move `this._record = record` to EditAddress.loadRecord since with this patch EditCreditCard.loadRecord will end up setting `this._record` twice (once before calling `generateYears()` and a second time when calling `super.loadRecord`.

Comment on attachment 8959381 [details] Bug 1428414 - Use the autofill credit card form in the Payment dialog. https://reviewboard.mozilla.org/r/228208/#review235152 I can't find a way to save the changes made by the add/edit page. When I click on "Add", I can type in whatever I want but the only option is "Back". Clicking on "Back" shows the same empty form. If I choose "Edit" on a selected card, the edit form shows the correct card details (though it does obfuscate the number which I think would be incorrect for "edit") but also doesn't persist any of the changes made. ::: toolkit/components/payments/res/containers/basic-card-form.js:22 (Diff revision 4) > + > +class BasicCardForm extends PaymentStateSubscriberMixin(HTMLElement) { > + constructor() { > + super(); > + > + this.backButton = document.createElement("button"); I think it is odd that we use an <a> to go to the add/edit screen and then a <button> to return to the summary screen. We should use <a> here to match the <a> for add/edit that took the user to this screen, especially if "back" is not supposed to persist the changes. ::: toolkit/components/payments/res/containers/payment-method-picker.js:25 (Diff revision 4) > this.spacerText = document.createTextNode(" "); > this.securityCodeInput = document.createElement("input"); > this.securityCodeInput.autocomplete = "off"; > this.securityCodeInput.size = 3; > this.securityCodeInput.addEventListener("change", this); > + this.addLink = document.createElement("a"); Please add `href="#"` attributes to addLink and editLink so they will be styled like links. Right now they don't have the underline or blue text color. ::: toolkit/components/payments/res/unprivileged-fallbacks.js:6 (Diff revision 4) > /* 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/. */ > > /** > - * This file defines a fallback log object to be used during development outside > + * This file defines a fallback objects to be used during development outside s/defines a fallback/defines fallback/

Comment on attachment 8959381 [details] Bug 1428414 - Use the autofill credit card form in the Payment dialog. https://reviewboard.mozilla.org/r/228208/#review235152 Right, this commit wasn't implementing the buttons since I wanted to unblock other bugs. > I think it is odd that we use an <a> to go to the add/edit screen and then a <button> to return to the summary screen. > > We should use <a> here to match the <a> for add/edit that took the user to this screen, especially if "back" is not supposed to persist the changes. Hmm… not sure I agree… forms usually use buttons for "cancel" which is what back basically is. > Please add `href="#"` attributes to addLink and editLink so they will be styled like links. Right now they don't have the underline or blue text color. FYI: That's actually not a good pattern to use because it will cause the page to scroll to the top when clicked. "javascript:void(0)" or an empty href with `evt.preventDefault();` is better but I get your point and I will add the href attribute.

As I kinda expected, saving the record isn't trivial if we want to handle errors with saving and if we want to auto-select the new records in the pickers since we need a way to know about the new record's guid.

Comment on attachment 8959381 [details] Bug 1428414 - Use the autofill credit card form in the Payment dialog. https://reviewboard.mozilla.org/r/228208/#review236790 ::: toolkit/components/payments/res/containers/basic-card-form.js:74 (Diff revisions 4 - 5) > selectedPaymentCard, > savedBasicCards, > } = state; > > + let editing = !!state.selectedPaymentCard; > + this.form.querySelector("#cc-number").disabled = editing; Why do we want to limit changing the cc-number when editing? What about users who need to fix a typo in the number? I think this might be trying to hard to prevent users from editing a single entry to use different cards.

Comment on attachment 8961650 [details] Bug 1428414 - Removed cleared record attributes on card and address options. https://reviewboard.mozilla.org/r/230524/#review236792

Comment on attachment 8959381 [details] Bug 1428414 - Use the autofill credit card form in the Payment dialog. https://reviewboard.mozilla.org/r/228208/#review236790 > Why do we want to limit changing the cc-number when editing? What about users who need to fix a typo in the number? > > I think this might be trying to hard to prevent users from editing a single entry to use different cards. Two reasons: a) What you said… we don't want users editing to swap between unrelated cards as it will mess with our telemetry and metadata and also lead them to having a worse experience if they intended to save multiple. b) The plan was to never give the decrypted credit card number to the unprivileged dialog to mitigate potential XSS damage so the user wouldn't be able to meaningfully edit the number… they would basically have to re-type it anyways and disabling seemed simpler for now.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #23) > Comment on attachment 8959381 [details] > Bug 1428414 - Use the autofill credit card form in the Payment dialog. > > https://reviewboard.mozilla.org/r/228208/#review236790 > > > Why do we want to limit changing the cc-number when editing? What about users who need to fix a typo in the number? > > > > I think this might be trying to hard to prevent users from editing a single entry to use different cards. > > Two reasons: > a) What you said… we don't want users editing to swap between unrelated > cards as it will mess with our telemetry and metadata and also lead them to > having a worse experience if they intended to save multiple. > b) The plan was to never give the decrypted credit card number to the > unprivileged dialog to mitigate potential XSS damage so the user wouldn't be > able to meaningfully edit the number… they would basically have to re-type > it anyways and disabling seemed simpler for now. I don't really think case (a) is going to be as frequent as you think, but case (b) is sufficient for doing this at least for now. But we might want to include a link there to edit the number, taking the user to the Preferences subdialog.

Comment on attachment 8961651 [details] Bug 1428414 - Support saving credit card changes in the Payment Request dialog. https://reviewboard.mozilla.org/r/230526/#review238416

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31) > (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from > comment #23) > > Comment on attachment 8959381 [details] > > Bug 1428414 - Use the autofill credit card form in the Payment dialog. > > > > https://reviewboard.mozilla.org/r/228208/#review236790 > > > > > Why do we want to limit changing the cc-number when editing? What about users who need to fix a typo in the number? > > > > > > I think this might be trying to hard to prevent users from editing a single entry to use different cards. > > > > Two reasons: > > a) What you said… we don't want users editing to swap between unrelated > > cards as it will mess with our telemetry and metadata and also lead them to > > having a worse experience if they intended to save multiple. > > b) The plan was to never give the decrypted credit card number to the > > unprivileged dialog to mitigate potential XSS damage so the user wouldn't be > > able to meaningfully edit the number… they would basically have to re-type > > it anyways and disabling seemed simpler for now. > > I don't really think case (a) is going to be as frequent as you think, but > case (b) is sufficient for doing this at least for now. But we might want to > include a link there to edit the number, taking the user to the Preferences > subdialog. (a) also applies to the pref subdialog and at the time that was implemented the number wasn't supposed to be editable either.

Comment on attachment 8961650 [details] Bug 1428414 - Removed cleared record attributes on card and address options. https://reviewboard.mozilla.org/r/230524/#review238934 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 ::: toolkit/components/payments/res/containers/address-picker.js:42 (Diff revision 5) > > /** > * De-dupe and filter addresses for the given set of fields that will be visible > * > * @param {object} addresses > - * @param {array?} fieldNames - optional list of field names that be used when de-duping entries > + * @param {array?} fieldNames - optional list of field names that be used when de-duping and excluding entries Error: Line 42 exceeds the maximum line length of 100. [eslint: max-len]

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/b4f0025c8303 Add a PaymentRequest local dev. server. r=jaws https://hg.mozilla.org/integration/autoland/rev/4fa0e33621e3 Support re-using autofill edit forms for different records. r=jaws https://hg.mozilla.org/integration/autoland/rev/b045a7779f4d Use the autofill credit card form in the Payment dialog. r=jaws https://hg.mozilla.org/integration/autoland/rev/6045035ad2ed Removed cleared record attributes on card and address options. r=jaws https://hg.mozilla.org/integration/autoland/rev/70eb77d5bd24 Support saving credit card changes in the Payment Request dialog. r=jaws

https://hg.mozilla.org/mozilla-central/rev/b4f0025c8303 https://hg.mozilla.org/mozilla-central/rev/4fa0e33621e3 https://hg.mozilla.org/mozilla-central/rev/b045a7779f4d https://hg.mozilla.org/mozilla-central/rev/6045035ad2ed https://hg.mozilla.org/mozilla-central/rev/70eb77d5bd24