Closed Bug 1320061 Opened 3 years ago Closed 3 years ago

Copying non-ascii URI from location bar

Categories

(Firefox :: Address Bar, defect, P5)

50 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox53 --- verified

People

(Reporter: sskarr, Assigned: valentin)

References

Details

(Keywords: user-doc-needed, Whiteboard: [fxsearch])

User Story

Allow to copy decoded urls through a "browser.urlbar.decodeURLsOnCopy" preference.

Attachments

(1 file, 3 obsolete files)

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://путин.президент.рф/биография
Check https://bugzilla.mozilla.org/show_bug.cgi?id=1271088
Component: Untriaged → Location Bar
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
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1271088
> 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?
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
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
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1271088
(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.
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.
Flags: needinfo?(valentin.gosu)
(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?
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.
Assignee: nobody → valentin.gosu
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(valentin.gosu)
Resolution: DUPLICATE → ---
MozReview-Commit-ID: CDnMnkqj8gW
Attachment #8816156 - Flags: review?(mak77)
Blocks: 1321705
(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.
(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 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.
Attachment #8816156 - Flags: review?(mak77)
Priority: -- → P5
Whiteboard: [fxsearch]
MozReview-Commit-ID: CDnMnkqj8gW
Attachment #8817215 - Flags: review?(mak77)
Attachment #8816156 - Attachment is obsolete: true
I tried to add a test, but for some reason it times out, and I can't figure out what it's doing wrong.
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}]
(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.
I think this can wait until next week. Thanks!
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 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.
Attachment #8817215 - Flags: review?(mak77) → review-
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!
Flags: needinfo?(mak77)
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.
Attachment #8822205 - Flags: review?(mak77)
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 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
Attachment #8822205 - Flags: review?(mak77) → review+
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?
Flags: needinfo?(mak77)
Attachment #8822205 - Flags: feedback?(valentin.gosu)
Attachment #8817215 - Attachment is obsolete: true
Attachment #8817223 - Attachment is obsolete: true
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!
Attachment #8822205 - Flags: feedback?(valentin.gosu) → feedback+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/d55a6be0bae2
Add pref that allows users to copy unescaped URL from the URL bar r=mak
https://hg.mozilla.org/mozilla-central/rev/d55a6be0bae2
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Duplicate of this bug: 1338991
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.
Keywords: user-doc-needed
(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.
Flags: needinfo?(sskarr)
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://екатеринбург.рф/справка
Flags: needinfo?(sskarr)
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://минобрнауки.рф/министерство
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
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.
User Story: (updated)
It's for example, because I do not have "browser.urlbar.decodeURLsOnCopy" parameter.
(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/
QA Whiteboard: [good first verify]
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
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1424175
Duplicate of this bug: 1271088
You need to log in before you can comment on or make changes to this bug.