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

Router test for urls with encoded characters #1156

Merged
merged 1 commit into from Apr 2, 2012

Conversation

@samuelvogel
Copy link
Contributor

samuelvogel commented Mar 29, 2012

This test works in IE, Safari, Chrome but not in Firefox (tested in v11).
Might be related to #967.

jashkenas added a commit that referenced this pull request Apr 2, 2012
Router test for urls with encoded characters
@jashkenas jashkenas merged commit 948d41c into jashkenas:master Apr 2, 2012
@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Apr 2, 2012

Test passes just fine in the latest firefox, using master backbone.

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Apr 2, 2012

Nevermind -- I had the wrong version, you're correct that the test fails. Looking into it...

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Apr 2, 2012

Reverted the test -- yes, this is where incorrect url encoding / decoding breaks the browser. There is an open ticket in Firefox's bugzilla for it, I believe.

@samuelvogel

This comment has been minimized.

Copy link
Contributor Author

samuelvogel commented Apr 3, 2012

After some research the situation in Firefox seems pretty complicated. The three major bug reports which relate to this behavior are:

  • Bug 135309 - location.hash should not be unescaped
  • Bug 483304 - location.hash getter returns the hash value unescaped ("%7C" turns into "|")
  • Bug 656691 - Setting location.hash mangles URLs with escaped characters

The last one claims to be fixed (or rather the one it's marked as duplicate of) but the test I wrote for Backbone still fails. I do not think this will be resolved in the near future in Firefox.
I would propose to fix this in Backbone by unescaping location.hash before matching routes against it. This will then break mixed URLs which contain both escaped and unescaped characters in all browsers, but this is rather exotic and a much smaller issue then having routing of escapable characters not work at all in Firefox.

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Apr 3, 2012

having routing of escapable characters not work at all in Firefox ...

I don't think that's currently a problem though -- just correctly URL-encode your characters before setting them in the URL, and Firefox can handle them just fine.

@samuelvogel

This comment has been minimized.

Copy link
Contributor Author

samuelvogel commented Apr 4, 2012

But when the URL is encoded, the regexp has to match against encoded value and gets really ugly. Or are you referring to something else?
The problem which my test exposes only relates to spaces in the hash tag, all other unsafe characters are tested work fine. This got introduced by the new getHash() function of #967.
So it seems like Backbone can either support manually encoded characters in the URL (current behavior) or non-encoded spaces (previous behavior) in Firefox because of it's buggy location.hash handling.

I would favor the previous behavior, because I don't see the value in the ability to have encoded characters in the URL to match routes. Who really write routes like this '%28test%29' instead of '(test)'?

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Apr 4, 2012

So it seems like Backbone can either support manually
encoded characters in the URL (current behavior) ...

Yep -- that's precisely the behavior we need to continue to support.

@samuelvogel

This comment has been minimized.

Copy link
Contributor Author

samuelvogel commented Apr 4, 2012

Okay, then we might document that spaces do not work in the hashtag in Firefox until Bug 483304 is fixed.

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Apr 4, 2012

That would be great.

@samuelvogel

This comment has been minimized.

Copy link
Contributor Author

samuelvogel commented Apr 13, 2012

Followup in #1219

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