Router hash value error in Firefox #1305

Closed
chuangbo opened this Issue May 10, 2012 · 40 comments

Comments

Projects
None yet
9 participants

In 0.9.2 and FIrefox, after 402508d, Router callback will be trigger twice if hash is not only ASCII, first callback is decoded (what I needed), second callback is undecoded.

Routes

routes: {
    ':name': 'test'
}

test: function(name) {
    console.log('name', name);
    console.log('hash', location.hash);
    console.log('href', location.href);
}

then I visit #中文域名测试.cn ONLY ONCE

name 中文域名测试.cn
hash #中文域名测试.cn
href http://localhost/#%E4%B8%AD%E6%96%87%E5%9F%9F%E5%90%8D%E6%B5%8B%E8%AF%95.cn

name %E4%B8%AD%E6%96%87%E5%9F%9F%E5%90%8D%E6%B5%8B%E8%AF%95.cn
hash #中文域名测试.cn
href http://localhost/#%E4%B8%AD%E6%96%87%E5%9F%9F%E5%90%8D%E6%B5%8B%E8%AF%95.cn

I think it's a bug. When I use name as ajax data, it will pass RAW encoded encoded string to backend.

Shouldn't routes be an object instead of an array?

@Francisc It's a spelling mistake, updated. Thanks.

@braddunbar Please help us, we must hack it in application with decodeURIComponent. Thank you very much.

Collaborator

braddunbar commented Jun 3, 2012

Hi @chuangbo! I can see that firefox returns the encoded version of the hash, but I haven't seen the event duplication. I'll see what I can do about the encoded hash.

If you get a chance, can you post a jsfiddle/jsbin demonstrating the route being triggered twice?

chuangbo commented Jun 4, 2012

Thank @braddunbar!

Triggered twice when I use router.navigate, ones when click link.
http://jsfiddle.net/7DJkR/8/

Collaborator

braddunbar commented Jun 4, 2012

Thanks @chuangbo, that helps a lot.

I'm experience this exact same issue too.

@braddunbar I have the same question. And I find that,when I back or forward,router callback will be trigger only once,and the callback is undecoded.

sokki commented Sep 25, 2012

anyone found a solution?

Collaborator

braddunbar commented Sep 25, 2012

I just picked this one back up, and I don't see the issue in Firefox using the code from the current master. I used the code from this gist and only saw the route triggered once. Anyone else seeing something different? Did I get the test page wrong?

@braddunbar

Triggered twice when I use router.navigate, ones when click link.
http://jsfiddle.net/7DJkR/8/

Owner

jashkenas commented Dec 7, 2012

The current master now has a router patch that leaves things encoded as much as possible. That should fix things. If not, let us know.

@jashkenas jashkenas closed this Dec 7, 2012

@jashkenas Not fixed yet. See this test http://jsfiddle.net/7DJkR/16/

@jashkenas jashkenas reopened this Dec 17, 2012

Owner

jashkenas commented Dec 17, 2012

@braddunbar -- do you have the time to take a peek at this? Let me know if not.

it's bug in that test case. it routes different URLs twice to the same test() action. see update version http://jsfiddle.net/7DJkR/19/

@Yahasana THE BUG in your comments line

router.navigate("/中文域名测试.cn", true);

Triggered twice with error param when you use router.navigate to a non-ascii path. Click link is ok.

Yup, router.navigate("/中文域名测试.cn", true); will be encoded by browser to %E4%B8%AD%E6%96%87%E5%9F%9F%E5%90%8D%E6%B5%8B%E8%AF%95.cn.

@Yahasana Yes, that's the bug. It should be decoded.

so the bug is router.navigate() should decode the encoded hash (done by browser) to the raw one, but not

Router callback will be trigger twice if hash is not only ASCII

Contributor

philfreo commented Dec 18, 2012

