Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get the hash value from location.href rather than location.hash #967

Merged
merged 2 commits into from Feb 27, 2012

Conversation

@jharding
Copy link
Contributor

jharding commented Feb 7, 2012

The method getFragment currently decodes the fragment before returning it. Due to a bug in Firefox where location.hash is decoded, I assume the reason why the fragment gets decoded is to normalize across browsers. However this can cause issues when attempting to navigate to a URL that has some encoded characters. For example if my router contained this:

routes = {
    '*path': 'goTo'
},

goTo: function(path) {
    ...
}

If router.navigate('has%20space', { trigger: true }); was called, I'd expect path to have the value 'has%20space' but instead it has the value 'has space'.

In the changes I made, the hash value is retrieved from location.href rather than location.hash as a workaround to the Firefox bug. I created a helper method getHash that will split location.href at the '#' character and returns what comes after it. This allowed me to remove the code that decoded the fragment in getFragment since it is no longer necessary.

…s a workaround to the Firefox bug where location.hash is decoded.
@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Feb 7, 2012

Oh man -- this would be a huge fix if it works around the Firefox problem correctly. Would it be possible for you to provide some test cases in the Router tests, so that I can verify it?

@jharding

This comment has been minimized.

Copy link
Contributor Author

jharding commented Feb 7, 2012

Added a test case. I'm not entirely sure what you're looking for in the test cases, so if there is something else you'd like me to add test case for just let me know.

@mudetroit

This comment has been minimized.

Copy link

mudetroit commented Feb 18, 2012

We have actually patched getFragment in a very similar way to address the Firefox bug locally, and we haven't been seeing any problems with it.

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Feb 27, 2012

Merged with some modifications/fixes, and works in firefox -- so thanks much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.