Closed Bug 601695 Opened 10 years ago Closed 10 years ago

Paste & Go should only appear when the clipboard is a URL

Categories

(Firefox :: Address Bar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: beltzner, Assigned: fryn)

References

Details

Attachments

(1 file, 2 obsolete files)

Paste & Go was implemented in bug 492544 in order to provide a convenient shortcut to cases where the user copied a link from another application and then put focus in the location bar. The deal was, though, that it was only supposed to show up when the clipboard contained a URL.

So we should make it work that way. :)

(Note: we could keep Paste & Search for cases where it's not a URL, but I honestly don't think that should be a goal, either)
How are we supposed to tell if the clipboard a URL?  Are we determining this from the drag flavors we receive or from the text content?
Also, see the use case discussed in Bug 599793 where the clipboard contents are explicitly not a URL.
(In reply to comment #1)
> How are we supposed to tell if the clipboard a URL?  Are we determining this
> from the drag flavors we receive or from the text content?

That is up to you clever developers to determine, really. Ideally we could do both. If the drag flavour says "URL", or if the text content contains an a.bb formulation, optionally with a protocol handler in the front.

(In reply to comment #2)
> Also, see the use case discussed in Bug 599793 where the clipboard contents are
> explicitly not a URL.

That bug is about how Paste & Go should replace the entire contents of the URL bar before "go"ing, and the discussion in it revealed that we should file this bug to make sure that the only thing that's pasted in a Paste & Go operation is actually a URL. We decided, pretty explicitly, not to morph that bug to be about whether or not we should support Paste & Go for non-URL content.
(In reply to comment #3)
> (In reply to comment #1)
> > How are we supposed to tell if the clipboard a URL?  Are we determining this
> > from the drag flavors we receive or from the text content?
> 
> That is up to you clever developers to determine, really. Ideally we could do
> both. If the drag flavour says "URL", or if the text content contains an a.bb
> formulation, optionally with a protocol handler in the front.

I ask because I don't think the drag flavors will work (copying an arbitrary URL from plain text on a webpage won't be marked as a URL) and I don't think we have any existing "is this string a URL?" infrastructure.
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #1)
> > > How are we supposed to tell if the clipboard a URL?  Are we determining this
> > > from the drag flavors we receive or from the text content?
> > 
> > That is up to you clever developers to determine, really. Ideally we could do
> > both. If the drag flavour says "URL", or if the text content contains an a.bb
> > formulation, optionally with a protocol handler in the front.
> 
> I ask because I don't think the drag flavors will work (copying an arbitrary
> URL from plain text on a webpage won't be marked as a URL) and I don't think we
> have any existing "is this string a URL?" infrastructure.

We absolutely do; we just don't use it on the clipboard contents directly yet.

AIUI, we would just use the same URI fixup that the url bar performs to determine with consistency what the url bar would do.
This perhaps?
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp
or just create a nsIURI and catch exceptions.
Assignee: nobody → fryn
Yes, the normal way to check for URI-ness is just to check whether newURI throws.
Attached patch patch (obsolete) — Splinter Review
Attachment #486138 - Flags: review?(dolske)
Comment on attachment 486138 [details] [diff] [review]
patch

>+            try {
>+              let cbSvc = Cc["@mozilla.org/widget/clipboard;1"].
>+                            getService(Ci.nsIClipboard);
>+              let xferable = Cc["@mozilla.org/widget/transferable;1"].
>+                              createInstance(Ci.nsITransferable);
>+              xferable.addDataFlavor("text/unicode");
>+              cbSvc.getData(xferable, cbSvc.kGlobalClipboard);
>+              let data = {};
>+              xferable.getTransferData("text/unicode", data, {});
>+              data = data.value.QueryInterface(Ci.nsISupportsString).data;
>+              Services.io.newURI(data, null, null);
>+            } catch (ex) { // clipboard data is not a URL
>+              Cu.reportError(ex);
>+              couldBeURL = false;
>+            }

Are there lines besides the newURI call where you'd expect exceptions? As for newURI, you shouldn't report that as an error.
(In reply to comment #9)
> Are there lines besides the newURI call where you'd expect exceptions? As for
> newURI, you shouldn't report that as an error.

An exception might be thrown from "xferable.getTransferData..." to "data = ...". I'll move the beginning of the try block to that.

Also, forgot to remove the debug line. Thanks.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #486138 - Attachment is obsolete: true
Attachment #486146 - Flags: review?(dao)
Attachment #486138 - Flags: review?(dolske)
(In reply to comment #10)
> An exception might be thrown from "xferable.getTransferData..." to "data =
> ...".

I don't see why these would throw, shouldn't controller.isCommandEnabled("cmd_paste") rule that out?
Attached patch patch v3Splinter Review
You're right.
Attachment #486146 - Attachment is obsolete: true
Attachment #486171 - Flags: review?(dao)
Attachment #486146 - Flags: review?(dao)
Comment on attachment 486171 [details] [diff] [review]
patch v3

You could replace Services.io.newURI(data, null, null) with makeURI(data).
Attachment #486171 - Flags: review?(dao) → review+
http://hg.mozilla.org/mozilla-central/rev/893228a83ecf

forgot to do ui-review! will do in the future. a retroactive one would be
appreciated.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #486171 - Flags: ui-review?(beltzner)
Target Milestone: --- → Firefox 4.0b8
Comment on attachment 486171 [details] [diff] [review]
patch v3

uir=beltzner
Attachment #486171 - Flags: ui-review?(beltzner) → ui-review+
Could anyone kindly do me a bit of JS, if its possible, to add to my UserChome.js (i have UserChrome.js extension installed)
That restores paste and go for the URLbar regardless of if its a valid URL or not? 
It was a very handy feature for paste & go'ing JavaScript to the end of URLs, amongst other things.
Umm, do i get it right that only "http(s)://" prefixed URLs are recognized now?
Is this really intended behavior?
(Asking here beforehand filling a new Bug Report)
(In reply to comment #18)
> Umm, do i get it right that only "http(s)://" prefixed URLs are recognized now?
> Is this really intended behavior?
> (Asking here beforehand filling a new Bug Report)

No. We simply validate the string without modifications, e.g. "about:" is a valid URI without any fixup. I had suggested in comment 5 that we use the same url fixup that the url bar uses when you press enter, but that was rejected.
I guess I should have been more specific in my summary, because IMO you should definitely be able to paste-and-go "mozilla.com".
(In reply to comment #20)
> I guess I should have been more specific in my summary, because IMO you should
> definitely be able to paste-and-go "mozilla.com".

If you'd like, file a followup and get UX nods.
(In reply to comment #21)
> If you'd like, file a followup and get UX nods.

*nod*
(In reply to comment #22)
> (In reply to comment #21)
> > If you'd like, file a followup and get UX nods.
> 
> *nod*

Marco and/or Gavin, do you think it sensible to use the same URL fixup that I suggested in comment 5, or do you have a better approach?
Target Milestone: Firefox 4.0b8 → Firefox 4.0b7
I'd prefer disabling the menu item rather than hiding it.  Hiding it made me think Firefox didn't have the feature.
It's tricky, since if you disable it, it's not clear *why* it's disabled.  That is, people who interpret "Paste & Go" as "paste the clipboard's contents and then go" could be disappointed when they select some part of what's in their location bar, right-click it, and "Paste & Go" is mysteriously disabled.

Anyway, I can see it going either way, personally.
Depends on: 611590
Sorry, as implemented, it doesn't make sense — in fact, it goes against the current behavior of the URL field. I wish I caught this before it was implemented, but I was away on vacation. :(

The URL field does URLs *and* search with the changes in Firefox 4; thus we shouldn't try to limit what you can "Paste & Go" here. It also breaks keywords like "bug NNNN", which are very convenient. It also doesn't work for stuff that most people consider to be URLs, like "cnn.com" without the http://.

We have predictable, simple, good behaviors for everything that can happen in this field, and saving an extra entry of "Paste & Go" in the context menu in the off chance that it isn't formatted as a proper URL only makes it more complicated.

I'd like to request that this change is reverted, trying to figure out whether something is a URL or something else will only create complexity, a non-predictable UI (Paste & Go appears sometimes, and sometimes not), and in general breaks the simplified model we were going for.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Personally, I'm also against this. Not only is it un-simple, it is also confusing. I have search keywords that I'd like to use, and I may or may not like to change the search engine just to search something as simple as "bug 601695".
Since it's been in the tree for a while and the behavior specified in the bug summary (even if not ideal) is in the builds, reopening doesn't make sense. As such, I'm uploading the patch to revert this change in bug 611590.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.