Right now in Firefox, right-clicking on a link produces a context menu with items such as "Bookmark this link," "Send link," etc. Selecting a domain that is not a link and right clicking produces some similar context menu items, such as "Open Link," but it doesn't have the same menu items. I suppose the argument is that a selected domain may not be a valid domain or link, but even a link may not be valid. I think we should stray on the side of consistency and treat them identically. Also, currently selecting "bob.com" treats the domain as a domain, but selecting "bob.com/" treats it as a string. Let's treat both as links.

The issue here was a not only did the RegExp not recognize selected text ending in a forward-slash, but it didn't recognize any type of path following a URL at all if a protocol wasn't provided at the beginning of the link (i.e., 'http://www.bob.com/bob/' would validate as a proper URL, whereas 'www.bob.com/bob/' would not). The old RegExp also did not recognize IPv4 or IPv6 addresses as possible valid URLs, didn't allow the use of port numbers, and also didn't check whether a domain name was even of the proper format to possibly be a valid URL. Below I've provided a bit of an explanation of the updated RegExp. With this RegExp we: 1) Ensure that the top-level domain is 2-4 characters in length, and contains only alphabetic characters. This encompasses all of the well-known TLDs that I could find or think of. 2) Ensure that the domain and subdomains do not begin or end with hyphens, as this is not allowed by domain registrars. I originally disallowed the use of consecutive hyphens as well, although it seems that some registrars allow for consecutive hyphens in a domain name, provided they meet the former rule as well. 3) Ensure that the domain and subdomains contain only alphabetic, numeric, or hyphen characters; with the above limitation on the use of hyphens. 4) If an IPv4 address is selected rather than a domain name, the RegExp will ensure that the format follows the 'xxx.xxx.xxx.xxx' pattern, and all ranges inbetween (meaning '0.0.0.1' is recognized as is '255.255.255.255'). 5) If an IPv6 address is selected rather than a domain name, the RegExp will ensure that the format follows the '[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx]' pattern, and all ranges inbetween (meaning '[::1]' will be valid, as will '[FEDC:BA98:7654:3210:FEDC:BA98:7654:FFFF]'). 6) If a port number is provided following either the TLD or the IP address, ensures that a colon and 1 to 5 numerical characters are provided. 6) Ensure that the path after the TLD (or IP address/port number - if provided) does not contain consecutive forward-slashes, although it can END with a foward-slash. Additionally, the options which are present when an actual link is right-clicked, rather than one that is selected manually are: "Bookmark This Link", "Save Link As...", "Send Link...", and "Copy Link Location". "Copy Link Location" would be redundant with "Copy" in this case, since they would both accomplish the same thing, so that option does not appear. The other three, however, now also appear on the context menu when the updated RegExp recognizes a valid URL pattern.

Comment on attachment 603082 [details] [diff] [review] Updated RegExp and changed visibility of some context menu options. Thanks for the patch! I think it's probably best to not make the RegExp change in this bug, since it's not strictly related to the other changes. It's also significantly more complicated to understand, for little benefit - the odds that someone would want to use this functionality for IPv6 literals are pretty low :) The other changes look OK. Can you post an updated patch without the regexp change, and then request review from ":jaws"?

Reverted RegExp changes made in previous patch submission.

Comment on attachment 603108 [details] [diff] [review] patch v2 Review of attachment 603108 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks good to me, but we'll need to update the respective unit tests. Also, the patch attached should be rebased on top of the latest mozilla-central trunk, since the patch didn't apply cleanly for me. The unit test for this is located at /browser/base/content/test/test_contextmenu.html and /browser/base/content/test/subtst_contextmenu.html. Let me know if you have any questions about adding the test. ::: browser/base/content/nsContextMenu.js @@ +1082,1 @@ > urlSecurityCheck(this.linkURL, doc.nodePrincipal); Should |this.linkURL| get updated based on the current value of |linkText|? Could script change the selection on the page after the context menu was opened such that |this.linkURL| and |linkText| are not in-sync? @@ +1399,5 @@ > + // If selected text is found to match valid URL pattern. > + if (this.onPlainTextLink) > + linkText = document.commandDispatcher.focusedWindow.getSelection().toString().trim(); > + else > + linkText = this.linkText(); nitpick: Can you please remove the trailing whitespace on this line and the one three lines above it?

(In reply to Jared Wein [:jaws] from comment #4) > Let me know if you have any questions about adding the test. I misspoke here. I don't think we'll need to *add* a test, but we'll need to *update* the current test.

> > + if (this.onPlainTextLink) > > + linkText = document.commandDispatcher.focusedWindow.getSelection().toString().trim(); > > + else > > + linkText = this.linkText(); > > nitpick: Can you please remove the trailing whitespace on this line and the > one three lines above it? Also, use two spaces for indentation.

Removed trailing whitespace, fixed indentation, rebased patch on top of latest trunk.

Added in a function in order to actually select the text, which is working fine. However, when running these two new test cases, you'll note that there exists two failures; namely, 'context-copy' is supposedly disabled when it should be enabled: failed | checking item #0 (context-copy) enabled state - got false, expected true failed | checking item #7 (context-copy) enabled state - got false, expected true If I manually open the context menu, this is not the case - so I'm unsure why this is happening. Everything else works fine. Sorry for the double flag here, wasn't quite sure how to handle it with two different file uploads.

(In reply to Chad Freeman from comment #7) > Created attachment 604611 [details] [diff] [review] (In reply to Chad Freeman from comment #8) > Created attachment 604612 [details] [diff] [review] > Sorry for the double flag here, wasn't quite sure how to handle it with two > different file uploads. You did the right thing :) I'll try to review this today.

These patches look good. I need more time to figure out why the context menuitem for copy is disabled though. Thanks for writing a test! We didn't know about this before :)

Chad: I talked with Justin Dolske about this and we decided on some next steps for this bug. 1) I'll send these patches to the try server to see which platforms this test fails on. 2) Since the test breakage wasn't introduced by your patches, we'll "fix" the test to get it passing and file a follow-up bug to investigate the erroneous disabling of the copy menuitem. We will also add a todo() to the code so that when running the test there is a message that part of the test needs to be fixed. 3) I'll send the "fixed" patches to the try server to make sure that they can be landed. 4) If all good at this point, then I'll go ahead and check in these patches for you.

Comment on attachment 604612 [details] [diff] [review] Updated Unit Tests r=me with the tests being "fixed" so that they pass. See comment #11 for background on what will be done to fix these tests.

Original patches pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=148368a44c89 "Fixed" patches pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=a66851f9f235

The try runs have shown that the test only fails on Windows (and doesn't run on Linux due to bug 513558). Instead of "fixing" the test to get it to pass on Windows, I've modified the test so it only checks the context menu in the selection cases when being run on Mac. This will keep the test running on as many platforms as possible. I've filed bug 736399 to investigate why the context menuitem for "context-copy" is disabled on Windows. I've pushed my test fixes to the try server, and pending a green run I'll check in these patches. See this link for the try server results: https://tbpl.mozilla.org/?tree=Try&rev=4f5923c60176

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/2c3ffd75d2d3

Very nice, but why didn't you take the regex? As it stands, this is useless due to bug 694856 - meaning it doesn't even work on Google search results.