Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attach 'payment request is showing boolean' to top-level browsing context #811

Merged
merged 7 commits into from
Jan 16, 2019

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Nov 21, 2018

closes #809

The following tasks have been completed:

Implementation commitment:

Optional, impact on Payment Handler spec?

Yes. Payment Handlers might need to have a means of keeping track of open payment sheets.


Preview | Diff

@marcoscaceres
Copy link
Member Author

@ianbjacobs, you want to give this first read?

@rsolomakhin, we will need to consider more fully what it means for Payment Handlers. I was a bit unsure if I should file a separate bug for Handlers itself.

Copy link
Collaborator

@rsolomakhin rsolomakhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A payment handler is able to enumerate all of its own windows through the Clients API in service worker, so it can take the appropriate actions inside of the 'paymentrequest' event to either abort the current payment, abort other ongoing payments, or allow multiple ongoing payments. We should not need to add anything else to the payment handler spec for now. Does that make sense?

@marcoscaceres
Copy link
Member Author

Perfect! Thanks @rsolomakhin!

Next big question: willing to change Chrome to match Firefox? :)

@rsolomakhin
Copy link
Collaborator

Yes, sir! From what I understand, you're proposing that each browser tab have ability to show its own payment sheet. I would be willing to enable this in Chrome.

@marcoscaceres
Copy link
Member Author

Awesome! Ok, cool. I think we have all we need to move forward on this 🥳

I’ll get a review from Domenic, just to make sure I didn’t miss anything.

@marcoscaceres
Copy link
Member Author

@rsolomakhin when possible, could you provide a link to a Chrome bug. No hurry, I know you all are on vacation next few days.

@rsolomakhin
Copy link
Collaborator

Linked in description.

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs more precision. "The top-level browsing context" is never correct; there are many of them per UA, which is why we're making this change, after all. So pick specific JS objects (presumably the PaymentRequest/PaymentResponse instances) and navigate from them to top-level BCs.

index.html Outdated
<li>If the <a>user agent</a>'s <a>payment request is showing</a>
boolean is true, then return <a>a promise rejected with</a> an
"<a>AbortError</a>" <a>DOMException</a>.
<li>If the <a>top-level browsing context</a>'s <a>payment request is
Copy link
Collaborator

@domenic domenic Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the top-level browsing context" is not well-defined. Consider a same-origin popup window (i.e. two separate top-level windows which can synchronously access each other). Then call window1.PaymentRequest.prototype.show.call(window2PaymentRequestInstance). Which top-level BC is this algorithm talking about in that case? The popup window or the original window?

You want "this PaymentRequest's relevant global object's browsing context's top-level browsing context". Maybe make a definition to shorten that (e.g. "payment-relevant browsing context of obj" or "payment request is showing in the same browsing context as obj").

And, if you're feeling evil, write tests for the case I described ^_^.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I had a bad feeling about "top-level browsing context"... the suggestion is great. I'll definitely add the test 😼.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

window1.PaymentRequest.prototype.show.call(window2PaymentRequestInstance)

@domenic, just to make sure I got this right.

Both of these would theoretically show the payment sheet in the popup:

button.onclick = () => {
    const popup = window.open("some.html");
    await new Promise(resolve => (popup.onload = resolve));
    const r = new popup.PaymentRequest(methods, details);
    await r.show();
}

And:

button.onclick = () => {
    const popup = window.open("some.html");
    await new Promise(resolve => (popup.onload = resolve));
    const r = new popup.PaymentRequest(methods, details);
    await PaymentRequest.prototype.show.call(r);
}

Is that correct? ....

And then could go from popup.opener, and show a payment sheet in the opener from the popup.

I've put up an initial set that use just an iframe:
#811

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, sorry, copypasta, tests are here web-platform-tests/wpt#14297
(I've not run them - so probably buggy)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these would theoretically show the payment sheet in the popup:

Yeah, that's right!

And then could go from popup.opener, and show a payment sheet in the opener from the popup.

I don't quite understand what you mean here.

I've put up an initial set that use just an iframe:

I'll take a look, but iframes are not as good a test here, as they aren't top-level browsing contexts. The payment-relevant browsing context of a PaymentRequest created inside an iframe is its parent browsing context. Popups are special because they are top-level; they don't have a parent, just an opener.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then could go from popup.opener, and show a payment sheet in the opener from the popup.

@domenic, imagine the following situation:

// this is actually a new browser tab...
const popupTab = window.open("tab.html");

// in which tab does the sheet show? In the popup tab or in opener tab? 
await new popupTab.PaymentRequest(method, details).show();

I guess "implementation detail"...

Similarly, consider this one (little window):

// We make a little window... imagine, without toolbars, etc.
const popupWindow = window.open("window.html", "width=100,height=100,...");

// in which window does the sheet show? In the little window or in opener? 
await new popupWindow.PaymentRequest(method, details).show();

Similarly, I guess "implementation detail"?... don't think we can do much in the spec, but just wanted to point this out.

index.html Outdated
<p>
Each <a>top-level browsing context</a> has a <dfn>payment request is
showing</dfn> boolean, which prevents showing more than one payment UI
at a time.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @marcoscaceres,

Editorial suggestion for the above

    ....showing</dfn> boolean to support <a>user agent</a> management of payment UI display. For example,
   a <a>user agent</a> might choose to show only one payment UI at a time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate this suggestion, but it doesn't quite capture what we are trying to restrict here (don't show two payment UI in the same browser tab). I'll add a note describing what you are saying above.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Nov 29, 2018

Initial test set: web-platform-tests/wpt#14297

@marcoscaceres
Copy link
Member Author

Sent an updated batch of tests for this pull request. Hope to merge this soon.

@rsolomakhin
Copy link
Collaborator

This still looks good to me. Do you need anything else from us?

@marcoscaceres
Copy link
Member Author

@rsolomakhin no, it's all good from the spec side (appreciate you rechecking!). It was mostly on the test side of things. I was worried because I couldn't get the tests to run in Chrome... will follow up on the WPT side.

@marcoscaceres marcoscaceres merged commit 6b5e0ee into gh-pages Jan 16, 2019
@marcoscaceres marcoscaceres deleted the show_global branch January 16, 2019 07:08
index.html Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rethink "payment request is showing" boolean
4 participants