Fixes #264 - replaceState was wonky

app/assets/javascripts/discourse/components/url.js

@@ -13,6 +13,18 @@ Discourse.URL = {
   // Used for matching a /more URL
   MORE_REGEXP: /\/more$/,
 
+  /**
+    @private
+
+    Get a handle on the application's router. Note that currently it uses `__container__` which is not
+    advised but there is no other way to access the router.
+
+    @method router
+  **/
+  router: function(path) {
+    return Discourse.__container__.lookup('router:main');
+  },
+
   /**
     Browser aware replaceState. Will only be invoked if the browser supports it.
 
@@ -20,12 +32,19 @@ Discourse.URL = {
     @param {String} path The path we are replacing our history state with.
   **/
   replaceState: function(path) {
+
     if (window.history &&
         window.history.pushState &&
         window.history.replaceState &&
         !navigator.userAgent.match(/((iPod|iPhone|iPad).+\bOS\s+[1-4]|WebApps\/.+CFNetwork)/) &&
         (window.location.pathname !== path)) {
-        return history.replaceState({ path: path }, null, path);
+
+        // Always use replaceState in the next runloop to prevent weird routes changing
+        // while URLs are loading. For example, while a topic loads it sets `currentPost`
+        // which triggers a replaceState even though the topic hasn't fully loaded yet!
+        Em.run.next(function() {
+          Discourse.URL.router().get('location').replaceURL(path);
+        });
     }
   },
 
@@ -36,9 +55,6 @@ Discourse.URL = {
     It contains the logic necessary to route within a topic using replaceState to
     keep the history intact.
 
-    Note that currently it uses `__container__` which is not advised
-    but there is no other way to access the router.
-
     @method routeTo
     @param {String} path The path we are routing to.
   **/
@@ -71,13 +87,13 @@ Discourse.URL = {
     }
 
     // If we transition from a /more path, scroll to the top
-    if (this.MORE_REGEXP.exec(oldPath) && (!this.MORE_REGEXP.exec(path))) {
+    if (this.MORE_REGEXP.exec(oldPath) && (oldPath.indexOf(path) === 0)) {
       window.scrollTo(0, 0);
     }
 
     // Be wary of looking up the router. In this case, we have links in our
     // HTML, say form compiled markdown posts, that need to be routed.
-    var router = Discourse.__container__.lookup('router:main');
+    var router = this.router();
     router.router.updateURL(path);
     return router.handleURL(path);
   }

app/assets/javascripts/discourse/routes/discourse_location.js

@@ -1,194 +1,191 @@
 /*global historyState:true */
-(function() {
-  /**
-  @module ember
-  @submodule ember-routing
-  */
 
-  var get = Ember.get, set = Ember.set;
+/**
-  var popstateReady = false;
+@module Discourse
+*/
+
+var get = Ember.get, set = Ember.set;
+var popstateReady = false;
+
+/**
+  `Ember.DiscourseLocation` implements the location API using the browser's
+  `history.pushState` API.
+
+  @class DiscourseLocation
+  @namespace Discourse
+  @extends Ember.Object
+*/
+Ember.DiscourseLocation = Ember.Object.extend({
+  init: function() {
+    set(this, 'location', get(this, 'location') || window.location);
+    if ( jQuery.inArray('state', jQuery.event.props) < 0 )
+      jQuery.event.props.push('state')
+    this.initState();
+  },
 
   /**
-    `Ember.DiscourseLocation` implements the location API using the browser's
+    @private
-    `history.pushState` API.
 
-    @class DiscourseLocation
+    Used to set state on first call to setURL
-    @namespace Ember
+
-    @extends Ember.Object
+    @method initState
   */
-  Ember.DiscourseLocation = Ember.Object.extend({
+  initState: function() {
-    init: function() {
+    this.replaceState(this.formatURL(this.getURL()));
-      set(this, 'location', get(this, 'location') || window.location);
+    set(this, 'history', window.history);
-      if ( jQuery.inArray('state', jQuery.event.props) < 0 )
+  },
-        jQuery.event.props.push('state')
-      this.initState();
-    },
 
-    /**
+  /**
-      @private
+    Will be pre-pended to path upon state change
 
-      Used to set state on first call to setURL
+    @property rootURL
+    @default '/'
+  */
+  rootURL: '/',
 
-      @method initState
+  /**
-    */
+    @private
-    initState: function() {
-      this.replaceState(this.formatURL(this.getURL()));
-      set(this, 'history', window.history);
-    },
 
-    /**
+    Returns the current `location.pathname` without rootURL
-      Will be pre-pended to path upon state change
 
-      @property rootURL
+    @method getURL
-      @default '/'
+  */
-    */
+  getURL: function() {
-    rootURL: '/',
+    var rootURL = get(this, 'rootURL'),
+        url = get(this, 'location').pathname;
 
-    /**
+    rootURL = rootURL.replace(/\/$/, '');
-      @private
+    url = url.replace(rootURL, '');
 
-      Returns the current `location.pathname` without rootURL
+    return url;
+  },
 
-      @method getURL
+  /**
-    */
+    @private
-    getURL: function() {
-      var rootURL = get(this, 'rootURL'),
-          url = get(this, 'location').pathname;
 
-      rootURL = rootURL.replace(/\/$/, '');
+    Uses `history.pushState` to update the url without a page reload.
-      url = url.replace(rootURL, '');
 
-      return url;
+    @method setURL
-    },
+    @param path {String}
+  */
+  setURL: function(path) {
+    path = this.formatURL(path);
 
-    /**
+    if (this.getState() && this.getState().path !== path) {
-      @private
+      popstateReady = true;
-
+      this.pushState(path);
-      Uses `history.pushState` to update the url without a page reload.
-
-      @method setURL
-      @param path {String}
-    */
-    setURL: function(path) {
-      path = this.formatURL(path);
-
-      if (this.getState() && this.getState().path !== path) {
-        popstateReady = true;
-        this.pushState(path);
-      }
-    },
-
-    /**
-      @private
-
-      Uses `history.replaceState` to update the url without a page reload
-      or history modification.
-
-      @method replaceURL
-      @param path {String}
-    */
-    replaceURL: function(path) {
-      path = this.formatURL(path);
-
-      if (this.getState() && this.getState().path !== path) {
-        popstateReady = true;
-        this.replaceState(path);
-      }
-    },
-
-    /**
-     @private
-
-     Get the current `history.state`
-
-     @method getState
-    */
-    getState: function() {
-      historyState = get(this, 'history').state;
-      if (historyState) return historyState;
-
-      return {path: window.location.pathname};
-    },
-
-    /**
-     @private
-
-     Pushes a new state
-
-     @method pushState
-     @param path {String}
-    */
-    pushState: function(path) {
-      if (!window.history.pushState) return;
-      this.set('currentState', { path: path } );
-      window.history.pushState({ path: path }, null, path);
-    },
-
-    /**
-     @private
-
-     Replaces the current state
-
-     @method replaceState
-     @param path {String}
-    */
-    replaceState: function(path) {
-      if (!window.history.replaceState) return;
-      this.set('currentState', { path: path } );
-      window.history.replaceState({ path: path }, null, path);
-    },
-
-    /**
-      @private
-
-      Register a callback to be invoked whenever the browser
-      history changes, including using forward and back buttons.
-
-      @method onUpdateURL
-      @param callback {Function}
-    */
-    onUpdateURL: function(callback) {
-      var guid = Ember.guidFor(this),
-          self = this;
-
-      $(window).bind('popstate.ember-location-'+guid, function(e) {
-        if (e.state) {
-          var currentState = self.get('currentState');
-          if (currentState) {
-            callback(e.state.path);
-          } else {
-            this.set('currentState', e.state);
-          }
-        }
-
-      });
-    },
-
-    /**
-      @private
-
-      Used when using `{{action}}` helper.  The url is always appended to the rootURL.
-
-      @method formatURL
-      @param url {String}
-    */
-    formatURL: function(url) {
-      var rootURL = get(this, 'rootURL');
-
-      if (url !== '') {
-        rootURL = rootURL.replace(/\/$/, '');
-      }
-
-      return rootURL + url;
-    },
-
-    willDestroy: function() {
-      var guid = Ember.guidFor(this);
-
-      Ember.$(window).unbind('popstate.ember-location-'+guid);
     }
-  });
+  },
 
-  Ember.Location.registerImplementation('discourse_location', Ember.DiscourseLocation);
+  /**
+    @private
 
-})(this);
+    Uses `history.replaceState` to update the url without a page reload
+    or history modification.
+
+    @method replaceURL
+    @param path {String}
+  */
+  replaceURL: function(path) {
+    path = this.formatURL(path);
+
+    if (this.getState() && this.getState().path !== path) {
+      popstateReady = true;
+      this.replaceState(path);
+    }
+  },
+
+  /**
+   @private
+
+   Get the current `history.state`
+
+   @method getState
+  */
+  getState: function() {
+    historyState = get(this, 'history').state;
+    if (historyState) return historyState;
+
+    return {path: window.location.pathname};
+  },
+
+  /**
+   @private
+
+   Pushes a new state
+
+   @method pushState
+   @param path {String}
+  */
+  pushState: function(path) {
+    if (!window.history.pushState) return;
+    this.set('currentState', { path: path } );
+    window.history.pushState({ path: path }, null, path);
+  },
+
+  /**
+   @private
+
+   Replaces the current state
+
+   @method replaceState
+   @param path {String}
+  */
+  replaceState: function(path) {
+    if (!window.history.replaceState) return;
+    this.set('currentState', { path: path } );
+    window.history.replaceState({ path: path }, null, path);
+  },
+
+  /**
+    @private
+
+    Register a callback to be invoked whenever the browser
+    history changes, including using forward and back buttons.
+
+    @method onUpdateURL
+    @param callback {Function}
+  */
+  onUpdateURL: function(callback) {
+    var guid = Ember.guidFor(this),
+        self = this;
+
+    $(window).bind('popstate.ember-location-'+guid, function(e) {
+      if (e.state) {
+        var currentState = self.get('currentState');
+        if (currentState) {
+          callback(e.state.path);
+        } else {
+          this.set('currentState', e.state);
+        }
+      }
+
+    });
+  },
+
+  /**
+    @private
+
+    Used when using `{{action}}` helper.  The url is always appended to the rootURL.
+
+    @method formatURL
+    @param url {String}
+  */
+  formatURL: function(url) {
+    var rootURL = get(this, 'rootURL');
+
+    if (url !== '') {
+      rootURL = rootURL.replace(/\/$/, '');
+    }
+
+    return rootURL + url;
+  },
+
+  willDestroy: function() {
+    var guid = Ember.guidFor(this);
+
+    Ember.$(window).unbind('popstate.ember-location-'+guid);
+  }
+});
+
+Ember.Location.registerImplementation('discourse_location', Ember.DiscourseLocation);

app/assets/javascripts/discourse/views/topic_view.js

@@ -69,14 +69,16 @@ Discourse.TopicView = Discourse.View.extend(Discourse.Scrolling, {
   }).observes('topic.highest_post_number'),
 
   currentPostChanged: (function() {
-    var current, postUrl, topic;
+    var current = this.get('controller.currentPost');
-    current = this.get('controller.currentPost');
+
-    topic = this.get('topic');
+    var topic = this.get('topic');
     if (!(current && topic)) return;
+
     if (current > (this.get('maxPost') || 0)) {
       this.set('maxPost', current);
     }
-    postUrl = topic.get('url');
+
+    var postUrl = topic.get('url');
     if (current > 1) {
       postUrl += "/" + current;
     } else {