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

try to more consistently handle URI-encoded URLs #108

Merged
0 commits merged into from May 30, 2013
Merged

try to more consistently handle URI-encoded URLs #108

0 commits merged into from May 30, 2013

Conversation

hrunting
Copy link
Contributor

@hrunting hrunting commented Oct 4, 2011

  • use document.URL where possible as it's more consistently URI-encoded
    across different browsers (e.g. Safari 5.1)
  • for HTML4 browsers, assume the URI passed to pushState() is URI-encoded,
    and make sure that every character is encoded like document.URL
  • fix a spelling mistake in the unsupport-fragment error
  • this code is based on String escaping - a safer solution #107, but
    extended to better handle some edge cases in IE and Safari

@dobesv
Copy link

dobesv commented Oct 7, 2011

Wow this is such a timely patch, I was just trying to figure out why my URls were getting messed up with Chinese characters. Thank you thank you thank you!

@hrunting
Copy link
Contributor Author

hrunting commented Oct 7, 2011

I'll have a better patch later today. This one doesn't handle some edge cases properly with encoded URI separators (&, #, ;, etc.). Also, I make no guarantees about whether or not this works in the older browsers for which history.js maintains compatibility. My testing setup is somewhat limited.

@andrew-sayers--slando
Copy link
Contributor

This is much better than what I had in #107, but there are still a few little bugs:

  • document.URL doesn't include the hash in FF3.6 (available at http://www.mozilla.org/en-US/firefox/all-older.html if you want to test)
  • there's a "g" missing in the encodeURI() calls
  • one encodeURI() doesn't have a replace (I assume this is an oversight? Please let me know if it was deliberate)

We're using the master branch on our site, so I can't add directly to this pull request. Instead I've updated #107 to match this request, plus an extra commit with the issues discussed above. Hopefully you can cherry-pick that last commit to get the fixes.

@andrew-sayers--slando
Copy link
Contributor

Ah, I forgot - I also spotted an extra "fragement" typo - fixed in commit 9234ea7, not in the last commit.

@hrunting
Copy link
Contributor Author

hrunting commented Nov 6, 2011

Try commit 31eb079 and see if that fixes your issues with FF3.6.

document.URL does actually include the hash in FF3.6 (got to be careful with that), but document.URL does not update as document.location.hash changes. I pulled in your fragment changes, but didn't pull in the other changes you identified (I left some comments highlighting what I think may be some problems with them).

@svanderbleek
Copy link

hrunting's changes definitely worked for me. I recommend merging them in.

@dobesv
Copy link

dobesv commented Jan 11, 2012

I seem to be running into an issue with IE9 (HTML4 browsers) that I think is related to this code. On these browsers the statechange event doesn't fire after pushState() when there are special characters which are already URL encoded in the URL passed to pushState(). The behavior is:

  1. Call pushState({}, 'title', '?@')
  2. Sets the hash (#
  3. onHashChange is called; it detects this is a new hash and calls pushState()
  4. pushState considers this to be a change to the hash and updates the hash
  5. onHashChange is called; it doesn't see any change to the hash and does nothing

The problem is in Step 4. When it compares the new hash to the old one (newStateHash !== html4Hash), the newStateHash is URL-decoded but the html4Hash has the %-encoded characters. i.e. '?t=@' !== '?t=%40'.

When constructing a URL that is a query string I do think I need to call encodeURIComponent() on each key and value just in case there are special characters like =, ?, or & in there.

Anyone else seen this? Maybe it's something I messed up on my end when I copied this code over?

@dobesv
Copy link

dobesv commented Jan 11, 2012

If I change the initialization of html4Hash as:

html4Hash = decodeURIComponent(History.getHash())

This seems to fix my issue ... at least for my limited test case. Not totally comfortable I've hit on the core issue here, though.

@jhilden
Copy link

jhilden commented Jan 18, 2012

I have the same problem as @dobesv in IE8 and IE9
html4Hash and newStateHash are never equal in my case, so there is like an infinite loop of pushState calls, because of this block: https://github.com/hrunting/history.js/blob/master/scripts/uncompressed/history.html4.js#L528

This is what i get with @hrunting's code:

newStateHash: page?utf8=%25E2%259C%2593
html4Hash:    page?utf8=%E2%9C%93

When I add
html4Hash = decodeURIComponent(History.getHash()) as @dobesv suggested i get the following:

newStateHash: page?utf8=%25E2%259C%2593
html4Hash:    page?utf8=✓

I also tried newStateHash = decodeURIComponent(History.getHashByState(newState)), which will result in this:

newStateHash: page?utf8=%E2%9C%93
html4Hash:    page?utf8=✓

Further help with this would be greatly appreciated.

By the way: Many thanks to @hrunting and @andrew-sayers--slando. their changes definitely work very well for me in FF an Chrome.

@dobesv
Copy link

dobesv commented Jan 18, 2012

Hmm interesting it appears that in your case the newStateHash has been double-encoded (%-encoded and then % itself further replaced with %25) and yet your html4Hash is just normally %-encoded.

How are you passing the URL into pushState()?

@jhilden
Copy link

jhilden commented Jan 18, 2012

Basically this is the URL that is pushed:

History.getState().url.split("?")[0] + "?" + $('#form').serialize()

@hrunting
Copy link
Contributor Author

hrunting commented Feb 2, 2012

I think there probably is an edge case with query arguments and HTML4 browsers. I'll need to spend some time going through some of the edge cases presented above. I think the "?@" is reasonably simple enough to test.

When you pass the result of History.getState().url.split("?")[0] + "?" + $('#form').serialize() to pushState(), are you passing the components through encodeURIComponent first? The changes I put in definitely require good URI encoding. It would probably benefit everyone to have some utility functions to sanely encode URLs. The problem I encountered most was that browsers simply don't return encoded URLs consistently. Sometimes some no characters are decoded. Sometimes some characters are decoded and others aren't. Sometimes all characters are decoded.

I'll see what I can put together. Thanks for the test cases.

@hrunting
Copy link
Contributor Author

hrunting commented Feb 5, 2012

Try that commit and see if it works for you. I haven't fully tested it yet, but it solved the condition with dobesv's test case.

@jhilden
Copy link

jhilden commented Feb 6, 2012

Thanks a million @hrunting for coming back to this issue and trying to find a solution! I pulled your commit but unfortunately it's still not working.

The problem is still that the hashes never become equal, so we get an infinite loop of pushState() calls. Here is the complete output I got for the individual pushState calls:
https://gist.github.com/d45cf88a520899fcc826

After that I looked a little more closely at the isHashEqual() function and actually I think a simple newHash.replace(/%25/g, "%") should already do the job of making the hashes equal from an encoding perspective. See this commit: https://github.com/9flats/history.js/commit/c272a9f229bcacf446b7f879d984cbf96765d827
But it took 3 runs before the main content of the hashes actually became equal and even then the suid's were still different:
https://gist.github.com/e3ba49618f724c4db8ba

So there must be more going on than a simple encoding problem. Unfortunately I haven't quite understood the whole workflow yet. Please let me know if I can provide any more information to solve this issue. How would I go about creating a "real" testcase that you could work with?

Also, to the question about the encoding of what I push into pushState in the first place: I do believe that jQuery's serialize() will encode all the components using encodeURIComponent. So that should be fine? Or should I try something different there?

Again, thanks a million and I hope this can get solved somehow.

@hrunting
Copy link
Contributor Author

hrunting commented Feb 7, 2012

jhilden, can you post the original initial call to pushState and the value that you're passing? It's weird that your newStateHash is getting double-encoded. I want to create a test case like I did with dobesv's example to look at it more closely.

The problem that I was looking at was pretty simple. When the State instance is initialized, the hash gets unescaped. In dobesv's case, that meant that "?%40" got translated internally to "?@". There's nothing wrong with that -- they're both equivalent URLs -- but it means you can't rely on straight-up string comparisons, and that's what isEqualHash() does -- reduces all the encoding ambiguity (I'd rather the history code be more consistent about encoding, but I'm not totally up to speed on all of that).

In your case, the two really aren't equivalent at all. Something like "?utf8=%25E2%259C%2593" is not equivalent to "?utf8=%E2%9C%93". But I can't reproduce that. In my test, when I pass in the first, I never end up at the latter.

@jhilden
Copy link

jhilden commented Feb 7, 2012

@hrunting this is the URL that gets pushed:

http://10.0.2.2:3000/searches?utf8=%E2%9C%93&search%5Bplace_type%5D=entire_home&search%5Bprice_min%5D=&search%5Bprice_max%5D=&search%5Bcurrency%5D=USD&search%5Bsort_by%5D=top_ranking&search%5Bview_index%5D=0&search%5Bnumber_of_bathrooms%5D=0&search%5Bnumber_of_bedrooms%5D=0&search%5Bradius%5D=&search%5Bamenities%5D=&search%5Bpage%5D=1&search%5Bwoeid%5D=667931&search%5Bbb_sw%5D=&search%5Bbb_ne%5D=&search%5Blat%5D=50.94165&search%5Blng%5D=6.95505&search%5Biso%5D=&search%5Bis_country%5D=&search%5Bcategory%5D=&search%5Bgeo_search%5D=&search%5Bquery%5D=Cologne&search%5Bstart_date%5D=&search%5Bend_date%5D=&search%5Bnumber_of_beds%5D=2

@hrunting
Copy link
Contributor Author

hrunting commented Feb 7, 2012

So you're pushing the full URL (proto + host + path + query) or are you pushing /searches?... or just ?...? I just want to make sure I reproduce it exactly.

@jhilden
Copy link

jhilden commented Feb 7, 2012

Yes, I'm pushing the full URL.

@hrunting
Copy link
Contributor Author

jhilden, I'm sorry, but I simply can't reproduce the problem you're seeing. I've tried every which way. It always works just fine in IE9. Does your IE happen to be running in quirks mode because of your page doesn't declare a DOCTYPE or something like that?

If you could send me the full HTML and JS for the system that's causing the problem, I could probably debug it, but in my simple cases, reproducing your URL as is, IE9 isn't having any problems.

@jhilden
Copy link

jhilden commented Feb 10, 2012

@hrunting thank you again for looking at my problem.
As far as I can tell I'm not in quirks mode. It says: Browser Mode: IE9 Document Mode: IE9 standards.

I guess it's not really possible to give you all the HTML and JS, but maybe you could give me what you were experimenting with, so I can play with it on my setup?

@hrunting
Copy link
Contributor Author

https://gist.github.com/1801624

Here's a basic page I've been using for testing. Put the URL you want to test in the go() call at line 51. I tested with all manner of variations that you posted and haven't been able to reproduce the endless loop you're seeing, so if you can reproduce the endless loop with this test page, let me know the go() call you made.

@evil-shrike
Copy link

Let me ask a question on URL (de)encoding here.
I call History.pushState with encoded URL (with help of encodeURIComponent). Something like this:
History.pushState(state, title, "/index[ObjectList%3Asurveys]", /queue/false);

But current browser URL has been changed to "/index[ObjectList:surveys]" - i.e. unencoded.
When I reload page from server I get an error as symbol ":" isn't acceptable.
How to work around this? I just want to encode url before sending to server.

@dobesv
Copy link

dobesv commented Mar 15, 2012

Did you do this with the patch attached to this thread, or a plain copy of
history.js?

On Thu, Mar 15, 2012 at 6:26 AM, evil-shrike <
reply@reply.github.com

wrote:

Let me ask a question on URL (de)encoding here.
I call History.pushState with encoded URL (with help of
encodeURIComponent). Something like this:
History.pushState(state, title, "/index[ObjectList%3Asurveys]",
/queue/false);

But current browser URL has been changed to "/index[ObjectList:surveys]" -
i.e. unencoded.
When I reload page from server I get an error as symbol ":" isn't
acceptable.
How to work around this? I just want to encode url before sending to
server.


Reply to this email directly or view it on GitHub:
#108 (comment)

@evil-shrike
Copy link

@dobesv:

Did you do this with the patch attached to this thread, or a plain copy of history.js?

with plain copy (1.7.1-r2)

@dobesv
Copy link

dobesv commented Mar 15, 2012

Ah, I see. Well if you download the patched version it might fix your
issues as it fixes various issues with URL encoding in history.js. For
reasons unknown to me it hasn't been integrated into the core release yet.
Perhaps the core maintainer is a tad busy and hasn't had time to vet and
merge the fixes yet.

On Thu, Mar 15, 2012 at 5:36 PM, evil-shrike <
reply@reply.github.com

wrote:

@dobesv:

Did you do this with the patch attached to this thread, or a plain copy
of history.js?

with plain copy (1.7.1-r2)


Reply to this email directly or view it on GitHub:
#108 (comment)

@floehopper
Copy link

We're seeing a problem with the encoding of the value of the utf8 hidden field that Rails adds to a form as a workaround to an IE bug. My colleague @chrisroos has setup a demo of the bug.

Is anything like this pull request likely to be accepted and merged in the near future?

@ghost ghost merged commit 7843891 into browserstate:master May 30, 2013
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants