Bug 454518 added the option to open selected non-linkified text urls as urls. However, lots of websites don't include the protocol when it's a regular website (a la google search). It would be very convenient if this feature could be extended to support that as well.

If a page says foobar.com and oobar.com is selected, would you treat that as a link?

Well, originally I was thinking of what beltzner said in bug 454518 that just right-clicking a non-linkified text url would be nice to get the context menu for, and the impl. would be to find the first white-space before and after the clicked-on text, try creating a url, if that fails try appending "http://" to it, and then bail out if that fails. However, in this case where the user explicitly selected only part of the url, I see nothing wrong with loading oobar.com. It should be going through the uriSecurityCheck anyways...

My question wasn't targeted at security but user experience. It's not clear to me that the user would expect to see these menu items in that case.

Ok, so we don't need to do it in that case, still seems simple enough (checking to make sure the user's selection is preceded by a white-space). I'm just saying that most sites won't include the "http://", like google search and plenty of others. Any text-linkification program (like Gmail, et al) recognize urls even without their leading protocol, I think it's reasonable that Firefox should do the same. While we're at it, what about foobar.com/baz and the user selects just foobar.com, IMHO in that case we should definitely show these choices as it's the same site, only top-level instead of a specific directory, seems more intentional to me...

Dao, what do you think about this?

Comment on attachment 399777 [details] [diff] [review] patch No good, matches too much stuff...

The problem is makeURI doesn't throw for http://google oddly.

You can use the effective TLD service to catch these cases.

This is what we'd need to get the desired effect. Is it still worth it, or will this never be accepted?

Oh, and the effective tld service doesn't catch http://google it thinks it's just fine. Selection ranges are weird, if the selection comes from a triple-click, then there's only one range object and startOffset is 0 and endOffset is 1...

