support lightbox & sidebar history #43

chenshay · 2017-06-20T23:04:58Z

ericfs · 2017-06-21T12:30:16Z

src/history.js

    });
  }

  /**
+   * Updates the history state.
+   */
+  decreaseCurrentStateId() {


ericfs · 2017-06-21T12:30:22Z

src/history.js

+  /**
+   * Updates the history state.
+   */
+  increaseCurrentStateId() {


ericfs · 2017-06-21T12:31:35Z

src/viewer-messaging.js

+   * Registers a method that will handle requests sent to the specified
+   * message name.
+   * @param {string} messageName The name of the message to handle.
+   * @param {!RequestHandler} requestHandler


Where is RequestHandler defined?

ericfs · 2017-06-21T12:33:50Z

src/viewer.js

@@ -50,18 +49,34 @@ class Viewer {
  /**
   * @param {!Function} showViewer method that shows the viewer.
   * @param {!Function} hideViewer method that hides the viewer.
+   * @param {!Function} isViewerHidden method that determines if viewer is hidden.


{!Function():boolean}

ericfs · 2017-06-21T12:43:24Z

src/viewer.js

  }

  /**
   * Attaches the AMP Doc Iframe to the Host Element.
   */
  attach() {
+    if (this.isLoaded_()) {
+      this.history_.pushState(this.ampDocUrl_);


Under what circumstance would attache get called when it is already attached? Are you sure you would want to push again? Couldn't the URL already be correct?

use case: if you went back & hid the viewer and then clicked on the element that opens the viewer again. It shouldn't push a new state but instead go forward in history.

On a click, you should always push a new state, even if it's the same state. That replicates how a browser would handle clicks. Whenever you click on a link, it pushes a new state, clearing all forward state.

That is, what you had the first time was likely fine. I think I didn't understand how it was being used.

Looks like this is still calling goForward() which may be wrong in some cases.

ericfs · 2017-06-21T12:56:27Z

src/history.js

+    } 
+
+    // The url should have /amp/ + url added to it. For example:
+    // example.com -> example.com/amp/https://www.ampproject.org


Is this actually the URL format you want? Google Search would be:
http://www.example.com -> /amp/www.example.com
https://www.example.com -> /amp/s/www.example.com

ericfs · 2017-06-21T13:02:31Z

src/viewer.js

+    if (isBack) {
+      this.viewerMessaging_.sendRequest(
+        'historyPopped', 
+        {'newStackIndex': opt_stackIndex - 1}, 


opt_stackIndex is the new stackIndex, right? So you shouldn't need to subtract one.

Can you also add jsdoc to clarify is opt_stackIndex is expected to be old or new stack index

updated comments to clarify. this is the old one so we need to subtract 1. this tells the amp doc to go to the previous state (close the lightbox or sidebar)

ericfs · 2017-06-21T13:04:50Z

src/history.js

+      }
+
+      // The popped browser history state id.
+      const poppedStateId = (typeof state.stateId !== 'undefined') ? state.stateId : null;


state.stateId !== undefined

ericfs · 2017-06-21T13:08:37Z

src/history.js

+        this.decreaseCurrentStateId();
+      } else {
+        this.increaseCurrentStateId();
+      }


Shouldn't you just take the value of poppedStateId instead of incrementing/decrementing?

ericfs · 2017-06-21T13:10:07Z

src/viewer.js

-      if (this.hideViewer_) this.hideViewer_();
+  handleChangeHistoryState_(isBack, isLastBack, opt_stackIndex) {
+    if (isBack) {
+      this.viewerMessaging_.sendRequest(


If this isLastBack, then I don't think you need to send this to the doc. You're going to a state the doc doesn't know about.

It's the last back within the viewer. The viewer needs to know when to hide.

ericfs · 2017-06-26T14:58:16Z

src/history.js

+    const parsedUrl = parseUrl(url);
+    let urlStr = '/amp/';
+    if (parsedUrl.protocol == 'https:') urlStr += 's/';
+    urlStr += parsedUrl.host;


Sorry, I didn't mean to bring URL construction into scope for this PR. But this is still incomplete since it doesn't include the path or query parameters in the url parameter.

It may make sense to pull this out into a function, but that doesn't need to be part of the is PR.

ericfs · 2017-06-26T15:01:43Z

src/viewer.js

+   * @param {boolean} rsvp
+   * @return {!Promise<*>|undefined}
+   */
+  messageHandler(name, data, rsvp) {


ericfs · 2017-06-26T15:06:52Z

src/viewer.js

-    } else {
-      if (this.hideViewer_) this.hideViewer_();
+  handleChangeHistoryState_(isLastBack, opt_poppedAmpHistoryIndex) {
+    if (this.showViewer_ && this.isViewerHidden_ && this.isViewerHidden_()) {


This could be true for history events that have nothing to do with the AMP viewer. That is, since the AMP Viewer is embedded on some other page, there could be totally unrelated history events occurring. AMP history needs to correctly ignore then.

I think this code is intended to reopen the viewer on forward. It should verify that the new history state does correspond to the AMP Viewer.

ericfs · 2017-06-26T15:07:57Z

src/viewer.js

+      this.showViewer_();
+      return;
+    }
+    if (isLastBack && this.hideViewer_) {


I think you want to return even if there is no this.hideViewer_. A history pop that closes the viewer should never to sent to the AMP doc.

ericfs · 2017-06-26T15:11:47Z

src/viewer.js

  }

  /**
   * Attaches the AMP Doc Iframe to the Host Element.
   */
  attach() {
+    if (this.isLoaded_()) {
+      this.history_.pushState(this.ampDocUrl_);


On a click, you should always push a new state, even if it's the same state. That replicates how a browser would handle clicks. Whenever you click on a link, it pushes a new state, clearing all forward state.

ericfs

Sorry for my slow response. I was travelling.

ericfs · 2017-06-30T18:35:39Z

src/viewer.js

@@ -140,16 +140,17 @@ class Viewer {

  /**
   * @param {boolean} isLastBack true if back button was hit and viewer should hide.
-   * @param {number} opt_poppedAmpHistoryIndex The AMP History Index that was just popped.
+   * @param {boolean} isAMP true if going to AMP document.
+   * @param {number|undefined} opt_poppedAmpHistoryIndex The AMP History Index that was just popped.


I think this is still a bit unclear. When you pop, the state in the event will be that of the incoming state, not the previous state. I think it's confusing what is meant by "just popped".

removed. should be more clear now

ericfs · 2017-06-30T18:36:39Z

src/viewer.js

+      // The back button was hit while a lightbox or sidebar is open.
+      // We subtract 1 from opt_poppedAmpHistoryIndex to tell the AMP
+      // page to go back to the previous state (close the lightbox/sidebar).
+      {'newStackIndex': opt_poppedAmpHistoryIndex - 1}, 


I don't think you want to subtract 1. The state should be the newly active state, which is what you want to provide to the document.

actually I don't need this part of the code at all

ericfs · 2017-06-30T18:37:38Z

src/viewer.js

+        this.history_.goBack();
+        return Promise.resolve();
+      default:
+        return undefined;


Consider rejecting with an unsupported message error.

ericfs · 2017-07-06T23:06:57Z

src/viewer.js

  }

  /**
   * Attaches the AMP Doc Iframe to the Host Element.
   */
  attach() {
+    if (this.isLoaded_()) {
+      this.history_.goForward();


I thought I had a comment about this before, but don't see it now. I think you don't want to depend on what's on the forward history stack when you're reattching. If something else on the page has manipulated the history, it could be totally unrelated to AMP.

Sorry if my previous comment mislead you. I think what you had initially seemed reasonable.

chenshay requested a review from ericfs June 20, 2017 23:04

ericfs reviewed Jun 21, 2017

View reviewed changes

ericfs reviewed Jun 26, 2017

View reviewed changes

ericfs reviewed Jun 30, 2017

View reviewed changes

ericfs approved these changes Jul 6, 2017

View reviewed changes

chenshay added 9 commits July 7, 2017 11:25

support lightbox & sidebar history

77f21e4

comments

2b6b679

fix

c285087

fix

45d6c8e

fix

d5146d0

fix

4227064

fix

9b3c705

fix

6f849cd

fix

05849c5

chenshay force-pushed the history2 branch from 9968131 to 05849c5 Compare July 7, 2017 18:27

chenshay merged commit 12ffdeb into ampproject:master Jul 7, 2017

chenshay deleted the history2 branch July 7, 2017 18:33

support lightbox & sidebar history #43

support lightbox & sidebar history #43

Conversation

chenshay commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenshay Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericfs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenshay Jun 21, 2017 •

edited

Loading