Last Comment Bug 668019 - prepend http:// to URL copy selection if URL has been selected (but not loaded) from location bar
: prepend http:// to URL copy selection if URL has been selected (but not loade...
Status: VERIFIED FIXED
[qa!]
: regression
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 9
Assigned To: Dão Gottwald [:dao]
:
Mentors:
: 669733 670298 672614 673839 674809 (view as bug list)
Depends on: 1254415 691690
Blocks: 665580
  Show dependency treegraph
 
Reported: 2011-06-28 13:59 PDT by Mike Beltzner [:beltzner, not reading bugmail]
Modified: 2016-03-18 11:10 PDT (History)
32 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
fixed


Attachments
WIP: fixup URL of selected text (1.79 KB, patch)
2011-07-30 20:08 PDT, Joe Drew (not getting mail)
gavin.sharp: feedback-
Details | Diff | Splinter Review
patch (7.01 KB, patch)
2011-08-03 02:21 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch (4.16 KB, patch)
2011-08-04 05:02 PDT, Dão Gottwald [:dao]
gavin.sharp: review+
Details | Diff | Splinter Review
patch v2 (4.87 KB, patch)
2011-09-19 18:34 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v2 (8.17 KB, patch)
2011-09-19 20:36 PDT, Dão Gottwald [:dao]
bugzilla: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Mike Beltzner [:beltzner, not reading bugmail] 2011-06-28 13:59:10 PDT
(similar to bug 666964)

STR:
1. Type "bug" in your location bar
2. Arrow down to a URL you like the look of
3. Select All, Copy
4. Open a text editor and Paste

Expected: http://url-you-liked
Actual: url-you-liked
Comment 1 Dão Gottwald [:dao] 2011-07-06 13:41:01 PDT
*** Bug 669733 has been marked as a duplicate of this bug. ***
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2011-07-08 19:31:35 PDT
*** Bug 670298 has been marked as a duplicate of this bug. ***
Comment 3 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-07-10 06:41:09 PDT
*** Bug 670503 has been marked as a duplicate of this bug. ***
Comment 4 Jan Bassez 2011-07-10 11:45:01 PDT
This looks like being fixed on the latest build (win7)

I copy a URL from the address bar, http:// is added in the paste result
I type a word in the URL bar (which would lead to a dead link), no http:// is added
I select only part of a URL in the address bar, no http:// is added

