Skip to content

Commit

Permalink
amp-bind: Delay init until viewer first visible (#9033)
Browse files Browse the repository at this point in the history
* make bind wait until viewer visible

* fix lint

* fix tests
  • Loading branch information
William Chou authored and jridgewell committed Apr 28, 2017
1 parent 5da74eb commit e38a43b
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 25 deletions.
41 changes: 24 additions & 17 deletions extensions/amp-bind/0.1/amp-state.js
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {bindForDoc} from '../../../src/services';
import {bindForDoc, viewerForDoc} from '../../../src/services';
import {fetchBatchedJsonFor} from '../../../src/batched-json';
import {isBindEnabledFor} from './bind-impl';
import {isJsonScriptTag} from '../../../src/dom';
Expand Down Expand Up @@ -52,11 +52,32 @@ export class AmpState extends AMP.BaseElement {
user().assert(isBindEnabledFor(this.win),
`Experiment "amp-bind" is disabled.`);

const TAG = this.getName_();

toggle(this.element, /* opt_display */ false);
this.element.setAttribute('aria-hidden', 'true');

// Don't parse or fetch in prerender mode.
const viewer = viewerForDoc(this.getAmpDoc());
viewer.whenFirstVisible().then(() => this.initialize_());
}

/** @override */
mutatedAttributesCallback(mutations) {
const src = mutations['src'];
if (src !== undefined) {
this.fetchSrcAndUpdateState_(/* isInit */ false);
}
}

/** @override */
renderOutsideViewport() {
// We want the state data to be available wherever it is in the document.
return true;
}

/** @private */
initialize_() {
const TAG = this.getName_();

// Fetch JSON from endpoint at `src` attribute if it exists,
// otherwise parse child script tag.
if (this.element.hasAttribute('src')) {
Expand All @@ -83,20 +104,6 @@ export class AmpState extends AMP.BaseElement {
}
}

/** @override */
mutatedAttributesCallback(mutations) {
const src = mutations['src'];
if (src !== undefined) {
this.fetchSrcAndUpdateState_(/* isInit */ false);
}
}

/** @override */
renderOutsideViewport() {
// We want the state data to be available wherever it is in the document.
return true;
}

/**
* @param {boolean} isInit
* @private
Expand Down
16 changes: 12 additions & 4 deletions extensions/amp-bind/0.1/bind-impl.js
Expand Up @@ -27,7 +27,11 @@ import {isExperimentOn} from '../../../src/experiments';
import {invokeWebWorker} from '../../../src/web-worker/amp-worker';
import {isFiniteNumber} from '../../../src/types';
import {reportError} from '../../../src/error';
import {ampFormServiceForDoc, resourcesForDoc} from '../../../src/services';
import {
ampFormServiceForDoc,
resourcesForDoc,
viewerForDoc,
} from '../../../src/services';
import {filterSplice} from '../../../src/utils/array';
import {rewriteAttributeValue} from '../../../src/sanitizer';

Expand Down Expand Up @@ -127,13 +131,17 @@ export class Bind {
this.evaluator_ = new BindEvaluator();
}

/** @const @private {!../../../src/service/viewer-impl.Viewer} */
this.viewer_ = viewerForDoc(this.ampdoc);

/**
* Resolved when the service is fully initialized.
* @const @private {Promise}
*/
this.initializePromise_ = this.ampdoc.whenReady().then(() => {
return this.initialize_();
});
this.initializePromise_ = Promise.all([
this.ampdoc.whenReady(),
this.viewer_.whenFirstVisible(), // Don't initialize in prerender mode.
]).then(() => this.initialize_());

/**
* Form implementations are not filled in until ampdoc is ready.
Expand Down
8 changes: 4 additions & 4 deletions extensions/amp-bind/0.1/test/test-amp-state.js
Expand Up @@ -48,17 +48,17 @@ describe('AmpState', () => {
sandbox.restore();
});

it('should parse its child script if `src` is not present at build', () => {
it('should parse its child script if `src` is not present at init', () => {
ampState.innerHTML = '<script type="application/json">' +
'{"foo": "bar"}</script>';
ampState.implementation_.buildCallback();
ampState.implementation_.initialize_();
expect(fetchStub).to.not.have.been.called;
expect(updateStub).calledWithMatch({foo: 'bar'});
});

it('should fetch json if `src` is present at build', () => {
it('should fetch json if `src` is present at init', () => {
ampState.setAttribute('src', 'https://foo.com/bar?baz=1');
ampState.implementation_.buildCallback();
ampState.implementation_.initialize_();
expect(fetchStub).calledWith(/* opt_isInit */ true);
expect(updateStub).to.not.have.been.called;
});
Expand Down

0 comments on commit e38a43b

Please sign in to comment.