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

VERIFIED FIXED in Firefox 65

Status

()

P1
normal
VERIFIED FIXED
8 months ago
5 months ago

People

(Reporter: MattN, Assigned: sfoster)

Tracking

Trunk
Firefox 65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 disabled, firefox64 disabled, firefox65 verified)

Details

(Whiteboard: [webpayments])

Attachments

(2 attachments)

To match the UX spec we shouldn't be using a plain <select>.
Flags: qe-verify+
QA Contact: hani.yacoub
Priority: -- → P3
Whiteboard: [webpayments] [triage] → [webpayments-reserve]
Duplicate of this bug: 1490995
Duplicate of this bug: 1492383
Priority: P3 → P2
Whiteboard: [webpayments-reserve] → [webpayments]
(Assignee)

Comment 3

6 months ago
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
Assignee: nobody → sfoster
Priority: P2 → P1
Status: NEW → ASSIGNED
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>.
(Assignee)

Comment 5

5 months ago
* 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
Attachment #9018763 - Attachment description: Bug 1482689 - Use AddressPicker for the card billing address UI → Bug 1482689 - (WIP) Use AddressPicker for the card billing address UI. r?MattN
status-firefox63: --- → disabled
status-firefox64: --- → disabled
Attachment #9018763 - Attachment description: Bug 1482689 - (WIP) Use AddressPicker for the card billing address UI. r?MattN → Bug 1482689 - Use AddressPicker for the card billing address UI. r?MattN
(Assignee)

Comment 6

5 months ago
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.
(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.
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.
(Assignee)

Comment 10

5 months ago
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 12

5 months ago
Pushed by mozilla@noorenberghe.ca:
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
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
Flags: needinfo?(sfoster)
(Assignee)

Comment 14

5 months ago
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
Flags: needinfo?(sfoster)

Comment 15

5 months ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea5d6df24de0
Use AddressPicker for the card billing address UI. r=MattN

Comment 16

5 months ago
Pushed by sfoster@mozilla.com:
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

5 months ago
Backout by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0cb0757649fa
Backed out changeset ea5d6df24de0 as requested by sfoster.

Comment 18

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aca271fb5175
https://hg.mozilla.org/mozilla-central/rev/4d0392a8b19e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
(Assignee)

Comment 19

5 months ago
(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

5 months ago
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.
Status: RESOLVED → VERIFIED
status-firefox65: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.