This is correct behavior, only the moment you select the full URL in the address bar would be better if it was also visible that you are actually selecting the 'http://'-prefix too. (So that What We See Is What We Get)
Comment 5 Jan Bassez 2011-07-10 11:48:29 PDT
(In reply to comment #4)
> This looks like being fixed on the latest build (win7)
> 
> I copy a URL from the address bar, http:// is added in the paste result
> I type a word in the URL bar (which would lead to a dead link), no http://
> is added
> I select only part of a URL in the address bar, no http:// is added
> 
> This is correct behavior, only the moment you select the full URL in the
> address bar would be better if it was also visible that you are actually
> selecting the 'http://'-prefix too. (So that What We See Is What We Get)

Oh, not completely fixed, when I select part of the address bar starting from the beginning of the URL, the http:// is not added in the pasted result.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-10 16:21:48 PDT
(In reply to comment #5)
> Oh, not completely fixed, when I select part of the address bar starting
> from the beginning of the URL, the http:// is not added in the pasted result.

That's bug 666964, and I landed its fix on mozilla-central earlier today.
Comment 7 Boris Zbarsky [:bz] 2011-07-10 18:50:32 PDT
Nothing in comment 4 has anything to do with this bug.  For this bug, the "from location bar" part is key.  That is, the url was selected from the autocomplete dropdown.
Comment 8 Johnathan Nightingale [:johnath] 2011-07-13 12:01:55 PDT
We're into aurora land, but this is bustage that's novel for Aurora 7 - margaret/gavin - is a fix here gonna be safe?
Comment 9 Jan Bassez 2011-07-13 13:22:58 PDT
On closer look this is indeed another bug...
Why was Bug 670503 marked as duplicate of this one if it's a different bug??
Comment 10 Boris Zbarsky [:bz] 2011-07-14 20:56:24 PDT
> Why was Bug 670503 marked as duplicate of this one if it's a different bug??

Bug 670503 comment 2 says why...
Comment 11 Cork 2011-07-19 13:31:55 PDT
*** Bug 672614 has been marked as a duplicate of this bug. ***
Comment 12 Matt Brubeck (:mbrubeck) 2011-07-25 08:16:55 PDT
*** Bug 673839 has been marked as a duplicate of this bug. ***
Comment 13 Cork 2011-07-27 21:04:02 PDT
*** Bug 674809 has been marked as a duplicate of this bug. ***
Comment 14 Joe Drew (not getting mail) 2011-07-30 20:08:50 PDT
Created attachment 549614 [details] [diff] [review]
WIP: fixup URL of selected text

The easiest way to solve this seems to be to always fixup the selected text. This has the side-effect of making a URL out of anything you enter into the URL bar, but I actually prefer that. It's a bit weird for copy and paste behaviour to depend on whether a page is loaded, after all.

The alternative is to set some sort of flag saying that the URL in the text box came from the location bar dropdown, but I have no idea even where to start with that.
Comment 15 Jan Bassez 2011-07-31 02:09:55 PDT
This makes me thinking of buying a car with buggy electronics, where electronics aren't actually needed...
When a gadget causes more problems and annoyance, than any good, it's better to leave it off altogether, if you want to achieve a solid quality product.

A Visual gadget should Never influence a good and clear workflow. I would make the http:// optional & standard off, so it only is applied for those wishing to hide that part of an URL, with the associated annoyances. Standard should always be the best working method. Also it has been proven that for people the What-You-See-Is-What-You-Get always works best. Stepping away from this doesn't seem to be a good idea. Makes things more vague instead of clear...
Comment 16 Boris Zbarsky [:bz] 2011-07-31 07:29:19 PDT
> This has the side-effect of making a URL out of anything you enter into the URL
> bar

This fails the "type some stuff, realize you actually wanted it in the search field, copy and paste it".

Actual user-typed values should not, imo, be messed with.

What would make sense to me is if selecting text from the dropdown actually showed the url until the user presses enter or something.
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-01 10:26:41 PDT
Comment on attachment 549614 [details] [diff] [review]
WIP: fixup URL of selected text

Indeed, this is unsuitable because of the problem Boris points out.

I had a patch for this that I was playing with that kept the untrimmed value around for use in _getSelectedValueForClipboard, but it was kind of messy, particularly given that we trim the values before they ever get added to the popup. I think we will need something along those lines to properly fix this, though.
Comment 18 Dão Gottwald [:dao] 2011-08-03 02:21:53 PDT
Created attachment 550325 [details] [diff] [review]
patch

Note that this doesn't cover:
autocomplete in tab A -> select tab B -> select tab A -> copy
We'd need to keep track of more things to fix this.
Comment 19 Dão Gottwald [:dao] 2011-08-04 05:02:38 PDT
Created attachment 550654 [details] [diff] [review]
patch

the previous patch contained changes that aren't needed for the approach I ended up taking
Comment 20 alex_mayorga 2011-08-25 11:17:16 PDT
(In reply to Boris Zbarsky (:bz) from comment #16)
> > This has the side-effect of making a URL out of anything you enter into the URL
> > bar
> 
> This fails the "type some stuff, realize you actually wanted it in the
> search field, copy and paste it".
> 
> Actual user-typed values should not, imo, be messed with.

Big +1
Comment 21 christian 2011-09-15 14:03:41 PDT
Is this behavior ok to ship with? Should we back out the feature?
Comment 22 Mike Beltzner [:beltzner, not reading bugmail] 2011-09-15 17:07:29 PDT
It's a regression of pretty core functionality (copying URLs for sharing elsewhere) but I'd hate to see this single issue resulting in the backout of the entire feature. Can someone pin Gavin down and tempt him with a cookie for the review? Bonus points for getting him beside Dao so any issues can be worked through ...
Comment 23 Joe Drew (not getting mail) 2011-09-15 17:14:24 PDT
FWIW, Chrome has this same issue IIRC.
Comment 24 Mike Beltzner [:beltzner, not reading bugmail] 2011-09-15 17:34:31 PDT
Yes, Chrome has the same issue. Chrome also ships with bug 666964, if that helps drivers decide (fwiw, I don't think that should make it OK for *us* to ship with either of those bugs)
Comment 25 Roman R. 2011-09-15 19:06:11 PDT
The feature is of dubious value (it hides just 7 characters), but the regression is very inconvenient. I run into it frequently. It doesn't matter what another browser does.
Comment 26 [:Cww] 2011-09-16 11:58:57 PDT
(In reply to Christian Legnitto [:LegNeato] from comment #21)
I was arguing for backing this out based on this because we are hearing complaints that users are having trouble switching to https because they're used to doing click on url bar, add "s", enter.

It's just become less clear if they're on https or not.
Comment 27 Mike Beltzner [:beltzner, not reading bugmail] 2011-09-16 12:07:25 PDT
Comment 26 should be filed as it's own bug, blocking bug 665580, and nominated for tracking against the next release of Firefox - it's not really related to the issue in this bug.

(I also don't think I agree with it, but we can talk about that in its own bug once filed!)
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-09-19 13:21:00 PDT
Comment on attachment 550654 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   gURLBar.value = value;
>+  gURLBar.valueIsTyped = !valid;

Do we really need this change? The only behavior it affects is the blank tab/location bar case, I guess?

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>       <method name="_getSelectedValueForClipboard">

>+          let uri = uriFixup.createFixupURI(inputVal, Ci.nsIURIFixup.FIXUP_FLAG_NONE);

I think it's probably best to wrap this in a try/catch - this can throw for obviously invalid URIs, and in some other cases too (e.g. when passed an empty string).

We should add a comment to trimURL mentioning the fact that we rely on it not modifying the value such that a createFixupURI will produce a different URI.

We should get test coverage for this.
Comment 29 Dão Gottwald [:dao] 2011-09-19 18:16:54 PDT
(In reply to Gavin Sharp from comment #28)
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >   gURLBar.value = value;
> >+  gURLBar.valueIsTyped = !valid;
> 
> Do we really need this change? The only behavior it affects is the blank
> tab/location bar case, I guess?

Without it, valueIsTyped would inconsistently be false when autocompleting in one tab and switching to a tab with pageproxystate = valid (as opposed to typing and loading a page in one tab).

> >       <method name="_getSelectedValueForClipboard">
> 
> >+          let uri = uriFixup.createFixupURI(inputVal, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
> 
> I think it's probably best to wrap this in a try/catch - this can throw for
> obviously invalid URIs, and in some other cases too (e.g. when passed an
> empty string).

This shouldn't happen, i.e. we shouldn't have such URIs with valueIsTyped = false. I'll add the try/catch, though.

> We should get test coverage for this.

Are there existing tests performing on the actual autocomplete UI rather than penetrating parts of the API?
Comment 30 Dão Gottwald [:dao] 2011-09-19 18:33:46 PDT
(In reply to Dão Gottwald [:dao] from comment #29)
> > >+          let uri = uriFixup.createFixupURI(inputVal, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
> > 
> > I think it's probably best to wrap this in a try/catch - this can throw for
> > obviously invalid URIs, and in some other cases too (e.g. when passed an
> > empty string).
> 
> This shouldn't happen, i.e. we shouldn't have such URIs with valueIsTyped =
> false. I'll add the try/catch, though.

Well I guess we can actually get invalid URIs when selecting part of the value as opposed to the whole value.
Comment 31 Dão Gottwald [:dao] 2011-09-19 18:34:41 PDT
Created attachment 561094 [details] [diff] [review]
patch v2
Comment 32 Dão Gottwald [:dao] 2011-09-19 20:36:47 PDT
Created attachment 561113 [details] [diff] [review]
patch v2

with test fixes
Comment 34 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-09-20 12:09:10 PDT
The only behavior change in this patch is related to copying text from the location bar. I'm still uncomfortable taking this on beta at the last minute, though, since it has the potential to break that functionality (which is fairly important). I think we should take it only on Aurora, and WONTFIX this bug for the Firefox 7 cycle.
Comment 35 Asa Dotzler [:asa] 2011-09-20 12:57:28 PDT
I agree with Gavin. We should get this into Aurora.
Comment 37 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 15:38:35 PDT
qa+ for verification on Firefox 8
Comment 38 Virgil Dicu [:virgil] [QA] 2011-10-05 02:47:25 PDT
Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a2) Gecko/20111003 Firefox/9.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111002 Firefox/10.0a1

Verified in F8 Beta 1,Firefox 9 and 10 on ubuntu 11.04, Mac OS 10.6, Windows XP and Windows 7 using the steps in Comment 0.
Comment 39 Alice0775 White 2011-10-18 09:16:30 PDT
*** Bug 695329 has been marked as a duplicate of this bug. ***
Comment 40 Roman R. 2011-11-16 16:38:57 PST
This does not work in 11.0a1 (2011-11-16)
Comment 41 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-16 21:04:49 PST
(In reply to Roman R. from comment #40)
> This does not work in 11.0a1 (2011-11-16)

Please check if this happens on a new profile (http://kb.mozillazine.org/Profile_manager) as this works fine for me using the same build. It's possible an add-on you have installed is altering this behaviour.

If you still can't get this to work, please file a new bug.

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