Fix local navigation history management by handling hash navigations

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

@@ -15,8 +15,9 @@
  */
 
 import {CSS} from '../../../build/amp-sidebar-0.1.css';
-import {tryFocus} from '../../../src/dom';
+import {closestByTag, tryFocus} from '../../../src/dom';
 import {Layout} from '../../../src/layout';
+import {dev} from '../../../src/log';
 import {historyForDoc} from '../../../src/history';
 import {platformFor} from '../../../src/platform';
 import {setStyles, toggle} from '../../../src/style';
@@ -122,6 +123,13 @@ export class AmpSidebar extends AMP.BaseElement {
     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 && target.href) {
+        this.close_();
+      }
+    }, true);
   }
 
  /**

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

@@ -41,6 +41,9 @@ describe('amp-sidebar', () => {
         list.appendChild(li);
       }
       ampSidebar.appendChild(list);
+      const anchor = iframe.doc.createElement('a');
+      anchor.href = '#section1';
+      ampSidebar.appendChild(anchor);
       if (options.side) {
         ampSidebar.setAttribute('side', options.side);
       }
@@ -271,6 +274,74 @@ describe('amp-sidebar', () => {
     });
   });
 
+
+  it('should close sidebar if clicked on anchor', () => {
+    return getAmpSidebar().then(obj => {
+      const sidebarElement = obj.ampSidebar;
+      const anchor = sidebarElement.getElementsByTagName('a')[0];
+      const impl = sidebarElement.implementation_;
+      impl.schedulePause = sandbox.spy();
+      impl.vsync_ = {
+        mutate: function(callback) {
+          callback();
+        },
+      };
+      sandbox.stub(timer, 'delay', function(callback) {
+        callback();
+      });
+      expect(sidebarElement.hasAttribute('open')).to.be.false;
+      impl.open_();
+      expect(sidebarElement.hasAttribute('open')).to.be.true;
+      expect(sidebarElement.getAttribute('aria-hidden')).to.equal('false');
+      const eventObj = document.createEventObject ?
+          document.createEventObject() : document.createEvent('Events');
+      if (eventObj.initEvent) {
+        eventObj.initEvent('click', true, true);
+      }
+      anchor.dispatchEvent ?
+          anchor.dispatchEvent(eventObj) :
+          anchor.fireEvent('onkeydown', eventObj);
+      expect(sidebarElement.hasAttribute('open')).to.be.false;
+      expect(sidebarElement.getAttribute('aria-hidden')).to.equal('true');
+      expect(sidebarElement.style.display).to.equal('none');
+      expect(impl.schedulePause.callCount).to.equal(1);
+    });
+  });
+
+
+  it('should not close sidebar if clicked on non-anchor', () => {
+    return getAmpSidebar().then(obj => {
+      const sidebarElement = obj.ampSidebar;
+      const li = sidebarElement.getElementsByTagName('li')[0];
+      const impl = sidebarElement.implementation_;
+      impl.schedulePause = sandbox.spy();
+      impl.vsync_ = {
+        mutate: function(callback) {
+          callback();
+        },
+      };
+      sandbox.stub(timer, 'delay', function(callback) {
+        callback();
+      });
+      expect(sidebarElement.hasAttribute('open')).to.be.false;
+      impl.open_();
+      expect(sidebarElement.hasAttribute('open')).to.be.true;
+      expect(sidebarElement.getAttribute('aria-hidden')).to.equal('false');
+      const eventObj = document.createEventObject ?
+          document.createEventObject() : document.createEvent('Events');
+      if (eventObj.initEvent) {
+        eventObj.initEvent('click', true, true);
+      }
+      li.dispatchEvent ?
+          li.dispatchEvent(eventObj) :
+          li.fireEvent('onkeydown', eventObj);
+      expect(sidebarElement.hasAttribute('open')).to.be.true;
+      expect(sidebarElement.getAttribute('aria-hidden')).to.equal('false');
+      expect(sidebarElement.style.display).to.equal('');
+      expect(impl.schedulePause.callCount).to.equal(0);
+    });
+  });
+
   it('should reflect state of the sidebar', () => {
     return getAmpSidebar().then(obj => {
       const sidebarElement = obj.ampSidebar;

src/document-click.js

@@ -64,12 +64,13 @@ export class ClickHandler {
     /** @private @const {boolean} */
     this.isIosSafari_ = platform.isIos() && platform.isSafari();
 
-    // Only intercept clicks when iframed.
-    if (this.viewer_.isIframed() && this.viewer_.isOvertakeHistory()) {
-      /** @private @const {!function(!Event)|undefined} */
-      this.boundHandle_ = this.handle_.bind(this);
-      this.ampdoc.getRootNode().addEventListener('click', this.boundHandle_);
-    }
+    /** @private @const {boolean} */
+    this.isIframed_ = (this.viewer_.isIframed() &&
+        this.viewer_.isOvertakeHistory());
+
+    /** @private @const {!function(!Event)|undefined} */
+    this.boundHandle_ = this.handle_.bind(this);
+    this.ampdoc.getRootNode().addEventListener('click', this.boundHandle_);
   }
 
   /**
@@ -89,7 +90,8 @@ export class ClickHandler {
    */
   handle_(e) {
     onDocumentElementClick_(
-        e, this.ampdoc, this.viewport_, this.history_, this.isIosSafari_);
+        e, this.ampdoc, this.viewport_, this.history_, this.isIosSafari_,
+        this.isIframed_);
   }
 }
 
