Skip to content
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

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

Merged
merged 27 commits into from
Feb 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
d6612a9
Make inabox-viewport measure the top level window directly if same do…
zombifier Jan 22, 2019
f03abfb
fix types
zombifier Jan 22, 2019
ea78dc5
Fix presubmit
zombifier Jan 22, 2019
4fbd385
Test disconnect function
zombifier Jan 25, 2019
f4b0d21
Fix lint in new test
zombifier Jan 25, 2019
4655d6a
Merge branch 'upstream' into inabox_friendly
zombifier Jan 28, 2019
5947bfb
Try enabling friendly, no host script inabox test on Safari
zombifier Jan 28, 2019
37d2b15
Trivial fix to rerun Travis
zombifier Jan 28, 2019
5b0627c
Try the no host script test with the example AMP ad
zombifier Jan 28, 2019
2dc3ce5
don't whitelist the iframe
zombifier Jan 28, 2019
a15e80f
More work
zombifier Jan 29, 2019
48771c6
Fix lint
zombifier Jan 29, 2019
18fcc29
Fix lint in test
zombifier Jan 29, 2019
f226409
fix oops
zombifier Jan 29, 2019
a862336
Throttle the listeners
zombifier Jan 29, 2019
ad8ca83
Use the existing inabox host observer instead of rewriting a new one
zombifier Jan 30, 2019
1d59138
fix type issue
zombifier Jan 30, 2019
bdbab11
Revert the changes to test-amp-inabox.js
zombifier Jan 31, 2019
d57a414
Gate the behavior behind experiment, and move canInspectWindow to hel…
zombifier Feb 4, 2019
5710e6f
Merge branch 'upstream' into inabox_friendly
zombifier Feb 6, 2019
46ef8d3
Merge branch 'upstream' into inabox_friendly
zombifier Feb 6, 2019
e36c257
Integrate the changes in #20599, and make sure the position observer …
zombifier Feb 6, 2019
1553d9c
Inline the function
zombifier Feb 6, 2019
bf4dc5a
Fix type
zombifier Feb 8, 2019
ee941d6
Merge branch 'upstream' into inabox_friendly
zombifier Feb 26, 2019
7f15bbb
Disconnect observer properly, and add a TODO to investigate a quirk i…
zombifier Feb 27, 2019
7780a9d
Merge branch 'upstream' into inabox_friendly
zombifier Feb 27, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 2 additions & 23 deletions ads/inabox/inabox-messaging-host.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
serializeMessage,
} from '../../src/3p-frame-messaging';
import {PositionObserver} from './position-observer';
import {canInspectWindow} from '../../src/iframe-helper';
import {dev, devAssert} from '../../src/log';
import {dict} from '../../src/utils/object';
import {getData} from '../../src/event-helper';
Expand Down Expand Up @@ -300,7 +301,7 @@ export class InaboxMessagingHost {
// this loop breaks immediately.
let topXDomainWin;
for (let j = 0, tempWin = win;
j < 10 && tempWin != tempWin.top && !canInspectWindow_(tempWin);
j < 10 && tempWin != tempWin.top && !canInspectWindow(tempWin);
j++, topXDomainWin = tempWin, tempWin = tempWin.parent) {}
// If topXDomainWin exists, we know that the frame we want to measure
// is a x-domain frame. Unfortunately, you can not access properties
Expand Down Expand Up @@ -347,25 +348,3 @@ export class InaboxMessagingHost {
}
}
}

/**
* Returns true if win's properties can be accessed and win is defined.
* This functioned is used to determine if a window is cross-domained
* from the perspective of the current window.
* @param {!Window} win
* @return {boolean}
* @private
*/
function canInspectWindow_(win) {
// TODO: this is not reliable. The compiler assumes that property reads are
// side-effect free. The recommended fix is to use goog.reflect.sinkValue
// but since we're not using the closure library I'm not sure how to do this.
// See https://github.com/google/closure-compiler/issues/3156
try {
// win['test'] could be truthy but not true the compiler shouldn't be able
// to optimize this check away.
return !!win.location.href && (win['test'] || true);
} catch (unusedErr) { // eslint-disable-line no-unused-vars
return false;
}
}
22 changes: 22 additions & 0 deletions src/iframe-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -546,3 +546,25 @@ export function disableScrollingOnIframe(iframe) {

return iframe;
}

/**
* Returns true if win's properties can be accessed and win is defined.
* This functioned is used to determine if a window is cross-domained
* from the perspective of the current window.
* @param {!Window} win
* @return {boolean}
* @private
*/
export function canInspectWindow(win) {
// TODO: this is not reliable. The compiler assumes that property reads are
// side-effect free. The recommended fix is to use goog.reflect.sinkValue
// but since we're not using the closure library I'm not sure how to do this.
// See https://github.com/google/closure-compiler/issues/3156
try {
// win['test'] could be truthy but not true the compiler shouldn't be able
// to optimize this check away.
return !!win.location.href && (win['test'] || true);
} catch (unusedErr) { // eslint-disable-line no-unused-vars
return false;
}
}
80 changes: 67 additions & 13 deletions src/inabox/inabox-viewport.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@

