1446203 - Basic Payment Request Shipping Address Add/Edit page

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

Use the factored out address form code from bug 1427954 to implement the basic add/edit address page.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

Review commit: https://reviewboard.mozilla.org/r/237336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237336/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237336/diff/1-2/

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

https://reviewboard.mozilla.org/r/237336/#review244044

::: browser/extensions/formautofill/content/autofillEditForms.js:118
(Diff revision 2)
> -    const {addressLevel1Label, postalCodeLabel, fieldsOrder} = this.getFormFormat(country);
> -    this._elements.addressLevel1Label.dataset.localization = addressLevel1Label;
> -    this._elements.postalCodeLabel.dataset.localization = postalCodeLabel;
> -    this.arrangeFields(fieldsOrder);
> +    // const {addressLevel1Label, postalCodeLabel, fieldsOrder} = this.getFormFormat(country);
> +    // this._elements.addressLevel1Label.dataset.localization = addressLevel1Label;
> +    // this._elements.postalCodeLabel.dataset.localization = postalCodeLabel;
> +    // this.arrangeFields(fieldsOrder);

These are commented out due to the following errors:

> [JavaScript Warning: "Security wrapper denied access to property "addressLevel1Label" on privileged Javascript object. Support for exposing privileged objects to untrusted content via __exposedProps__ has been removed - use WebIDL bindings or Components.utils.cloneInto instead. Note that only the first denied property access from a given global object will be reported." {file: "resource://payments/formautofill/autofillEditForms.js" line: 118}]
> JavaScript error: resource://payments/formautofill/autofillEditForms.js, line 134: TypeError: fieldsOrder is undefined

Do you see what I am doing wrong in paymentDialogFrameScript.js that is not letting autofillEditForms.js work as intended?

::: toolkit/components/payments/content/paymentDialogFrameScript.js:72
(Diff revision 2)
>    exposeUtilityFunctions() {
>      let PaymentDialogUtils = {
> +      DEFAULT_REGION: FormAutofillUtils.DEFAULT_REGION,
> +
> +      // WebPayment support is limited to "US" and "CA" currently.
> +      supportedCountries: ["US", "CA"],

I didn't use FormAutofillUtils.supportedCountries because it includes DE. We could introduce a new pref for this but I didn't think that was worthwhile for the time being.

::: toolkit/components/payments/test/browser/browser_address_edit.js:39
(Diff revision 2)
> +    info("\n\n\n\ndocument\n\n\n\n" +
> +         content.document.body.outerHTML +
> +         "\n\n\n\nend document\n\n\n\n");

This is just here for debugging purposes. Due to the JS errors shown above, the form is missing the two <button> elements. With the code commented out the buttons get added but are missing their textContent because the commented out code is necessary for the textContent.

::: toolkit/components/payments/test/mochitest/mochitest.ini:5
(Diff revision 2)
> +   ../../../../../browser/extensions/formautofill/content/editAddress.xhtml
>     !/browser/extensions/formautofill/content/editCreditCard.xhtml
>     ../../../../../browser/extensions/formautofill/content/autofillEditForms.js

editAddress.xhtml and autofillEditForms.js actually cannot use the ! syntax because they are not included by other test manifests.

Comment 4

[Matthew N. [:MattN]]

Attachment: Fix to getFormFormat

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Comment on attachment 8968654 [details]
> Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.
> 
> https://reviewboard.mozilla.org/r/237336/#review244044
> 
> ::: browser/extensions/formautofill/content/autofillEditForms.js:118
> (Diff revision 2)
> > -    const {addressLevel1Label, postalCodeLabel, fieldsOrder} = this.getFormFormat(country);
> > -    this._elements.addressLevel1Label.dataset.localization = addressLevel1Label;
> > -    this._elements.postalCodeLabel.dataset.localization = postalCodeLabel;
> > -    this.arrangeFields(fieldsOrder);
> > +    // const {addressLevel1Label, postalCodeLabel, fieldsOrder} = this.getFormFormat(country);
> > +    // this._elements.addressLevel1Label.dataset.localization = addressLevel1Label;
> > +    // this._elements.postalCodeLabel.dataset.localization = postalCodeLabel;
> > +    // this.arrangeFields(fieldsOrder);
> 
> These are commented out due to the following errors:
> 
> > [JavaScript Warning: "Security wrapper denied access to property "addressLevel1Label" on privileged Javascript object. Support for exposing privileged objects to untrusted content via __exposedProps__ has been removed - use WebIDL bindings or Components.utils.cloneInto instead. Note that only the first denied property access from a given global object will be reported." {file: "resource://payments/formautofill/autofillEditForms.js" line: 118}]
> > JavaScript error: resource://payments/formautofill/autofillEditForms.js, line 134: TypeError: fieldsOrder is undefined
> 
> Do you see what I am doing wrong in paymentDialogFrameScript.js that is not
> letting autofillEditForms.js work as intended?

Two issues:
1) You were calling the function in address-form.js instead of passing it in to the EditAddress constructor. This also meant that the argument wasn't being passed.
2) cloneInto doesn't automatically clone the return values of the cloned functions so you need to clone the return value if it's not a primitive… in this case we have an object.

> ::: toolkit/components/payments/content/paymentDialogFrameScript.js:72
> (Diff revision 2)
> >    exposeUtilityFunctions() {
> >      let PaymentDialogUtils = {
> > +      DEFAULT_REGION: FormAutofillUtils.DEFAULT_REGION,
> > +
> > +      // WebPayment support is limited to "US" and "CA" currently.
> > +      supportedCountries: ["US", "CA"],
> 
> I didn't use FormAutofillUtils.supportedCountries because it includes DE. We
> could introduce a new pref for this but I didn't think that was worthwhile
> for the time being.

We could remove DE from the pref and use FormAutofillUtils.supportedCountries since that pref only controls whether Germany shows in the country picker for Nightly at the moment (the feature is available in all countries for Nightly regardless).

> ::: toolkit/components/payments/test/browser/browser_address_edit.js:39
> (Diff revision 2)
> > +    info("\n\n\n\ndocument\n\n\n\n" +
> > +         content.document.body.outerHTML +
> > +         "\n\n\n\nend document\n\n\n\n");
> 
> This is just here for debugging purposes. Due to the JS errors shown above,
> the form is missing the two <button> elements. With the code commented out
> the buttons get added but are missing their textContent because the
> commented out code is necessary for the textContent.
> 
> ::: toolkit/components/payments/test/mochitest/mochitest.ini:5
> (Diff revision 2)
> > +   ../../../../../browser/extensions/formautofill/content/editAddress.xhtml
> >     !/browser/extensions/formautofill/content/editCreditCard.xhtml
> >     ../../../../../browser/extensions/formautofill/content/autofillEditForms.js
> 
> editAddress.xhtml and autofillEditForms.js actually cannot use the ! syntax
> because they are not included by other test manifests.

Oh yeah.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

https://reviewboard.mozilla.org/r/237336/#review244354

I'm not sure I should do a thorough review until more of the known issues are fixed. At quick glance things look good

::: toolkit/components/payments/test/mochitest/mochitest.ini:5
(Diff revision 2)
>  [DEFAULT]
>  prefs =
>     dom.webcomponents.customelements.enabled=false
>  support-files =
> +   ../../../../../browser/extensions/formautofill/content/editAddress.xhtml

Hmm… I thought you would need to do the same as editCreditCard here and add editAddress.xhtml to the mochitest.ini in the formautofill subdirectory of this directory.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237336/diff/2-3/

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

https://reviewboard.mozilla.org/r/237336/#review245558

Comment 8

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 36f1bbeb2057158cb2b2dfc6b71af953366b6193 -d 0b8f0217bda4: rebasing 460467:36f1bbeb2057 "Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page. r=MattN" (tip)
merging browser/app/profile/firefox.js
merging toolkit/components/payments/res/containers/basic-card-form.js and toolkit/components/payments/res/containers/address-form.js to toolkit/components/payments/res/containers/address-form.js
merging toolkit/components/payments/res/containers/payment-dialog.js
merging toolkit/components/payments/res/paymentRequest.xhtml
merging toolkit/components/payments/test/browser/browser_card_edit.js and toolkit/components/payments/test/browser/browser_address_edit.js to toolkit/components/payments/test/browser/browser_address_edit.js
merging toolkit/components/payments/test/browser/browser_card_edit.js
merging toolkit/components/payments/test/mochitest/mochitest.ini
warning: conflicts while merging toolkit/components/payments/res/containers/address-form.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/components/payments/res/paymentRequest.xhtml! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/components/payments/test/browser/browser_address_edit.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237336/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5de60fc0f2da
Basic Payment Request Shipping Address Add/Edit page. r=MattN

Comment 11

[Dorel Luca [:dluca]]

Backed out changeset 5de60fc0f2da (bug 1446203) for Mochitest failure on browser/extensions/formautofill/test/mochitest/test_address_level_1_submission.html. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=175647170&repo=autoland&lineNumber=2829

 INFO - TEST-PASS | browser/extensions/formautofill/test/mochitest/test_address_level_1_submission.html | Receive add storage changed event 
[task 2018-04-26T00:41:00.997Z] 00:41:00     INFO - expecting the chrome task finished: FormAutofillTest:CheckAddresses
[task 2018-04-26T00:41:00.998Z] 00:41:00     INFO - Buffered messages finished
[task 2018-04-26T00:41:00.999Z] 00:41:00     INFO - TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/mochitest/test_address_level_1_submission.html | Address saved as expected 
[task 2018-04-26T00:41:01.000Z] 00:41:00     INFO -     test_form_will_submit_without_sub_keys@browser/extensions/formautofill/test/mochitest/test_address_level_1_submission.html:72:3
[task 2018-04-26T00:41:01.001Z] 00:41:01     INFO -     async*add_task/</<@SimpleTest/AddTask.js:57:34
[task 2018-04-26T00:41:01.002Z] 00:41:01     INFO -     async*add_task/<@SimpleTest/AddTask.js:31:10
[task 2018-04-26T00:41:01.003Z] 00:41:01     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:676:12
[task 2018-04-26T00:41:01.004Z] 00:41:01     INFO -     add_task@SimpleTest/AddTask.js:30:7
[task 2018-04-26T00:41:01.005Z] 00:41:01     INFO -     @browser/extensions/formautofill/test/mochitest/test_address_level_1_submission.html:31:1
[task 2018-04-26T00:41:01.005Z] 00:41:01     INFO - TEST-PASS | browser/extensions/formautofill/test/mochitest/test_address_level_1_submission.html | Check form submitted 
[task 2018-04-26T00:41:01.006Z] 00:41:01     INFO - AddTask.js | Leaving test test_form_will_submit_without_sub_keys
[task 2018-04-26T00:41:01.007Z] 00:41:01     INFO - expecting the storage cleanup

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5de60fc0f2da8acd02bcff1b3ded47ea6d788d37

Backout:
https://hg.mozilla.org/integration/autoland/rev/e331c3b551445f8f2c4747eb5e9777b5cbbf41c4

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237336/diff/4-5/

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237336/diff/5-6/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d03421ea2005
Basic Payment Request Shipping Address Add/Edit page. r=MattN

Comment 15

[Cosmin Sabou [:CosminS]]

Backed out changeset for suspicion of causing browser chrome failures on browser_address_edit.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=64583d3e0f6fd6ec9049bea1039948ce871f4209&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=browser%20chrome

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175894555&repo=autoland&lineNumber=5500

Backout link: https://hg.mozilla.org/integration/autoland/rev/61784cdfb9059f0a9849d6423eba6cc5fceaaab3

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237336/diff/6-7/

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237336/diff/7-8/

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8968654 [details]
Bug 1446203 - Basic Payment Request Shipping Address Add/Edit page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237336/diff/8-9/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f6c1ddff499
Basic Payment Request Shipping Address Add/Edit page. r=MattN

Comment 20

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9f6c1ddff499