search

olivernn / davis.js

RESTful degradable JavaScript routing using pushState
http://davisjs.com
532 stars 58 forks source link

Really fixing the popstate hasPopped() stuff #42

Closed mschulkind closed 12 years ago

mschulkind commented 12 years ago

Maybe something changed in a recent version of chrome, but it seems this isn't working 100%.

These two situations have the exact same sequence of events as observed by the current Davis.history code, both on 64-bit Gentoo Linux: 1) This situation is in Firefox 10.0.4: -Load a page by typing in a URL (so there is no state associated) -Click a link to a route -Refresh the page -Click the back button

2) This situation is in Chrome 19.0.1084.52 -Load a page by typing in a URL (so there is no state associated) -Refresh the page

In both of these situations, onpopstate is fired, window.history.state == null and event.state == null, and they are both the first onpopstate received. It seems like the if statement at https://github.com/olivernn/davis.js/blob/master/lib/davis.history.js#L35 once may have helped differentiate this case, but it can't any more.

Was window.history.state once undefined in chrome?

The symptom of this is that in chrome, the route is fired twice on page refresh or initial load. I don't see an easy way to fix this without checking to see webkit or non-webkit. Any ideas?

mschulkind commented 12 years ago

I just pushed a commit to my fork (referenced above). I think this fixes it. I'd like to see what you think about it. I tested it in the latest stable versions of Firefox, Chrome, and Opera.

olivernn commented 12 years ago

Yeah, I noticed this issue in Chrome 19 just the other day. I haven't had a chance to look into this in any detail yet but Chrome has always handled the initial popstate in a different way to other browsers. I have checked this on Safari and it doesn't have the same problem, so I'm not convinced is a webkit thing, more likely a chrome thing.

I'll take a look at your patch later today, it certainly does need fixing. I'd also like to make sure this works with older versions of chrome too.

It also might be worth getting hold of someone from Chrome to see if this was an intentional change or a bug/regression that has been introduced in the latest version of the browser.

Thanks for looking into this, your help is much appreciated!

mschulkind commented 12 years ago

Just pushed a commit (above) that fixes what I broke in IE. I'm working on getting an old Chrome version installed, but I expect the change works since it doesn't rely on any specifics.

Glad to help fix this :) Davis.js has been quite the help to me already.

mschulkind commented 12 years ago

Definitely not a webkit thing as you say. My safari (on windows) doesn't even seem to ever fill in window.history.state, and if I type window.history.st in the console, the browser just instantly crashes.

mschulkind commented 12 years ago

Just tested Chrome 17 and 18. I now realize what the change was in 19. It was definitely deliberate. <= 18, Chrome never fills in window.history.state. >= 19, Chrome always fills it in with the current state. Latest Firefox at least also always fills this in. This actually brings Chrome more in line with Firefox. Previous to Chrome 19, the check in hasPopped() with 'state' in window.history was really only acting to determine Chrome vs. Firefox and not much more.

The fix I'm proposing should sidestep the whole issue and just make things more consistent all around. I wouldn't be surprised if some logic elsewhere could be slightly simplified since you will always have an event.state object on every navigation regardless (except for the initial webkit event which is ignored).

olivernn commented 12 years ago

Just deployed a new version which should address this issue, if you have any issues please let me know.

Your pull request included a fix for IE, is this still relevant? If so please could you extract it into a separate issue/pull request.

Massive apologies for how long this took, I haven't been able to spend enough time on this in the past month or so. Going to make sure that doesn't happen again, thanks again for taking the time to look into this issue for me.