@@ -106,9 +108,10 @@ export class ClickHandler {
  * @param {!./service/viewport-impl.Viewport} viewport
  * @param {!./service/history-impl.History} history
  * @param {boolean} isIosSafari
+ * @param {boolean} isIframed
  */
 export function onDocumentElementClick_(
-    e, ampdoc, viewport, history, isIosSafari) {
+    e, ampdoc, viewport, history, isIosSafari, isIframed) {
   if (e.defaultPrevented) {
     return;
   }
@@ -119,10 +122,30 @@ export function onDocumentElementClick_(
   }
   urlReplacementsForDoc(ampdoc).maybeExpandLink(target);
 
-  /** @const {!Window} */
-  const win = ampdoc.win;
   const tgtLoc = parseUrl(target.href);
+  // Handle custom protocols only if the document is iframe'd.
+  if (isIframed) {
+    handleCustomProtocolClick_(e, target, tgtLoc, ampdoc, isIosSafari);
+  }
+
+  if (tgtLoc.hash) {
+    handleHashClick_(e, tgtLoc, ampdoc, viewport, history);
+  }
+}
+
 
+/**
+ * Handles clicking on a custom protocol link.
+ * @param {!Event} e
+ * @param {!Element} target
+ * @param {!Location} tgtLoc
+ * @param {!./service/ampdoc-impl.AmpDoc} ampdoc
+ * @param {boolean} isIosSafari
+ * @private
+ */
+function handleCustomProtocolClick_(e, target, tgtLoc, ampdoc, isIosSafari) {
+  /** @const {!Window} */
+  const win = ampdoc.win;
   // On Safari iOS, custom protocol links will fail to open apps when the
   // document is iframed - in order to go around this, we set the top.location
   // to the custom protocol href.
@@ -141,11 +164,21 @@ export function onDocumentElementClick_(
     // in the case where there's no app to handle the custom protocol.
     e.preventDefault();
   }
+}
 
-  if (!tgtLoc.hash) {
-    return;
-  }
 
+/**
+ * Handles clicking on a link with hash navigation.
+ * @param {!Event} e
+ * @param {!Location} tgtLoc
+ * @param {!./service/ampdoc-impl.AmpDoc} ampdoc
+ * @param {!./service/viewport-impl.Viewport} viewport
+ * @param {!./service/history-impl.History} history
+ * @private
+ */
+function handleHashClick_(e, tgtLoc, ampdoc, viewport, history) {
+  /** @const {!Window} */
+  const win = ampdoc.win;
   /** @const {!Location} */
   const curLoc = parseUrl(win.location.href);
   const tgtHref = `${tgtLoc.origin}${tgtLoc.pathname}${tgtLoc.search}`;
@@ -168,7 +201,7 @@ export function onDocumentElementClick_(
   let elem = null;
 
   if (hash) {
-    const escapedHash = escapeCssSelectorIdent(ampdoc.win, hash);
+    const escapedHash = escapeCssSelectorIdent(win, hash);
     elem = (ampdoc.getRootNode().getElementById(hash) ||
         // Fallback to anchor[name] if element with id is not found.
         // Linking to an anchor element with name is obsolete in html5.
@@ -177,14 +210,25 @@ export function onDocumentElementClick_(
 
   // If possible do update the URL with the hash. As explained above
   // we do `replace` to avoid messing with the container's history.
-  // The choice of `location.replace` vs `history.replaceState` is important.
-  // Due to bugs, not every browser triggers `:target` pseudo-class when
-  // `replaceState` is called. See http://www.zachleat.com/web/moving-target/
-  // for more details. Do this only if fragment has changed.
   if (tgtLoc.hash != curLoc.hash) {
-    win.location.replace(`#${hash}`);
+    history.replaceStateForTarget(tgtLoc.hash).then(() => {
+      scrollToElement(elem, win, viewport, hash);
+    });
+  } else {
+    // If the hash did not update just scroll to the element.
+    scrollToElement(elem, win, viewport, hash);
   }
+}
+
 
+/**
+ * Scrolls the page to the given element.
+ * @param {?Element} elem
+ * @param {!Window} win
+ * @param {!./service/viewport-impl.Viewport} viewport
+ * @param {string} hash
+ */
+function scrollToElement(elem, win, viewport, hash) {
   // Scroll to the element if found.
   if (elem) {
     // The first call to scrollIntoView overrides browsers' default
@@ -203,11 +247,4 @@ export function onDocumentElementClick_(
     dev().warn('HTML',
         `failed to find element with id=${hash} or a[name=${hash}]`);
   }
-
-  if (tgtLoc.hash != curLoc.hash) {
-    // Push/pop history.
-    history.push(() => {
-      win.location.replace(`${curLoc.hash || '#'}`);
-    });
-  }
 }