1482689 - Use a <rich-select> for the billing address picker instead of a <select>

Status: VERIFIED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

To match the UX spec we shouldn't be using a plain <select>.

Comment 3

[Sam Foster [:sfoster] (he/him)]

I'll take this. I'm going to assume we want this change to affect both formautofill preferences UI and web payments - as the picker is created at https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/content/editCreditCard.xhtml#75

Comment 4

[Matthew N. [:MattN]]

Yeah, the complication is with autofill depending on a custom element (and its dependencies) from …/payments/. Perhaps we should avoid that problem more and only change the dropdown for when it's within the payment dialog and then we can go further than <rich-select> and use <address-picker>.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1482689 - Use AddressPicker for the card billing address UI. r?MattN

* Got the address-picker in there. It gets populated with addresses.
* Styling basically ok. Need to see about which fields to display etc.
* Address picker gets its value and updates state as selection changes
* Fix previousId so add/edit of the billing address returns back to the edit card page

TODO:
* Resolve the no-empty-option question and get the test_add_link browser_card_edit.js test passing
* Un-skip the others and get those working too
* Check invalid address is still correctly handled

Comment 6

[Sam Foster [:sfoster] (he/him)]

Matt: I've updated the patch. This is pretty much review ready - with new tests and existing tests passing. Except.. I'm getting this from the test_address_picker.html:

[JavaScript Error: "ReferenceError: can't access lexical declaration `AddressPicker' before initialization" {file: "http://mochi.test:8888/tests/browser/components/payments/res/containers/billing-address-picker.js" line: 13}]
@http://mochi.test:8888/tests/browser/components/payments/res/containers/billing-address-picker.js:13:1

..which is weird because that test file should make no reference to the billing-address-picker.js. I know I've tracked this down and fixed it before, but the reason eludes me for the moment. I assume you can review without it if you get to looking at this before I have uploaded a fix.  

I know this patch has been a long time in the making, but I'm still not 100% sure this is the right solution. I mean it works and makes its own sense, but its a bit at odds with how the other components interact with the requestStore. I've tried to make the design decisions explicit - see the billing_address_picker.html tests - so we can at least have that discussion.

Comment 7

[Matthew N. [:MattN]]

