After lengthy discussions, Patrick proposed that we disable or backout bug 1093611 because it would be able to produce "invalid" URLs. This feature is now on Beta. I suggest we just disable it by setting the dom.url.encode_decode_hash pref to false. This would also require uplifting bug 1145812 to beta (fairly trivial patch). We can also simply revert the patches. There are a few arguments for keeping this behaviour as is: * compatibility with Chrome, IE * bug 1093611 comment 56 * some developers and libraries put JSON in the hash of a URL Followups include fixing the root cause of 1093611 (not decoding the hash in the getter).

Compatibility with the web seems more important than "invalid URLs", whatever that means. How do we explain the behavior of e.g. data:text/plain,(foo bar) which would similarly be "invalid"?

I largely agree with Anne. Copying a URL from IE which contains a space in the hash, then pasting it into Fx and getting another result seems a bit off. We could just disable the feature, and re-enable it at a later time (once we figure out all the kinks)

I think we should WONTFIX this and fix bug 1148861 instead per your patch. I explained in bug 1148861 comment 38 why that is a better course of action. If there's any other considerations I'm missing I'd love to see them written down someplace.

(In reply to Anne (:annevk) from comment #1) > Compatibility with the web seems more important than "invalid URLs", > whatever that means. For instance, having a space in a URL breaks every single (native or Web) application trying to detect URLs in a blob of text. Facebook, your console, your favorite discussion forum, Gmail, Thunderbird, you name it. > Copying a URL from IE which contains a space in the hash, then pasting it into Fx and getting another > result seems a bit off. See above; copied URLs shouldn't have spaces in the hash in the first place, because pasting them in other URL-processing applications is the primary use case for copying them. Have we asked MS whether they're interested in fixing this?

Copied URLs hardly ever contain fragments so I don't think the problem is as big as you try to make it out to be. And yes, other vendors have been repeatedly asked to comply with the URL standard. At some point something has got to give.

(In reply to Anne (:annevk) from comment #5) > Copied URLs hardly ever contain fragments so I don't think the problem is as > big as you try to make it out to be. I wasn't claiming that most let alone all copied URLs would be broken. Complex hashes are certainly a minority case, but one that should work nonetheless. By the way, I hear that IE is being discontinued, so I wonder if MS' shiny new EdgeHTML copied IE's behavior. I guess both might be using some system library where this behavior is present and might be harder to change. If not, maybe MS is willing to reconsider this for their new engine where their compatibility constraints are somewhat different.

(In reply to Dão Gottwald [:dao] from comment #6) > (In reply to Anne (:annevk) from comment #5) > > Copied URLs hardly ever contain fragments so I don't think the problem is as > > big as you try to make it out to be. > > I wasn't claiming that most let alone all copied URLs would be broken. > Complex hashes are certainly a minority case, but one that should work > nonetheless. The patch in bug 1148861 would have the bug where valid URLs are turned into invalid ones. I still urge you to consider it. Unescaping URLs, then trying to fix them by encoding the invalid characters does not always lead to the original URL. I see this as a bigger issue. > > By the way, I hear that IE is being discontinued, so I wonder if MS' shiny > new EdgeHTML copied IE's behavior. I guess both might be using some system > library where this behavior is present and might be harder to change. If > not, maybe MS is willing to reconsider this for their new engine where their > compatibility constraints are somewhat different. I am a bit skeptical that their new browser would break compatibility with its older version. But you're welcome to try and change their minds. --- Given that there's no consensus in sight, I'll submit a patch to disable this behaviour. It's obviously not ready to go to release, since we're still arguing about it, and I'd rather we don't push it to beta at the last minute.

Valentin, could you find another reviewer? I would like to see the patch backout from 38. It is an ESR release and we should take the time to fix.

Comment on attachment 8588457 [details] [diff] [review] Backout or disable bug 1093611 r?ing Honza as well. Maybe he can get to this sooner.

Comment on attachment 8588457 [details] [diff] [review] Backout or disable bug 1093611 Review of attachment 8588457 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/test/test_url.html @@ +1,1 @@ > +f ? ::: testing/web-platform/meta/url/a-element.html.ini @@ +354,5 @@ > + [Parsing: <#β> against <http://example.org/foo/bar>] > + expected: FAIL > + > + [Parsing: <http://www.google.com/foo?bar=baz# »> against <about:blank>] > + expected: FAIL it would be better if you just 'patch -Rp1' your patch. This is not a clear back out. ::: testing/web-platform/meta/workers/WorkerLocation_hash_encoding.htm.ini @@ +1,4 @@ > +[WorkerLocation_hash_encoding.htm] > + type: testharness > + [ WorkerLocation.hash with url encoding string ] > + expected: FAIL what's this?

(In reply to Honza Bambas (:mayhemer) from comment #11) > Comment on attachment 8588457 [details] [diff] [review] > Backout or disable bug 1093611 > > Review of attachment 8588457 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/base/test/test_url.html > @@ +1,1 @@ > > +f > > ? I'm not sure how that slipped through. Good catch. > > ::: testing/web-platform/meta/url/a-element.html.ini > @@ +354,5 @@ > > + [Parsing: <#β> against <http://example.org/foo/bar>] > > + expected: FAIL > > + > > + [Parsing: <http://www.google.com/foo?bar=baz# »> against <about:blank>] > > + expected: FAIL > > it would be better if you just 'patch -Rp1' your patch. This is not a clear > back out. I hit a bunch of conflicts when trying to backout, so I had to do it by hand. > > ::: testing/web-platform/meta/workers/WorkerLocation_hash_encoding.htm.ini > @@ +1,4 @@ > > +[WorkerLocation_hash_encoding.htm] > > + type: testharness > > + [ WorkerLocation.hash with url encoding string ] > > + expected: FAIL > > what's this? This test was added after I pushed my patch to m-c, so I added this exception as well.

Green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb4ff1325862

thanks honza!

Comment on attachment 8592320 [details] [diff] [review] Backout or disable bug 1093611 Review of attachment 8592320 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/meta/url/a-element.html.ini @@ +354,5 @@ > + [Parsing: <#β> against <http://example.org/foo/bar>] > + expected: FAIL > + > + [Parsing: <http://www.google.com/foo?bar=baz# »> against <about:blank>] > + expected: FAIL the thing is that your patch removes something else than this patch adds.. (the xhtml file as well). up to you to explain or fix

(In reply to Honza Bambas (:mayhemer) from comment #15) > the thing is that your patch removes something else than this patch adds.. > (the xhtml file as well). > > up to you to explain or fix Those tests were added after bug 1093611 landed: james 222490 2015-01-07: \#\u03B2 s:http h:example.org p:/foo/bar f:#\u03B2 james 222490 2015-01-07: http://www.google.com/foo?bar=baz#\s\u00BB s:http h:www.google.com p:/foo q:?bar=baz f:#\s\u00BB These are now expected: FAIL Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fb883edad3a Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ef2f83420d2

This needs to be landed to mozilla-beta (38) and mozilla-aurora (39), right? Once backed out, I'll remove the compat note from https://developer.mozilla.org/en-US/Firefox/Releases/38/Site_Compatibility

Yes, we do want an uplift, I am going to untrack bug 1148861. Valentin, could you fill this uplift request?

Comment on attachment 8595378 [details] [diff] [review] Disable bug 1093611. Set pref dom.url.encode_decode_hash to true. (Uplift) Approval Request Comment [Feature/regressing bug #]: Bug 1093611 [User impact if declined]: Bug 1148861 - copying the URL from the location bar transforms %20 to space [Describe test coverage new/current, TreeHerder]: Currently on m-c. Several unit tests exist for this behaviour. [Risks and why]: Low risk. This bug reverts to the previous behaviour by flipping the pref, and changing the expectation of unit tests. Bug 1145812 (attachment 8581054 [details] [diff] [review]) also needs to be uplifted to beta along with this patch. [String/UUID change made/needed]: None.

Comment on attachment 8595378 [details] [diff] [review] Disable bug 1093611. Set pref dom.url.encode_decode_hash to true. (Uplift) Thanks. Should be in 38 beta 7.

https://developer.mozilla.org/en-US/docs/Web/API/URLUtils/hash and https://developer.mozilla.org/en-US/Firefox/Releases/38 fixed to their previous state.