fix(url.ml): set_fragment need not any urlencode

[erikmd]

Dear js_of_ocaml maintainers,

This patch comes from an issue spotted in learn-ocaml (which heavily relies on js_of_ocaml), namely:

• Feature: rewrite URLs so that %3D chars are rendered as = ocaml-sf/learn-ocaml#539

Note: I didn't recompile js_of_ocaml itself yet to test it, but I'm confident of the fix; to summarize what I understood:

1. learn-ocaml handles local urls of the form http://localhost:8080/exercises/tp1/#tab=report;
2. it contains some (js_of_)ocaml code such as Url.Current.set_fragment "tab=report" to this aim;
3. unfortunately the browser URL was always http://localhost:8080/exercises/tp1/#tab%3Dreport, which is not that legible!;
4. replacing the line of point 2. above with Dom_html.window##.location##.hash := Js.bytestring "tab=report" solves the issue.

• FTR, the document.location.hash is documented at https://developer.mozilla.org/en-US/docs/Web/API/Location/hash
• I noticed there was an old bug in Firefox w.r.t. encoding/decoding only for the getter:
  • see https://stackoverflow.com/questions/1703552/encoding-of-window-location-hash
  • and https://bugzilla.mozilla.org/show_bug.cgi?id=483304
• But this bug was fixed 8 years ago and anyway, this bug only dealt with the getter of the fragment.
• So there was no need for manually url-encoding the fragment for the setter as well!

Hence this small patch; WDYT?

[hhugo]

We should probably check that get_fragment (set_fragment ()) round trip

[erikmd]

Hi @hhugo, that's a good point, thanks!
on the one hand, I'm pretty sure this equality now holds;
on the other hand, I'm not aware enough of the js_of_ocaml codebase to be able to add a unit test for this myself!

[hhugo]

@erikmd, the round-tripping seems to work more often with your PR. In particular, it works with -._~!$&'()*+,;=:@.
As far as I understand, location.hash just returns what's displayed on the url bar of my web browser. Setting location.hash will percent encode some characters. @dbuenzli, it seems you've dived into this mess recently. Any opinion on what should be done here ?

[dbuenzli]

Not really it's difficult for me to say what is right here because:

1. This API doesn't say exactly what it tries to expose and in particular if arguments are supposed to be percent encoded or not.
2. I do not know how browsers render locations. Some stuff gets percent encoded some doesn't (e.g. the encoding of accented characters) I don't know if there's a specification for that.
3. This module seems to be doing its own URL encoding rather than use encodeURI or encodeURIComponent.

Also Brr doesn't use Location objects which seems to be a different URI like structure (I conjecture kept for compat reasons). The API exposed by brr to interact with the browser location is to capture the location as an URL object and set the location from an URL object (and Brr does not try to mutate URL objects).

In any case if you are interested in what happened in Brr the API is here and the recent discussion is there.

[erikmd]

@hhugo the round-tripping seems to work more often with your PR

Good! but out of curiosity, when do you think this does not "work"?

As far as I understand, location.hash just returns what's displayed on the url bar of my web browser.

yes, almost: it returns what follows the #. You can also try running location.hash; in the browser console.

Setting location.hash will percent encode some characters

this is maybe another issue. See my initial comment. To sum up, more than 8 years ago, there was a bug in firefox about the location.getter. Hence the somewhat involved code before this PR that did not simply call the location.hash property.

This bug is now solved, so I'd suggest we also simplify the getter code along the way.

I will push another commit, @hhugo could you run the same tests? (maybe with Firefox + another Chromium-based browser)

[erikmd]

Hi @dbuenzli (disclaimer: I'm not expert in js_of_ocaml! I just give my thoughts based on standard specs)

• This API doesn't say exactly what it tries to expose and in particular if arguments are supposed to be percent encoded or not.

IMO the API should faithfully map JavaScript properties to OCaml ones.

And according to https://developer.mozilla.org/en-US/docs/Web/API/Location/hash:
"The fragment is not URL decoded."

• I do not know how browsers render locations.

According to examples from https://stackoverflow.com/questions/1703552/encoding-of-window-location-hash:

1. running location.hash = "a=b"; location.hash in the console round-trips, so = should not be escaped (it was before this PR)
2. the browser renders UTF-8 location.hash as is but internally they are escaped, e.g. if we run location.hash="ü" in the console, we see #ü as is, but if we copy the entire URL (or do location.hash in the console), we see that internally, the hash only uses ASCII chars. Anyway, this does not hinder URL semantics. Indeed if one creates a dummy HTML page with <div id="ü> at the bottom, both file:///tmp/foo.html#%C3%BC and file:///tmp/foo.html#ü scrool to the anchor.

• Some stuff gets percent encoded some doesn't (e.g. the encoding of accented characters) I don't know if there's a specification for that.

RFC 3986 says:
the data should first be encoded as octets according to the UTF-8 character encoding; then only those octets that do not correspond to characters in the unreserved set should be percent-encoded.

• This module seems to be doing its own URL encoding rather than use encodeURI or encodeURIComponent.

Indeed, there was a spurious urlencoding before this PR, which should not be there.

[hhugo]

Good! but out of curiosity, when do you think this does not "work"?

• set "#" -> get ""
• set "#ABC" -> get "ABC"
• set " " -> get "%20"
• set "\001" -> get "%01"

@hhugo could you run the same tests?

You can test your self by building the toplevel

dune build toplevel/examples/lwt_toplevel/ && chromium-browser _build/default/toplevel/examples/lwt_toplevel/index.html

[dbuenzli]

IMO the API should faithfully map JavaScript properties to OCaml ones.

I don't really care about what it does. I'm just saying that the API docs should mention what is being done.

RFC 3986 says:

That's unrelated to the problem I mentioned (and so is checking stuff in the browser console).

Indeed, there was a spurious urlencoding before this PR, which should not be there.

That was not my point. The point is that this module has its own functions to perform url encoding.