Fix amp-sidebar local navigation and history

examples/viewer.html

@@ -381,7 +381,10 @@
     };
 
 
-    Viewer.prototype.onPopState_ = function() {
+    Viewer.prototype.onPopState_ = function(e) {
+      if (this.visibilityState_ != 'visible') {
+        return;
+      }
       // Even more trivial. Always assumes that only one step was popped in
       // history.
       this.stackIndex_--;

extensions/amp-sidebar/0.1/amp-sidebar.js

@@ -14,9 +14,12 @@
  * limitations under the License.
  */
 
+import {closestByTag} from '../../../src/dom';
 import {CSS} from '../../../build/amp-sidebar-0.1.css';
+import {dev} from '../../../src/log';
 import {Layout} from '../../../src/layout';
 import {historyForDoc} from '../../../src/history';
+import {parseUrl} from '../../../src/url';
 import {platformFor} from '../../../src/platform';
 import {setStyles, toggle} from '../../../src/style';
 import {vsyncFor} from '../../../src/vsync';
@@ -107,14 +110,25 @@ export class AmpSidebar extends AMP.BaseElement {
     screenReaderCloseButton.classList.add('-amp-screen-reader');
     // This is for screen-readers only, should not get a tab stop.
     screenReaderCloseButton.tabIndex = -1;
-    screenReaderCloseButton.addEventListener('click', () => {
-      this.close_();
-    });
+    screenReaderCloseButton.addEventListener('click', () => this.close_());
     this.element.appendChild(screenReaderCloseButton);
 
     this.registerAction('toggle', this.toggle_.bind(this));
     this.registerAction('open', this.open_.bind(this));
     this.registerAction('close', this.close_.bind(this));
+
+    this.element.addEventListener('click', e => {
+      const target = closestByTag(dev().assertElement(e.target), 'A');
+      if (!target) {
+        return;
+      }
+      const tgtLoc = parseUrl(target.href);
+
+      if (!tgtLoc.hash) {
+        return;
+      }
+      this.close_();
+    });
   }
 
  /**
@@ -172,7 +186,7 @@ export class AmpSidebar extends AMP.BaseElement {
         }, ANIMATION_TIMEOUT);
       });
     });
-    this.getHistory_().push(this.close_.bind(this)).then(historyId => {
+    this.getHistory_().push(() => this.close_()).then(historyId => {
       this.historyId_ = historyId;
     });
   }
@@ -213,9 +227,7 @@ export class AmpSidebar extends AMP.BaseElement {
     if (!this.maskElement_) {
       const mask = this.document_.createElement('div');
       mask.classList.add('-amp-sidebar-mask');
-      mask.addEventListener('click', () => {
-        this.close_();
-      });
+      mask.addEventListener('click', () => this.close_());
       this.element.parentNode.appendChild(mask);
       mask.addEventListener('touchmove', e => {
         e.preventDefault();

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.