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

zombifier · 2019-01-22T16:22:31Z

Instead of sending a post message to the host script, if the inabox iframe can directly measure the host window then get its position directly by doing so.

Specifically, inabox-viewport will add scroll and resize event listeners on the top level window that will trigger the measurement functions, which is mostly copied over from the inabox host's position observer.

Related issue: #19869

In the future requesting and cancel full overlay can be changed to do the same thing. This is very low priority however.

…main 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.

zombifier · 2019-01-22T20:41:48Z

TODO: Integration tests. I believe selectively reenabling some inabox tests added in #20154 on Safari (in particular, friendly iframe without the host script) would be a good indicator.

Also if #10877 is resolved I can also piggy back off it with an amp-position-observer in FIE without host script test.

Checking if the serve mode of the example AMP test is changed somehow

This makes the code cleaner and changes in the future easier

jeffkaufman · 2019-01-30T15:46:19Z

@zombifier is this ready for review or still WIP?

zombifier · 2019-01-30T17:58:50Z

Still sorting out some things, will notify you if ready for review.

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.

zombifier · 2019-01-31T16:09:16Z

@jeffkaufman this is ready for review, but it likely won't go in until #20599 is merged since that PR changes the position observer that this one uses. The integration tests are not ready yet though (see the comment on the latest commit to this branch).

src/inabox/inabox-viewport.js

jeffkaufman · 2019-02-01T23:07:43Z

src/inabox/inabox-viewport.js

+    } catch (unusedErr) { // eslint-disable-line no-unused-vars
+      return false;
+    }
+  }


This is copied from inabox-messaging-host.js: can we share the code instead?

Moved the function into src/iframe-helper.js, since that seems to be the most logical location.

jeffkaufman · 2019-02-01T23:08:41Z

src/inabox/inabox-viewport.js

    dev().fine(TAG, 'initialized inabox viewport');
  }

  /** @override */
  connect() {
-    this.listenForPosition_();
+    if (this.canInspectTopWindow_()) {


Let's gate this new behavior on an experiment so we can test it.

Done. Will create an issue for cleaning up later.

jeffkaufman · 2019-02-01T23:09:10Z

src/inabox/inabox-viewport.js

+        resolve(layoutRectFromDomRect(
+            this.win.frameElement./*OK*/getBoundingClientRect()));
+      });
+    }


This needs to be gated by the experiment too, right?

…per file

jeffkaufman · 2019-02-04T16:58:16Z

tools/experiments/experiments.js

@@ -424,6 +424,13 @@ const EXPERIMENTS = [
    spec: 'https://github.com/ampproject/amphtml/issues/20395',
    cleanupIssue: 'https://github.com/ampproject/amphtml/issues/20394',
  },
+  {
+    id: 'inabox-viewport-friendly',


Since we want to run experiments on driven by the ad server, you'll need to add this to allow-doc-opt-in in prod-config.json and canary-config.json

I see. Can this be done in the same PR? Since config changes are supposed to be separate.

Not a problem to add it later. It's just a thing you'll need to add before you can start running experiments.

zombifier · 2019-02-04T23:21:03Z

Cc @lannka, please take a look.

lannka · 2019-02-06T17:57:13Z

src/inabox/inabox-viewport.js

+   * @param {!../layout-rect.LayoutRectDef} viewportRect
+   * @param {!../layout-rect.LayoutRectDef} targetRect
+   */
+  updateLayoutRects_(viewportRect, targetRect) {


coding style nit: move this method down below where it;'s been called

lannka · 2019-02-06T17:57:35Z

src/inabox/inabox-viewport.js

-            this.fireScrollThrottle_();
-          }
+  /** @visibleForTesting */
+  setUpNativeListener() {


lannka · 2019-02-06T17:58:06Z

src/inabox/inabox-viewport.js

        });
  }

+  /** @private */
+  listenForPositionSameDomain_() {
+    this.setUpNativeListener();


can this method be inlined?

lannka · 2019-02-06T18:00:03Z

src/inabox/inabox-viewport.js

+  setUpNativeListener() {
+    // Set up listener but only after the resources service is properly
+    // registered (since it's registered after the inabox services so it won't
+    // be available immediately).


didn't quite understand why this is needed? Is there a race condition you try to avoid? Can you explain?

The inabox-viewport requires the Resources service to remeasure child elements. It's registered after the viewport service is set up, so when we setup the native listener it will try to call getChildResources() immediately and will throw since it can't find the service. I did manual testing within a browser; without this guard the console will be swarmed by thrown errors.

I might missed something, but my intuition is that a native listener should not need anything particular from resource service, because the parent window is not even AMP. is that a wrong dependency?

The Resources service comes from the AMP Inabox window itself.

Though I admit that I haven't looked very deep into what it actually does.

Could you dig deep into it? I'm afraid something is wrong here.

So I think the Resource service is responsible for positioning and sizing the child elements within an AMP document. As part of its regular flow the inabox viewport calls remeasureAllElements_() when the iframe size/scroll changes, which triggers this service.
In the cross domain case the postMessage from the host window usually comes back after the Resources service is set up, but in the same domain case added by this PR the listener is set up and updates positioning immediately, which will happen before the Resources service is installed due to their order of install in amp-inabox.js. If I'm correct then this race condition has always existed within the cross domain case.

… observer is not null.

lannka · 2019-02-06T23:08:54Z

src/inabox/inabox-viewport.js

+        .then(() => {
+          this.topWindowPositionObserver_ = new PositionObserver(this.win.top);
+          this.topWindowPositionObserver_.observe(
+              /** @type {!HTMLIFrameElement} */(this.win.frameElement),


shall we sanity check that win.top is win.parent? otherwise I don't know if the positionObserver still works as intended.

/cc @zhouyx

I thought that the inabox iframe does not have to necessarily be the direct child of the top level window? #20599 was created specifically to handle situations where this is not the case.

jeffkaufman · 2019-02-13T13:48:52Z

@zombifier is this ready to go in? If not, who is it blocked on?

lannka · 2019-02-13T18:39:07Z

hi @jeffkaufman , had a chat with @zombifier offline that I suspect there is a bad dependency in the current inabox implementation as I don't think there is a need for the native event listener to depend on the resourceService.

I plan to take a look sometime this week. Let let me know if it's blocking anything. We can certainly get this in and fix it later.

jeffkaufman · 2019-02-13T23:21:04Z

@lannka thanks! It's blocking our ability to experiment with stopping loading the host script in some cases for performance, but it's not that high a priority. Definitely not as high as amp-mraid!

lannka · 2019-02-26T22:45:21Z

woops.. haven't got a chance to look at this yet. @zombifier to not block you any longer, could you add a TODO and assign me as owner?

…n the listener. Specifically, why is Resources needed.

lannka · 2019-02-27T17:27:02Z

Thanks! Is it good to go? I can help merge.

Created #21149 for the TODO

zombifier · 2019-02-27T18:00:27Z

You can merge it. Thanks.

…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-viewport measure the top level window directly if same do…

d6612a9

…main 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.

googlebot added the cla: yes label Jan 22, 2019

zombifier added 2 commits January 22, 2019 11:36

fix types

f03abfb

Fix presubmit

ea78dc5

jeffkaufman mentioned this pull request Jan 22, 2019

inabox: amp-inabox should be able to get viewability information inside a friendly frame without IntersectionObserver or the host script #19869

Closed

zombifier added 11 commits January 25, 2019 10:24

Test disconnect function

4fbd385

Fix lint in new test

f4b0d21

Merge branch 'upstream' into inabox_friendly

4655d6a

Try enabling friendly, no host script inabox test on Safari

5947bfb

Trivial fix to rerun Travis

37d2b15

Try the no host script test with the example AMP ad

5b0627c

Checking if the serve mode of the example AMP test is changed somehow

don't whitelist the iframe

2dc3ce5

More work

a15e80f

Fix lint

48771c6

Fix lint in test

18fcc29

fix oops

f226409

zombifier changed the title ~~[WIP] Make inabox-viewport measure the top level window directly if same domain~~ Make inabox-viewport measure the top level window directly if same domain Jan 29, 2019

Throttle the listeners

a862336

zombifier changed the title ~~Make inabox-viewport measure the top level window directly if same domain~~ [WIP] Make inabox-viewport measure the top level window directly if same domain Jan 29, 2019

Use the existing inabox host observer instead of rewriting a new one

ad8ca83

This makes the code cleaner and changes in the future easier

zombifier added 2 commits January 30, 2019 13:10

fix type issue

1d59138

Revert the changes to test-amp-inabox.js

bdbab11

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.

jeffkaufman suggested changes Feb 1, 2019

View reviewed changes

Gate the behavior behind experiment, and move canInspectWindow to hel…

d57a414

…per file

jeffkaufman approved these changes Feb 4, 2019

View reviewed changes

zombifier changed the title ~~[WIP] Make inabox-viewport measure the top level window directly if same domain~~ Make inabox-viewport measure the top level window directly if same domain Feb 4, 2019

Merge branch 'upstream' into inabox_friendly

5710e6f

lannka reviewed Feb 6, 2019

View reviewed changes

zombifier added 3 commits February 6, 2019 13:42

Merge branch 'upstream' into inabox_friendly

46ef8d3

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

e36c257

… observer is not null.

Inline the function

1553d9c

lannka reviewed Feb 6, 2019

View reviewed changes

Fix type

bf4dc5a

Merge branch 'upstream' into inabox_friendly

ee941d6

zombifier added 2 commits February 27, 2019 09:47

Disconnect observer properly, and add a TODO to investigate a quirk i…

7f15bbb

…n the listener. Specifically, why is Resources needed.

Merge branch 'upstream' into inabox_friendly

7780a9d

lannka merged commit 9397b59 into ampproject:master Feb 27, 2019

zombifier mentioned this pull request Feb 28, 2019

Allow doc opt-in for inabox-viewport-friendly. #21184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zombifier commented Jan 22, 2019 •

edited

zombifier commented Jan 22, 2019 •

edited

jeffkaufman commented Jan 30, 2019

zombifier commented Jan 30, 2019

zombifier commented Jan 31, 2019

jeffkaufman Feb 1, 2019

zombifier Feb 4, 2019

jeffkaufman Feb 1, 2019

zombifier Feb 4, 2019

jeffkaufman Feb 1, 2019

zombifier Feb 4, 2019

jeffkaufman Feb 4, 2019

zombifier Feb 4, 2019

jeffkaufman Feb 4, 2019

zombifier commented Feb 4, 2019

lannka Feb 6, 2019

zombifier Feb 6, 2019

lannka Feb 6, 2019

zombifier Feb 6, 2019

lannka Feb 6, 2019

zombifier Feb 6, 2019

lannka Feb 6, 2019

zombifier Feb 6, 2019

lannka Feb 6, 2019

zombifier Feb 6, 2019

lannka Feb 6, 2019

zombifier Feb 7, 2019

lannka Feb 6, 2019

zombifier Feb 8, 2019

jeffkaufman commented Feb 13, 2019

lannka commented Feb 13, 2019

jeffkaufman commented Feb 13, 2019

lannka commented Feb 26, 2019

lannka commented Feb 27, 2019 •

edited

zombifier commented Feb 27, 2019

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

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

Conversation

zombifier commented Jan 22, 2019 • edited

zombifier commented Jan 22, 2019 • edited

jeffkaufman commented Jan 30, 2019

zombifier commented Jan 30, 2019

zombifier commented Jan 31, 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

zombifier commented Feb 4, 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

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

jeffkaufman commented Feb 13, 2019

lannka commented Feb 13, 2019

jeffkaufman commented Feb 13, 2019

lannka commented Feb 26, 2019

lannka commented Feb 27, 2019 • edited

zombifier commented Feb 27, 2019

zombifier commented Jan 22, 2019 •

edited

zombifier commented Jan 22, 2019 •

edited

lannka commented Feb 27, 2019 •

edited