amp-twitter breaks when amp-3p-iframe-src is defined

ampproject / amphtml

The AMP web component framework.

https://amp.dev

Apache License 2.0

14.89k stars 3.89k forks source link

amp-twitter breaks when amp-3p-iframe-src is defined #5400

Closed apaleslimghost closed 7 years ago

apaleslimghost commented 8 years ago

amp-twitter uses the base URL specified by the amp-3p-iframe-src meta tag, which I don't think is valid. There should be no reason not to use the Twitter URL for this frame.

jridgewell commented 8 years ago

/to @cramforce

cramforce commented 8 years ago

@quarterto there is definitely an argument for changing this. It definitely shouldn't "break" in this case, though. The default documented frame content should work just fine for Twitter (and facebook, etc.)

Do you maintain a site where this is actually broken?

apaleslimghost commented 8 years ago

Do you maintain a site where this is actually broken?

Yes, example article where it's broken: http://amp.ft.com/content/b1be7a87-0f52-3159-bf8e-1a72742bf8be (you'll probably get the paywall, but there are ways around it). We're using the meta tag to configure amp-ad so we can use our own targeting. I'm guessing we could go about that with src instead?

cramforce commented 8 years ago

@quarterto A few comments on that

I'd recommend calling draw3p synchronously. You can just slightly rearrange things. Of course, you can still call done at the same time you do now.
As the first line you want to look at config.type and only if the type is whatever ad network you use, go into the krux code path. Otherwise just call done without doing anything. (That will fix Twitter)
Not all browsers that AMP supports have promises available and AMP does not ship a polyfill into the integration.js file.
You currently fail to pass these additional args https://github.com/ampproject/amphtml/blob/master/3p/remote.html#L19 which are important security wise.

apaleslimghost commented 8 years ago

Thanks for the pointers, I'll look into it. We're including a polyfill.

cramforce commented 8 years ago

You definitely want to put that polyfill script tag after the inline script. Otherwise it blocks fetching AMP's iframe JS.

On Wed, Oct 5, 2016 at 8:22 AM, Matt Brennan notifications@github.com wrote:

Thanks for the pointers, I'll look into it. We're including a polyfill https://github.com/Financial-Times/google-amp/blob/master/views/ads-iframe.html#L7 .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ampproject/amphtml/issues/5400#issuecomment-251707276, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFeT2xAwdXcUE_ywzWiog5JFvGw35cUks5qw8DFgaJpZM4KN7CY .