add paymentmethodchange event #695

marcoscaceres · 2018-03-22T05:51:43Z

closes #662

The following tasks have been completed:

Confirmed there are no ReSpec errors/warnings.
Added Web platform tests
added MDN Docs
added MDN compat data

Implementation commitment:

Safari (link to issue)
Chrome
Firefox
Edge (public signal)

Preview | Diff

rsolomakhin

Does this change affect ShippingAddressUpdateEvent and ShippingOptionUpdateEvent?

marcoscaceres · 2018-03-22T14:16:09Z

Only indirectly and in a backwards compatibile way. I hope to have a draft ready for review tomorrow.

rsolomakhin · 2018-03-22T19:57:27Z

No rush! Just checking.

marcoscaceres · 2018-04-30T05:51:33Z

Basic card integration w3c/payment-method-basic-card#53

domenic

Some issues, in particular confusion about which payment handler is being referenced.

domenic · 2018-04-30T17:05:44Z

index.html

+          <li>Let <var>methodDetails</var> be null.
+          </li>
+          <li>If <var>request</var>'s <a>payment handler</a> defines a <a>user
+          changes payment method</a> algorithm, set <var>methodDetails</var> be


This link is not going anywhere in the output. It should probably be a <dfn>?

domenic · 2018-04-30T17:06:03Z

index.html

+          changes payment method</a> algorithm, set <var>methodDetails</var> be
+          a <a data-cite="!WEBIDL#idl-dictionary">dictionary</a> or
+          <a data-cite="!WEBIDL#idl-object">object</a> resulting the user
+          selecting a different <a>payment method</a>.


Shouldn't this be "resulting from running that algorithm"?

domenic · 2018-04-30T17:07:14Z

index.html

+          selecting a different <a>payment method</a>.
+          </li>
+          <li>Let <var>methodName</var> be the <a>payment method identifier</a>
+          of the <a>payment handler</a> the user is interacting with.


Is "the payment handler the user is interacting with" different from "request's payment handler"?

Actually, given that the user is potentially switching payment handlers, is the original or the new one "request's payment handler"? Is the original or the new one "the payment handler the user is interacting with"?

domenic · 2018-04-30T17:12:20Z

index.html

          Promise&lt;boolean&gt; canMakePayment();

          readonly attribute DOMString id;
+          readonly attribute object? methodDetails;


A good test to write will be that pr.methodDetails === pr.methodDetails. The spec makes this requirement clear but it's something implementers sometimes miss.

Will need to test this with Basic Card.

Added the test you suggested:
https://github.com/w3c/web-platform-
tests/pull/10912/commits/29a384b326fc514da854a9041cdfbde13e0eee54

marcoscaceres · 2018-05-09T08:19:24Z

Sent tests for review web-platform-tests/wpt#10912

marcoscaceres · 2018-05-10T05:32:08Z

Realized that when other events fire (e.g., onshippingaddresschange) or generally things happen on the user interaction task source (e.g., user aborts, request.abort()), the methodDetails needs to be reset to null... same with methodName.

marcoscaceres · 2018-05-10T07:41:32Z

Hmmm... having spec'ed this out, now I don't know if I like this design anymore 😓 Doesn't feel right that the only thing that changes .methodName and .methodData is this event.

Maybe this should switch back to having special event for this, as per #662 (comment)

The name paymentmethodchange is also super ambiguous tho, because it's really a "Instrument change" of a payment method. So, I I'm kind thinking "instrumentchange" is more appropriate as an event name.

I think I'd like to change this to:

PaymentMethodChangeEvent : PaymentRequestUpdateEvent {
   readonly attribute DOMString methodName;
   readonly attribute object? methodDetails;
}

So that:

request.oninstrumentchange = ev => {
  const { type: cardType } = ev.methodDetails;
  if (ev.methodName === "https://apple.com/apple-pay") {
    switch (cardType) {
      case "store":
        // do Apple Pay specific handling for store card...
        break;
    }
  }
  // finally...
  ev.updateWith(newStuff);
};

@aestes wdyt?

marcoscaceres · 2018-05-19T02:18:28Z

Updated this to match #695 (comment)

marcoscaceres · 2018-05-23T00:18:16Z

Updated WPTests to match updated spec.

w3c/payment-request#695 (review)

marcoscaceres · 2018-06-05T07:56:39Z

@domenic, this one too should now be good to go (🤞).

domenic

Confused how the payment method change steps are supposed to be implemented...

domenic · 2018-06-05T22:35:19Z

index.html

+        </h2>
+        <p>
+          The user agent MUST run the <dfn>payment method changed
+          algorithm</dfn> runs when the user changes <a>payment method</a>. The


extra word "runs"

domenic · 2018-06-05T22:36:12Z

index.html

+          <a data-cite="!WEBIDL#idl-object">object</a> or null, and a
+          <var>methodName</var>, which is a DOMString that represents the
+          <a>payment method identifier</a> of the <a>payment handler</a> the
+          user is interacting with:


How does the user agent determine methodName and methodDetails?

Will rephrase this, as it's actually the Payment Handler that needs to invoke this. From discussions over in Basic Card, we are going to make this optional... so Basic Card won't fire this, but Apply Pay will.

domenic · 2018-06-05T22:36:48Z

index.html

+              further action. The <a>user agent</a> user interface SHOULD
+              ensure that this never occurs.
+              </li>
+              <li data-link-for="PaymentMethodChangeEvent">Fire an event


Ideally "fire an event" should be linked

domenic · 2018-06-05T22:39:45Z

index.html

+          };
+        </pre>
+        <section>
+          <h2>


Soooo I know it goes against all previous advice and good sense, but currently for events we don't use internal slots :(. Instead we use the old-style, terrible, "When getting, returns the value it was initialized with". I think making this less terrible is mostly tracked by whatwg/dom#364, /cc @annevk. But in the end we do want to end up in a world that doesn't require people to explicitly define slots and return them for every event property, so I would take the slots out for now. Especially since nothing in this spec sets them.

Um, ok... um... we kinda added internal slots PaymentRequestUpdateEvent also.

I guess I should remove and rephrase those too?

No, that one makes sense, as its not exposed. It's the ones that are 1:1 with public properties that are less-good.

ah, got it. Ok :)

marcoscaceres · 2018-06-12T07:02:33Z

@domenic, I think I've addressed all the feedback 🤞

domenic

LGTM with nits

domenic · 2018-06-12T15:12:15Z

index.html

+          </h2>
+          <p data-link-for="PaymentMethodChangeEventInit">
+            When getting, returns the value it was initialized with. See
+            <a>methodDetails</a> member for more information.


Here and below, "member of PaymentMethodChangeEventInit" perhaps? Otherwise before you click on it it looks like it'll just link to itself.

domenic · 2018-06-12T15:13:55Z

index.html

+              ensure that this never occurs.
+              </li>
+              <li data-link-for="PaymentMethodChangeEvent">
+                <a>Fire an event</a> "<a>paymentmethodchange</a>" at


Fire an event named

marcoscaceres · 2018-06-12T16:48:52Z

Added link to Firefox tracking bug.

w3c/payment-request#695 (review)

marcoscaceres · 2018-06-21T15:51:39Z

Added developer documentation. Please review. https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/onpaymentmethodchange

marcoscaceres · 2018-07-02T18:10:21Z

PaymentMethodChangeEvent is now in Firefox Nightly behind a pref. We don't currently have any plans to fire it for Basic Card, but other payment handlers could theoretically make use of it.

marcoscaceres force-pushed the paymentmethodchanged branch from d26aeed to fc132b8 Compare March 22, 2018 05:52

rsolomakhin reviewed Mar 22, 2018

View reviewed changes

marcoscaceres requested a review from aestes April 19, 2018 07:47

marcoscaceres requested a review from domenic April 30, 2018 05:51

domenic requested changes Apr 30, 2018

View reviewed changes

domenic reviewed Apr 30, 2018

View reviewed changes

marcoscaceres mentioned this pull request May 2, 2018

Retrying a payment #705

Closed

marcoscaceres force-pushed the paymentmethodchanged branch from 5915441 to 5587596 Compare May 9, 2018 07:04

marcoscaceres mentioned this pull request May 9, 2018

PaymentRequest "paymentmethodchanged" event web-platform-tests/wpt#10912

Merged

marcoscaceres mentioned this pull request May 9, 2018

User changes payment method w3c/payment-method-basic-card#53

Merged

1 task

marcoscaceres changed the title ~~WIP: add paymentmethodchanged event~~ add paymentmethodchanged event May 10, 2018

marcoscaceres changed the title ~~add paymentmethodchanged event~~ add paymentmethodchange event May 10, 2018

marcoscaceres force-pushed the paymentmethodchanged branch 2 times, most recently from 134fd66 to a863d31 Compare May 10, 2018 04:58

marcoscaceres force-pushed the paymentmethodchanged branch from d00e49b to edb52f8 Compare May 10, 2018 06:45

marcoscaceres added a commit to web-platform-tests/wpt that referenced this pull request May 23, 2018

Add test suggested by @domenic

29a384b

w3c/payment-request#695 (review)

marcoscaceres force-pushed the paymentmethodchanged branch from a520c57 to 87f22bd Compare May 31, 2018 02:11

domenic requested changes Jun 5, 2018

View reviewed changes

marcoscaceres force-pushed the paymentmethodchanged branch from 5801ded to 47db0ee Compare June 6, 2018 02:19

domenic approved these changes Jun 12, 2018

View reviewed changes

marcoscaceres added 19 commits June 12, 2018 09:39

WIP: add paymentmethodchanged event (closes #662)

220eedd

WIP

6983c75

wip

0c7fc04

Move attributes to PaymentRequest

7b3fc32

add PR.onpaymentmethodchanged

18a4131

fixup hook terminology

9967b58

fix respec warning

c0a961f

Fixup algorithms/steps mess

dbaaa63

s/paymentmethodchanged/paymentmethodchange

1837a76

Null out methodName + methodDetails when things change

4cb9a43

link tests

f492a24

Defining payment method changed steps is optional

aa7f155

use dot notation for internal slots

68217a3

refactor to PaymentMethodChangeEvent

dfcb317

cleaup

6f28026

method name passed as an argument

e535034

review nits/typos

04bc08e

review feedback

bd8a06e

review fixes

6a79bfb

marcoscaceres force-pushed the paymentmethodchanged branch from 47db0ee to 6a79bfb Compare June 12, 2018 16:41

marcoscaceres added a commit to web-platform-tests/wpt that referenced this pull request Jun 12, 2018

Add test suggested by @domenic

773c20a

w3c/payment-request#695 (review)

marcoscaceres merged commit b336891 into gh-pages Jun 21, 2018

marcoscaceres deleted the paymentmethodchanged branch June 21, 2018 15:55

add paymentmethodchange event #695

add paymentmethodchange event #695

Uh oh!

Conversation

marcoscaceres commented Mar 22, 2018 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rsolomakhin left a comment

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Mar 22, 2018

Uh oh!

rsolomakhin commented Mar 22, 2018

Uh oh!

marcoscaceres commented Apr 30, 2018

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented May 9, 2018

Uh oh!

marcoscaceres commented May 10, 2018

Uh oh!

marcoscaceres commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcoscaceres commented May 19, 2018

Uh oh!

marcoscaceres commented May 23, 2018

Uh oh!

marcoscaceres commented Jun 5, 2018

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Jun 12, 2018

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Jun 12, 2018

Uh oh!

marcoscaceres commented Jun 21, 2018

Uh oh!

marcoscaceres commented Jul 2, 2018

Uh oh!

marcoscaceres commented Mar 22, 2018 •

edited by pr-preview bot

Loading

marcoscaceres commented May 10, 2018 •

edited

Loading