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
Modified A4A from shadow DOM to installFriendlyIframeEmbed #5196
Conversation
The previous change was made in error (wrong branch).
/to @lannka |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general. some minor comments
bodyElement./*OK*/innerHTML = bodyBlock; | ||
this.rendered_ = true; | ||
this.onAmpCreativeShadowDomRender(); | ||
return this.vsync_.mutatePromise().then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to do this.vsync_.mutatePromise(() => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It definitely should be mutatePromise(() => {...})
. But generally, only appendChild
would need to go in vsync
. Since installFriendlyIframeEmbed
is not quite ready for that, I recommend to just to just remove vsync.mutate
completely and I will instead implement vsync.mutate
inside installFriendlyIframeEmbed
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you push already? Not seeing changes...
this.onAmpCreativeShadowDomRender(); | ||
return this.vsync_.mutatePromise().then(() => { | ||
// Create and setup friendly iframe. | ||
const iframe = this.element.ownerDocument.createElement('iframe'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can leverage the util createElementWithAttributes
in dom.js
to save some lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
for (let i = 0; i <= extensions.length; i++) { | ||
// Remove AMP runtime as well. | ||
const extensionName = | ||
(i == extensions.length) ? 'v0\.js' : extensions[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, this is not that intuitive.
instead, we can do
extensions.push('v0\.js`);
before the for loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done... note that I have to then pop it from the array as it is passed to installFriendlyIframeEmbed
bodyElement./*OK*/innerHTML = bodyBlock; | ||
this.rendered_ = true; | ||
this.onAmpCreativeShadowDomRender(); | ||
return this.vsync_.mutatePromise().then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It definitely should be mutatePromise(() => {...})
. But generally, only appendChild
would need to go in vsync
. Since installFriendlyIframeEmbed
is not quite ready for that, I recommend to just to just remove vsync.mutate
completely and I will instead implement vsync.mutate
inside installFriendlyIframeEmbed
.
creativeMetaData.customElementExtensions || []; | ||
for (let i = 0; i <= extensions.length; i++) { | ||
// Remove AMP runtime as well. | ||
const extensionName = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quiet understand these changes. Why not just use the old way of doing this as was before, but simply combine cssBlock
and bodyBlock
into one string before sending it to installFriendlyIframeEmbed
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, I want to minimize that amount of work done by the validator rewrite. Had we done this from the beginning, we would only have required it provide an array of extension ids and how to remove script tags (including v0.js).
@@ -410,18 +405,19 @@ export class AmpA4A extends AMP.BaseElement { | |||
// valid AMP. | |||
this.timerId_ = incrementLoadingAds(this.win); | |||
return this.adPromise_.then(rendered => { | |||
console.log('layoutCallback promise result', rendered, this.rendered_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
bodyElement./*OK*/innerHTML = bodyBlock; | ||
this.rendered_ = true; | ||
this.onAmpCreativeShadowDomRender(); | ||
return this.vsync_.mutatePromise().then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you push already? Not seeing changes...
Yes apologies, just pushed update to a4a.js. Working on test modifications now. |
this.rendered_ = true; | ||
this.onAmpCreativeShadowDomRender(); | ||
// Create and setup friendly iframe. | ||
const iframe = createElementWithAttributes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what i understand is A4A will always create an iframe (friendly / cross domain) now. Can we make iframe a variable of the class this.iframe = iframe
here to make code sharing class cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I actually start to think this is a good idea now. we could instantiate the class with this.iframe = null
. Making it a public field so that other shared modules can access.
But not necessary in this PR. We can change it in your own refactoring PR for code-sharing .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some indentation nitpicks.
// validation providing offsets to extension & AMP runtime | ||
// locations. | ||
const extensions = | ||
creativeMetaData.customElementExtensions || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for forced line breaks, I think we indent 4 spaces.
extensions.push('v0\.js'); | ||
extensions.forEach(extension => { | ||
creative = creative.replace(new RegExp( | ||
`<script[^>]+${extensionName}[^>]+>\s*</script\s*>\n?`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 spaces here as well.
the previous line is fine though, as it's not a forced line break.
// have moved to AMP AD document format validation, ensure it does | ||
// not match amp4ads-boilerplate. | ||
creative = creative.replace( | ||
/<style\s+([^>]*\s+)?amp-boilerplate[^>]*>[^>]+<\/style\s*>/, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here
creative = creative.replace( | ||
/<style\s+([^>]*\s+)?amp-boilerplate[^>]*>[^>]+<\/style\s*>/, ''); | ||
return installFriendlyIframeEmbed( | ||
iframe, this.element, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
The previous change was made in error (wrong branch).
We need this PR to get in to unblock some other work. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
@lannka @dvoytenko - after discussions with AMP team, modified code to stitch together creative from validator rewrite meta object as opposed to using regexps to remove portions. Validator team is working on modifying output to place portions to be removed (scripts & boilerplate) next to each other and provide offsets to ease removal. PTAL |
CLAs look good, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of nits.
const iframe = /** @type {!HTMLIFrameElement} */( | ||
createElementWithAttributes( | ||
/** @type {!Document} */(this.element.ownerDocument), 'iframe', { | ||
'frameborder': '0', 'allowfullscreen': '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I property per line.
const modifiedCreative = | ||
`<!doctype html><html ⚡4ads> | ||
<head> | ||
<style amp4ads-boilerplate>body{visibility:hidden}</style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, this is not needed for friendly iframes. My code manages the visibility state.
I'll merge this PR now to unblock other's work, and we can get the nit fixed in separate PR. |
…t#5196) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial integration with installFriendlyIframeEmbed * Review comments * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial integration with installFriendlyIframeEmbed * Review comments * Change to stitching creative instead of removing portions * Update unit tests * Fix after merge with upstream/master; correct type failures within friendly-iframe-embed * Fix lint errors * remove console log
…t#5196) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial integration with installFriendlyIframeEmbed * Review comments * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial integration with installFriendlyIframeEmbed * Review comments * Change to stitching creative instead of removing portions * Update unit tests * Fix after merge with upstream/master; correct type failures within friendly-iframe-embed * Fix lint errors * remove console log
Only implementation, still need to update tests.