(In reply to comment #10) > Oh, and the effective tld service doesn't catch http://google it thinks it's > just fine. What tldSuffix do you get?

Comment on attachment 399850 [details] [diff] [review] wip > var onPlainTextLink = false; This should be defined outside of this if-block. All the other ifs should be lets... >- if (uri && /^(https?|ftp)/i.test(uri.scheme) && uri.host) { >+ if (uri && tldSuffix && /^(https?|ftp)/i.test(uri.scheme) && uri.host) { The tldSuffix check looks wrong for the case that the first makeURI succeeds.

It returns "google". This patch has a problem with selecting google.com/, it misses it and doesn't display the linking stuff.

It doesn't look like the effective tld service adds any value then.

A tripple-click seems to include the trailing white-space (unless the last char is a special char - e.g., "/"). It would be great if this bug still linkefied a URL when the selection includes a trailing (or also leading) white-space.

I'd like to see this make it. I just tried one today, noticing that the lack of http:// is what would really come in handy trying links like this too: blogs.msdn.com/e7

Much simplified and more correct. Now comes with tests, both for this bug and bug 454518.

Sorry for the spam, there was an update that bitrotted the previous patch, same otherwise.

Found another bug with mailto: links (or any links that use ":" as opposed to "://"). Added another test. Also, fixed the RegExp a tiny bit.

*Sigh* this RegExp stuff is really more than I had bargained for :( Slight update to the RegExp and added a test so that "m|ail.example.com|" (where | denotes selection) doesn't activate the link menu options (because it doesn't start at a word boundary and can be unreliable/unexpected).

Fix triple-click selection, [end|start]Offset can be the index of a string or the index of a node, depending on what range.[start|end]Container is. Added a test for that. Also, ftp links (eg, ftp.example.com) were getting lost due to the first check being too harsh. Fixed that and added a test. This should be it (R)

(In reply to comment #21) > Also, ftp links (eg, ftp.example.com) were getting lost due to the first check > being too harsh. Tangentially related to this, it would be nice if URLs like ftp.mozilla.org became ftp://ftp.mozilla.org rather than http://ftp.mozilla.org. This would match the behaviour of entering ftp.mozilla.org into the Location bar.

This needed another update as the test was changed and bitrotted this patch. I updated the RegExp as well to include selected text that includes extra whitespace at the end, added a test for that as well. Other updates are thanks to some IRC discussion with mzz and Waldo.

Bugzilla interdiff isn't catching the test change, ftr I simply had to change newBrowser.contentDocument to gBrowser.selectedBrowser.contentDocument as newBrowser was removed.

This test doesn't seem to depend on the rest of that file at all, so please create a separate file.

I'll get to that after the weekend.

I placed the test in a separate file and cleaned up some of the functions.

Comment on attachment 402197 [details] [diff] [review] patch + separate test > if (this.isTextSelected) { > // ok, we have some text, let's figure out if it looks like a URL >+ var selection = document.commandDispatcher.focusedWindow >+ .getSelection(); >+ var someText = selection.toString(); >+ var linkText = someText.trim(); >+ var uri; s/var/let/ Can you find a better name for someText? >+ if (/^(https?:|ftp:)/i.test(linkText)) { nit: /^(?:https?:|ftp:)/ >+ else if (linkText.indexOf(":") == -1) { >+ let beginRange = selection.getRangeAt(0); >+ let endRange = selection.getRangeAt(selection.rangeCount - 1); >+ let beginText = beginRange.startContainer.textContent; >+ let endText = endRange.endContainer.textContent; >+ if (beginRange.startContainer.nodeType == Node.TEXT_NODE && beginRange.startOffset > 0) >+ someText = beginText.charAt(beginRange.startOffset - 1) + >+ beginText.charAt(beginRange.startOffset) + someText; >+ else >+ someText = " " + someText; >+ if (endRange.endContainer.nodeType == Node.TEXT_NODE && endRange.endOffset < endText.length) >+ someText += endText.charAt(endRange.endOffset - 1) + >+ endText.charAt(endRange.endOffset); >+ else >+ someText += " "; >+ if (someText.search(/^\W{1,2}\w+\.\D{1}\S*\W{1}\s*(\w|\W)?$/) != -1) { Add comment that explain what this is trying to achieve. "{1}" doesn't do anything and "(\w|\W)" equals ".", I think. >+ browser_linkSelection.js \ I'd suggest something like browser_plainTextLinks.js. >+function test() { >+ waitForExplicitFinish(); >+ let tab = gBrowser.selectedTab = gBrowser.addTab(); >+ tab.linkedBrowser.addEventListener("load", function() { you can drop let tab and use gBrowser.selectedBrowser.

Addressed the comments about s/var/let, changed the name of the test, change someText to selectedText and added a bunch of comments. (In reply to comment #28) > "{1}" doesn't do anything and "(\w|\W)" equals ".", I think. "{1}" is unnecessary, but more explicit. With the comment added it isn't necessary at all though, so I removed them. "(\w|\W)" is different than "." as it matches newlines (which is good for the end of word test).

dao: gentle review ping? Looks like bug 454518 never made 1.9.2, so this isn't really time critical per se, it's just been sitting in my queue for some time...

Comment on attachment 403084 [details] [diff] [review] patch - comments addressed >+ * This RegExp boils down to this: That's not what I meant with "explain what this is trying to achieve". You're basically just reading out the regular expression, which I think is mostly unnecessary. I was more interested in an abstract description of that code's goals.

Done.

Comment on attachment 408064 [details] [diff] [review] patch - comment fix >+ if (/^(?:https?:|ftp:)/i.test(linkText)) { >+ try { >+ uri = makeURI(linkText); The indentation is off. >+ selectedText = beginText.charAt(beginRange.startOffset - 1) + >+ beginText.charAt(beginRange.startOffset) + selectedText; Isn't beginText.charAt(beginRange.startOffset) already the first character of selectedText? >+ if (selectedText.search(/^\W{1,2}\w+\.\D\S*\W\s*(?:\w|\W)?$/) != -1) { /.../.test(selectedText) instead of selectedText.search(/.../) != -1 [\w\W] instead of (?:\w|\W) >+ try { >+ linkText = "http://" + linkText; >+ uri = makeURI(linkText); The indentation is off. Also move linkText = ... before try. >+ } catch(exc) {} catch (

> Isn't beginText.charAt(beginRange.startOffset) already the first character of selectedText? No, selection.toString() will trim the whitespace, so m| example.com| won't get caught since it will evaluate to |mexample.com| without the selected space. I remember doing it just for that, turns out there wasn't a specific test for that so I added one and also had to fix the RegExp to be able to match correctly for that case. All other comments addressed.

Comment on attachment 408316 [details] [diff] [review] patch - comments addressed (In reply to comment #34) > > Isn't beginText.charAt(beginRange.startOffset) already the first character of selectedText? > > No, selection.toString() will trim the whitespace, Unless no whitespace has been selected. In that case your code lies about selectedText, which may not be a problem right now, but makes it generally fragile and hard to follow. To unwind this, I suggest you test linkText separately, then explicitly the first range's first character and the last range's last character, and then the surrounding characters.

Dao, what do you think of this. This tests the actual link for "link-i-ness" and separately tests each edge for a word-break in either the end of the selection of 1 past the end. It does clean up the RegExp a lot, so this should be better.

The linkText test can be done earlier, next to linkText.indexOf(":") == -1, right?

Yup. In fact that test isn't necessary anymore, since it won't match the RegExp.

Comment on attachment 408436 [details] [diff] [review] patch - comment addressed This is better, but not as straightforward as I think it should be. This: > let beginRange = selection.getRangeAt(0); > let beginText = beginRange.startContainer.textContent; > let beginEdge = " "; > if (beginRange.startContainer.nodeType == Node.TEXT_NODE && beginRange.startOffset > 0) > beginEdge = beginText.charAt(beginRange.startOffset - 1) + > beginText.charAt(beginRange.startOffset); > if (/.*\W.*/.test(beginEdge) ... ... would be better structured like this, I think: > let beginRange = selection.getRangeAt(0); > // selection.toString() trims trailing whitespace, so we look for > // that explicitly in the first and last ranges. > let delimitedAtStart = /^\s/.test(beginRange); > if (!delimitedAtStart) { > let container = beginRange.startContainer; > let offset = beginRange.startOffset; > if (container.nodeType == Node.TEXT_NODE && offset > 0) > delimitedAtStart = /\W/.test(container.textContent[offset - 1]); > else > delimitedAtStart = true; > } > if (delimitedAtStart ...

This should be better.

Comment on attachment 408629 [details] [diff] [review] patch - comment cleanup >+ let selectedText = selection.toString(); >+ let linkText = selectedText.trim(); selectedText is unused, except for that single line. >+ if (/^(?:https?:|ftp:)/i.test(linkText)) { I think I prefer /^(?:https?|ftp):/... >+ linkText = "http://" + linkText; >+ try { >+ uri = makeURI(linkText); >+ } catch (exc) {} We should probably use nsIURIFixup instead of prepending http:// manually.

This (the nsIURIFixup addition) fixes the ftp.example.com --> ftp://ftp.example.com case mentioned in comment 22. I added a test for that as well.

Comment on attachment 408779 [details] [diff] [review] patch - comment addressed >+ uri = Cc["@mozilla.org/docshell/urifixup;1"]. >+ getService(Ci.nsIURIFixup). >+ createFixupURI(linkText, Ci.nsIURIFixup.FIXUP_FLAGS_MAKE_ALTERNATE_URI); Why FIXUP_FLAGS_MAKE_ALTERNATE_URI? Since www is sometimes special-cased, can you make the test use a subdomain different from www?

Comment on attachment 408779 [details] [diff] [review] patch - comment addressed >+ // Now let's see if this is an intentional link selection (our guess is >+ // based on if the selected text begins and ends with a non-word character >+ // or is preceded and followed by one) "begins/ends with whitespace or is preceded/followed by a non-word character" instead of "begins and ends with a non-word character or is preceded and followed by one"?

(In reply to comment #43) > (From update of attachment 408779 [details] [diff] [review]) > >+ uri = Cc["@mozilla.org/docshell/urifixup;1"]. > >+ getService(Ci.nsIURIFixup). > >+ createFixupURI(linkText, Ci.nsIURIFixup.FIXUP_FLAGS_MAKE_ALTERNATE_URI); > > Why FIXUP_FLAGS_MAKE_ALTERNATE_URI? Fixed, added a test (since that prepends www in the example.com case). > Since www is sometimes special-cased, can you make the test use a subdomain > different from www? Done. Also changed the comment as indicated.

Comment on attachment 408924 [details] [diff] [review] patch - comments addressed >+ // Now let's see if this is an intentional link selection (our guess is >+ // based on if the selected text begins/ends with whitespace or is >+ // preceded/followed by a non-word character. >+ let beginRange = selection.getRangeAt(0); >+ let delimitedAtStart = /^\s/.test(beginRange); >+ if (!delimitedAtStart) { >+ // selection.toString() trims trailing whitespace, so we look for >+ // that explicitly in the first and last ranges. That comment appears to be in the wrong block, should be moved up. >+function test() { >+ waitForExplicitFinish(); >+ gBrowser.selectedTab = gBrowser.addTab(); >+ gBrowser.selectedBrowser.addEventListener("load", function() { >+ gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true); >+ doc = gBrowser.selectedBrowser.contentDocument; content.document >+ range = doc.createRange(); >+ selection = gBrowser.selectedBrowser.contentWindow.getSelection(); content.getSelection() >+ gBrowser.selectedBrowser.contentWindow.focus(); You should probably use waitForFocus(..., content) here.

Thanks.

Comment on attachment 409161 [details] [diff] [review] patch - nits addressed >+ let beginRange = selection.getRangeAt(0); >+ let delimitedAtStart = /^\s/.test(beginRange); >+ // selection.toString() trims trailing whitespace, so we look for >+ // that explicitly in the first and last ranges. >+ if (!delimitedAtStart) { This comment still annotates the wrong line. >+ let delimitedAtEnd = /^\s/.test(endRange); This needs to be /\s$/. >+ waitForFocus(runSelectionTests, content); >+ content.focus(); content.focus() is redundant, waitForFocus does that. I'll address these things before landing the patch.

This should have used mozITXTToHTMLConv.findURLInPlaintext() http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/streamconv/public/mozITXTToHTMLConv.idl#111 It basically just duplicates this code, just that mozITXTToHTMLConv is much more elaborated.

File a new bug? From a very cursory glance at the idl, it doesn't necessarily cover what we need, but I can definitely look into it...