@Yahasana your "updated" test case ( http://jsfiddle.net/7DJkR/19/ ) is using the older Backbone 0.9.2.

@philfreo Oops, update from the old one. renew to master http://jsfiddle.net/7DJkR/24/. and it'd be clear for this bug now

One more inconsistent thing, arguments for the events are different from route set by routes:{} object and this.route(). see http://jsfiddle.net/7DJkR/25/

Collaborator

braddunbar commented Dec 18, 2012

@jashkenas I've looked at this one several times and I'm not able to duplicate the behavior.

Owner

jashkenas commented Dec 18, 2012

Closing then, on @braddunbar's good authority. PRs adding more valid test cases to the test suite about these are still warmly welcome.

@jashkenas jashkenas closed this Dec 18, 2012

Please check this jsfiddle on Firefox. It's definitly bug, from 0.9.2 to 0.9.9. http://jsfiddle.net/7DJkR/27/

Here are my log in my firefox 15.0.1

name 中文域名测试.cn
name %E4%B8%AD%E6%96%87%E5%9F%9F%E5%90%8D%E6%B5%8B%E8%AF%95.cn

new clear test case http://jsfiddle.net/7DJkR/30/

WORK: Chrome 21, IE 8, Opera 11
NOT WORK: FF 17.0.1 <-------------------- it seem that Firefox encode url characters by default.

Collaborator

braddunbar commented Dec 19, 2012

Digging in to this one more time, it appears that Firefox has bugs surrounding both location.hash and location.href. Given the url http://example.com#中文:

Chrome:

> location.href
"http://example.com#中文"
> location.hash
"#中文"

Firefox:

> location.href
"http://localhost:8080/backbone/test.html#%E4%B8%AD%E6%96%87"
> location.hash
"#中文"

Given the results above, it would appear that we should use location.hash instead of location.href, as we currently do in History#getHash. However, History#getHash was added to fix a Firefox bug in which the hash is always decoded.

I'm going to do some digging regarding the correct behavior here, but at least the bug is clear now.

@chuangbo Thanks for your patience and help getting this resolved.

@braddunbar braddunbar reopened this Dec 19, 2012

Contributor

philfreo commented Dec 19, 2012

It seems that recently the location is always encoded in the router, and I'm not really sure that should be the correct behavior (it required me to add decodeURIComponent on every parameter in router methods). It'd be good to at least clarify exactly why that is.

Collaborator

braddunbar commented Dec 19, 2012

The reason the fragment is always handled encoded in the router is that it can't be meaningfully compared after being decoded. Take, for instance, the following router.

var Router = Backbone.Router.extend({
  routes: {
    'search/:query/*extra': 'search'
  },
  search: function(query, extra) {
    console.log(query, extra);
  }
});

Given the encoded fragment search/foo%2Fbar/p2, the arguments to search would be "foo%2Fbar", "p2". However, if you decode the fragment ("search/foo/bar/p2") the arguments would be (incorrectly) "foo", "bar/p2".

Contributor

philfreo commented Dec 19, 2012

Pardon my ignorance, but in that case shouldn't you leave it encoded, split it up per parameter, and then decode each parameter before passing it to the route methods (to end up with arguments being ["foo/bar", "p2"])?

Collaborator

braddunbar commented Dec 19, 2012

@philfreo I like that idea, and the only reason it's not already implemented is splats, because they include multiple URI components.

routes: {
  'search/*path': 'search'
},
search: function(path) {
  // For the fragment `search/foo%2Fbar/baz`, is the path `foo/bar/baz`, `foo/bar%2Fbaz`, etc.?
}

Perhaps we could treat splats differently though?

Contributor

philfreo commented Dec 19, 2012

I would assume in that example that you should get a path of foo%2Fbar/baz, since it seems like people using splats want to be the ones to manually split upon /. Seems doable if splats can only appear at the end of a route.

Collaborator

braddunbar commented Dec 21, 2012

It would take some extra bookkeeping, but it's probably doable. Seems that it would be nice to get decoded params since that's usually what you want anyway. I'll work up a patch.

Collaborator

braddunbar commented Dec 21, 2012

For posterity, the location.hash bug: https://bugzilla.mozilla.org/show_bug.cgi?id=483304

Collaborator

braddunbar commented Dec 21, 2012

Entered the location.href bug here: https://bugzilla.mozilla.org/show_bug.cgi?id=824039

braddunbar added a commit to braddunbar/backbone that referenced this issue Dec 22, 2012

Collaborator

braddunbar commented Dec 22, 2012

In his response to the location.href bug, Boris Zbarsky points out that it is not, in fact, a bug (I'm still not certain what's correct after reading the spec, but I'm certain that Boris has more expertise than I do). Since firefox is only escaping unicode characters (not special characters) we can safely decode them with decodeURI (not decodeURIComponent). The fix is in #2010.

@chuangbo Many thanks for your persistence in getting this fixed.

Contributor

philfreo commented Dec 22, 2012

@braddunbar are you still planning on making a PR that prevents having to do decodeURIComponent in routes in most cases, as described above? (#2010 doesn't seem to do it)

Collaborator

braddunbar commented Dec 22, 2012

@philfreo Definitely! It's already working locally but I need to polish it a bit before pushing. :)

Owner

jashkenas commented Mar 19, 2013

Let's move this conversation over to the open PR.

@jashkenas jashkenas closed this Mar 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment