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.

Sign up for GitHub

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

Jump to bottom

Clean up: Move viewport specific logic out from viewer. #5887

Merged
merged 4 commits into from Nov 1, 2016
Merged

Clean up: Move viewport specific logic out from viewer. #5887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b326ff
Move viewport specific logic out from viewer.
lannka Oct 28, 2016
58fa30d
Fix linter.
lannka Oct 28, 2016
092970d
Fix linter.
lannka Oct 28, 2016
09fa4ff
Fix type checks.
lannka Nov 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 0 additions & 53 deletions src/service/viewer-impl.js
View file
Open in desktop
Expand Up @@ -16,7 +16,6 @@

import {Observable} from '../observable';
import {documentStateFor} from '../document-state';
import {getMode} from '../mode';
import {getServiceForDoc} from '../service';
import {dev} from '../log';
import {
Expand All @@ -25,7 +24,6 @@ import {
parseUrl,
removeFragment,
} from '../url';
import {platformFor} from '../platform';
import {timerFor} from '../timer';
import {reportError} from '../error';
import {VisibilityState} from '../visibility-state';
Expand All @@ -43,29 +41,6 @@ const SENTINEL_ = '__AMP__';
*/
const VIEWER_ORIGIN_TIMEOUT_ = 1000;

/**
* The type of the viewport.
* @enum {string}
*/
export const ViewportType = {

/**
* Viewer leaves sizing and scrolling up to the AMP document's window.
*/
NATURAL: 'natural',

/**
* This is AMP-specific type and doesn't come from viewer. This is the type
* that AMP sets when Viewer has requested "natural" viewport on a iOS
* device.
* See:
* https://github.com/ampproject/amphtml/blob/master/spec/amp-html-layout.md
* and {@link ViewportBindingNaturalIosEmbed_} for more details.
*/
NATURAL_IOS_EMBED: 'natural-ios-embed',
};


/**
* These domains are trusted with more sensitive viewer operations such as
* propagating the referrer. If you believe your domain should be here,
Expand Down Expand Up @@ -136,9 +111,6 @@ export class Viewer {
/** @private {number} */
this.prerenderSize_ = 1;

/** @private {!ViewportType} */
this.viewportType_ = ViewportType.NATURAL;

/** @private {number} */
this.paddingTop_ = 0;

Expand Down Expand Up @@ -223,22 +195,6 @@ export class Viewer {
this.prerenderSize_;
dev().fine(TAG_, '- prerenderSize:', this.prerenderSize_);

this.viewportType_ = this.params_['viewportType'] || this.viewportType_;
// Configure scrolling parameters when AMP is iframed on iOS.
const platform = platformFor(this.win);
if (this.viewportType_ == ViewportType.NATURAL && this.isIframed_ &&
platform.isIos()) {
this.viewportType_ = ViewportType.NATURAL_IOS_EMBED;
}
// Enable iOS Embedded mode so that it's easy to test against a more
// realistic iOS environment.
if (platform.isIos() &&
this.viewportType_ != ViewportType.NATURAL_IOS_EMBED &&
(getMode(this.win).localDev || getMode(this.win).development)) {
this.viewportType_ = ViewportType.NATURAL_IOS_EMBED;
}
dev().fine(TAG_, '- viewportType:', this.viewportType_);

this.paddingTop_ = parseInt(this.params_['paddingTop'], 10) ||
this.paddingTop_;
dev().fine(TAG_, '- padding-top:', this.paddingTop_);
Expand Down Expand Up @@ -645,15 +601,6 @@ export class Viewer {
return this.prerenderSize_;
}

/**
* See `ViewportType` enum for the set of allowed values.
* See {@link Viewport} and {@link ViewportBinding} for more details.
* @return {!ViewportType}
*/
getViewportType() {
return this.viewportType_;
}

/**
* Returns the top padding requested by the viewer.
* @return {number}
Expand Down
46 changes: 43 additions & 3 deletions src/service/viewport-impl.js
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ import {installVsyncService} from './vsync-impl';
import {installViewerServiceForDoc} from './viewer-impl';
import {isExperimentOn} from '../experiments';
import {waitForBody} from '../dom';

import {getMode} from '../mode';

const TAG_ = 'Viewport';

Expand Down Expand Up @@ -1687,7 +1687,7 @@ function createViewport(ampdoc) {
const viewer = installViewerServiceForDoc(ampdoc);
let binding;
if (ampdoc.isSingleDoc() &&
viewer.getViewportType() == 'natural-ios-embed') {
getViewportType(ampdoc.win, viewer) == ViewportType.NATURAL_IOS_EMBED) {
if (isExperimentOn(ampdoc.win, 'ios-embed-wrapper')) {
binding = new ViewportBindingIosEmbedWrapper_(ampdoc.win);
} else {
Expand All @@ -1699,6 +1699,46 @@ function createViewport(ampdoc) {
return new Viewport(ampdoc, binding, viewer);
}

/**
* The type of the viewport.
* @enum {string}
*/
const ViewportType = {

/**
* Viewer leaves sizing and scrolling up to the AMP document's window.
*/
NATURAL: 'natural',

/**
* This is AMP-specific type and doesn't come from viewer. This is the type
* that AMP sets when Viewer has requested "natural" viewport on a iOS
* device.
* See:
* https://github.com/ampproject/amphtml/blob/master/spec/amp-html-layout.md
* and {@link ViewportBindingNaturalIosEmbed_} for more details.
*/
NATURAL_IOS_EMBED: 'natural-ios-embed',
};

/**
* @param {!Window} win
* @param {!./viewer-impl.Viewer} viewer
* @return {string}
*/
function getViewportType(win, viewer) {
let viewportType = viewer.getParam('viewportType') || ViewportType.NATURAL;
if (platformFor(win).isIos()
&& ((viewportType == ViewportType.NATURAL && viewer.isIframed())
// Enable iOS Embedded mode so that it's easy to test against a more
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's' move this comment to the assignment.

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 comment is specific to dev mode, so better be here

// realistic iOS environment.
|| getMode(win).localDev
|| getMode(win).development)) {
viewportType = ViewportType.NATURAL_IOS_EMBED;
}
dev().fine(TAG_, '- viewportType:', viewportType);
return viewportType;
}

/**
* @param {!./ampdoc-impl.AmpDoc} ampdoc
Expand All @@ -1707,4 +1747,4 @@ function createViewport(ampdoc) {
export function installViewportServiceForDoc(ampdoc) {
return /** @type {!Viewport} */ (getServiceForDoc(ampdoc, 'viewport',
ampdoc => createViewport(ampdoc)));
};
}
28 changes: 1 addition & 27 deletions test/functional/test-viewer.js
View file
Open in desktop
Expand Up @@ -16,7 +16,6 @@

import {Viewer} from '../../src/service/viewer-impl';
import {dev} from '../../src/log';
import {platformFor} from '../../src/platform';
import {installDocService} from '../../src/service/ampdoc-impl';
import {installPlatformService} from '../../src/service/platform-impl';
import {installPerformanceService} from '../../src/service/performance-impl';
Expand All @@ -36,7 +35,6 @@ describe('Viewer', () => {
let clock;
let events;
let errorStub;
let platform;

function changeVisibility(vis) {
windowApi.document.hidden = vis !== 'visible';
Expand Down Expand Up @@ -89,7 +87,6 @@ describe('Viewer', () => {
events = {};
errorStub = sandbox.stub(dev(), 'error');
windowMock = sandbox.mock(windowApi);
platform = platformFor(windowApi);
viewer = new Viewer(ampdoc);
});

Expand All @@ -100,16 +97,14 @@ describe('Viewer', () => {
sandbox.restore();
});

it('should configure as natural viewport by default', () => {
expect(viewer.getViewportType()).to.equal('natural');
it('should configure as 0 padding top by default', () => {
expect(viewer.getPaddingTop()).to.equal(0);
});

it('should configure correctly based on window name and hash', () => {
windowApi.name = '__AMP__viewportType=natural';
windowApi.location.hash = '#paddingTop=17&other=something';
const viewer = new Viewer(ampdoc);
expect(viewer.getViewportType()).to.equal('natural');
expect(viewer.getPaddingTop()).to.equal(17);

// All of the startup params are also available via getParam.
Expand Down Expand Up @@ -340,27 +335,6 @@ describe('Viewer', () => {
expect(send.callCount).to.equal(0);
});

it('should configure correctly for iOS embedding', () => {
windowApi.name = '__AMP__viewportType=natural';
windowApi.parent = {};
sandbox.mock(platform).expects('isIos').returns(true).atLeast(1);
const viewer = new Viewer(ampdoc);

expect(viewer.getViewportType()).to.equal('natural-ios-embed');
});

it('should NOT configure for iOS embedding if not embedded', () => {
windowApi.name = '__AMP__viewportType=natural';
windowApi.parent = windowApi;
sandbox.mock(platform).expects('isIos').returns(true).atLeast(1);
windowApi.AMP_MODE = {
localDev: false,
development: false,
};
const viewportType = new Viewer(ampdoc).getViewportType();
expect(viewportType).to.equal('natural');
});

it('should receive viewport event', () => {
let viewportEvent = null;
viewer.onViewportEvent(event => {
Expand Down
55 changes: 55 additions & 0 deletions test/functional/test-viewport.js
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@

import {AmpDocSingle, installDocService} from '../../src/service/ampdoc-impl';
import {
installViewportServiceForDoc,
Viewport,
ViewportBindingDef,
ViewportBindingIosEmbedWrapper_,
Expand Down Expand Up @@ -1532,3 +1533,57 @@ describes.realWin('ViewportBindingIosEmbedWrapper', {ampCss: true}, env => {
});
});
});

describe('createViewport', () => {

describes.fakeWin('in Android', {win: {navigator: {userAgent: 'Android'}}},
env => {
let win;

beforeEach(() => {
win = env.win;
installPlatformService(win);
installTimerService(win);
});

it('should bind to "natural" when not iframed', () => {
win.parent = win;
const ampDoc = installDocService(win, true).getAmpDoc();
const viewport = installViewportServiceForDoc(ampDoc);
expect(viewport.binding_).to.be.instanceof(ViewportBindingNatural_);
});

it('should bind to "naturual" when iframed', () => {
win.parent = {};
const ampDoc = installDocService(win, true).getAmpDoc();
const viewport = installViewportServiceForDoc(ampDoc);
expect(viewport.binding_).to.be.instanceof(ViewportBindingNatural_);
});
});

describes.fakeWin('in iOS', {win: {navigator: {userAgent: 'iPhone'}}},
env => {
let win;

beforeEach(() => {
win = env.win;
installPlatformService(win);
installTimerService(win);
});

it('should bind to "natural" when not iframed', () => {
win.parent = win;
const ampDoc = installDocService(win, true).getAmpDoc();
const viewport = installViewportServiceForDoc(ampDoc);
expect(viewport.binding_).to.be.instanceof(ViewportBindingNatural_);
});

it('should bind to "natural iOS embed" when iframed', () => {
win.parent = {};
const ampDoc = installDocService(win, true).getAmpDoc();
const viewport = installViewportServiceForDoc(ampDoc);
expect(viewport.binding_).to
.be.instanceof(ViewportBindingNaturalIosEmbed_);
});
});
});