Closed Bug 1387221 Opened 8 years ago Closed 8 years ago

Shipping Address selector for when profiles exist in autofill storage

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox60 --- fixed

People

(Reporter: jonathanGB, Assigned: MattN)

References

Details

(Whiteboard: [webpayments])

Attachments

(2 files, 3 obsolete files)

Bug 1387221 - Connect the shipping address picker with autofill address storage.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1387221 - Use registerCleanupFunction not on 'SimpleTest' to workaround bug 1432690.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Integrate the autofill storage to pre-populate the UI.
Priority: -- → P1
Comment on attachment 8896485 [details] Bug 1387221 - Integrate the autofill storage for shipping addresses & handle UI address change. https://reviewboard.mozilla.org/r/167734/#review197994 Clearing review since this needs some work and I'll take it over.
Attachment #8896485 - Flags: review?(MattN+bmo)
Comment on attachment 8896486 [details] Bug 1387221 - integrate DOM API "changeshippingaddress" when switching address selected. https://reviewboard.mozilla.org/r/167736/#review197996
Attachment #8896486 - Flags: review?(MattN+bmo)
Assignee: jonathan.guillotte.blouin → MattN+bmo
Blocks: 1427950
Attachment #8896485 - Attachment is obsolete: true
Attachment #8896486 - Attachment is obsolete: true
Attachment #8896487 - Attachment is obsolete: true
Tests are coming. Also note that <rich-select> will need a follow-up to properly handle dynamic changes (probably with a MutationObserver)
Depends on: 1429976
Comment on attachment 8941968 [details] Bug 1387221 - Connect the shipping address picker with autofill address storage. https://reviewboard.mozilla.org/r/212156/#review217982 ::: toolkit/components/payments/res/components/address-option.js:8 (Diff revision 3) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /** > * <rich-select> > - * <address-option addressLine="1234 Anywhere St" > - * city="Some City" > + * <address-option guid="98hgvnbmytfc" > + * address-level1="MI"></address-option> The sample markup got corrupted here. ::: toolkit/components/payments/res/containers/address-select.js:11 (Diff revision 3) > + > +"use strict"; > + > +/** > + * <address-select></address-select> > + * Container around <rich-select> with <address-option> listening to savedAddresses. I don't think it makes sense to have an <address-select> be the parent for <rich-select>. We should just make <address-select> extend <rich-select> ::: toolkit/components/payments/res/containers/address-select.js:17 (Diff revision 3) > + */ > + > +class AddressSelect extends PaymentStateSubscriberMixin(HTMLElement) { > + constructor() { > + super(); > + this._richSelect = document.createElement("rich-select"); Can we just create this inside of connectedCallback? And then I don't think you should store a reference to it, I ran in to leaks when I was doing that before. It is easy enough to use `this.querySelector(":scope > rich-select")` to get a reference to it. (Note that my above issue negates the need for this child element altogether, but I still thought it would be worth pointing this out)
Attachment #8941968 - Flags: review?(jaws) → review-
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #10) > Also note that <rich-select> will need a follow-up to > properly handle dynamic changes (probably with a MutationObserver) I filed bug 1430200 for this.
Comment on attachment 8941968 [details] Bug 1387221 - Connect the shipping address picker with autofill address storage. https://reviewboard.mozilla.org/r/212156/#review217982 > The sample markup got corrupted here. Oops, this got messed up when I sorted the lines. > I don't think it makes sense to have an <address-select> be the parent for <rich-select>. > > We should just make <address-select> extend <rich-select> My thinking was that the <address-select> would include more than just the <rich-select>, such as the Add/Edit links and any error messages. The credit card equivalent would also include the CVV input. > Can we just create this inside of connectedCallback? And then I don't think you should store a reference to it, I ran in to leaks when I was doing that before. > > It is easy enough to use `this.querySelector(":scope > rich-select")` to get a reference to it. > > (Note that my above issue negates the need for this child element altogether, but I still thought it would be worth pointing this out) If the only motivation for this is the leaks then we should investigate that since storing the reference should be more performant. The leak could be in the shim or a bug in the DOM code. It's also possible that we shouldn't store references like that as it is a real leak according to the spec but I doubt that. If we can't keep a strong element reference from the constructor I would rather use a weak reference than use querySelector every time as it should be more performant.
Comment on attachment 8941968 [details] Bug 1387221 - Connect the shipping address picker with autofill address storage. (I didn't really mean to request review again as I didn't test for leaks but wanted to needinfo you on my reply)
Flags: needinfo?(jaws)
Attachment #8941968 - Flags: review?(jaws)
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #16) > > I don't think it makes sense to have an <address-select> be the parent for <rich-select>. > > > > We should just make <address-select> extend <rich-select> > > My thinking was that the <address-select> would include more than just the > <rich-select>, such as the Add/Edit links and any error messages. > > The credit card equivalent would also include the CVV input. The naming here doesn't signal that. <html:select> only holds <html:option>, and I had never intended for <rich-select> or <*-select> to hold anything other than the <select> functionality. We could have <address-form> and <basic-card-form>, which would tie closer semantically to <form>. > > Can we just create this inside of connectedCallback? And then I don't think you should store a reference to it, I ran in to leaks when I was doing that before. > > > > It is easy enough to use `this.querySelector(":scope > rich-select")` to get a reference to it. > > > > (Note that my above issue negates the need for this child element altogether, but I still thought it would be worth pointing this out) > > If the only motivation for this is the leaks then we should investigate that > since storing the reference should be more performant. The leak could be in > the shim or a bug in the DOM code. It's also possible that we shouldn't > store references like that as it is a real leak according to the spec but I > doubt that. Leaks are my only motivation against storing a reference. > If we can't keep a strong element reference from the constructor I would > rather use a weak reference than use querySelector every time as it should > be more performant.
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19) > (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from > comment #16) > > > I don't think it makes sense to have an <address-select> be the parent for <rich-select>. > > > > > > We should just make <address-select> extend <rich-select> > > > > My thinking was that the <address-select> would include more than just the > > <rich-select>, such as the Add/Edit links and any error messages. > > > > The credit card equivalent would also include the CVV input. > > The naming here doesn't signal that. <html:select> only holds <html:option>, > and I had never intended for <rich-select> or <*-select> to hold anything > other than the <select> functionality. We could have <address-form> and > <basic-card-form>, which would tie closer semantically to <form>. If the issue is only naming, then what about address-selector or address-picker? The whole PaymentRequest dialog is like one <form> so I don't think *-form is great. Since the components that I'm talking about work together as one unit, I think it makes sense for them to be together in one component/container. > > > Can we just create this inside of connectedCallback? And then I don't think you should store a reference to it, I ran in to leaks when I was doing that before. > > > > > > It is easy enough to use `this.querySelector(":scope > rich-select")` to get a reference to it. > > > > > > (Note that my above issue negates the need for this child element altogether, but I still thought it would be worth pointing this out) > > > > If the only motivation for this is the leaks then we should investigate that > > since storing the reference should be more performant. The leak could be in > > the shim or a bug in the DOM code. It's also possible that we shouldn't > > store references like that as it is a real leak according to the spec but I > > doubt that. > > Leaks are my only motivation against storing a reference. > > > If we can't keep a strong element reference from the constructor I would > > rather use a weak reference than use querySelector every time as it should > > be more performant. OK, I'll try switch to a weakref if it leaks for me.
Flags: needinfo?(jaws)
<address-picker> works for me. "selector" sounds too similar to "select".
Flags: needinfo?(jaws)
Comment on attachment 8941968 [details] Bug 1387221 - Connect the shipping address picker with autofill address storage. https://reviewboard.mozilla.org/r/212156/#review219832 clearing review for now since i know this is still a wip for the tests. all else looks good so far. ::: toolkit/components/payments/test/mochitest/mochitest.ini:3 (Diff revision 5) > [DEFAULT] > +prefs = > + dom.webcomponents.customelements.enabled=false Were you getting memory leaks with this pref enabled? I would prefer that we get test coverage on the DOM custom elements ASAP so we can get issues fixed sooner. Potentially we could run the tests twice, once with the pref enabled and once without. ::: toolkit/components/payments/test/mochitest/test_address_picker.html:51 (Diff revision 5) > + > +add_task(async function test_initialSet() { > +// TODO: check initially empty > + picker1.requestStore.setState({ > + savedAddresses: { > +"13gnb8dj5": { indentation here and below looks messed up.
Attachment #8941968 - Flags: review?(jaws)
Comment on attachment 8941968 [details] Bug 1387221 - Connect the shipping address picker with autofill address storage. https://reviewboard.mozilla.org/r/212156/#review219844 ::: toolkit/components/payments/res/components/address-option.js:59 (Diff revision 5) > if (!this.parentNode) { > return; > } > > - this.querySelector(".recipient").textContent = this.recipient; > - this.querySelector(".addressLine").textContent = > + this.querySelector(".name").textContent = this.name; > + this.querySelector(".street-address").textContent = `${this.streetAddress} ${this.addressLevel2} ` + This causes an eslint error for having more than 100 columns of characters (currently 105).
Comment on attachment 8941968 [details] Bug 1387221 - Connect the shipping address picker with autofill address storage. https://reviewboard.mozilla.org/r/212156/#review220338 Thanks for fixing the issues I had pointed out. With respect to the disabling of the custom elements pref, I am OK with doing so and continuing to use our shim for now until we have bug 1413418 fixed and we see more activity on the custom elements implementation.
Attachment #8941968 - Flags: review?(jaws) → review+
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/3139e03e1335 Connect the shipping address picker with autofill address storage. r=jaws
Comment on attachment 8944974 [details] Bug 1387221 - Use registerCleanupFunction not on 'SimpleTest' to workaround bug 1432690. https://reviewboard.mozilla.org/r/215120/#review220730
Attachment #8944974 - Flags: review?(jaws) → review+
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/3a416a44273e Use registerCleanupFunction not on 'SimpleTest' to workaround bug 1432690. r=jaws
https://hg.mozilla.org/mozilla-central/rev/3139e03e1335 https://hg.mozilla.org/mozilla-central/rev/3a416a44273e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Whiteboard: [webpayments]
Product: Toolkit → Firefox
Target Milestone: mozilla60 → Firefox 60
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: