Dont block UI if .updateWith() not called (closes #589) #591

marcoscaceres · 2017-08-22T07:08:06Z

domenic · 2017-08-24T16:51:46Z

index.html

@@ -2417,6 +2422,9 @@
            <li>If <var>event</var>'s <a>isTrusted</a> attribute is false, then
            then <a>throw</a> a "<a>InvalidStateError</a>" <a>DOMException</a>.
            </li>
+            <li>If <var>event</var>.<a>[[\didUpdate]]</a> is true, then


So this would happen if someone calls updateWith(p), p settles, and then someone calls updateWith(q), right?

Whereas the next line would happen if someone calls updateWith(p), and before p settles, someone calls updateWith(q)?

If I got that right, then I'd suggest adding quick examples or notes inside these steps re-stating the above.

So this would happen if someone calls updateWith(p), p settles, and then someone calls updateWith(q), right?

I was thinking of this case:

request.onaddresschange = ev => { Promise.resolve({...details}) // next tick, so [[didUpdate]] is now true. Throws InvalidStateError. .then(ev.updateWith.bind(ev)); }

Whereas the next line would happen if someone calls updateWith(p), and before p settles, someone calls updateWith(q)?

Correct. It also prevents someone holding a reference to event and calling .updateWith(p) after p has settled. The problem in the current spec is that [[\waitForUpdate]] and request.[[updating]] are both reset to false in step 14 of updateWith() - so theoretically, a developer could have just waited like this:

request.onaddresschange = async ev => { const p = Promise.resolve({...details}); ev.updateWith(p); await p; // [[waitForUpdate]] and request.[[updating]] are both reset to `false`! // [[didUpdate]] is true, this now throws const q = Promise.resolve({...details}); ev.updateWith(q); }

Fixed first case above...

Ok, added example above.

domenic · 2017-08-24T16:52:03Z

index.html

-          <li>
-            <a>Fire an event</a> named <var>name</var> at <var>request</var>
-            using <a>PaymentRequestUpdateEvent</a>.
+          <li>Let <var>event</var> be the result of <a>fire an event</a> named


This doesn't work. Fire an event returns a boolean, not an event.

You should instead:

Let event be the result of creating an event using Event.

Initialize event's type attribute to name

Dispatch event at target.

This doesn't work. Fire an event returns a boolean, not an event.

Argh, I should have remembered that. I've used that boolean before elsewhere.

domenic · 2017-08-24T16:52:23Z

index.html

+          <var>event</var>.<a>[[\waitForUpdate]]</a> is true, disable any part
+          of the user interface that could cause another update event to be
+          fired. Wait for <var>event</var>.<a>updateWith()</a> method's
+          <var>detailsPromise</var> to settle.


", then re-enable the relevant parts of user interface", I think.

Maybe this should be a series of nested steps under the "If ... is true".

.updateWith() controls re-enabling the user interface, in step 14. It might be redundant to re-state it here.

Ah ok, in that case, agreed.

domenic · 2017-08-25T20:45:04Z

index.html

-          </p>
+          <aside class="note">
+            <p>
+              If a developer wants to update the payment request, then they


I'm just noticing that this repeats text from slightly above. Let's delete the above text, as it feels pretty out of place anyway.

deleted the one above.

domenic · 2017-08-25T20:46:50Z

index.html

+              };
+
+              // ✅ Good - UI will wait.
+              request.onshippingaddresschange = async ev =&gt; {


No async here

domenic · 2017-08-25T20:47:21Z

index.html

+
+              // ✅ Good - UI will wait.
+              request.onshippingaddresschange = async ev =&gt; {
+                //  This is fine, of course.


"of course" is a bit aggressive and implies the reader understands what's going on. You shouldn't really say "of course" while teaching someone something like this, IMO.

domenic · 2017-08-25T20:47:43Z

index.html

+              // ❌ Bad - this won't work!
+              request.onshippingaddresschange = async ev =&gt; {
+                // `await` goes to next tick, so [[didUpdate]] is now true.
+                const details = await Promise.resolve({ ...details, ...updatedProps });


Let's do await getNewDetails(oldDetails) to mirror the below.

domenic · 2017-08-25T20:48:09Z

index.html

+              <a>[[\didUpdate]]</a> becomes true and <a>updateWith()</a>
+              will throw.
+            </p>
+            <pre class="example" title="how to use `updateWith()` correctly.">


Backticks don't work in example titles. Respec bug? Or just remove it for now?

Same below.

I plan to add support for this in ReSpec, so will leave it for now and we will get this for free soon.

domenic · 2017-08-25T20:49:06Z

index.html

+            </p>
+            <pre class="example" title="can only call `upadteWith()` once">
+              // ❌ Bad - calling updateWith() doesn't work!
+              request.addEventListener("shippingaddresschange", async ev =&gt; {


domenic · 2017-08-25T20:54:52Z

index.html

+            <pre class="example" title="how to use `updateWith()` correctly.">
+              // ❌ Bad - this won't work!
+              request.onshippingaddresschange = async ev =&gt; {
+                // `await` goes to next tick, so [[didUpdate]] is now true.


To distinguish this from the last example, we should say something like "await goes to next tick, and updateWith() was not called before that, so [[didUpdate]] gets set to true".

Although that calls into question whether didUpdate is the right name, since nothing actually updated.

I know... "didUpdate" is not a great name... I wanted something that denoted "consumed" or "expired" or "this can't be used again". I'm open to suggestions.

Could flip it and go with "canUpdate" or "updatePermitted" and turn it off after the opportunity to update is lost or has already been taken.

Although that calls into question whether didUpdate is the right name, since nothing actually updated.

Rethinking this: it actually did update... but instead of using the developer's details, the sheet used the details that were provided by the payment sheet itself. So maybe updatedUI might be better.

@dlongley, I'd prefer not to flip it. I don't want to rewrite it :(

domenic · 2017-08-25T20:55:31Z

index.html

+                const p = Promise.resolve({ ...details });
+                ev.updateWith(p);
+                await p;
+                // 💥 [[didUpdate]] is now true, throws "InvalidStateError".


To distinguish this from the first example, we can say "the update has now completed, so [[didUpdate]] gets set to true; this throws "InvalidStateError".

domenic · 2017-08-25T20:57:25Z

index.html

+          "!DOM#concept-event-create">creating an event</a> using
+          <a>PaymentRequestUpdateEvent</a>.
+          </li>
+          <li>Initialize <var>event</var>'s type attribute to <var>name</var>.


Ideally link type, or at least <code>ify it.

domenic · 2017-08-25T20:59:29Z

index.html

+          <var>event</var>.<a>[[\waitForUpdate]]</a> is true, disable any part
+          of the user interface that could cause another update event to be
+          fired. Wait for <var>event</var>.<a>updateWith()</a> method's
+          <var>detailsPromise</var> to settle.


event.updateWith()'s detailsPromise is pretty terrible. Can we store it in an internal slot?

Actually, what is the point of this "wait" step? Nothing happens after it. I think it should just be removed.

Agree. I wasn't sure if it would be clear. But if one follows the updateWith() path, it provides all the steps there.

zkoch · 2017-08-25T21:23:30Z

Is there a TL;DR for what we're trying to accomplish here? It's not obvious to me. :)

marcoscaceres · 2017-08-26T03:04:14Z

@zkoch, tldr:

Don't block the payment sheet if the developer doesn't call .updateWith().
Don't allow developers to keep recycling the event (e.g., keep a reference to a shipping change event, and then use it to update the the payment sheet when an option change event happens - see code below).

===

Note to self, add this test:

var methods = [{ supportedMethods: ["basic-card"] }];
var details = {
  total: { label: "", amount: { currency: "USD", value: "1" } },
};
var shippingOptions = [
  {
    id: "standard",
    label: "🚛 Ground Shipping (2 days)",
    amount: { currency: "USD", value: "5.00" },
    selected: true,
  },
  {
    id: "drone",
    label: "🚀 Drone Express (2 hours)",
    amount: { currency: "USD", value: "25.00" },
  },
];
details.shippingOptions = shippingOptions;
var r = new PaymentRequest(methods, details, { requestShipping: true });
r.addEventListener(
  "shippingaddresschange",
  e => {
    ev = e;
    ev.updateWith(details);
    var listener = ev2 => {
      var newDetails = { ...details };
      newDetails.total.label = "FAIL" + Math.random();
      ev.updateWith(newDetails);
    };
    r.addEventListener("shippingoptionchange", listener);
    r.addEventListener("shippingaddresschange", listener);
  },
  { once: true }
);
r.show();

domenic

LGTM with one remaining nit; please don't wait on another round of review before merging :).

domenic · 2017-08-31T00:26:23Z

index.html

+          <a>PaymentRequestUpdateEvent</a>.
+          </li>
+          <li>Initialize <var>event</var>'s <a data-cite="!DOM#dom-event-type">
+            type</a> attribute to <var>name</var>.


Can this be code-ified and linked? :)

adrianba · 2017-08-31T22:39:45Z

I think the current spec already blocks this. The waitForUpdate slot is essentially equivalent to the new slot introduced. To avoid re-use of events, we should remove step 14, part 1, which sets waitForUpdate back to false.

marcoscaceres · 2017-09-01T04:58:57Z

I think the current spec already blocks this. The waitForUpdate slot is essentially equivalent to the new slot introduced. To avoid re-use of events, we should remove step 14, part 1, which sets waitForUpdate back to false.

Nice!

marcoscaceres · 2017-09-01T05:02:26Z

@adrianba, I removed the new slot as you suggested. Adding appropriate tests and see if this works. Will carry over domenic's R+ if it works ok.

marcoscaceres · 2017-09-05T02:58:56Z

Rebased.

marcoscaceres · 2017-09-08T03:53:18Z

Moved test requirement to WPT repo.
web-platform-tests/wpt#7288

I've got this mostly tested locally. Tests will hopefully land in the next few days.

I feel confident merging this now having tested it.

marcoscaceres requested a review from domenic August 22, 2017 07:08

marcoscaceres added needs tests proposal - needs discussion labels Aug 22, 2017

domenic requested changes Aug 24, 2017

View reviewed changes

marcoscaceres mentioned this pull request Aug 25, 2017

Empty shippingOptions (ab)used as error #595

Closed

domenic reviewed Aug 25, 2017

View reviewed changes

marcoscaceres removed the proposal - needs discussion label Aug 28, 2017

domenic approved these changes Aug 31, 2017

View reviewed changes

marcoscaceres requested a review from adrianba September 1, 2017 04:58

marcoscaceres and others added 16 commits September 5, 2017 12:58

dont block UI if .updateWith() not called (closes #589)

e4788ec

Add [[didUpdate]] internal slot

19e7e70

fixed typo

395bbc2

Simplify wording

f39ef2a

editorial: fix up updateWith() examples

dd079a4

prettier code

1da6e34

Make example a little neater

f5cef17

fix: create and dispatch event correctly

c1141d1

editorial: add .updateWith() good/bad example

12efe20

Simplify examples a little bit more

eb4bc06

make punctuation consistent

fe39adc

fixup up wording

a997192

fix whitespace in example

660b5c7

Address second round of feedback from @domenic

3b7f6f5

fix markup

94dd1e4

fix markup

8c50403

marcoscaceres added 2 commits September 5, 2017 12:58

Keep WaitForUpdate set to true as guard

06934db

Remove updatedUI from example

0e8e780

marcoscaceres force-pushed the cancel_flag branch from 4ebbb7d to 0e8e780 Compare September 5, 2017 02:58

marcoscaceres merged commit 6bc1125 into gh-pages Sep 8, 2017

marcoscaceres deleted the cancel_flag branch September 8, 2017 03:53

marcoscaceres added cr-substantive and removed needs tests labels Sep 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dont block UI if .updateWith() not called (closes #589) #591

Dont block UI if .updateWith() not called (closes #589) #591

marcoscaceres commented Aug 22, 2017 •

edited by pr-preview bot

Loading

domenic Aug 24, 2017

marcoscaceres Aug 25, 2017 •

edited

Loading

marcoscaceres Aug 25, 2017

marcoscaceres Aug 25, 2017

domenic Aug 24, 2017

marcoscaceres Aug 25, 2017

domenic Aug 24, 2017

marcoscaceres Aug 25, 2017

domenic Aug 25, 2017

domenic Aug 25, 2017

marcoscaceres Aug 28, 2017

domenic Aug 25, 2017

domenic Aug 25, 2017

domenic Aug 25, 2017

domenic Aug 25, 2017

marcoscaceres Aug 28, 2017

domenic Aug 25, 2017

domenic Aug 25, 2017

marcoscaceres Aug 28, 2017

dlongley Aug 28, 2017

marcoscaceres Aug 28, 2017

domenic Aug 25, 2017

domenic Aug 25, 2017

domenic Aug 25, 2017

marcoscaceres Aug 28, 2017

zkoch commented Aug 25, 2017

marcoscaceres commented Aug 26, 2017 •

edited

Loading

domenic left a comment

domenic Aug 31, 2017

adrianba commented Aug 31, 2017

marcoscaceres commented Sep 1, 2017

marcoscaceres commented Sep 1, 2017

marcoscaceres commented Sep 5, 2017

marcoscaceres commented Sep 8, 2017

Dont block UI if .updateWith() not called (closes #589) #591

Dont block UI if .updateWith() not called (closes #589) #591

Conversation

marcoscaceres commented Aug 22, 2017 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

marcoscaceres Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkoch commented Aug 25, 2017

marcoscaceres commented Aug 26, 2017 • edited Loading

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianba commented Aug 31, 2017

marcoscaceres commented Sep 1, 2017

marcoscaceres commented Sep 1, 2017

marcoscaceres commented Sep 5, 2017

marcoscaceres commented Sep 8, 2017

marcoscaceres commented Aug 22, 2017 •

edited by pr-preview bot

Loading

marcoscaceres Aug 25, 2017 •

edited

Loading

marcoscaceres commented Aug 26, 2017 •

edited

Loading