✨Allow fake ads in amp stories
#21025
Conversation
examples/amp-story/fake-ad.html
Outdated
| "ad-attributes": { | ||
| "type": "fake", | ||
| "src": "/examples/amp-story/ads/app-install.html", | ||
| "id": "i-amphtml-demo-fake", |
There was a problem hiding this comment.
Does amp-story-auto-ads check the validness of the id?
There was a problem hiding this comment.
amp-story-auto ads just creates an <amp-ad type="fake" id="i-amphtml..">. There is no additional logic around checking the id, so it should behave the same as a normal "fake" ad. Are you worried about the doc being invalid?
There was a problem hiding this comment.
After thinking about this more, I think I understand. The doc will be valid when it shouldn't be. I don't have a great solution. We could have the runtime not allow this in certain contexts, but I'm not sure what the signal would be. What do you think?
There was a problem hiding this comment.
To allowing fake ad specifically we can have some kind of workaround. For example, one must put id=i-amphtml-... to the <amp-story-auto-ads> component. So the fake ad only loads for invalid AMP page.
However i think this is related to #20003. Allowing developers to randomly assign attribute of the inserted ad in general sounds very unsafe to me. I can easily inject something like i-amphtml-ghost without validator catching that.
There was a problem hiding this comment.
This is a good point. I agree a whitelist sounds good, but needs to be fleshed out. For instance id is allowed, but certain values are not. I will make some comments on the issue.
To solve this problem I think that having an invalid signal on the amp-story-auto-ads tag is a good idea. I will refactor.
There was a problem hiding this comment.
a different issue: are we inserting multiple amp-ad with the same ID?
There was a problem hiding this comment.
yes, is this not ok for testing?
There was a problem hiding this comment.
changed this around a bit so auto ads injects a unique id. Pubs just add the attribute to the auto-ads tag itself.
|
@calebcordry what's the status of this PR? |
|
I got distracted by other things :) I need to refactor this to make sure the doc is invalid when in use. This is very helpful for me / devrel when working on making these creatives so I will get it done. |
|
@zhouyx Could you re-review when you have a chance? |
| @@ -0,0 +1,101 @@ | |||
| <!doctype html> | |||
There was a problem hiding this comment.
How is this example related to the fake ads? 🤔 No worries if it isn't just curious 😄
There was a problem hiding this comment.
This is the fake ad that gets loaded.
There was a problem hiding this comment.
Oh haha! Rad! Sounds good 😄
| @@ -334,6 +336,10 @@ export class AmpStoryAutoAds extends AMP.BaseElement { | |||
| */ | |||
| schedulePage_() { | |||
| const page = this.createAdPage_(); | |||
| if (!page) { | |||
| // Creation of ad has failed. | |||
There was a problem hiding this comment.
Should we throw an error or warning here? 🤔
It may be a bit noisy though if we did.
There was a problem hiding this comment.
I think this should only happen in the fake case which will already user().warn()
| const id = this.element.getAttribute('id'); | ||
| if (!id || !startsWith(id, 'i-amphtml-demo-')) { | ||
| user().warn(TAG, 'id must start with i-amphtml-demo- to use fake ads'); | ||
| return null; |
There was a problem hiding this comment.
Have we handle the case where this.createAdElement_ returns null?
There was a problem hiding this comment.
Nope you are right, null check was in wrong place. Instead of adding a bunch of logic for fake ad destruction, what do you think about creating a dummy element instead like <p>Fake ad discarded.<p> and inserting that?
There was a problem hiding this comment.
What about we keep it simple, and use userAssert() instead?
|
LGTM |
Before this change the ad would not render due to incorrect metadata around the
ctaTypeandctaUrl.This allows a configuration like the following to render