Notify the web page with shippingaddresschange when the user finishes editing/adding an address and wait for the response from the merchant to see if it's acceptable. Show the error message string if it's provided. Another bug will handle the field-specific errors.

I'm trying to figure out if this depends on bug 1443735. AFAICT there is an open question on how to both create a friction-less UX and also avoid surprises regarding leaking of address info to the merchant. This may mean changes to when/where we dispatch the shippingaddresschange event, but we may not have a definitive answer for some time. Meanwhile, we do want an initial front-end implementation of this to exercise the DOM implementation and have a way of surfacing errors. So, I'm inclined to work up an initial patch with the understanding that the UX spec may change and we could need to re-work or even throw out this work eventually. I've amended the bug title, hopefully I'm understanding this right. Let me know if not.

Notes: Currently, we dispatch the shippingaddresschange event in the stateChangeHandler for the payment-dialog. This means an event is dispatched when the address-picker is populated, and any time thereafter when the guid in state.selectedShippingAddress changes. That initial dispatch is part of the issue that bug 1443735 is concerned with - it happens without input from the user: we load addresses from the profile and push them into the picker, automatically select the first and send this selected address off to the merchant. One quick fix here pending the outcome of bug 1443735 is to always keep the "None selected" option in the picker and default to it. It then requires the user to explicitly change the selected option before any address data is send to the merchant page. If an address gets changed in the add/edit page, as I understand it the guid does *not* change. But we do need to treat this as an address change as any of the fields - including country - could be different.

While testing a WIP patch for this, I noticed that if we *un-set* selectedShippingAddress, changeShippingAddress in the payment-dialog ends up sending null up to the frame script where an exception is thrown when formAutofillStorage.addresses.get(guid) returns falsey. This seems like a bug? Particularly in the light of bug 1443735, we should be able to clear the selectedShippingAddress without causing an un-caught exception to be thrown. Glancing over the DOM code that should dispatch shippingaddresschange, I'm not clear we have support for this scenario there either?

(In reply to Sam Foster [:sfoster] from comment #2) > Notes: Currently, we dispatch the shippingaddresschange event in the > stateChangeHandler for the payment-dialog. To be clear, we signal the change via the frame script which ultimately ends up with the shippingaddresschange event being dispatched on the web page.

Is the ability to unset the shipping address a matter of all UI selectors having to be reversible as a matter of principle? Is there any actual use case for unsetting the shipping address? As a privacy matter, the user can't make the site unsee a shipping address. I'd be inclined to violate the principle of making all UI selectors reversible and make unsetting the shipping address impossible in the UI in the interest of avoiding the creation of Gecko-specific Web-visible event dispatch scenarios which are likely to become Web compat problems.

(In reply to Henri Sivonen (:hsivonen) (away from Bugzilla until 2018-04-03) from comment #5) > Is the ability to unset the shipping address a matter of all UI selectors > having to be reversible as a matter of principle? > > Is there any actual use case for unsetting the shipping address? As a > privacy matter, the user can't make the site unsee a shipping address. Yes. If the user deletes the address they have selected (e.g. via options/preferences) rather than auto-select the next one in the list, I think we should leave it null/empty. Which for the merchant constitutes a shippingaddresschange they'll need to know about. There is also the possibility the user could remove all saved addresses while mid payment flow. We could just not signal the address change until the user enters and/or selects a new one. But in the meantime the costs and total they are looking at may not be accurate. If we elect to not auto-select an address when first entering the payment flow to avoid leaking info, ISTM we should be able to return to this state.

Comment on attachment 8963691 [details] Bug 1427958 - Check modified dates when determining if the shipping address has changed. https://reviewboard.mozilla.org/r/232588/#review237986 Code analysis found 2 defects in this patch: - 2 defects 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/payment-dialog.js:238 (Diff revision 1) > super.stateChangeCallback(state); > > // Don't dispatch change events for initial selectedShipping* changes at initialization > // if requestShipping is false. > if (state.request.paymentOptions.requestShipping) { > - if (state.selectedShippingAddress != this._cachedState.selectedShippingAddress) { > + log.debug("PaymentDialog stateChangeCallback, state.selectedShippingAddress: " + state.selectedShippingAddress); Error: Line 238 exceeds the maximum line length of 100. [eslint: max-len] ::: toolkit/components/payments/res/containers/payment-dialog.js:239 (Diff revision 1) > > // Don't dispatch change events for initial selectedShipping* changes at initialization > // if requestShipping is false. > if (state.request.paymentOptions.requestShipping) { > - if (state.selectedShippingAddress != this._cachedState.selectedShippingAddress) { > + log.debug("PaymentDialog stateChangeCallback, state.selectedShippingAddress: " + state.selectedShippingAddress); > + let cacheEntry = state.selectedShippingAddress && this._cachedState.get(state.selectedShippingAddress); Error: Line 239 exceeds the maximum line length of 100. [eslint: max-len]

Does the spec currently allow us dispatching a change to no address? If not, we should raise a spec issue via Marcos. A privacy concern I have that kinda affects this bug is that it may not be clear that changing the PR-selected address in preferences while the payment dialog is open would immediately notify the merchant with the open PR. Perhaps we should only dispatch the change if/while the PR dialog is focused? I've added this to https://wiki.mozilla.org/Firefox/Features/Web_Payments/Privacy_%26_Security_Considerations to make sure we don't forget about it. We should probably write a test for it eventually to ensure we don't regress that.

Comment on attachment 8963691 [details] Bug 1427958 - Check modified dates when determining if the shipping address has changed. https://reviewboard.mozilla.org/r/232588/#review239824 ::: toolkit/components/payments/res/containers/payment-dialog.js:144 (Diff revision 2) > setStateFromParent(state) { > this.requestStore.setState(state); Rather than making a more complicated `_cachedState`, did you consider comparing the new `state` to the old state (via `getState()`) before the `setState` call? At that point in time I think you have enough information to compare the old and new addresses.

Comment on attachment 8963691 [details] Bug 1427958 - Check modified dates when determining if the shipping address has changed. https://reviewboard.mozilla.org/r/232588/#review239824 > Rather than making a more complicated `_cachedState`, did you consider comparing the new `state` to the old state (via `getState()`) before the `setState` call? At that point in time I think you have enough information to compare the old and new addresses. I didn't. I tried this out and its quite a bit simpler so I'm updating the patch. I don't think the last-modified/cache thing is wrong per-se, but possibly not warranted here.

Comment on attachment 8963691 [details] Bug 1427958 - Check modified dates when determining if the shipping address has changed. https://reviewboard.mozilla.org/r/232588/#review240222 ::: commit-message-b9060:1 (Diff revision 3) > +Bug 1427958 - Check modified dates when determining if the shipping address has changed. r?MattN This commit message is currently stale but I think a solution to the Canonical JSON issue is to go back to using dates instead of serializing… it's also much more performant though it's not in a super hot path. ::: toolkit/components/payments/res/containers/payment-dialog.js:132 (Diff revision 3) > // Ensure `selectedShippingAddress` never refers to a deleted address and refers > - // to an address if one exists. > - if (!savedAddresses[selectedShippingAddress]) { > + // to an address if one exists. We also compare address contents to handle changes > + // made outside the payments UI. > + if (shippingAddress) { > + // invalidate the cached value if the address changed > + if (JSON.stringify(shippingAddress) != JSON.stringify(oldShippingAddress)) { I didn't think there was a guarantee two objects which are identical for our purposes would be serialized in the same way e.g. the key order may differ. This is why we have CanonicalJSON.jsm which unfortunately isn't available in this context. I'm not sure if it's currently an issue in practice but it doesn't seem great to rely on this. Please confirm that your test would cause a failure if this changes. ::: toolkit/components/payments/test/browser/head.js:228 (Diff revision 3) > +add_task(async function setup_head() { > + setupFormAutofillStorage(); Don't we still need to `await` on the return value of `setupFormAutofillStorage` so it doesn't race with the next task?

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #14) > I didn't think there was a guarantee two objects which are identical for our > purposes would be serialized in the same way e.g. the key order may differ. > This is why we have CanonicalJSON.jsm which unfortunately isn't available in > this context. I'm not sure if it's currently an issue in practice but it > doesn't seem great to rely on this. Please confirm that your test would > cause a failure if this changes. Yeah, I had started writing/looking around for a deepEqual() function to do the comparison, but settled on the stringified equality check as an expedient solution. Maybe rather than comparing each field in the address object, something like: (newAddress.guid == oldAddress.guid && newAddress.lastModifiedDate == oldAddress.lastModifiedDate) ..as an equality check? > ::: toolkit/components/payments/test/browser/head.js:228 > (Diff revision 3) > > +add_task(async function setup_head() { > > + setupFormAutofillStorage(); > > Don't we still need to `await` on the return value of > `setupFormAutofillStorage` so it doesn't race with the next task? Yes, thanks for catching that.

(In reply to Sam Foster [:sfoster] from comment #15) > (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from > comment #14) > > I didn't think there was a guarantee two objects which are identical for our > > purposes would be serialized in the same way e.g. the key order may differ. > > This is why we have CanonicalJSON.jsm which unfortunately isn't available in > > this context. I'm not sure if it's currently an issue in practice but it > > doesn't seem great to rely on this. Please confirm that your test would > > cause a failure if this changes. > > Yeah, I had started writing/looking around for a deepEqual() function to do > the comparison, but settled on the stringified equality check as an > expedient solution. Maybe rather than comparing each field in the address > object, something like: > > (newAddress.guid == oldAddress.guid && newAddress.lastModifiedDate == > oldAddress.lastModifiedDate) > > ..as an equality check? Yeah, I think that's fine as storage should always change the lastModifiedDate.

Comment on attachment 8963691 [details] Bug 1427958 - Check modified dates when determining if the shipping address has changed. https://reviewboard.mozilla.org/r/232588/#review240332 Thanks

Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/96426862f7af Check modified dates when determining if the shipping address has changed. r=MattN

Updating the summary to be more correct. Bug 1429189 handled showing shippingaddresschange errors on the summary screen.