Last Comment Bug 666964 - Prepend http:// to URL copy selection if the first character is included in the selection
: Prepend http:// to URL copy selection if the first character is included in t...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 8
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
:
Mentors:
: 667211 667231 670503 (view as bug list)
Depends on: 669483
Blocks: 665580
  Show dependency treegraph
 
Reported: 2011-06-24 10:23 PDT by :Margaret Leibovic
Modified: 2012-08-21 19:58 PDT (History)
35 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
patch? (3.52 KB, patch)
2011-06-24 14:07 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
WIP (6.63 KB, patch)
2011-06-24 14:08 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch (8.69 KB, patch)
2011-06-25 18:29 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
updated patch (9.11 KB, patch)
2011-06-29 19:03 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
dao+bmo: review+
Details | Diff | Splinter Review
comments addressed (9.20 KB, patch)
2011-07-08 11:50 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description :Margaret Leibovic 2011-06-24 10:23:44 PDT
From bug 665580 comment 9:
It would be really useful if the copy behaviour matched Chrome too: If the first character is included in the copy selection then prepend the protocol.

I've run into this since the patch for bug 665580 landed and agree with Chrome's approach. This only applies to the case where you are copying the beginning of the URL, not the entire URL.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-24 14:07:52 PDT
Created attachment 541789 [details] [diff] [review]
patch?

This is the approach I took initially because I didn't want to depend on the specifics of trimURL and simply prepend "http://" (particularly when it does not always strip "http://"). However it may be a little bit weird to have the encoded/decoded behavior for partial selections depend on where the selection starts. It may be worth investigating what Chrome does.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-24 14:08:50 PDT
Created attachment 541790 [details] [diff] [review]
WIP

forgot to qref
Comment 3 Ori Avtalion (salty-horse) 2011-06-25 05:46:34 PDT
Example of a usability issue solved by the suggested behavior:

I'm at <http://example.com/article#comments>. I want to send the article to a friend. I go to the URL, remove "#comments" and then copy it. I now have "example.com/article" on the clipboard. Pasting it into an email or instant messenger usually won't highlight the URL.

With this suggestion, the copied URL will include the protocol prefix.
Comment 4 Dão Gottwald [:dao] 2011-06-25 10:08:27 PDT
*** Bug 667211 has been marked as a duplicate of this bug. ***
Comment 5 Dão Gottwald [:dao] 2011-06-25 13:46:40 PDT
*** Bug 667231 has been marked as a duplicate of this bug. ***
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-25 18:29:14 PDT
Created attachment 541976 [details] [diff] [review]
patch

That approach didn't work because of encoding differences between the URI and the URL bar value. This patch just explicitly re-adds the trimmed portion when trimmed URLs are selected beginning at the start. I also reworked _getSelectedValueForClipboard a little bit.
Comment 7 Dão Gottwald [:dao] 2011-06-29 09:05:55 PDT
Comment on attachment 541976 [details] [diff] [review]
patch

>+    copyVal: "[e]xample.com",

I'd prefer <> instead of [], as these are less likely to appear in actual URLs.

>+      is(gURLBar.value, test.expectedURL, "url bar value is correct: " + test.expectedURL);

No need to spam logs with the expected URLs, is() will report them for failures. I'd suggest "checking urlbar value" or even just "urlbar value", since "TEST-UNEXPECTED-FAIL | foo is correct" is slightly strange.

>+  info("Expecting copy of: " + targetValue);

Looks like waitForClipboard should just do this if really deemed necessary.

>+    is(gBrowser.currentURI.spec, aURL, aURL + " loaded");

see above

>--- a/browser/base/content/test/browser_urlbarTrimURLs.js
>+++ b/browser/base/content/test/browser_urlbarTrimURLs.js

