Closed Bug 1427954 Opened 6 years ago Closed 6 years ago

Factor the address and card edit forms out of the autofill edit dialogs to share with PaymentRequest

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webpayments])

Attachments

(9 files, 1 obsolete file)

Bug 1427954 - Remove unused Services and XPCOMUtils imports in editDialog.js.
59 bytes, text/x-review-board-request
sfoster
: review+
Details
Bug 1427954 - Don't pass child elements into the constructor.
59 bytes, text/x-review-board-request
sfoster
: review+
Details
Bug 1427954 - Split autofill dialog logic from field logic.
59 bytes, text/x-review-board-request
sfoster
: review+
Details
Bug 1427954 - Remove unused uninit code from editDialog.js and autofillEditForms.js.
59 bytes, text/x-review-board-request
sfoster
: review+
Details
Bug 1427954 - Remove string bundle references from autofillEditForms.js.
59 bytes, text/x-review-board-request
sfoster
: review+
Details
Bug 1427954 - Move autofill edit dialog l10n to mutation observers.
59 bytes, text/x-review-board-request
sfoster
: review+
Details
Bug 1427954 - Pass supported and default countries to EditAutofillForm and move loadInitialValues.
59 bytes, text/x-review-board-request
sfoster
: review+
Details
Bug 1427954 - Pass getFormFormat to EditAddress.
59 bytes, text/x-review-board-request
sfoster
: review+
Details
Bug 1427954 - Pass FormAutofillUtils.isCCNumber to EditCreditCard.
59 bytes, text/x-review-board-request
sfoster
: review+
Details 
Share logic/UI with about:preferences (editAddress.xhtml[1]) from the management UI.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/extensions/formautofill/content/editAddress.xhtml
Blocks: 1435105
Blocks: 1440504
Assignee: MattN+bmo → sfoster
Assignee: sfoster → MattN+bmo
So one slight wrinkle is that Custom Elements aren't enabled by default yet so we probably shouldn't use them in the pref dialogs and I don't think we should ship the polyfill. For now I will keep using an ES class instead.
Marking for qe-verification:
Please ensure that the address and credit card add/edit dialogs in preferences continue to work properly (with and without a master password).
Flags: qe-verify+
Comment on attachment 8959071 [details]
Bug 1427954 - (WIP) Use the autofill credit card form in the Payment dialog