import {MessageType} from '../../src/3p-frame-messaging';
import {Observable} from '../observable';
import {PositionObserver} from '../../ads/inabox/position-observer';
import {Services} from '../services';
import {Viewport} from '../service/viewport/viewport-impl';
import {ViewportBindingDef} from '../service/viewport/viewport-binding-def';
import {canInspectWindow} from '../iframe-helper';
import {dev, devAssert} from '../log';
import {iframeMessagingClientFor} from './inabox-iframe-messaging-client';
import {isExperimentOn} from '../experiments';
Expand Down Expand Up @@ -149,35 +151,74 @@ export class ViewportBindingInabox {
/** @private @const {boolean} */
this.useLayers_ = isExperimentOn(this.win, 'layers');

/** @private {?../../ads/inabox/position-observer.PositionObserver} */
this.topWindowPositionObserver_ = null;

/** @private {?UnlistenDef} */
this.unobserveFunction_ = null;

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

/** @override */
connect() {
this.listenForPosition_();
if (isExperimentOn(this.win, 'inabox-viewport-friendly') &&
canInspectWindow(this.win.top)) {
this.listenForPositionSameDomain();
} else {
this.listenForPosition_();
}
}

/** @private */
listenForPosition_() {

this.iframeClient_.makeRequest(
MessageType.SEND_POSITIONS, MessageType.POSITION,
data => {
dev().fine(TAG, 'Position changed: ', data);
const oldViewportRect = this.viewportRect_;
this.viewportRect_ = data['viewportRect'];

this.updateBoxRect_(data['targetRect']);
this.updateLayoutRects_(data['viewportRect'], data['targetRect']);
});
}

if (isResized(this.viewportRect_, oldViewportRect)) {
this.resizeObservable_.fire();
}
if (isMoved(this.viewportRect_, oldViewportRect)) {
this.fireScrollThrottle_();
}
/** @visibleForTesting */
listenForPositionSameDomain() {
// 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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// TODO(lannka): Investigate why this is the case.
if (this.topWindowPositionObserver_) {
return Promise.resolve();
}
return Services.resourcesPromiseForDoc(this.win.document.documentElement)
.then(() => {
this.topWindowPositionObserver_ = new PositionObserver(this.win.top);
this.unobserveFunction_ = this.topWindowPositionObserver_.observe(
/** @type {!HTMLIFrameElement} */(this.win.frameElement),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/cc @zhouyx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

data => {
this.updateLayoutRects_(
data['viewportRect'],
data['targetRect']);
});
});
}

/**
* @private
* @param {!../layout-rect.LayoutRectDef} viewportRect
* @param {!../layout-rect.LayoutRectDef} targetRect
*/
updateLayoutRects_(viewportRect, targetRect) {
const oldViewportRect = this.viewportRect_;
this.viewportRect_ = viewportRect;
this.updateBoxRect_(targetRect);
if (isResized(this.viewportRect_, oldViewportRect)) {
this.resizeObservable_.fire();
}
if (isMoved(this.viewportRect_, oldViewportRect)) {
this.fireScrollThrottle_();
}
}

/** @override */
getLayoutRect(el) {
const b = el./*OK*/getBoundingClientRect();
Expand Down Expand Up @@ -283,6 +324,13 @@ export class ViewportBindingInabox {

/** @override */
getRootClientRectAsync() {
if (isExperimentOn(this.win, 'inabox-viewport-friendly') &&
canInspectWindow(this.win.top)) {
// Set up the listener if we haven't already.
return this.listenForPositionSameDomain().then(() =>
this.topWindowPositionObserver_.getTargetRect(
/** @type {!HTMLIFrameElement} */(this.win.frameElement)));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (!this.requestPositionPromise_) {
this.requestPositionPromise_ = new Promise(resolve => {
this.iframeClient_.requestOnce(
Expand Down Expand Up @@ -378,7 +426,13 @@ export class ViewportBindingInabox {
return dev().assertElement(this.win.document.body);
}

/** @override */ disconnect() {/* no-op */}
/** @override */
disconnect() {
if (this.unobserveFunction_) {
this.unobserveFunction_();
}
}

/** @override */ updatePaddingTop() {/* no-op */}
/** @override */ hideViewerHeader() {/* no-op */}
/** @override */ showViewerHeader() {/* no-op */}
Expand Down
9 changes: 9 additions & 0 deletions src/services.js
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,15 @@ export class Services {
getServiceForDoc(elementOrAmpDoc, 'resources'));
}

/**
* @param {!Element|!./service/ampdoc-impl.AmpDoc} elementOrAmpDoc
* @return {!Promise<!./service/resources-impl.Resources>}
*/
static resourcesPromiseForDoc(elementOrAmpDoc) {
return /** @type {!Promise<!./service/resources-impl.Resources>} */ (
getServicePromiseForDoc(elementOrAmpDoc, 'resources'));
}

/**
* @param {!Window} win
* @return {?Promise<?{incomingFragment: string, outgoingFragment: string}>}
Expand Down
Loading