Wait for the popping confirmation to arrive before pushing anything t…

…o history
ampproject · Oct 14, 2016 · 27f10e9 · 27f10e9
1 parent 43828ce
commit 27f10e9
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 24 deletions.
diff --git a/examples/viewer.html b/examples/viewer.html
@@ -364,7 +364,7 @@
         throw new Error('Only one step push allowed');
       }
       this.stackIndex_ = stackIndex;
-      window.history.pushState({stackIndex: stackIndex}, '');
+      window.history.pushState({}, '');
       return Promise.resolve();
     };
 
@@ -385,18 +385,9 @@
       if (this.visibilityState_ != 'visible') {
         return;
       }
-      console.log('popping: ', e.state);
-      var poppedIndex = e.state && e.state.stackIndex;
-
-      if (poppedIndex <= this.stackIndex_) {
-        // Going Back in history.
-        // Even more trivial. Always assumes that only one step was popped in
-        // history.
-        this.stackIndex_--;
-      } else {
-        // Going Forward in history.
-        this.stackIndex_++;
-      }
+      // Even more trivial. Always assumes that only one step was popped in
+      // history.
+      this.stackIndex_--;
       this.sendRequest_('historyPopped', {
         newStackIndex: this.stackIndex_
       }, false);

diff --git a/src/document-click.js b/src/document-click.js
@@ -217,11 +217,9 @@ export function onDocumentElementClick_(
   }
 
   if (tgtLoc.hash != curLoc.hash) {
-    timerFor(win).delay(() => {
     // Push/pop history.
-      history.push(() => {
-        win.location.replace(`${curLoc.hash || '#'}`);
-      });
-    }, 1000);
+    history.push(() => {
+      win.location.replace(`${curLoc.hash || '#'}`);
+    });
   }
 }
diff --git a/src/service/history-impl.js b/src/service/history-impl.js
@@ -573,8 +573,12 @@ export class HistoryBindingVirtual_ {
 
   /**
    * @param {!./viewer-impl.Viewer} viewer
+   * @param {!Window} win
    */
-  constructor(viewer) {
+  constructor(viewer, win) {
+    /** @private @const {!../service/timer-impl.Timer} */
+    this.timer_ = timerFor(win);
+
     /** @private @const */
     this.viewer_ = viewer;
 
@@ -587,6 +591,12 @@ export class HistoryBindingVirtual_ {
     /** @private {!UnlistenDef} */
     this.unlistenOnHistoryPopped_ = this.viewer_.onHistoryPoppedEvent(
         this.onHistoryPopped_.bind(this));
+
+    /** @private {!Promise} */
+    this.awaitPoppingPromise_ = Promise.resolve();
+
+    /** @private {!function(*)) */
+    this.awaitPoppingPromiseResolver_ = null;
   }
 
   /** @override */
@@ -601,14 +611,25 @@ export class HistoryBindingVirtual_ {
 
   /** @override */
   push() {
-    // Current implementation doesn't wait for response from viewer.
-    this.updateStackIndex_(this.stackIndex_ + 1);
-    this.viewer_.postPushHistory(this.stackIndex_);
-    return Promise.resolve(this.stackIndex_);
+    // Waiting for popping confirmation to happen otherwise a race condition
+    // between popping and pushing might happen causing history to be messed
+    // up. This will only wait 100ms to avoid getting stuck if confirmation
+    // never arrived.
+    return Promise.race([this.timer_.promise(100), this.awaitPoppingPromise_])
+        .then(() => {
+          // Current implementation doesn't wait for response from viewer.
+          this.updateStackIndex_(this.stackIndex_ + 1);
+          this.viewer_.postPushHistory(this.stackIndex_);
+          return Promise.resolve(this.stackIndex_);
+        });
   }
 
   /** @override */
   pop(stackIndex) {
+    this.awaitPoppingPromise_ = new Promise(resolve => {
+      this.awaitPoppingPromiseResolver_ = resolve;
+    });
+
     if (stackIndex > this.stackIndex_) {
       return Promise.resolve(this.stackIndex_);
     }
@@ -623,6 +644,10 @@ export class HistoryBindingVirtual_ {
    */
   onHistoryPopped_(event) {
     this.updateStackIndex_(event.newStackIndex);
+    if (this.awaitPoppingPromiseResolver_) {
+      this.awaitPoppingPromiseResolver_();
+      this.awaitPoppingPromiseResolver_ = null;
+    }
   }
 
   /**
@@ -652,7 +677,7 @@ function createHistory(ampdoc) {
   let binding;
   if (viewer.isOvertakeHistory() || getMode(ampdoc.win).test ||
           ampdoc.win.AMP_TEST_IFRAME) {
-    binding = new HistoryBindingVirtual_(viewer);
+    binding = new HistoryBindingVirtual_(viewer, ampdoc.win);
   } else {
     // Only one global "natural" binding is allowed since it works with the
     // global history stack.