https://reviewboard.mozilla.org/r/227958/#review233780


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/basic-card-form.js:62
(Diff revision 1)
> +    } = this.requestStore.getState();
> +
> +    if (state.selectedPaymentCard) {
> +      record = savedBasicCards[selectedPaymentCard]; // TODO: validate?
> +    }
> +    new EditCreditCard({

Error: 'editcreditcard' is not defined. [eslint: no-undef]
Comment on attachment 8959042 [details]
Bug 1427954 - Remove unused Services and XPCOMUtils imports in editDialog.js.

https://reviewboard.mozilla.org/r/227924/#review233980

Looks good
Attachment #8959042 - Flags: review?(sfoster) → review+
Comment on attachment 8959043 [details]
Bug 1427954 - Don't pass child elements into the constructor.

https://reviewboard.mozilla.org/r/227926/#review233984

Looks good
Attachment #8959043 - Flags: review?(sfoster) → review+
Comment on attachment 8959044 [details]
Bug 1427954 - Split autofill dialog logic from field logic.

https://reviewboard.mozilla.org/r/227928/#review234022

Looks great. Just that question about the module change

::: browser/extensions/formautofill/content/editAddress.xhtml:76
(Diff revision 2)
>    </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[
> +  <script type="module"><![CDATA[

Is this change necessary? It seems like we're not using es6 modules anywhere else yet outside js and web-platform tests. 

We could use an iife, or just assign to EditAddress instance to thefieldContainer property directly.

::: browser/extensions/formautofill/content/editCreditCard.xhtml:58
(Diff revision 2)
>    </div>
>    <script type="application/javascript"><![CDATA[
>      "use strict";
> -    /* global EditCreditCard */
> -    new EditCreditCard({
> -      title: document.querySelector("title"),
> +    /* import-globals-from autofillEditForms.js */
> +    /* import-globals-from editDialog.js */
> +    let fieldContainer = new EditCreditCard({

Ditto.
Attachment #8959044 - Flags: review?(sfoster) → review+
Priority: P2 → P1
Whiteboard: [webpayments]
Comment on attachment 8959045 [details]
Bug 1427954 - Remove unused uninit code from editDialog.js and autofillEditForms.js.

https://reviewboard.mozilla.org/r/227930/#review234028

Looks good.
Attachment #8959045 - Flags: review?(sfoster) → review+
Comment on attachment 8959046 [details]
Bug 1427954 - Remove string bundle references from autofillEditForms.js.

https://reviewboard.mozilla.org/r/227932/#review234032

Looks good.
Attachment #8959046 - Flags: review?(sfoster) → review+
Comment on attachment 8959044 [details]
Bug 1427954 - Split autofill dialog logic from field logic.

https://reviewboard.mozilla.org/r/227928/#review234022

> Is this change necessary? It seems like we're not using es6 modules anywhere else yet outside js and web-platform tests. 
> 
> We could use an iife, or just assign to EditAddress instance to thefieldContainer property directly.

Sorry, that was leftover from when I was thinking about using modules but realized that should be a separate bug (bug 1446179).

Re: inlining the EditAddress constructor, you'll see in later commits that it gets more complicated so it seemed less readable that way IMO.
Re-summarizing so this bug just handles the refactoring to that the edit form doesn't depend on chrome-privileged stuff since I want to unblock parallel work on the address and credit card screens and there is enough in this bug.
No longer blocks: 1435105
Summary: Basic Payment Request Shipping Address Add/Edit page → Factor the address and card edit forms out of the autofill edit dialogs to share with PaymentRequest
Blocks: 1446203
Attachment #8959071 - Attachment is obsolete: true
Blocks: 1428414
Comment on attachment 8959047 [details]
Bug 1427954 - Move autofill edit dialog l10n to mutation observers.

https://reviewboard.mozilla.org/r/227934/#review234040

Looks good, with the caveat that I don't know what I don't know. If you want to land before :jaws gets back, maybe someone else can give it a once over?

::: browser/extensions/formautofill/content/l10n.js:29
(Diff revision 3)
> +  }
> +});
> +
> +document.addEventListener("DOMContentLoaded", function onDCL() {
> +  FormAutofillUtils.localizeMarkup(document);
> +  mutationObserver.observe(document, {

This looks good, but I don't know enough about the pitfall/peculiarities of mutation observers to know for sure. Maybe worth getting another pair of eyes on it?

::: browser/extensions/formautofill/test/browser/head.js:342
(Diff revision 3)
>      BrowserTestUtils.waitForEvent(win, "FormReady"),
>    ]);
>  }
>  
>  async function testDialog(url, testFn, arg) {
> -  let win = window.openDialog(url, null, null, arg);
> +  let win = window.openDialog(url, null, "width=600,height=600", arg);

Nit/curious: why this change?
Attachment #8959047 - Flags: review?(sfoster) → review+
Comment on attachment 8959048 [details]
Bug 1427954 - Pass supported and default countries to EditAutofillForm and move loadInitialValues.

https://reviewboard.mozilla.org/r/227936/#review234042

Looks good. I'm not really familiar with the manage dialog so I'm really just doing a sanity check here. If we want to re-name loadInitialValues (and if I've understood that correctly) it could be in a later patch/bug.

::: browser/extensions/formautofill/content/editDialog.js
(Diff revision 3)
>      this.localizeDocument();
>      window.addEventListener("DOMContentLoaded", this, {once: true});
>    }
>  
>    async init() {
> -    if (this._record) {

Is loadInitialValues misnamed now? From the comment below I gather it is no longer responsible for loading, and is instead just responsible for populating the inputs with the values from the record.
Attachment #8959048 - Flags: review?(sfoster) → review+
Comment on attachment 8959049 [details]
Bug 1427954 - Pass getFormFormat to EditAddress.

https://reviewboard.mozilla.org/r/227938/#review234274

::: commit-message-d49f4:2
(Diff revision 3)
> +Bug 1427954 - Pass getFormFormat to EditAddress. r=sfoster
> +

Could you say something here or in the bug about *why* this refactoring is necessary. I see that you are removing the hard dependency on FormAutofillUtils, but at least in this and the following patch, its not clear what that accomplishes.
Attachment #8959049 - Flags: review?(sfoster) → review+
Comment on attachment 8959050 [details]
Bug 1427954 - Pass FormAutofillUtils.isCCNumber to EditCreditCard.

https://reviewboard.mozilla.org/r/227940/#review234276

::: commit-message-26d3f:2
(Diff revision 3)
> +Bug 1427954 - Pass FormAutofillUtils.isCCNumber to EditCreditCard. r=sfoster
> +

Same comment as previous patch - a note about the intention here would be useful. If this is setting us up to be able to pass in payment-specific utilities, lets say so.
Attachment #8959050 - Flags: review?(sfoster) → review+
Comment on attachment 8959047 [details]
Bug 1427954 - Move autofill edit dialog l10n to mutation observers.

https://reviewboard.mozilla.org/r/227934/#review234040

> This looks good, but I don't know enough about the pitfall/peculiarities of mutation observers to know for sure. Maybe worth getting another pair of eyes on it?

Since this is only temporary until we use Fluent and only used for the autofill edit dialogs and the only shipping one is the address one (to 20% of US users on en-US) so I'm not really worried about it. I'll comment in bug 1446164 to make sure this gets removed.

> Nit/curious: why this change?

It's because the default size wasn't very tall and so it made it hard to see what was wrong since you'd have to scroll. Fixing this also means that screenshots on failure show the problem.
Comment on attachment 8959048 [details]
Bug 1427954 - Pass supported and default countries to EditAutofillForm and move loadInitialValues.

https://reviewboard.mozilla.org/r/227936/#review234042

> Is loadInitialValues misnamed now? From the comment below I gather it is no longer responsible for loading, and is instead just responsible for populating the inputs with the values from the record.

Yeah, I rename it in a later patch… to avoid the rebase pain I think I'll leave.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/e4c41608a13e
Remove unused Services and XPCOMUtils imports in editDialog.js. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/89a5d0d89c05
Don't pass child elements into the constructor. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/2f8bd1f13394
Split autofill dialog logic from field logic. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/e2eae899dc4a
Remove unused uninit code from editDialog.js and autofillEditForms.js. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/ac23b9ba0603
Remove string bundle references from autofillEditForms.js. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/4e5ce4f1f579
Move autofill edit dialog l10n to mutation observers. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/6ab0e835a7f4
Pass supported and default countries to EditAutofillForm and move loadInitialValues. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/101a4cf38f89
Pass getFormFormat to EditAddress. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/eee1fae80932
Pass FormAutofillUtils.isCCNumber to EditCreditCard. r=sfoster
Product: Toolkit → Firefox
Target Milestone: mozilla61 → Firefox 61
The issue has been verified on Nightly 61.0a1 (2018-05-06), Windows 10, Windows 7, Ubuntu 16.04 and Mac OS X 13.
Marking it as Verified:Fixed.
Status: RESOLVED → VERIFIED
status-firefox61: 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: