diff --git a/extensions/amp-bind/0.1/bind-impl.js b/extensions/amp-bind/0.1/bind-impl.js index b8366bc4dcc6..835625940903 100644 --- a/extensions/amp-bind/0.1/bind-impl.js +++ b/extensions/amp-bind/0.1/bind-impl.js @@ -86,6 +86,23 @@ let BoundPropertyDef; */ let BoundElementDef; +/** + * @enum {number} + */ +const HistoryOp = { + REPLACE: 0, + PUSH: 1, +}; + +/** + * @typedef {{ + * op: !HistoryOp, + * data: (!JsonObject|undefined), + * onPop: (!Function|undefined) + * }} + */ +let HistoryTaskDef; + /** * A map of tag names to arrays of attributes that do not have non-bind * counterparts. For instance, amp-carousel allows a `[slide]` attribute, @@ -140,7 +157,7 @@ export class Bind { */ this.actionSequenceIds_ = []; - /** @const @private {!Function} */ + /** @const @private {function()} */ this.eventuallyClearActionSequenceIds_ = debounce( this.win_, () => { @@ -219,12 +236,25 @@ export class Bind { /** @const @private {!Deferred} */ this.addMacrosDeferred_ = new Deferred(); - /** @private {Promise} */ + /** @private {?Promise} */ this.setStatePromise_ = null; /** @private @const {!../../../src/utils/signals.Signals} */ this.signals_ = new Signals(); + /** + * A queue of history replace/push to adhere to browser rate-limits. + * @private @const {!Array} + */ + this.historyQueue_ = []; + + /** @private @const {!Function} */ + this.debouncedFlushHistoryQueue_ = debounce( + this.win_, + () => this.flushHistoryQueue_(), + 1000 + ); + // Install debug tools. const g = self.AMP; g.printState = g.printState || this.debugPrintState_.bind(this); @@ -346,12 +376,12 @@ export class Bind { dev().info(TAG, 'setState:', expression); this.setStatePromise_ = this.evaluateExpression_(expression, scope) .then(result => this.setState(result)) - .then(() => this.getDataForHistory_()) - .then(data => { - // Don't bother calling History.replace with empty data. - if (data) { - this.history_.replace(data); - } + .then(() => { + return this.getDataForHistory_().then(data => { + if (data) { + this.pushToHistoryQueue_({op: HistoryOp.REPLACE, data}); + } + }); }); return this.setStatePromise_; } @@ -378,32 +408,76 @@ export class Bind { }); const onPop = () => this.setState(oldState); - return this.setState(result) - .then(() => this.getDataForHistory_()) - .then(data => { - this.history_.push(onPop, data); + return this.setState(result).then(() => { + return this.getDataForHistory_().then(data => { + this.pushToHistoryQueue_({op: HistoryOp.PUSH, data, onPop}); }); + }); + }); + } + + /** + * @param {!HistoryTaskDef} task + * @private + */ + pushToHistoryQueue_(task) { + let enqueue = true; + + // Collapse if consecutive "replace" operations. + if (task.op === HistoryOp.REPLACE && this.historyQueue_.length > 0) { + const lastTask = this.historyQueue_[this.historyQueue_.length - 1]; + if (lastTask.op === HistoryOp.REPLACE) { + lastTask.data = task.data; + enqueue = false; + } + } + + if (enqueue) { + this.historyQueue_.push(task); + } + + if (task.op === HistoryOp.PUSH) { + // Immediately flush queue on "push" to avoid adding latency to + // the browser "back" functionality. + this.flushHistoryQueue_(); + } else { + // Debounce flush queue on "replace" to rate limit History.replace. + this.debouncedFlushHistoryQueue_(); + } + } + + /** + * @private + */ + flushHistoryQueue_() { + this.historyQueue_.forEach(({op, data, onPop}) => { + if (op === HistoryOp.REPLACE) { + this.history_.replace(data); + } else if (op === HistoryOp.PUSH) { + this.history_.push(onPop, data); + } }); + this.historyQueue_.length = 0; } /** * Returns data that should be saved in browser history on AMP.setState() or * AMP.pushState(). This enables features like restoring browser tabs. - * @return {!Promise} + * @return {!Promise<(!JsonObject|undefined)>} */ getDataForHistory_() { + // Copy so subsequent changes don't modify the stored object reference. const data = dict({ - 'data': dict({'amp-bind': this.state_}), + 'data': dict({'amp-bind': this.copyJsonObject_(this.state_)}), 'title': this.localWin_.document.title, }); if (!this.viewer_.isEmbedded()) { - // CC doesn't recognize !JsonObject as a subtype of (JsonObject|null). - return /** @type {!Promise} */ (Promise.resolve(data)); + return Promise.resolve(data); } // Only pass state for history updates to trusted viewers, since they // may contain user data e.g. form input. return this.viewer_.isTrustedViewer().then(trusted => { - return trusted ? data : null; + return trusted ? data : undefined; }); } diff --git a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js index c0e3e2af42db..a2507ab7e1d9 100644 --- a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js +++ b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js @@ -828,8 +828,8 @@ describe describe('history', () => { beforeEach(() => { - sandbox.spy(history, 'replace'); - sandbox.spy(history, 'push'); + sandbox.stub(history, 'replace'); + sandbox.stub(history, 'push'); sandbox.stub(viewer, 'isEmbedded').returns(true); }); @@ -842,6 +842,9 @@ describe {one: 1} ); return promise.then(() => { + // History.replace has a 1s debounce. + clock.tick(1000); + // Shouldn't call replace() with null `data`. expect(history.replace).to.not.be.called; }); @@ -855,7 +858,10 @@ describe return promise.then(() => { expect(history.push).to.be.called; // `data` param should be null on untrusted viewers. - expect(history.push).to.be.calledWith(sinon.match.func, null); + expect(history.push).to.be.calledWith( + sinon.match.func, + undefined + ); }); }); }); @@ -875,6 +881,11 @@ describe {one: 1} ); return promise.then(() => { + expect(history.replace).to.not.be.called; + + // History.replace has a 1s debounce. + clock.tick(1000); + expect(history.replace).calledOnce; // `data` param should exist on trusted viewers. expect(history.replace).calledWith({ @@ -898,6 +909,39 @@ describe }); }); }); + + it('should collapse consecutive replaces', async () => { + await bind.setStateWithExpression('{foo: 1}', {}); + await bind.setStateWithExpression('{foo: 2}', {}); + + expect(history.replace).to.not.be.called; + + clock.tick(1000); + + expect(history.replace).calledOnce; + expect(history.replace).calledWith({ + data: {'amp-bind': {foo: 2}}, + title: '', + }); + }); + + it('should flush history immediately after push', async () => { + await bind.setStateWithExpression('{foo: 1}', {}); + + expect(history.replace).to.not.be.called; + expect(history.push).to.not.be.called; + + await bind.pushStateWithExpression('{bar: 2}', {}); + + expect(history.replace).calledWith({ + data: {'amp-bind': {foo: 1}}, + title: '', + }); + expect(history.push).calledWith(sinon.match.func, { + data: {'amp-bind': {foo: 1, bar: 2}}, + title: '', + }); + }); }); });