454518 - allow opening URLs that are not linked from the context menu (if selected)

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement, P3)

Description

[Mike Connor [:mconnor]]

Attachment: open selected URLs via context menu v1

Ultimately, we don't want to go down the path of auto-linkifying every URI we encounter, especially in text/plain docs and the like, since we don't really want to modify content intentionally served as text/plain.  However, it is trivial and desirable to make it easier to visit these URLs, by detecting that a URL is selected and offering the normal link context options, as well as the option to open in the current tab.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 337798 [details] [diff] [review]
open selected URLs via context menu v1

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

>+      var focusedWindow = document.commandDispatcher.focusedWindow;
>+      if (focusedWindow == window)
>+        focusedWindow = content;
>+      var someText = focusedWindow.getSelection().toString();

Why is this needed? Is there really a way to trigger the context menu for a given window without it being focused? If so, the check probably belongs in getBrowserSelection() anyways (so that we never even set isTextSelected if the window isn't focused). Would be good to use getBrowserSelection here too, or otherwise limit the selection length (getBrowserSelection might be unsuitable because of the "important chars" thing - not sure).

>+      var onPlainTextLink = false;
>+      if (uri && uri.host) {

GetHost will throw for nsSimpleURIs (e.g. data/javascript), and I don't think it will return null/empty for an nsStandardURL, so the host check seems superfluous. QI to nsIStandardURL is probably better, but even that isn't really the right way of filtering out "abnormal links". Perhaps an explicit check for "https?/ftp" would be best?

Otherwise this looks OK, but there are a couple of tabs in this that should be removed.

Comment 2

[Mike Connor [:mconnor]]

Attachment: v2

I swiped the selection stuff from the view partial source bits, but this seems to work fine, not sure quite why it was necessary, it seems rather solid in testing locally.

I made the check more robust, you can get an http URI without a host, which is why I added the check in the first place.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> I made the check more robust, you can get an http URI without a host

How, out of curiosity?

Comment 4

[Mike Connor [:mconnor]]

Attachment: test URLs

Comment 5

[Mike Connor [:mconnor]]

Gavin, it seems to work like that if you just pass in http://, for example.

var ioService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService); var uri = ioService.newURI("http://", null, null); uri; evaluates to an nsIURI

maybe that shouldn't be the case, but, meh, I can file a followup

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 339858 [details] [diff] [review]
v2

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

>+      var ioService = Cc["@mozilla.org/network/io-service;1"].
>+                      getService(Ci.nsIIOService);
>+      try {
>+       var uri = ioService.newURI(someText, null, null);
>+      }
>+      catch (ex) { }

Could use makeURI here (and in getLinkURI for that matter). Still need the try/catch though.

>+      if (uri && /^(https?|ftp)/i.test(uri.scheme) && uri.host) {
>+	this.linkURI = uri;
>+	this.linkURL = this.linkURI.spec;

nit: still tabs here! :)

>+  // open URL in current tab
>+  openLinkInCurrent: function() {
>+    openUILink(this.linkURL, null, null, null, null, null, 

>+	       this.target.ownerDocument.documentURIObject);

and here!

Would be clearer to just call openUILinkIn() directly with "current", I think.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 339858 [details] [diff] [review]
v2

Mrgh, guess this makes 3.2

Beltzner, ui-r?

Comment 10

[Steffen Wilberg]

Half a year later, still waiting for ui-review on the "Open Link" string in the context menu, shown only when selecting URL which is not linkified.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 339858 [details] [diff] [review]
v2

It'd be even nicer if you didn't need to select the entire URL, and just needed to right-click on a string which was a URL.

Anyway, yes, this is fine. Clever, even.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Not going to block; we should get it on mozilla-central then ask for 1.9.2 approval.

Comment 13

[Mike Connor [:mconnor]]

Attachment: comments addressed

rebuilding to test in the AM, will commit to trunk then and request approval as appropriate.  I don't remember now why I decided to just have it be for selected links for ease of implementation, though I'll file a followup on making this smarter in the future.

Comment 14

[Nochum Sossonko [:Natch]]

Ugh, links without http:// in front of them don't get this behavior, filed bug 515512

Comment 15

[[not reading bugmail]]

I was able to load the middle 3 as links in a new tab using Vista.

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/921c0a30c2f3
http://hg.mozilla.org/mozilla-central/rev/7ed220eb3a9f

Comment 17

[Jesse Ruderman]

This sends an HTTP referrer.  For example, when following a link from http://www.squarefree.com/bug454518/a.txt, that URL is sent as the referrer.  This may or may not be a problem.

Comment 18

[Peter Lairo]

(In reply to comment #14)
> Ugh, links without http:// in front of them don't get this behavior, filed bug
> 515512

What about https:// (with "s")?

Comment 19

[eyal gruss]

can this mechanism help with Bug 475045?

Comment 20

[Alex Bishop]

(In reply to comment #17)
> This sends an HTTP referrer.  For example, when following a link from
> http://www.squarefree.com/bug454518/a.txt, that URL is sent as the referrer. 
> This may or may not be a problem.

I think it's a problem. People sometimes intentionally don't linkify URLs in pages to prevent the Referer header from being sent. It's also inconsistent with legacy mechanisms for visiting unlinked URLs (e.g. copying the URL and pasting into the Location bar or selecting the URL and dragging to the tab bar), which don't send Referer headers.

Comment 21

[Jesse Ruderman]

Did bug 515932 fix this to not send a referrer?

Comment 22

[Dão Gottwald [:dao]]

nope

Comment 23

[Jesse Ruderman]

k, filed bug 517316 for that.

Comment 24

[eyal gruss]

why do we just add the context menu items for opening the link, but not the whole link context menu (bookmark, save, send, etc.)? this also means that add-on actions on links such as "open in ie tab" will not be available.

Comment 25

[eyal gruss]

submitted Bug 518005 per comment #11
submitted Bug 518006 per comment #24

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090930 Minefield/3.7a1pre ID:20090930030840

Verified on all platforms that non-linkyfied url's with a leading protocol will have the context menu items. 

Would be nice to have some automated tests for this feature.

Comment 27

[Nochum Sossonko [:Natch]]

Tests for this bug are included in the tests for bug 515512.

Comment 29

[Gingerbread Man]

Was it the intention to have this feature work only for valid URLs? Because as it is now, it offers "Open Link" pretty much randomly. It's irritating when I select plain text, and due to muscle memory I click the first option in the context menu. I expect it to be "Copy", but instead of copying to the clipboard, I end up with a confusing "Firefox can't find the server" error.

Examples:
[e-mail]
^ The above opens http://example.com/, which may be a valid address, but hardly desirable or expected.
12.ian.2007
Video.1080p.mkv
Quarterly_report.docx
usermanual.pdf

I've tested in safe mode with the nightly dated March 7th 2011.