(In reply to Sam Foster [:sfoster] from comment #6)
> I know this patch has been a long time in the making, but I'm still not 100%
> sure this is the right solution. I mean it works and makes its own sense,
> but its a bit at odds with how the other components interact with the
> requestStore. I've tried to make the design decisions explicit - see the
> billing_address_picker.html tests - so we can at least have that discussion.

I didn't review that test yet since I reviewed the revision before you added it but so far I think the approach is fine.

Comment 8

[Matthew N. [:MattN]]

See some analysis at https://mozilla.logbot.info/payments/20181101#c15551112

Comment 9

[Matthew N. [:MattN]]

Attachment: Bug 1482689 - Don't import payment-dialog.js from other elements. r?sfoster

Importing payment-dialog.js ends up importing every other element because it imports the elements needed to render the full dialog.
We don't want all of our elements to depend on every other payment one so we shouldn't import payment-dialog.js anywhere in non-test code.

Comment 10

[Sam Foster [:sfoster] (he/him)]

Ok I've updated the patch to address the review feedback. It looks like you already set up that payment-dialog.js patch as a parent revision. 

You'll notice I updated the browser_card_edit.js test to explicitly ensure the edit button is hidden when there's no billing address selected. I didn't see the previous behavior in the spec, so I'm not sure if its important to preserve it.

Comment 11

[Sam Foster [:sfoster] (he/him)]

Patch updated with the requested changes. 

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8480fdf5a2267b99f2bc835c59c657b88b3ad2bc

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c810cab5dbdb
Don't import payment-dialog.js from other elements. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/61b49da4b9cb
Use AddressPicker for the card billing address UI. r=MattN

Comment 13

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=61b49da4b9cb5b4c01ed7f0382a6975bf92cd496&tochange=364bdcbdc13be5f3e03fad20e0210a1649061005&searchStr=browser%2Cchrome&selectedJob=209847603

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

Backout link: https://hg.mozilla.org/integration/autoland/rev/364bdcbdc13be5f3e03fad20e0210a1649061005

[task 2018-11-05T16:10:08.374Z] 16:10:08     INFO - TEST-PASS | browser/components/payments/test/browser/browser_address_edit.js | Asterisk should be on Province - "attr(fieldRequiredSymbol)" == "attr(fieldRequiredSymbol)" - 
[task 2018-11-05T16:10:08.375Z] 16:10:08     INFO - Buffered messages logged at 16:10:02
[task 2018-11-05T16:10:08.377Z] 16:10:08     INFO - Console message: [JavaScript Error: "Content Security Policy: The page’s settings blocked the loading of a resource at inline (“default-src”)." {file: "resource://payments/paymentRequest.xhtml" line: 0}]
[task 2018-11-05T16:10:08.378Z] 16:10:08     INFO - Buffered messages finished
[task 2018-11-05T16:10:08.379Z] 16:10:08     INFO - TEST-UNEXPECTED-FAIL | browser/components/payments/test/browser/browser_address_edit.js | Uncaught exception - Switched back to payment-summary - timed out after 50 tries.
[task 2018-11-05T16:10:08.381Z] 16:10:08     INFO - Leaving test bound test_countrySpecificFieldsGetRequiredness
[task 2018-11-05T16:10:08.381Z] 16:10:08     INFO - GECKO(2233) | --DOMWINDOW == 14 (0x7f3912478400) [pid = 2331] [serial = 12] [outer = (nil)] [url = resource://payments/paymentRequest.xhtml]
[task 2018-11-05T16:10:08.383Z] 16:10:08     INFO - GECKO(2233) | --DOMWINDOW == 13 (0x7f3912beac00) [pid = 2331] [serial = 7] [outer = (nil)] [url = https://example.com/browser/browser/components/payments/test/browser/blank_page.html]
[task 2018-11-05T16:10:08.384Z] 16:10:08     INFO - GECKO(2233) | --DOMWINDOW == 12 (0x7f3912bf6800) [pid = 2331] [serial = 9] [outer = (nil)] [url = about:blank]
[task 2018-11-05T16:10:08.385Z] 16:10:08     INFO - GECKO(2233) | --DOMWINDOW == 11 (0x7f3911cbcc00) [pid = 2331] [serial = 6] [outer = (nil)] [url = about:blank]
[task 2018-11-05T16:10:08.386Z] 16:10:08     INFO - Console message: SENTINEL
[task 2018-11-05T16:10:08.387Z] 16:10:08     INFO - GECKO(2233) | --DOMWINDOW == 10 (0x7f391247e000) [pid = 2331] [serial = 14] [outer = (nil)] [url = about:blank]
[task 2018-11-05T16:10:08.387Z] 16:10:08     INFO - GECKO(2233) | --DOMWINDOW == 9 (0x7f3910092400) [pid = 2331] [serial = 19] [outer = (nil)] [url = about:blank]
[task 2018-11-05T16:10:08.393Z] 16:10:08     INFO - GECKO(2233) | --DOMWINDOW == 8 (0x7f390f55f000) [pid = 2331] [serial = 16] [outer = (nil)] [url = about:blank]
[task 2018-11-05T16:10:08.394Z] 16:10:08     INFO - GECKO(2233) | --DOMWINDOW == 7 (0x7f3912bf1800) [pid = 2331] [serial = 10] [outer = (nil)] [url = https://example.com/browser/browser/components/payments/test/browser/blank_page.html]
[task 2018-11-05T16:10:08.396Z] 16:10:08     INFO - GECKO(2233) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2018-11-05T16:10:08.397Z] 16:10:08     INFO - GECKO(2233) | MEMORY STAT | vsize 1879MB | residentFast 321MB | heapAllocated 101MB
[task 2018-11-05T16:10:08.398Z] 16:10:08     INFO - TEST-OK | browser/components/payments/test/browser/browser_address_edit.js | took 37854ms
[task 2018-11-05T16:10:08.399Z] 16:10:08     INFO - Not taking screenshot here: see the one that was previously logged

Comment 14

[Sam Foster [:sfoster] (he/him)]

I think I found the issue - the page.previousId was ending up with the wrong value so the first click on the back button didnt take us back to the summary page. 

I also fixed what looks like another previousId issue with returning to the basic-card-page and preserveFieldValues. May be the cause of bug 1492399? 

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2df872c4053b4a3b41991d32fbfbf7316a14a4

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea5d6df24de0
Use AddressPicker for the card billing address UI. r=MattN

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aca271fb5175
Don't import payment-dialog.js from other elements. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/4d0392a8b19e
Use AddressPicker for the card billing address UI. r=MattN

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0cb0757649fa
Backed out changeset ea5d6df24de0 as requested by sfoster.

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/aca271fb5175
https://hg.mozilla.org/mozilla-central/rev/4d0392a8b19e

Comment 19

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] from comment #14)
> I also fixed what looks like another previousId issue with returning to the
> basic-card-page and preserveFieldValues. May be the cause of bug 1492399? 

This turned out to be a red herring, so I don't expect any change to this particular issue with the patch that landed. I'll pursue this in its own bug.

Comment 20

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 65.0a1 (2018-11-07) on Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and on Ubuntu 16.04 x64.