Last Comment Bug 720023 - Context menus for selected URLs should be the identical as for links
: Context menus for selected URLs should be the identical as for links
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Menus (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Chad Freeman
:
:
Mentors:
: 518006 664970 (view as bug list)
Depends on: 454518
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-20 15:21 PST by Jennifer Morrow [:Boriss] (UX)
Modified: 2012-11-05 09:27 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Updated RegExp and changed visibility of some context menu options. (8.19 KB, patch)
2012-03-05 15:38 PST, Chad Freeman
gavin.sharp: review-
Details | Diff | Splinter Review
patch v2 (7.25 KB, patch)
2012-03-05 16:38 PST, Chad Freeman
jaws: feedback+
Details | Diff | Splinter Review
patch v3 (6.96 KB, patch)
2012-03-09 22:43 PST, Chad Freeman
jaws: review+
Details | Diff | Splinter Review
Updated Unit Tests (5.63 KB, patch)
2012-03-09 22:50 PST, Chad Freeman
jaws: review+
Details | Diff | Splinter Review

Description Jennifer Morrow [:Boriss] (UX) 2012-01-20 15:21:36 PST
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.
Comment 1 Chad Freeman 2012-03-05 15:38:49 PST
Created attachment 603082 [details] [diff] [review]
Updated RegExp and changed visibility of some context menu options.

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 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-05 16:02:09 PST
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"?
Comment 3 Chad Freeman 2012-03-05 16:38:49 PST
Created attachment 603108 [details] [diff] [review]
patch v2

Reverted RegExp changes made in previous patch submission.
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-03-06 10:16:15 PST
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?
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-03-06 10:17:28 PST
(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.
Comment 6 Dão Gottwald [:dao] 2012-03-06 10:45:38 PST
> > +    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.
Comment 7 Chad Freeman 2012-03-09 22:43:07 PST
Created attachment 604611 [details] [diff] [review]
patch v3

Removed trailing whitespace, fixed indentation, rebased patch on top of latest trunk.
Comment 8 Chad Freeman 2012-03-09 22:50:57 PST
Created attachment 604612 [details] [diff] [review]
Updated Unit Tests

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.
Comment 9 Jared Wein [:jaws] (please needinfo? me) 2012-03-12 14:10:26 PDT
(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.
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-03-12 19:00:34 PDT
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 :)
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2012-03-15 17:30:44 PDT
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 12 Jared Wein [:jaws] (please needinfo? me) 2012-03-15 17:31:49 PDT
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.
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2012-03-15 19:07:04 PDT
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
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-03-16 00:34:20 PDT
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
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-03-16 12:42:18 PDT
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/2c3ffd75d2d3
Comment 16 Tim Taubert [:ttaubert] 2012-03-17 01:29:17 PDT
https://hg.mozilla.org/mozilla-central/rev/2c3ffd75d2d3
Comment 17 Jesse Ruderman 2012-03-18 17:02:53 PDT
*** Bug 664970 has been marked as a duplicate of this bug. ***
Comment 18 Jesse Ruderman 2012-03-18 17:03:24 PDT
*** Bug 518006 has been marked as a duplicate of this bug. ***
Comment 19 brbzilla 2012-03-18 22:30:17 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.