Closed Bug 668019 Opened 13 years ago Closed 13 years ago

prepend http:// to URL copy selection if URL has been selected (but not loaded) from location bar

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 9
Tracking Status
firefox7 + wontfix
firefox8 --- fixed

People

(Reporter: beltzner, Assigned: dao)

References

Details

(Keywords: regression, Whiteboard: [qa!])

Attachments

(1 file, 4 obsolete files)

(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
Blocks: 665580
Keywords: regression
Hardware: x86 → All
Version: unspecified → Trunk
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)
(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.
(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.
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.
We're into aurora land, but this is bustage that's novel for Aurora 7 - margaret/gavin - is a fix here gonna be safe?
Assignee: nobody → dao
On closer look this is indeed another bug...
Why was Bug 670503 marked as duplicate of this one if it's a different bug??
> Why was Bug 670503 marked as duplicate of this one if it's a different bug??

Bug 670503 comment 2 says why...
Blocks: 673528
No longer blocks: 673528
Attached patch WIP: fixup URL of selected text (obsolete) — Splinter Review
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.
Attachment #549614 - Flags: feedback?(gavin.sharp)
Attachment #549614 - Flags: feedback?(dao)
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...
> 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 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.
Attachment #549614 - Flags: feedback?(gavin.sharp)
Attachment #549614 - Flags: feedback?(dao)
Attachment #549614 - Flags: feedback-
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #550325 - Flags: review?(gavin.sharp)
Attached patch patch (obsolete) — Splinter Review
the previous patch contained changes that aren't needed for the approach I ended up taking
Attachment #550325 - Attachment is obsolete: true
Attachment #550654 - Flags: review?(gavin.sharp)
Attachment #550325 - Flags: review?(gavin.sharp)
(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
Is this behavior ok to ship with? Should we back out the feature?
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 ...
FWIW, Chrome has this same issue IIRC.
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)
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.
(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 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!)
Blocks: 687571
No longer blocks: 687571
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.
Attachment #550654 - Flags: review?(gavin.sharp) → review+
Attachment #549614 - Attachment is obsolete: true
(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?
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #550654 - Attachment is obsolete: true
Whiteboard: [inbound]
Attached patch patch v2Splinter Review
with test fixes
Attachment #561094 - Attachment is obsolete: true
Attachment #561113 - Flags: approval-mozilla-beta?
Attachment #561113 - Flags: approval-mozilla-aurora?
Target Milestone: --- → Firefox 9
https://hg.mozilla.org/mozilla-central/rev/575dcce28978
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
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.
I agree with Gavin. We should get this into Aurora.
Attachment #561113 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
qa+ for verification on Firefox 8
Whiteboard: [qa+]
Attachment #561113 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Depends on: 691690
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Depends on: 683777
No longer depends on: 683777
Target Milestone: Firefox 9 → Firefox 8
Target Milestone: Firefox 8 → Firefox 9
This does not work in 11.0a1 (2011-11-16)
(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.
Depends on: 1254415
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: