Closed Bug 454518 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox :: Menus, enhancement, P3)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 3.7a1

People

(Reporter: mconnor, Assigned: mconnor)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

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.
Attachment #337798 - Flags: ui-review?(beltzner)
Attachment #337798 - Flags: review?(gavin.sharp)
Attachment #337798 - Flags: review?(gavin.sharp)
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.
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #337798 - Attachment is obsolete: true
Attachment #339858 - Flags: review?(gavin.sharp)
Attachment #337798 - Flags: ui-review?(beltzner)
(In reply to comment #2)
> I made the check more robust, you can get an http URI without a host

How, out of curiosity?
Attached file test URLs
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
Attachment #339858 - Flags: review?(gavin.sharp) → review+
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.
Duplicate of this bug: 433195
Duplicate of this bug: 475928
Comment on attachment 339858 [details] [diff] [review]
v2

Mrgh, guess this makes 3.2

Beltzner, ui-r?
Attachment #339858 - Flags: ui-review?(beltzner)
Priority: -- → P3
Target Milestone: --- → Firefox 3.2a1
Status: NEW → ASSIGNED
Whiteboard: [has patch][needs ui-review beltzner]
Flags: wanted-firefox3.6?
Flags: blocking-firefox3.6?
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.
Attachment #339858 - Flags: ui-review?(beltzner) → ui-review?(faaborg)
Attachment #339858 - Flags: ui-review?(faaborg) → ui-review+
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.
Not going to block; we should get it on mozilla-central then ask for 1.9.2 approval.
Flags: wanted-firefox3.6?
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
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.
Attachment #339858 - Attachment is obsolete: true
Blocks: 515512
Ugh, links without http:// in front of them don't get this behavior, filed bug 515512
I was able to load the middle 3 as links in a new tab using Vista.
http://hg.mozilla.org/mozilla-central/rev/921c0a30c2f3
http://hg.mozilla.org/mozilla-central/rev/7ed220eb3a9f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs ui-review beltzner]
Target Milestone: Firefox 3.6a1 → Firefox 3.7a1
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.
(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")?
No longer depends on: 515932
can this mechanism help with Bug 475045?
(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.
Did bug 515932 fix this to not send a referrer?
nope
Depends on: 517316
k, filed bug 517316 for that.
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.
submitted Bug 518005 per comment #11
submitted Bug 518006 per comment #24
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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Tests for this bug are included in the tests for bug 515512.
Blocks: 518005
Blocks: 518006
Blocks: 227922
Flags: in-testsuite? → in-testsuite+
Duplicate of this bug: 227922
Blocks: 596628
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:
firstname.lastname@example.com
^ 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.
Blocks: 664970
Blocks: 720023
Duplicate of this bug: 490207
You need to log in before you can comment on or make changes to this bug.