> function testCopy(originalValue, targetValue, cb) {
>   waitForClipboard(targetValue, function () {
>-    is(gURLBar.value, originalValue);
>+    is(gURLBar.value, originalValue, "URL bar value is " + originalValue);

ditto

>--- a/browser/base/content/urlbarBindings.xml
>+++ b/browser/base/content/urlbarBindings.xml

>+          // If the URL bar is modified, nothing else to do here.
>+          if (this.getAttribute("pageproxystate") != "valid")
>+            return selectedVal;

You can return early if selectionStart is > 0 as well.

>+          } else if (this.selectionStart == 0 && spec != trimURL(spec)) {

This looks like it should call this.trimValue, in case trimming is disabled.

>+            // NOTE: this assumes trimURL will can only truncate the URL at

"will can"

Has "NOTE:" a formalized meaning or is this just there to grab attention?

>+            let trimmedSegments = spec.split(trimURL(spec));

Can you put the trimmed value in a variable rather than trimming twice?
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-29 19:03:39 PDT
(In reply to comment #7)
> No need to spam logs with the expected URLs, is() will report them for
> failures.

I did that intentionally because I actually appreciate the "spam" - it makes it clear in the log which cases are being tested, and makes debugging the test easier (seeing the full progression of checks for a given value rather than generic "test pass" messages).

> >+  info("Expecting copy of: " + targetValue);
> 
> Looks like waitForClipboard should just do this if really deemed necessary.

It accepts a test function instead of an expected value, so it can't always do that. I had a separate patch that improved output for clipboard failures that I planned to follow up on separately (https://people.mozilla.com/~gavin/clipboardDebug.diff).

> Has "NOTE:" a formalized meaning or is this just there to grab attention?

The latter.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-29 19:03:57 PDT
Created attachment 543032 [details] [diff] [review]
updated patch
Comment 10 Dão Gottwald [:dao] 2011-06-29 23:36:25 PDT
*** Bug 668266 has been marked as a duplicate of this bug. ***
Comment 11 Dão Gottwald [:dao] 2011-06-30 00:58:19 PDT
Comment on attachment 543032 [details] [diff] [review]
updated patch

>+          let trimmedSpec = this.trimValue(spec);
> 
>+          // If the entire URL is selected, just use the actual loaded URI.
>+          if (inputVal == selectedVal) {
>+            // ... but only if  isn't a javascript: or data: URI, since those
>+            // are hard to read when encoded
>+            if (!uri.schemeIs("javascript") && !uri.schemeIs("data")) {
>               // Parentheses are known to confuse third-party applications (bug 458565).
>-              val = val.replace(/[()]/g, function (c) escape(c));
>+              selectedVal = uri.spec.replace(/[()]/g, function (c) escape(c));
>             }

You could:
1. return selectedVal here,
2. remove the following 'else',
3. then declare trimmedSpec
4. and either check if (spec != trimmedSpec) or return early if (spec == trimmedSpec).

>+          } else if (spec != trimmedSpec) {
>+            // If the URL is trimmed, we want to prepend the portion that
>+            // trimValue removed from the beginning.
>+            // This assumes trimValue will only truncate the URL at
>+            // the beginning or end (or both).
>+            let trimmedSegments = spec.split(trimmedSpec);
>+            selectedVal = trimmedSegments[0] + selectedVal;
>           }
> 
>-          return val;
>+          return selectedVal;
Comment 12 Dão Gottwald [:dao] 2011-06-30 01:05:43 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > No need to spam logs with the expected URLs, is() will report them for
> > failures.
> 
> I did that intentionally because I actually appreciate the "spam" - it makes
> it clear in the log which cases are being tested, and makes debugging the
> test easier (seeing the full progression of checks for a given value rather
> than generic "test pass" messages).

In that case is() itself should just output the expected value, but I suspect we don't really need/want that. What you usually want to debug is the failure case.

I had the pleasure to sift through debug mochitest-other logs. They're currently about 27 MB in size and cause Firefox as well as some text editors to grind to death. They're also unpleasant to look at because of the amount of data you're currently uninterested in.
Comment 13 B.J. Herbison 2011-07-03 18:42:18 PDT
The test cases show selecting the "e" from example.com prepends the http://. I'd argue that the http:// is only useful if the full domain name is copied -- http://e isn't useful.
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-05 12:28:01 PDT
(In reply to comment #13)
> The test cases show selecting the "e" from example.com prepends the http://.
> I'd argue that the http:// is only useful if the full domain name is copied
> -- http://e isn't useful.

That's probably true, but more complicated to implement. It'd be great if you could file that as a followup bug.
Comment 15 Roman R. 2011-07-05 22:03:04 PDT
I think that the essence of this bug should be restated so that correct behavior will get implemented. It should be possible to copy a part of a URL without muddying it with "http://"
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-08 11:50:28 PDT
Created attachment 544875 [details] [diff] [review]
comments addressed
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-08 14:36:37 PDT
http://hg.mozilla.org/integration/fx-team/rev/3c2ffce461c1
Comment 18 Mardeg 2011-07-10 09:38:35 PDT
*** Bug 670531 has been marked as a duplicate of this bug. ***
Comment 19 Emanuele Alimonda 2011-07-10 10:57:10 PDT
Since nobody seems to have mentioned it yet, I'd like to point out that not only copy operations but also drag and drop operations should be affected by this.
A method I often use to load the current URL into another browser (i.e. Safari on Mac) is to drag the URL from the Firefox location bar to the Safari location bar, and this no longer works, since Safari location bar (and supposedly other applications too) seems to be a drop target only for strings that represent URLs.

I've tested the same by dragging URLs from the Chrome location bar to Safari, and it correctly adds the protocol to complete the URL.
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-10 13:34:45 PDT
(In reply to comment #19)
> Since nobody seems to have mentioned it yet, I'd like to point out that not
> only copy operations but also drag and drop operations should be affected by
> this.

Can you file a new bug, and CC me?
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-10 13:44:49 PDT
http://hg.mozilla.org/mozilla-central/rev/3c2ffce461c1
Comment 22 NightsoN Blaze 2011-07-11 00:59:58 PDT
Why not copy the behaviour of Opera?  In my opinion it's much better.
Comment 23 Kelley Cook 2011-07-11 15:35:05 PDT
Could we get this in Aurora too?

Without it there is a change in behavior from the previous versions of Firefox.
Comment 24 Fred Wenzel [:wenzel] 2011-07-12 13:49:09 PDT
(In reply to comment #23)
> Could we get this in Aurora too?
> 
> Without it there is a change in behavior from the previous versions of
> Firefox.

+1. I frequently run into the problem now that I change a URL a little bit, then copy it out of the URL bar, and end up without a protocol. Sadface.
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-12 15:57:41 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/335addb86778
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2011-07-14 20:56:37 PDT
*** Bug 670503 has been marked as a duplicate of this bug. ***
Comment 27 Lozzy 2011-08-09 04:59:27 PDT
(In reply to nightson1988 from comment #22)
> Why not copy the behaviour of Opera?  In my opinion it's much better.

I agree with that.
Comment 28 Virgil Dicu [:virgil] [QA] 2011-08-19 07:16:15 PDT
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0

Verified fixed on Firefox beta using the following steps to reproduce:

1) Access any site (www.google.ro, for example)
2) Copy the link from the location bar.
3) Paste in a text editor

The protocol of the website is also displayed when pasting in a text editor.

Setting resolution to Verified Fixed.
Comment 29 Landry Breuil (:gaston) 2011-08-31 05:41:54 PDT
One could object that if you select only the domain name in the url to paste it in a terminal to ping the hostname, you don't want http:// to be added.. oh well :)
Comment 30 Roman R. 2011-08-31 12:34:48 PDT
Good point, Landry! They should've left things as they were without trying to be clever about the URL. They've opened a can of worms, and some of the worms are still crawling around.
Comment 31 Carl 2011-09-28 01:24:40 PDT
So this is the reason why I have to go to about:config and tell firefox 7 not to hide the http protocol.  What a stupid idea to hide it.  Undo both "fixes" that break this functionality.
Comment 32 Waldir 2011-12-04 14:16:48 PST
(In reply to B.J. Herbison from comment #13)
> The test cases show selecting the "e" from example.com prepends the http://.
> I'd argue that the http:// is only useful if the full domain name is copied
> -- http://e isn't useful.

FYI: I submitted a bug specifically for this: Bug 707567
Comment 33 Andreas van dem Helge 2012-06-06 20:30:35 PDT
I have issues with this behaviour. For e.g. I am veiwing a site and I want to nslookup the domain. I select www.site.com and then attempt to "nslookup [paste]" which fails miserably.

HOW do you guys just submit a feature request as a bug report and actually get it resolved? Every time I submit something I just get a "sorry this is the behviour live with it" type of response.
Comment 34 Kelley Cook 2012-06-07 04:10:07 PDT
Please don't respond to closed bugs. Use the mozilla support forums.

But very few people need to do what you do, so its not a compelling reason to roll it back, IMO.

But for you and any others who may find this bug via google, It is a simple fix:
* Type "about:config" in the urlbar.
* Search for "browser.urlbar.trimURLs" and set it to false.

Disliked feature disabled.
Comment 35 Carl 2012-06-07 16:02:20 PDT
I think apple is the only one that did this feature the correct way.  They don't show the http:// when showing the url.  But if you click on the url to select some of it or change some of it, http:// is put right back in.

But either way, this "feature" annoys the hell out of me.  I don't want to have to go into about:config every time to disable it.

Besides, you show https:// already, why don't you just make it consistent and either show both or show none?
Comment 36 Carl 2012-06-07 16:04:55 PDT
And I would like to point out one thing, every single program out there expects a url to begin with http:// or https:// in order for it to pick up it is a url and make it clickable.  It's a standard, don't hide it.
Comment 37 Andreas van dem Helge 2012-06-07 17:01:52 PDT
Carl, I am testing with Opera Browser Version 11.64 and it hides http:// and https:// prefix of the URL when focus is brought to the URL bar the prefix is un-hidden. Regardless of the state, the URL does not move in position which is pretty neat.

This proposed new bar is even more convoluted: http://cl.ly/401E0Z3A0e1F3T2u1J3C
Comment 38 Jim Jeffery not reading bug-mail 1/2/11 2012-06-07 17:27:41 PDT
This bug has been closed and has had this behavior through several releases now.  If you have a personal use case, either make an addon, or file bugs blocking this one.

In the meantime quit spamming this bug and take the discussion to Google Groups.

http://groups.google.com/group/mozilla.dev.apps.firefox/topics?lnk
Comment 39 Jan Bassez 2012-06-09 02:22:37 PDT
This bug (or one of it's dupes) should have been kept open in the first place...

Only a very small minority agreed on closing it...
Comment 40 Carl 2012-06-11 14:57:12 PDT
Clearly this was not thoroughly thought through.
Comment 41 Andreas van dem Helge 2012-08-21 19:58:51 PDT
Kelly:

It is not about who wants or doesn't want the http:// perpended. It is the fact that the behaviour of a fundamental operating system feature (copy/cut+paste) have been modified. If you select certain text and then copy it, you do not expect any text but what you copied to end up on the clipboard.

Note You need to log in before you can comment on or make changes to this bug.