Remove modifiers

[marcoscaceres]

This includes #955 ... will rebase this once it's merged. Don't review this yet.

closes #???

The following tasks have been completed:

• [ ] Confirmed there are no ReSpec errors/warnings.
• [ ] Modified Web platform tests (link)
• [ ] Modified MDN Docs (link)
• [ ] Has undergone security/privacy review (link)

Implementation commitment:

• [ ] Safari (link to issue)
• [ ] Chrome (link to issue)
• [ ] Firefox (link to issue)
• [ ] Edge (public signal)

Optional, impact on Payment Handler spec?

---

Preview | Diff

[stephenmcgruer]

I don't believe we can remove modifiers.

Google Pay currently uses the modifiers to pass transaction details, and to handle updates from the merchant if something changes (e.g. the user changes the shipping address). This is done via the onpaymentmethodchange callback, and the updateWith method on the PaymentMethodChangeEvent it passes.

You can see this via this test page https://output.jsbin.com/xotoxog (which is a copy of https://developers.google.com/pay/api/web/guides/resources/demos#dynamic-price-update-example). If one examines pay.js and looks for modifiers, you'll find the code using them. Dropping a breakpoint into the pretty-printed source and playing with the demo, I saw:

Initial clicking of button:

Updating the shipping method:

[marcoscaceres]

It might still be ok for Google Pay to use them (and for Chrome to continue to support them), if they are just passed as part of method data. However, I'm still wondering if we can remove them from the spec.

[ianbjacobs]

@stephenmcgruer, do you have any thoughts on @marcoscaceres' comment?

[stephenmcgruer]

Apologies @marcoscaceres , I don't quite follow the suggestion (likely due to my lack of knowledge of the PR and PH specs). Currently, I believe pay.js depends on the following setup 'working' (this is all rough pseudo-code, not real code):

/* Inside the PaymentHandler */
handleUserChangingAddress() {
  data = getShippingAddressDetails(); // a JS dict with new shipping address
  const promise = paymentRequestEvent.changePaymentMethod("https://google.com/pay", data);
  promise.then(response => {
    // Here, 'data' contains the information from the merchant on new shipping options.
    updateUIWith(response.modifiers[0].data);
  });
}

/* In the merchant page */
pr.onpaymentmethodchange = function(e) {
   // e.methodDetails here is effectively the dict created by getShippingAddressDetails() above.
  data = getNewShippingOptionsFor(e.methodDetails)
  e.updateWith({
    modifiers: [{
      supportedMethods: ["https://google.com/pay"],
      data: data
    }]
  }
});
}

(Interestingly supportedMethods seems to be passed as a sequence here, but if I drop a breakpoint in the service-worker JS it's a string by that point. Maybe a quirk of the Chromium implementation, I'm not sure.)

Can you be more specific on how you think things should change, Marcos? To be clear, from the Chrome point of view we would consider it a web-compat issue if the spec changes such that the above javascript code doesn't work by spec, and we would be unlikely to follow the spec change. But I'd like to discuss and understand your thoughts :).

[marcoscaceres]

Can you be more specific on how you think things should change, Marcos? To be clear, from the Chrome point of view we would consider it a web-compat issue if the spec changes such that the above javascript code doesn't work by spec, and we would be unlikely to follow the spec change. But I'd like to discuss and understand your thoughts :).

So, similarly to what we did with MerchantValidation, we could either relocate modifiers to a Note: https://www.w3.org/TR/merchant-validation/

Alternatively, we could just add a note to the spec and say they are deprecated for future Payment Handlers.

The main worry is that other payment handlers apart from Google Pay will try to use them... they are quite messy and add largely needless complexity.

[ianbjacobs]

+1 to deprecating them

[stephenmcgruer]

It would be quite unfortunate if Firefox one day shipped support for PaymentRequest and PaymentHandler, and Google Pay were 'punished' for writing their code to a valid version of a spec because we felt something was was messy and had removed it. (I.e. Firefox would not pass the 'modifiers' through to the app, and so it would break)

If we're going to do that, I feel the bare minimum is there being an alternative path for GPay to migrate to, that would work in a browser that was just following the spec. Maybe there is today, but I'm not clear what it would be (sorry, I lack a background knowledge in PR and PH).

EDIT: To be clear, adding a note saying it is deprecated is totally fine by me but feels like a compromise that doesn't really achieve your goals (and your goals aren't unreasonable). I'm more speaking to any case where we make this use-case unachievable by the spec.

[marcoscaceres]

It would be quite unfortunate if Firefox one day shipped support for PaymentRequest and PaymentHandler, and Google Pay were 'punished' for writing their code to a valid version of a spec because we felt something was was messy and had removed it. (I.e. Firefox would not pass the 'modifiers' through to the app, and so it would break)

That presupposes quite a few things - in particular, that GPay couldn't be updated to not use modifiers and achieve the same functionality. However, it's fair to ask, "how could GPay be updated to work without the use of modifiers?"

Again looking at the code above, I concede that the use of .data serves the purpose of communicating out-of-band things (e.g., "'data' contains the information from the merchant on new shipping options").

A future version of the specification should work like this:

/* In the merchant page */
pr.onpaymentmethodchange = function(e) {
  const data = getNewShippingOptionsFor(e.methodDetails)
  // e is already coming from a `methodName` (e.g., GPay), so: 
  e.updateWith({ data });
}

As modifiers are just being used to hack around a flaw in the spec (I can't think of a reason why one would modify multiple payment methods at once).

[marcoscaceres]

Closing, based on the above. Let's come back to this after REC.