Fix local navigation history management by handling hash navigations #5961

mkhatib · 2016-11-01T23:47:55Z

Previously we were only handling hash navigations in iframe'd docs, this change expands this to always install the document.click handler for standalone docs and let runtime handle hash navigation.

This allows our natural history implementation to properly queue popping/pushing states.

This should solve both natural and virtual history.

Related to: #4184

…in standalone mode

camelburrito · 2016-11-03T04:50:46Z

Ping me back after tests are in place!

mkhatib · 2016-11-09T20:54:54Z

src/service/history-impl.js

      dev().fine(TAG_, 'popstate event: ' + this.win.history.length + ', ' +
          JSON.stringify(e.state));
-      eventPass.schedule();
-    };
-    this.hashchangeHandler_ = () => {


Dropping hashchange handler as it is not needed now that we handle our own hash-navigations when users click on anchors. And after investigating browsers, they all fire popstate on navigating backward or forward in history.

mkhatib · 2016-11-09T20:59:07Z

Adding @dvoytenko to this since this make a bit of more changes to history-impl.

Tested this in a standalone doc here and inside the viewer in this link.

dvoytenko · 2016-11-09T22:46:22Z

examples/article.amp.html

@@ -219,7 +219,8 @@
  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
  <meta name="apple-itunes-app" content="app-id=828256236, app-argument=medium://p/cb7f223fad86">
  <link rel="amp-manifest" href="medium-manifest.json">
-  <script async src="./viewer-integr.js" data-amp-report-test="viewer-integr.js"></script>
+  <!--<script async src="./viewer-integr.js" data-amp-report-test="viewer-integr.js"></script>-->
+  <script async src="https://cdn.ampproject.org/viewer/google/v5.js"></script>


Let's rollback. This file is used for viewer.html regularly for testing.

dvoytenko · 2016-11-09T22:47:32Z

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

+
+    this.element.addEventListener('click', e => {
+      const target = dev().assertElement(e.target);
+      if (!target) {


This wouldn't happen with the assertElement above.

dvoytenko · 2016-11-09T22:47:56Z

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

+      if (!target) {
+        return;
+      }
+      if (target.tagName == 'A' && target.href) {


It's common to nest elements into <a>, so we need to do closest test.

dvoytenko · 2016-11-09T22:51:16Z

src/service/history-impl.js

+   *
+   * @param {string} target
+   * @param {string} previousTarget
+   * @returns {Promise.<T>}


No dots in types and what's T?

dvoytenko · 2016-11-09T22:51:58Z

src/service/history-impl.js

@@ -109,6 +112,20 @@ export class History {
  }

  /**
+   *
+   * @param {string} target
+   * @param {string} previousTarget


Not super happy about previousTarget. First, it's previousHash since it may not be a target. Second, seems like we should know it here anyway.

I was trying to avoid re-parsing URL but I am ok with getting it from win.location though in here.

dvoytenko · 2016-11-09T22:53:27Z

src/service/history-impl.js

    this.popstateHandler_ = e => {
+      if (this.ignoreUpcomingPopstate_) {


As I mentioned. I'm a bit concerned with sync/async behavior.

Tested this out. All browsers are firing the event handler synchronously.

dvoytenko · 2016-11-09T22:55:46Z

src/service/history-impl.js

+    this.whenReady_(() => {
+      this.ignoreUpcomingPopstate_ = true;
+      // TODO(mkhatib, #6095): Chrome iOS will add extra states for location.replace.
+      this.win.location.replace(target);


target to me sort of assumes that there's no #. In which case it should be added here. Generally, I think we'll be served well by supporting both cases where target is passed here with and w/o #

dvoytenko · 2016-11-09T22:56:06Z

src/service/history-impl.js

   * @param {*=} state
   * @param {(string|undefined)=} title
   * @param {(string|undefined)=} url
   * @private
   */
  historyReplaceState_(state, title, url) {
-    this.assertReady_();


Also from earlier experiments, reverted this back.

dvoytenko · 2016-11-09T22:56:34Z

src/service/history-impl.js

  /** @override */
  cleanup_() {
    this.unlistenOnHistoryPopped_();
+    if (this.origReplaceState_) {


What about pushState?

Sorry this is not needed and is risidual from earlier experimentation. Will drop. We do not override either pushState or replaceState in virtual history.

mkhatib · 2016-11-10T19:58:09Z

@dvoytenko let me know if this is looks good so I can update tests and add some more.

jridgewell · 2016-11-10T20:42:56Z

src/service/history-impl.js

+    const previousHash = this.ampdoc_.win.location.hash;
+    return this.push(() => {
+      this.ampdoc_.win.location.replace(previousHash || '#');
+    }).then(() => {


Why separate this into two blocks?

Not sure where is the 2nd block, but if you mean inside the push that's the wrong place to have it, the push argument is a callback to be called when popping that state.

That's a super confusing API. 👍

jridgewell · 2016-11-10T20:44:10Z

src/service/history-impl.js

@@ -590,6 +636,12 @@ export class HistoryBindingVirtual_ {
  }

  /** @override */
+  replaceStateForTarget(target) {
+    let hash = target.indexOf('#') == 0 ? target : `#${target}`;


Just test target[0].

We actually dropped this statement all together and now just assert that it starts with a #. Github is probably showing you a previous change. This should be done.

jridgewell · 2016-11-10T20:44:58Z

src/service/history-impl.js

@@ -590,6 +636,12 @@ export class HistoryBindingVirtual_ {
  }

  /** @override */
+  replaceStateForTarget(target) {
+    let hash = target.indexOf('#') == 0 ? target : `#${target}`;


Why would we ever get a non # leading string?

Just supporting either cases.

jridgewell · 2016-11-10T20:46:01Z

src/service/history-impl.js

+   */
+  replaceStateForTarget(target) {
+    this.whenReady_(() => {
+      let hash = target.indexOf('#') == 0 ? target : `#${target}`;


jridgewell · 2016-11-10T20:49:06Z

src/service/history-impl.js

+  replaceStateForTarget(target) {
+    const previousHash = this.ampdoc_.win.location.hash;
+    return this.push(() => {
+      this.ampdoc_.win.location.replace(previousHash || '#');


Shouldn't this already have happened?

This is the onpop callback, when the user hit back we execute this. What do you mean shouldn't this already have happened?

I thought this was a #then callback, not a pop.

jridgewell · 2016-11-10T20:50:45Z

src/service/history-impl.js

+      // TODO(mkhatib, #6095): Chrome iOS will add extra states for location.replace.
+      this.win.location.replace(hash);
+      this.ignoreUpcomingPopstate_ = false;
+      this.historyReplaceState_();


Can we call this first? If so, we could drop the ignoreUpcomingPopstate_ and instead check if the navigation is to the same URL as the previous. If it is, then we know there's not really a popstate.

Not sure what do you mean navigation to the same URL. We're going to a new URL here, where do we get access to the previous Url?

The popstate happens for any call for location.replace in this case we don't want that to be considered a popstate.

Mind elaborating? I've tried testing what you suggested but couldn't get it to work.

#replaceState should trigger a popstate, too right? Well, call it first, that's your "real" popstate. On every popstate, record the current URL. Now, we need to call #replace to get :target support, but it'll trigger it's own popstate (a duplicate). But because we know what URL we were at, we know this is a duplicate URL. Don't record this popstate.

No replaceState does not trigger a popstate. Ok if I understand correctly, we want to replace ignoreUpcomingPopstate_ with something like this.previouseUrl_ hash and then check that and ignore that popstate event if the current hash is equal to the previous hash? This should be doable I believe.

What advantages does this have over the ignoreUpcomingPopstate_?

No replaceState does not trigger a popstate

Well that's dumb.

What advantages does this have over the ignoreUpcomingPopstate_?

This try-finally boolean is icky. 😄

Done. Let me know if this looks good!

mkhatib · 2016-11-16T22:26:40Z

testing/fake-dom.js

@@ -427,6 +435,7 @@ export class FakeHistory {
      throw new Error('can\'t go forward');
    }
    this.index = newIndex;
+    this.win.eventListeners.fire({type: 'popstate'});


popstate should fire on all history navigation. @dvoytenko you also seem to allow an optional fireEvent param for pushState and replaceState. These never actually fire so not sure what we try to test there, let me know if we should just drop them or keep them. Keeping them is not hurting but might give the impression that pushState and replaceState needs to fire.

Well. It's a very optional argument. I just didn't want to explode number of private/public methods here. And replaceState/pushState are used internally for FakeLocation.

mkhatib · 2016-11-16T22:27:14Z

testing/fake-dom.js

@@ -331,7 +339,7 @@ class FakeLocation {
   */
  change_(args) {
    const change = parseUrl(this.url_.href);
-    Object.assign(change, args);
+    Object.assign({}, change, args);


This used to throw a can't set property because the change is frozen. This is ok, no?

Definitely!

mkhatib · 2016-11-16T23:11:40Z

@dvoytenko this is ready for another look - added tests and all 👀 PTAL

dvoytenko · 2016-11-16T23:58:42Z

src/service/history-impl.js

+  replaceStateForTarget(target) {
+    this.whenReady_(() => {
+      const hash = target[0] == '#' ? target : `#${target}`;
+      this.ignoreUpcomingPopstate_ = true;


If the events are synchronous, does it mean that replace can fail? Please check. If so, we may need a try/finally

Looks like it fails if the location is invalid or different origin than executing script.

We should probably limit the replaceStateForTarget to be just for hash navigation. Which is we kinda do with the first like appending a # but maybe we should just handle the explicit # and throw if the target passed does not start with a #.

What do you think?

I'll also add the try-finally

SG. Should definitely be limited to the same everything - only hash.

dvoytenko · 2016-11-17T00:00:25Z

LGTM with one request.

mkhatib · 2016-11-17T02:26:42Z

@dvoytenko mind taking one last look 👀

dvoytenko · 2016-11-17T18:43:36Z

LGTM. Pending comments by others.

mkhatib · 2016-11-18T17:48:11Z

@jridgewell Done. Let me know if this looks 👀 good

jridgewell · 2016-11-18T18:26:13Z

src/service/history-impl.js

@@ -374,6 +403,9 @@ export class HistoryBindingNatural_ {
    // On pop, stack is not allowed to go prior to the starting point.
    stackIndex = Math.max(stackIndex, this.startIndex_);
    return this.whenReady_(() => {
+      // Popping history forget the last navigated hash since we can't really
+      // know what hash the browser is going to go to.
+      this.lastNavigatedHash_ = null;


Shouldn't this be whatever the hash is after #back is called?

Yes. But we don't really know what's that going to be (Browser history API doesn't expose that for privacy reasons). However, we set the hash in the popstate event once we already been back in history.

jridgewell · 2016-11-18T18:26:53Z

src/service/history-impl.js

+  replaceStateForTarget(target) {
+    dev().assert(target[0] == '#', 'target should start with a #');
+    this.whenReady_(() => {
+      this.lastNavigatedHash_ = target;


Add a comment.

mkhatib · 2016-11-18T19:20:21Z

Yay! 💃 💃 💃 💃 💃

…mpproject#5961)

Fix local navigation history management by handling hash navigations …

c9142a5

…in standalone mode

mkhatib added the NEEDS REVIEW label Nov 1, 2016

mkhatib assigned camelburrito Nov 1, 2016

Mohammad Khatib added 2 commits November 8, 2016 12:45

Fix natural and virtual history and doc-click

6b730b8

change api.

2e66c7f

mkhatib commented Nov 9, 2016

View reviewed changes

mkhatib assigned dvoytenko Nov 9, 2016

dvoytenko suggested changes Nov 9, 2016

View reviewed changes

Mohammad Khatib added 2 commits November 9, 2016 16:28

fix comments

ac47fe4

fix comments

de90625

mkhatib mentioned this pull request Nov 10, 2016

Fix amp-sidebar local navigation and history #5520

Closed

jridgewell reviewed Nov 10, 2016

View reviewed changes

Tests and fixes

798c565

mkhatib commented Nov 16, 2016

View reviewed changes

fix types

ec8d448

dvoytenko reviewed Nov 16, 2016

View reviewed changes

dvoytenko approved these changes Nov 17, 2016

View reviewed changes

try-finally and dev.assert starts with #

9f083ff

Mohammad Khatib added 2 commits November 17, 2016 19:29

Use lastNavigatedHash_ instead of ignorePopstate

7fe396c

fixes

3aa40f5

jridgewell reviewed Nov 18, 2016

View reviewed changes

add comment

b30b6a0

jridgewell approved these changes Nov 18, 2016

View reviewed changes

mkhatib merged commit ff7b32b into ampproject:master Nov 18, 2016

mkhatib deleted the doc-click-non-embed branch November 18, 2016 19:20

derat mentioned this pull request Dec 4, 2016

<amp-sidebar> is scrolling back to the page top on local navigation #4184

Closed

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Fix local navigation history management by handling hash navigations (a…

97b9529

…mpproject#5961)

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Fix local navigation history management by handling hash navigations (a…

c097070

…mpproject#5961)

dreamofabear mentioned this pull request Aug 8, 2017

History: Fix pop callback not invoked in sequential back navigations #10841

Merged

		this.popstateHandler_ = e => {
		if (this.ignoreUpcomingPopstate_) {

Fix local navigation history management by handling hash navigations #5961

Fix local navigation history management by handling hash navigations #5961

Conversation

mkhatib commented Nov 1, 2016 • edited

camelburrito commented Nov 3, 2016

Choose a reason for hiding this comment

mkhatib commented Nov 9, 2016

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

mkhatib commented Nov 10, 2016

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

mkhatib Nov 17, 2016 • edited

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

mkhatib commented Nov 16, 2016

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

dvoytenko commented Nov 17, 2016

mkhatib commented Nov 17, 2016

dvoytenko commented Nov 17, 2016

mkhatib commented Nov 18, 2016

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

mkhatib commented Nov 18, 2016

mkhatib commented Nov 1, 2016 •

edited

mkhatib Nov 17, 2016 •

edited