Make inabox host correctly get position of a nested iframe #20599

zombifier · 2019-01-30T17:53:47Z

Fixes #14010.

Fixes ampproject#14010.

lannka · 2019-01-30T18:15:58Z

thanks @zombifier for the quick fix! will take a look later today.

Meantime, have you checked the local example mentioned in #14010 with your fix?

zombifier · 2019-01-30T18:16:26Z

I had, and it works.

lannka · 2019-01-30T18:20:11Z

awesome. is it easy to add an integration test ?

zombifier · 2019-01-30T18:34:29Z

I can try; the main issue is that the main test iframe is scrollable and by default much smaller than the browser window, so even if the creative is not visible it could technically still be within the size of the viewport and trigger viewability. I can try to see if resizing the test iframe to full height can help.

Visibility detection in general does not work properly if any of the nested iframes can be scrolled so its content enter the viewport rectangle while remaining invisible (and it's more or less impossible to do so without native IntersectionObserver), but that's not the case we ever need to worry about outside of testing.

zombifier · 2019-01-30T18:43:10Z

I should probably add some unit tests though.

lannka · 2019-01-30T18:57:18Z

I should probably add some unit tests though.

Might be hard for unit test to simulate the nested iframe situation though.

lannka · 2019-01-30T21:31:22Z

ads/inabox/position-observer.js

+
+/**
+ * Get the element's layout rect relative to the viewport.
+ * If parentWin is provided, then attempt to walk up the DOM and add the


shall we just always walk up to the host window?

the parentWin parameter has to be provided, since this is a general-purpose position observer and there's no way to get the immediate window/iframe that contains the observed element.

I can change it so that if parentWin is not provided then automatically assume the host window to be its immediate parent. What do you think?

Does targetElement.ownerDocument.defaultView not work?

TIL. I will try that. If it works then the parameter wouldn't be needed.

lannka · 2019-01-30T21:31:48Z

ads/inabox/position-observer.js

+export function getTargetRect(element, parentWin) {
+  let targetRect = layoutRectFromDomRect(element./*OK*/getBoundingClientRect());
+  if (parentWin) {
+    for (let j = 0, tempWin = parentWin;


nit: while loop might fit better here?

lannka · 2019-01-30T21:32:33Z

ads/inabox/position-observer.js

+  let targetRect = layoutRectFromDomRect(element./*OK*/getBoundingClientRect());
+  if (parentWin) {
+    for (let j = 0, tempWin = parentWin;
+      j < 10 && tempWin != tempWin.top; j++, tempWin = tempWin.parent) {


so the assumption is tempWin.top is the host window? I feel it's not that reliable. Any better way?

I agree. The host window is the window that has the position observer installed, so I moved the function into the class and have the DOM climb stop at that window. The top condition remains just in case.

sorry, I still didn't quite get it. where is the code to stop the DOM climb at host window?

ah, sorry, was looking at outdated file.

rsimha · 2019-01-30T23:48:49Z

Removing myself, since the latest commit doesn't seem to increase the bundle size. Feel free to re-add me if that's not the case.

zombifier · 2019-01-31T20:42:52Z

Since the amp-inabox integration tests are in nested iframe mode, if they're successfully reenabled on Safari I think that's a good enough integration test for this PR. I'm not pushing the commit until Sauce is reenabled on master, but manual local Sauce testing seems to confirm that it works.

lannka · 2019-01-31T20:55:07Z

Since the amp-inabox integration tests are in nested iframe mode, if they're successfully reenabled on Safari I think that's a good enough integration test for this PR. I'm not pushing the commit until Sauce is reenabled on master, but manual local Sauce testing seems to confirm that it works.

that sounds good. thanks for checking

…element

zombifier · 2019-02-06T15:53:16Z

@lannka Is this ready? I resolved all the flaky tests by merging with upstream.

lannka · 2019-02-06T17:46:41Z

Merged, good work!

… observer is not null.

…t#20599) * Make inabox host correctly get position of a nested iframe Fixes ampproject#14010. * Fix unused import * Fix the existing unit test * Add a unit test for getFrameRect and fix fatal typo in loop body * oops * oops pt. 2 * Don't assume the top window is the host. * Use element.ownerDocument.defaultView to get the parent window of an element * lint

…main (#20459) * Make inabox-viewport measure the top level window directly if same domain Instead of sending a post message, if the inabox iframe can directly measure the host window then have it get its position directly from there. * fix types * Fix presubmit * Test disconnect function * Fix lint in new test * Try enabling friendly, no host script inabox test on Safari * Trivial fix to rerun Travis * Try the no host script test with the example AMP ad Checking if the serve mode of the example AMP test is changed somehow * don't whitelist the iframe * More work * Fix lint * Fix lint in test * fix oops * Throttle the listeners * Use the existing inabox host observer instead of rewriting a new one This makes the code cleaner and changes in the future easier * fix type issue * Revert the changes to test-amp-inabox.js I believe that the while the integration tests do test the inabox host script, they're are not actually testing the local AMP inabox run time. This might have to be fixed in a separate PR. * Gate the behavior behind experiment, and move canInspectWindow to helper file * Integrate the changes in #20599, and make sure the position observer is not null. * Inline the function * Fix type * Disconnect observer properly, and add a TODO to investigate a quirk in the listener. Specifically, why is Resources needed.

…t#20599) * Make inabox host correctly get position of a nested iframe Fixes ampproject#14010. * Fix unused import * Fix the existing unit test * Add a unit test for getFrameRect and fix fatal typo in loop body * oops * oops pt. 2 * Don't assume the top window is the host. * Use element.ownerDocument.defaultView to get the parent window of an element * lint

…main (ampproject#20459) * Make inabox-viewport measure the top level window directly if same domain Instead of sending a post message, if the inabox iframe can directly measure the host window then have it get its position directly from there. * fix types * Fix presubmit * Test disconnect function * Fix lint in new test * Try enabling friendly, no host script inabox test on Safari * Trivial fix to rerun Travis * Try the no host script test with the example AMP ad Checking if the serve mode of the example AMP test is changed somehow * don't whitelist the iframe * More work * Fix lint * Fix lint in test * fix oops * Throttle the listeners * Use the existing inabox host observer instead of rewriting a new one This makes the code cleaner and changes in the future easier * fix type issue * Revert the changes to test-amp-inabox.js I believe that the while the integration tests do test the inabox host script, they're are not actually testing the local AMP inabox run time. This might have to be fixed in a separate PR. * Gate the behavior behind experiment, and move canInspectWindow to helper file * Integrate the changes in ampproject#20599, and make sure the position observer is not null. * Inline the function * Fix type * Disconnect observer properly, and add a TODO to investigate a quirk in the listener. Specifically, why is Resources needed.

Make inabox host correctly get position of a nested iframe

9939e65

Fixes ampproject#14010.

googlebot added the cla: yes label Jan 30, 2019

Fix unused import

a58810d

lannka self-requested a review January 30, 2019 18:19

zombifier added 4 commits January 30, 2019 13:59

Fix the existing unit test

374e9d0

Add a unit test for getFrameRect and fix fatal typo in loop body

dd737c1

oops

e336873

oops pt. 2

db7d594

lannka requested changes Jan 30, 2019

View reviewed changes

Don't assume the top window is the host.

7c570ec

amp-bundle-size bot requested a review from rsimha January 30, 2019 22:24

rsimha removed their request for review January 30, 2019 23:48

zombifier mentioned this pull request Jan 31, 2019

Make inabox-viewport measure the top level window directly if same domain #20459

Merged

zombifier added 2 commits January 31, 2019 17:47

Use element.ownerDocument.defaultView to get the parent window of an …

e05ad0d

…element

lint

b99f2f4

lannka approved these changes Jan 31, 2019

View reviewed changes

zombifier added 3 commits February 4, 2019 14:42

Merge branch 'upstream' into inabox_nested_fix

b96141f

Merge branch 'upstream' into inabox_nested_fix

ee480ad

Merge branch 'upstream' into inabox_nested_fix

d7f2bd1

lannka merged commit 37dba15 into ampproject:master Feb 6, 2019

zombifier added a commit to zombifier/amphtml that referenced this pull request Feb 6, 2019

Integrate the changes in ampproject#20599, and make sure the position…

e36c257

… observer is not null.

ampprojectbot added the PR use: In Beta / Experimental label Feb 8, 2019

zombifier mentioned this pull request Feb 20, 2019

Fix various issues with the inabox integration tests #20972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make inabox host correctly get position of a nested iframe #20599

Make inabox host correctly get position of a nested iframe #20599

zombifier commented Jan 30, 2019

lannka commented Jan 30, 2019

zombifier commented Jan 30, 2019

lannka commented Jan 30, 2019

zombifier commented Jan 30, 2019 •

edited

Loading

zombifier commented Jan 30, 2019

lannka commented Jan 30, 2019

lannka Jan 30, 2019

zombifier Jan 30, 2019

lannka Jan 31, 2019

zombifier Jan 31, 2019

lannka Jan 30, 2019

lannka Jan 30, 2019

zombifier Jan 30, 2019

lannka Jan 31, 2019

lannka Jan 31, 2019

rsimha commented Jan 30, 2019

zombifier commented Jan 31, 2019

lannka commented Jan 31, 2019

zombifier commented Feb 6, 2019

lannka commented Feb 6, 2019

Make inabox host correctly get position of a nested iframe #20599

Make inabox host correctly get position of a nested iframe #20599

Conversation

zombifier commented Jan 30, 2019

lannka commented Jan 30, 2019

zombifier commented Jan 30, 2019

lannka commented Jan 30, 2019

zombifier commented Jan 30, 2019 • edited Loading

zombifier commented Jan 30, 2019

lannka commented Jan 30, 2019

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

rsimha commented Jan 30, 2019

zombifier commented Jan 31, 2019

lannka commented Jan 31, 2019

zombifier commented Feb 6, 2019

lannka commented Feb 6, 2019

zombifier commented Jan 30, 2019 •

edited

Loading