Friendly frames embeds #4916

dvoytenko · 2016-09-10T00:03:04Z

Partial for #4915.

This outlines major changes for "friendly frames" support. It also contains a manual test setup to demonstrate the use of the API. Let's discuss before tests. Some important points:

Boundary traversal rules: who and when is allowed traversal
Analytics will have to have some major changes to support friendly frames (although it'd have to have the same for shadow DOM as well)
Things like UrlReplacements are still very unclear. Specifically we probably shouldn't allow a blanket access to parent document's URL vars - some scoping is necessary. Likewise, shadow DOM has the same issues most likely.

cramforce · 2016-09-13T14:06:20Z

src/custom-element.js

+ * Copies the specified element to child window (friendly iframe). This way
+ * all implementations of the AMP elements are shared between all friendly
+ * frames.
+ * @param {!Window} win


Maybe name child

cramforce · 2016-09-13T14:19:42Z

This looks very reasonable 👍

dvoytenko · 2016-09-13T16:01:55Z

@cramforce So, another thing that will be an issue as well is URL resolution. E.g. resolving child element's relative URLs, as in the case with amp-img and even basic links.

cramforce · 2016-09-13T16:07:10Z

You can use a base href, no?

On Tue, Sep 13, 2016 at 9:01 AM, Dima Voytenko notifications@github.com
wrote:

@cramforce https://github.com/cramforce So, another thing that will be
an issue as well is URL resolution. E.g. resolving child element's relative
URLs, as in the case with amp-img and even basic links.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4916 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT8GEnNkEl79rKjGZ02DeENlx6fjSks5qpsj0gaJpZM4J5k6k
.

dvoytenko · 2016-09-13T20:58:47Z

@cramforce Good point on BASE. I added it. PTAL.

That solves <a> and similar issues. But doesn't solve the parseUrl and similar calls in the url.js, which are many.

cramforce · 2016-09-13T22:26:57Z

Do we actually expect relative URLs in ads?

On Tue, Sep 13, 2016 at 1:58 PM, Dima Voytenko notifications@github.com
wrote:

@cramforce https://github.com/cramforce Good point on BASE. I added it.
PTAL.

That solves and similar issues. But doesn't solve the parseUrl and
similar calls in the url.js, which are many.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4916 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT38DA5ycVHlftFhiQcXSPBQ9QnjSks5qpw6HgaJpZM4J5k6k
.

dvoytenko · 2016-09-13T23:33:09Z

@cramforce

Do we actually expect relative URLs in ads?

Hard to tell. The signer will simply confirm and sign, not rewrite/optimize the content. Right?

cramforce · 2016-09-14T00:24:22Z

We at least reserialize and sign that. Not sure about resources. Definitely
right now things would only work with absolute URLs.

On Tue, Sep 13, 2016 at 4:33 PM, Dima Voytenko notifications@github.com
wrote:

@cramforce https://github.com/cramforce

Do we actually expect relative URLs in ads?

Hard to tell. The signer will simply confirm and sign, not
rewrite/optimize the content. Right?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4916 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT-RexBzt60YIpXlBYiLQQRXyhQXNks5qpzK2gaJpZM4J5k6k
.

dvoytenko · 2016-09-15T20:56:55Z

@cramforce This is ready for review. The "demo" files will be removed before merge (marked with DO NOT SUBMIT).

/cc @tdrl @michaelkleber @bobcassels @lannka @avimehta For what's coming for friendly iframes.

tdrl · 2016-09-15T21:06:27Z

+a4a-cam

On Thu, Sep 15, 2016 at 4:57 PM, Dima Voytenko notifications@github.com
wrote:

@cramforce https://github.com/cramforce This is ready for review. The
"demo" files will be removed before merge (marked with DO NOT SUBMIT).

/cc @tdrl https://github.com/tdrl @michaelkleber
https://github.com/michaelkleber @bobcassels
https://github.com/bobcassels @lannka https://github.com/lannka
@avimehta https://github.com/avimehta For what's coming for friendly
iframes.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4916 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AQ4oaKx0nkpijUM80z4pg9zji7wMAIMdks5qqbEpgaJpZM4J5k6k
.

Terran Lane email: tdrl@google.com
Google Cambridge, MA Publisher Tags & Ads Latency team
"But I don't want to go among mad people," Alice remarked.
"Oh, you can't help that," said the Cat: "we're all mad here. I'm
mad. You're mad."
"How do you know I'm mad?" said Alice.
"You must be," said the Cat, "or you wouldn't have come here."

cramforce · 2016-09-15T21:07:58Z

src/dom.js

+export function getFrameElement(win) {
+  try {
+    return win.frameElement;
+  } catch (e) {


In Safari this would yield a logged error independent of the catch. Can we instead only call the function when we know we are allowed to call it?

Could you clarify? When we call this method, we already know for certain that the window belongs to a friendly iframe. How else can we protect against an error here?

Please document that that is the contract. If we only call this when we know it will work, then it is fine. Although I wonder then why we would have a try block.

I thought you were just concerned about failures on some random browser. Originally there wasn't a try/catch. Really by the time I check frameElement in all cases, I'm already 100% sure I'm working with a friendly frame. I can either kill this method, or do defense in depth here and check that this iframe been marked as friendly. WDYT?

Lets keep it.

cramforce · 2016-09-15T21:09:31Z

src/friendly-iframe-embed.js

+    childDoc.open();
+    childDoc.write(spec.html);
+    childDoc.close();
+    readyPromise = Promise.resolve();


cramforce · 2016-09-15T21:10:21Z

src/friendly-iframe-embed.js

+    const childDoc = childWin.document;
+
+    // Add <BASE> tag.
+    childDoc.head.appendChild(createElementWithAttributes(


Don't we have to do this earlier?

Well, we could try, but the document is not visible yet and not functional - none of the extensions have been installed. Thus this is still a good time to do it, imo. And it's a lot easier than trying to insert it into a HTML string.

If there is anything in the doc directly in would potentially go to a different URL, though. I think we should either add it early or never.

Good point. I will update.

cramforce · 2016-09-15T21:11:17Z

src/friendly-iframe-embed.js

+}
+
+
+/**


cramforce · 2016-09-15T21:13:14Z

src/service/viewport-impl.js

+
+    // Go up the window hierarchy through friendly iframes.
+    const win = (el.ownerDocument || el).defaultView;
+    if (win && win != this.win_ && getTopWindow(win) == this.win_) {


Can we tag the window with something like win.ownedByParent?

That's what getTopWindow does. The window has __AMP_TOP property and this is what used here. I just hid it behind API.

Makes sense.

Same logic as https://github.com/ampproject/amphtml/pull/4916/files#diff-b596009fc4aa42dfa7d85c56dccf78eeR125? Can we extract this into a helper?

dvoytenko · 2016-09-16T20:55:45Z

@cramforce all changes have been done. PTAL.

cramforce · 2016-09-16T21:26:12Z

LGTM

jridgewell · 2016-09-16T21:41:50Z

src/friendly-iframe-embed.js

+  }
+  return readyPromise.then(() => {
+    // Add extensions.
+    extensionsFor(win).installExtensionsInChildWindow(


We already have extensions variable.

jridgewell · 2016-09-16T21:44:39Z

src/friendly-iframe-embed.js

+ * @param {!FriendlyIframeSpec} spec
+ * @visibleForTesting
+ */
+export function mergeHtmlForTesting(spec) {


Why not just expose the mergeHtml function?

mergeHtmlForTesting will be DCE'd. This way:

I don't need to worry about anyone ignoring @visibleForTesting

mergeHtml will be obfuscated.

jridgewell · 2016-09-16T21:46:29Z

src/service/ampdoc-impl.js

@@ -120,6 +120,17 @@ export class AmpDocService {
        }
      }

+      // Traverse the boundary of a friendly iframe.
+      const win = (n.ownerDocument || n).defaultView;
+      if (win && win != this.win && getTopWindow(win) == this.win) {


Can you comment how this works?

Good idea. I refactored it into a common method and added docs there.

jridgewell · 2016-09-16T21:48:55Z

src/service/viewport-impl.js

+
+    // Go up the window hierarchy through friendly iframes.
+    const win = (el.ownerDocument || el).defaultView;
+    if (win && win != this.win_ && getTopWindow(win) == this.win_) {


Same logic as https://github.com/ampproject/amphtml/pull/4916/files#diff-b596009fc4aa42dfa7d85c56dccf78eeR125? Can we extract this into a helper?

* Friendly iframe embed system * getFrameElement refactored to include parent tests

dvoytenko force-pushed the ife1 branch from d65eb7e to 7044c75 Compare September 13, 2016 00:50

dvoytenko assigned cramforce Sep 13, 2016

cramforce reviewed Sep 13, 2016
View reviewed changes

dvoytenko force-pushed the ife1 branch 5 times, most recently from cc78ca5 to 656ac04 Compare September 15, 2016 20:42

dvoytenko changed the title ~~[DO NOT SUBMIT] prototype for friendly frames~~ Prototype for friendly frames Sep 15, 2016

dvoytenko changed the title ~~Prototype for friendly frames~~ Friendly frames embeds Sep 15, 2016

dvoytenko added the NEEDS REVIEW label Sep 15, 2016

cramforce requested changes Sep 15, 2016

View reviewed changes

dvoytenko force-pushed the ife1 branch 2 times, most recently from 89b323a to 0d089ec Compare September 16, 2016 16:51

cramforce added the LGTM label Sep 16, 2016

cramforce removed the NEEDS REVIEW label Sep 16, 2016

cramforce approved these changes Sep 16, 2016

View reviewed changes

Friendly iframe embed system

cb557fc

dvoytenko force-pushed the ife1 branch from 7ab77b0 to cb557fc Compare September 16, 2016 21:35

jridgewell reviewed Sep 16, 2016

View reviewed changes

getFrameElement refactored to include parent tests

f6985c5

dvoytenko merged commit d50eeae into ampproject:master Sep 16, 2016

dvoytenko deleted the ife1 branch September 16, 2016 22:39

avimehta mentioned this pull request Sep 19, 2016

Render <amp-analytics> within <amp-ad> for A4A in the parent doc instead of the shadow dom/iframe #4770

Closed

mityaha pushed a commit to ooyala/amphtml that referenced this pull request Nov 30, 2016

Friendly frames embeds (ampproject#4916)

17dcc54

* Friendly iframe embed system * getFrameElement refactored to include parent tests

ampprojectbot mentioned this pull request Jan 19, 2022

🚨 Error: viewport has zero dimensions ampproject/error-reporting#137

Open

		}


		/**

Friendly frames embeds #4916

Friendly frames embeds #4916

Conversation

dvoytenko commented Sep 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce commented Sep 13, 2016

dvoytenko commented Sep 13, 2016

cramforce commented Sep 13, 2016

dvoytenko commented Sep 13, 2016

cramforce commented Sep 13, 2016

dvoytenko commented Sep 13, 2016

cramforce commented Sep 14, 2016

dvoytenko commented Sep 15, 2016

tdrl commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Sep 16, 2016

cramforce commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Sep 10, 2016 •

edited

Loading