1320061 - Copying non-ascii URI from location bar

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P5)

Description

[Skar]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161104212021

Steps to reproduce:

Does no matter network.standard-url.escape-utf8 is true or false

1. Open http://путин.президент.рф/
2. Navigate to "Биография"
3. Try to copy opened URI from location bar ( http://путин.президент.рф/биография )


Actual results:

Copied URI: http://путин.президент.рф/%D0%B1%D0%B8%D0%BE%D0%B3%D1%80%D0%B0%D1%84%D0%B8%D1%8F


Expected results:

Copied URI: http://путин.президент.рф/биография

Comment 1

[Skar]

Check https://bugzilla.mozilla.org/show_bug.cgi?id=1271088

Comment 2

[Marco Bonardo [:mak]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1271088#c7 states that pref was going away and
https://bugzilla.mozilla.org/show_bug.cgi?id=1271088#c11 explains the reasoning

There are 2 workarounds at
https://bugzilla.mozilla.org/show_bug.cgi?id=1271088#c9
and
https://bugzilla.mozilla.org/show_bug.cgi?id=1271088#c16

Comment 3

[Skar]

> https://bugzilla.mozilla.org/show_bug.cgi?id=1271088#c7 states that pref was going away
Why I should worry about something in Chrome?

https://bugzilla.mozilla.org/show_bug.cgi?id=1271088#c9
> I am sorry, but could not care less about the behaviour of Chrome, and even less I understand why it's negative sides should be brought to FF.


> https://bugzilla.mozilla.org/show_bug.cgi?id=1271088#c11 explains the reasoning
Explains what? It's explains absolutely nothing.
Did you check my example?
How cyrillic path can break something that can not be broken by cyrillic hostname?

 
> There are 2 workarounds at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1271088#c9
> and
> https://bugzilla.mozilla.org/show_bug.cgi?id=1271088#c16
Workaround for URL copy-paste? Addon? Really? Maybe screenshooting rather?

Comment 4

[Marco Bonardo [:mak]]

what has Chrome to do with this? it was merely said Chrome does the same, as an example of behavior in other browsers. That's just a tiny part of the decision process.

If you can bring up points that were not discussed in the original bug please do, otherwise nothing is ever going to happen here. Afaict, there's nothing new here compared to what was discussed in the original bug.

(In reply to Valentin Gosu [:valentin] from comment #11)
> Most people who copy the URL from the URL bar want a URL that works in other
> applications. If you want a URL that is user readable, then what you want is
> probably a different way/UI/shortcut of copying that.

Please, don't reopen unless you have new points to bring up to that discussion table, and please check our netiquette https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Comment 5

[Skar]

(In reply to Marco Bonardo [::mak] from comment #4)
> what has Chrome to do with this? it was merely said Chrome does the same, as
> an example of behavior in other browsers. That's just a tiny part of the
> decision process.
> 
> If you can bring up points that were not discussed in the original bug
> please do, otherwise nothing is ever going to happen here. Afaict, there's
> nothing new here compared to what was discussed in the original bug.
> 
> (In reply to Valentin Gosu [:valentin] from comment #11)
> > Most people who copy the URL from the URL bar want a URL that works in other
> > applications. If you want a URL that is user readable, then what you want is
> > probably a different way/UI/shortcut of copying that.


Did you see this question?

(In reply to Skar from comment #3)
> Did you check my example?
> How cyrillic path can break something that can not be broken by cyrillic hostname?

Just answer the question.

Comment 6

[Marco Bonardo [:mak]]

I don't have an answer cause I didn't even investigate the original report. I guess Valentin may have evidence those paths caused problems in third party software, or the host part is a different bug. Let's ni him to bring on a constructive discussion.

Comment 7

[Skar]

(In reply to Marco Bonardo [::mak] from comment #6)
> I don't have an answer cause I didn't even investigate the original report.

You close my request, mark as duplicate and you say that you didn't investigate the problem?

Comment 8

[Valentin Gosu [:valentin] (he/him)]

So, there is actually no reason we couldn't support that behaviour, but this is a UI issue, not a URL parser issue. As such, the network.standard-url.escape-utf8 pref has nothing to do with this, and as I stated before, changing it can make your browser misbehave at times.

I am reopening the bug, and will be submitting a small patch soon.
Even though this is a very odd corner case to be supporting, it might be very useful to some people.

(In reply to Skar from comment #7)
> (In reply to Marco Bonardo [::mak] from comment #6)
> > I don't have an answer cause I didn't even investigate the original report.
> You close my request, mark as duplicate and you say that you didn't
> investigate the problem?

@Skar: I would appreciate it if we can keep the conversation civil. Marco was correct in assessing that the two bugs are the same, and deferred to our previous decision. If you disagree with that decision, you may state your reasons against it, but please refrain from personal attacks, sarcasm or making a fuss. With that said, thanks for your input.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Attachment: Add pref that allows users to copy unescaped URL from the URL bar

MozReview-Commit-ID: CDnMnkqj8gW

Comment 10

[Skar]

(In reply to Valentin Gosu [:valentin] from comment #8)
> it might be very useful to some people.

It's very important for some people.
Thank you!

(In reply to Valentin Gosu [:valentin] from comment #8)
> @Skar: I would appreciate it if we can keep the conversation civil. Marco
> was correct in assessing that the two bugs are the same, and deferred to our
> previous decision. If you disagree with that decision, you may state your
> reasons against it, but please refrain from personal attacks, sarcasm or
> making a fuss. With that said, thanks for your input.

I was correct at first. But Marco do not try to reproduce it, He check title and link to old bug and just close report.

I was create this report because I can't reopen old reports.

I gave an extremely clear case where the bug is obvious (cyrillic hostname + cyrillic path).
(In this case Chrome gave something like http://xn--h1akeme.xn--d1abbgf6aiiy.xn--p1ai/%D0%B1%D0%B8%D0%BE%D0%B3%D1%80%D0%B0%D1%84%D0%B8%D1%8F)

Firefox with 
 - network.standard-url.escape-utf8=true must escape hostname too
   http://xn--h1akeme.xn--d1abbgf6aiiy.xn--p1ai/%D0%B1%D0%B8%D0%BE%D0%B3%D1%80%D0%B0%D1%84%D0%B8%D1%8F

 - network.standard-url.escape-utf8=false must do not escape anything (except space symbols)
   http://путин.президент.рф/биография

So, it's bug wider than 1271088 and similar.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Skar from comment #10)
> I was correct at first. But Marco do not try to reproduce it, He check title
> and link to old bug and just close report.

Nope, I tried to reproduce it actually, but that doesn't mean anything, we have hundreds of reproducible requests that end up being wontfix. I also went through every single comment in bug 1271088, until it appeared to be pretty much the same kind of request. I closed and asked you to provide reasoning why this was differemt, and you did, so I ni? the most relevant person. This is pretty much the normal handling of a bug.

>  - network.standard-url.escape-utf8=false must do not escape anything
> (except space symbols)
>    http://путин.президент.рф/биография

The patch won't fix this, as explained in comment 8 you should not touch that pref, and it's exactly the same request as bug 1271088, and it's exactly the reason this was duped to that one originally. The behavior of network.standard-url.escape-utf8 is not going to change, and I still don't know if that pref will be removed or not, so you should not rely on it.

The new suggested pref will do what you want though.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8816156 [details] [diff] [review]
Add pref that allows users to copy unescaped URL from the URL bar

Review of attachment 8816156 [details] [diff] [review]:
-----------------------------------------------------------------

could you please add a couple sub-tests to browser/base/content/test/general/browser_urlbarCopying.js to check the pref works correctly?

Btw, it is strange that when we select the whole url we use the encoded spec, while when we select part of the url we just use part of the input value. But afaict, that's exactly what we do.
That looks bogus if I look at the original reasoning in bug 1271088, either we always copy encoded and provide a pref to override, or we always copy decoded. If you try selecting the whole url vs the whole url minus the last char you will see what I mean. The last return always uses decoded values, regardless. We don't have to fix everything here, but it would be nice to have a clear position on this.

::: browser/app/profile/firefox.js
@@ +310,5 @@
>  #endif
>  
> +// If changed to true, copying the entire URL from the location bar will put the
> +// human readable (percent-decoded) URL on the clipboard.
> +pref("browser.urlbar.precentDecodeCopy", false);

apart from the obvious typo, I think I'd prefer something more readable like browser.urlbar.decodeURLsOnCopy

::: browser/base/content/urlbarBindings.xml
@@ +802,5 @@
>                selectedVal = uri.spec;
>              }
>  
> +            if (Services.prefs.getBoolPref("browser.urlbar.precentDecodeCopy")) {
> +              return losslessDecodeURI(uri);

Couldn't this just be merged with the previous if like:

if (!uri.schemeIs("javascript") && !uri.schemeIs("data") &&
    !Services.prefs.getBoolPref("our.new.pref")) {
  selectedVal = ...

selectedVal by default is the value shown in the urlbar input field, that is prettyfied, so there should be no need to losslessDecode uri again.

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Attachment: Add pref that allows users to copy unescaped URL from the URL bar

MozReview-Commit-ID: CDnMnkqj8gW

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Attachment: Add test for copying non-ascii URL from the urlbar

I tried to add a test, but for some reason it times out, and I can't figure out what it's doing wrong.

Comment 15

[Marco Bonardo [:mak]]

where does it timeout? it's possible that loadEvent here:
http://searchfox.org/mozilla-central/rev/b9c9d257366379ac984622dbef38432a7fecd2e9/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#183
is getting url in a different encoding and thus this simple comparison fails:
http://searchfox.org/mozilla-central/rev/b9c9d257366379ac984622dbef38432a7fecd2e9/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#191

Comment 16

[Valentin Gosu [:valentin] (he/him)]

I don't think that's it. I seem to be getting this error in the console:
60 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "data:text/html,(%C3%A9%20%25%50)" line: 0}]

Comment 17

[Marco Bonardo [:mak]]

(In reply to Valentin Gosu [:valentin] from comment #16)
> I don't think that's it. I seem to be getting this error in the console:
> 60 INFO Console message: [JavaScript Error: "The character encoding of the
> HTML document was not declared. The document will render with garbled text
> in some browser configurations if the document contains characters from
> outside the US-ASCII range. The character encoding of the page must be
> declared in the document or in the transfer protocol." {file:
> "data:text/html,(%C3%A9%20%25%50)" line: 0}]

I've seen that often without any timeout problems, it may be a red herring.
I hope to find some time to try to reproduce, but there are too many meetings/talks this week! On the other side this is not urgent, so in the worst case it will delay for some days.

Comment 18

[Valentin Gosu [:valentin] (he/him)]

I think this can wait until next week. Thanks!

Comment 19

[Marco Bonardo [:mak]]

As I suspected in comment 15, the problem is the url comparison done by browserLoaded, it just compares the passed in href with the one returned by the "browser-test-utils:loadEvent" message that is encoded. It compares encoded and decoded versions and fails. Since it only resolves when the url matches, it times out.

So the test should read:
  loadURL: "http://example.com/%D0%B1%D0%B8%D0%BE%D0%B3%D1%80%D0%B0%D1%84%D0%B8%D1%8F",

There's still a problem though, the code path we hit returns selectedVal, that is trimmed, while we rather want the untrimmed result. So the test still fails on copyExpected.

Looks like moving up the condition to the previous if may fix that:
if (inputVal == selectedVal && !Services.prefs.getBoolPref("browser.urlbar.decodeURLsOnCopy")) {
This way we hit the final code path. The code as-is is not great and looks confusing. The bug this uncovers is not a problem for data: or javascript: just because we don't trim those urls...

This I think a more readable code path could be:

// If the entire URL is selected, just use the actual loaded URI,
// unless we want a decoded URI, or it's a data: or javascript: URI,
// since those are hard to read when encoded.
if (inputVal == selectedVal &&
    !uri.schemeIs("javascript") && !uri.schemeIs("data") &&
    !Services.prefs.getBoolPref("browser.urlbar.decodeURLsOnCopy")) {
  return uri.spec;
}

// Just the beginning of the URL is selected, or we want a decoded
// url. First check for a trimmed value.
...

finally, the partial copy test should read:
copyExpected: toUnicode("http://example.com/би")
cause when copying from the beginning we are expected to add back the trimmed part.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8817215 [details] [diff] [review]
Add pref that allows users to copy unescaped URL from the URL bar

based on previous comment, unfortunately this is still not correct.

Comment 21

[Valentin Gosu [:valentin] (he/him)]

Marco, I'm having trouble getting the test to work. Do you think you could take it, or assign it to someone on the Fx team? Thanks!

Comment 22

[Marco Bonardo [:mak]]

I suppose I can make a review in form of a merged patch, it's not a big difference from you addressing my review comments.

Comment 23

[Marco Bonardo [:mak]]

Attachment: Bug 1320061 - Add pref that allows users to copy unescaped URL from the URL bar

* * *
Bug 1320061 - Add test for copying non-ascii URL from the urlbar

Review commit: https://reviewboard.mozilla.org/r/101190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101190/

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 8822205 [details]
Bug 1320061 - Add pref that allows users to copy unescaped URL from the URL bar

https://reviewboard.mozilla.org/r/101188/#review101680

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 8822205 [details]
Bug 1320061 - Add pref that allows users to copy unescaped URL from the URL bar

https://reviewboard.mozilla.org/r/101190/#review101682

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 8822205 [details]
Bug 1320061 - Add pref that allows users to copy unescaped URL from the URL bar

Does this look good to you?

Comment 27

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8822205 [details]
Bug 1320061 - Add pref that allows users to copy unescaped URL from the URL bar

Thanks for this. Now I get why my test was failing :)
Looks good!

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d55a6be0bae2
Add pref that allows users to copy unescaped URL from the URL bar r=mak

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d55a6be0bae2

Comment 31

[Valentin Gosu [:valentin] (he/him)]

Since this is a bug that regularly comes up, it would be useful to have some documentation about it, at least a release note pointing out that this pref exists.

Comment 32

[Tanvir Rahman]

(In reply to Skar from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101
> Firefox/50.0
> Build ID: 20161104212021
> 
> Steps to reproduce:
> 
> Does no matter network.standard-url.escape-utf8 is true or false
> 
> 1. Open http://путин.президент.рф/
> 2. Navigate to "Биография"
> 3. Try to copy opened URI from location bar (
> http://путин.президент.рф/биография )
> 
> 
> Actual results:
> 
> Copied URI:
> http://путин.президент.рф/
> %D0%B1%D0%B8%D0%BE%D0%B3%D1%80%D0%B0%D1%84%D0%B8%D1%8F
> 
> 
> Expected results:
> 
> Copied URI: http://путин.президент.рф/биография

For verification purpose, i tried to reproduce this bug by following str. But the links are not working.

Comment 33

[Skar]

Updated Steps to reproduce:

1. Open https://екатеринбург.рф/
2. Navigate to "Справка"
3. Try to copy opened URI from location bar ( https://екатеринбург.рф/справка )


Actual results:

Copied URI: https://екатеринбург.рф/%D1%81%D0%BF%D1%80%D0%B0%D0%B2%D0%BA%D0%B0


Expected results:

Copied URI: https://екатеринбург.рф/справка

Comment 34

[Skar]

Hm, for some reasons with https works correct sometimes, but with http it doesn't work always.

Updated Updated Steps to reproduce:

1. Open http://минобрнауки.рф/
2. Navigate to "Справка"
3. Try to copy opened URI from location bar ( http://минобрнауки.рф/министерство )


Actual results:

Copied URI: http://минобрнауки.рф/%D0%BC%D0%B8%D0%BD%D0%B8%D1%81%D1%82%D0%B5%D1%80%D1%81%D1%82%D0%B2%D0%BE


Expected results:

Copied URI: http://минобрнауки.рф/министерство

Comment 35

[Skar]

So... Sorry for previous comments, I hurried.

Firefox 51.0.1

1) network.standard-url.escape-utf8 = false

1.1) URL is HTTP:

1. Open http://минобрнауки.рф/
2. Navigate to "Министерство"
3. Try to copy opened URI from location bar ( http://минобрнауки.рф/министерство )

Actual results:
Copied URI: http://минобрнауки.рф/%D0%BC%D0%B8%D0%BD%D0%B8%D1%81%D1%82%D0%B5%D1%80%D1%81%D1%82%D0%B2%D0%BE

Expected results:
Copied URI: http://минобрнауки.рф/министерство


1.2) URL is HTTPS (seems to be correct):

1. Open https://екатеринбург.рф/
2. Navigate to "Справка"
3. Try to copy opened URI from location bar ( https://екатеринбург.рф/справка )

Actual results:
Copied URI: https://екатеринбург.рф/справка

Expected results:
Copied URI: https://екатеринбург.рф/справка


2) network.standard-url.escape-utf8 = true

2.1) URL is HTTP:

1. Open http://минобрнауки.рф/
2. Navigate to "Министерство"
3. Try to copy opened URI from location bar ( http://минобрнауки.рф/министерство )

Actual results:
Copied URI: http://минобрнауки.рф/%D0%BC%D0%B8%D0%BD%D0%B8%D1%81%D1%82%D0%B5%D1%80%D1%81%D1%82%D0%B2%D0%BE

Expected results:
Copied URI: http://xn--80abucjiibhv9a.xn--p1ai/%D0%BC%D0%B8%D0%BD%D0%B8%D1%81%D1%82%D0%B5%D1%80%D1%81%D1%82%D0%B2%D0%BE

2.2) URL is HTTPS:

1. Open https://екатеринбург.рф/
2. Navigate to "Справка"
3. Try to copy opened URI from location bar ( https://екатеринбург.рф/справка )

Actual results:
Copied URI: https://екатеринбург.рф/%D1%81%D0%BF%D1%80%D0%B0%D0%B2%D0%BA%D0%B0

Expected results:
Copied URI: https://xn--80acgfbsl1azdqr.xn--p1ai/%D1%81%D0%BF%D1%80%D0%B0%D0%B2%D0%BA%D0%B0

Comment 36

[Marco Bonardo [:mak]]

You should not be touching the "network.standard-url.escape-utf8" pref, see comment 8. Unfortunately Comment 0 pointed that out, but it's unrelated to this fix.

This bug added a "browser.urlbar.decodeURLsOnCopy" pref to fix this, that's what you should set to true.

Comment 37

[Skar]

It's for example, because I do not have "browser.urlbar.decodeURLsOnCopy" parameter.

Comment 38

[Valentin Gosu [:valentin] (he/him)]

(In reply to Skar from comment #37)
> It's for example, because I do not have "browser.urlbar.decodeURLsOnCopy"
> parameter.

This landed in Firefox53 - so you should be using Firefox Dev Edition to test it: https://www.mozilla.org/en-US/firefox/channel/desktop/

Comment 39

[Kazi Nuzhat Tasnem]

I have reproduced this bug with Firefox nightly 52.0a1 (build id:20161104030212)on
windows 7(64 bit)

I have verified this bug as fixed with Firefox beta 53.0b3 (build id:20170316045436)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0

Comment 42

[Satoru Takagi]

I would like this option to be true by default. Also, all browsers should behave that way, so I posted this issue on WICG Discourse as well, referring to this thread
https://discourse.wicg.io/t/need-to-stop-encoding-non-ascii-characters-when-copying-urls-in-the-locaition-bar/6011