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

IFrame support in web payments #147

Closed
rsolomakhin opened this issue Nov 22, 2016 · 8 comments
Closed

IFrame support in web payments #147

rsolomakhin opened this issue Nov 22, 2016 · 8 comments

Comments

@rsolomakhin
Copy link

Hello TAG!

I'm requesting a TAG review of:

@dbaron
Copy link
Member

dbaron commented Nov 22, 2016

At first glance, the spec seems a bit informal, i.e., lacking a clear processing model. Such a model would make it clearer what happens in the case of nested iframes. Is the allowpaymentrequest attribute needed on the iframe at each origin boundary between the browsing context that wants to be allowed to make payment requests and the toplevel browsing context, or is it needed on more or fewer iframes?

Also, why is this extending only HTMLIFrameElement and not also HTMLFrameElement and HTMLObjectElement? Those are the three interfaces that have a contentDocument property. That said, the allegedly-deprecated (the editor's draft sort of agrees) getSVGDocument() seems to be on 2 of the 3 (not HTMLFrameElement, but with HTMLEmbedElement added. Maybe a common interface here is needed...

@domenic
Copy link
Member

domenic commented Nov 23, 2016

In particular the spec should be using https://html.spec.whatwg.org/#allowed-to-use instead of the undefined term "allowed to make payment requests". It answers @dbaron's question about nesting (you need it at every iframe; origins are not consulted except in the initial check inside the PaymentRequest constructor---but see below for more on that).

All other allow* attributes are currently only on HTMLIFrameElement.

Also this needs [CEReactions] since it's a reflected content attribute.

Ideally this should be pull-requested to HTML as it defines a new content attribute, but developing it in the payment request spec is fine for now.

Also "the browsing context of the script calling the constructor whose origin..." is poorly defined. Scripts don't have browsing contexts and browsing contexts don't have origins. We shouldn't be doing call-stack-walking type checks about the scripts calling things. And "different from" is not the correct check. You instead want "The current settings object's origin is not same-origin with its top level browsing context's Window's relevant settings object's origin.

@annevk
Copy link
Member

annevk commented Nov 23, 2016

FWIW, "allowed to use" and the general uplifting to HTML is w3c/payment-request#311.

@domenic
Copy link
Member

domenic commented Nov 29, 2016

See whatwg/html#2112 for a discussion of making it easier for spec authors to ask the question "am I being invoked cross origin"

@travisleithead
Copy link
Contributor

Reviewed at TAG Boston f2f. Should followup that @annevk and @domenic comments were addressed.

@travisleithead
Copy link
Contributor

Also note from our minutes: Resolved: Attribute proposed in the spec LGTM

@domenic
Copy link
Member

domenic commented Feb 8, 2017

In general this has been largely straightened out (as long as you ignore the references to the W3C HTML fork in the Payment Request, and use the corresponding definitions in the HTML Standard) by the heroic work of @zcorpan. He's also added web platform tests, which is super-great.

I still have an open action item to see if whatwg/html#2112 is still worthwhile and worth pursuing further as something generally useful, but the payment request-specific stuff has been straightened out nicely for now.

@triblondon
Copy link

Extracting from the minutes: we spent some time figuring out whether this attribute was appropriate for the iframe tag or whether it belonged in the sandbox attribute as a directive. It does seem correct to make it an attribute of the tag itself, since it is currently consistently the case that sandbox directives re-enable things that the presence of the sandbox attribute disables.

That wraps things up from a TAG review perspective

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

No branches or pull requests

6 participants