Last Comment Bug 638949 - Please add Copy Location in the context menu for URLs in the web console
: Please add Copy Location in the context menu for URLs in the web console
Status: RESOLVED FIXED
[console-1][good first bug]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: Firefox 18
Assigned To: Anton Kovalyov (:anton)
:
: Brian Grinstead [:bgrins]
Mentors:
: 793297 (view as bug list)
Depends on:
Blocks: 776939
  Show dependency treegraph
 
Reported: 2011-03-04 14:51 PST by Jeff Balogh (:jbalogh)
Modified: 2012-09-22 00:35 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First try (1.38 KB, patch)
2011-12-06 09:30 PST, Aleksandr Motsjonov
no flags Details | Diff | Splinter Review
Second try - wrong curly brackets and line numbering fixed. (2.05 KB, patch)
2011-12-06 10:53 PST, Aleksandr Motsjonov
rcampbell: review+
Details | Diff | Splinter Review
Third try. Code Style corrected. (2.08 KB, patch)
2011-12-06 11:56 PST, Aleksandr Motsjonov
no flags Details | Diff | Splinter Review
Fourth try. Now with item title change (5.36 KB, patch)
2011-12-07 08:26 PST, Aleksandr Motsjonov
rcampbell: review+
Details | Diff | Splinter Review
Fifth Try. Correction according to suggestions. No tests yet. (6.73 KB, patch)
2011-12-08 05:28 PST, Aleksandr Motsjonov
no flags Details | Diff | Splinter Review
Fifth Try. Correction according to suggestions. No tests yet. (6.73 KB, patch)
2011-12-08 05:32 PST, Aleksandr Motsjonov
mihai.sucan: feedback+
Details | Diff | Splinter Review
Patch with tests. (3.63 KB, patch)
2012-09-17 15:17 PDT, Anton Kovalyov (:anton)
no flags Details | Diff | Splinter Review
Patch with tests (and all files this time). (6.60 KB, patch)
2012-09-17 17:09 PDT, Anton Kovalyov (:anton)
no flags Details | Diff | Splinter Review
New approach (12.78 KB, patch)
2012-09-18 17:20 PDT, Anton Kovalyov (:anton)
mihai.sucan: review+
Details | Diff | Splinter Review
Final patch (with Mihai's comments addressed) (12.86 KB, patch)
2012-09-19 14:41 PDT, Anton Kovalyov (:anton)
no flags Details | Diff | Splinter Review

Description Jeff Balogh (:jbalogh) 2011-03-04 14:51:36 PST
Usually when I hit Copy in the context menu on the web console I'm trying to get a URL. Then I paste it into the URL bar and manually delete the metadata surrounding the URL. It's annoying. Firebug provides a better experience with Copy Location. (It also has Copy Request/Response Headers and Response Body. I don't know if I've ever used those.)
Comment 1 David Dahl :ddahl 2011-03-31 08:44:03 PDT
The Scope for this bug is a single context button with "Copy URL"
Comment 2 Panos Astithas [:past] 2011-06-16 01:47:10 PDT
Unfortunately I won't have time for this in the near future, so it's up for grabs.
Comment 3 Aleksandr Motsjonov 2011-11-20 13:16:34 PST
Should this new menu item be disabled in case of non-link lines or it should appear only in cases with link?
Comment 4 Paul Rouget [:paul] 2011-11-25 08:26:10 PST
No, I think the copy item should always be enable. What should change is what is copied.

In a line like this one:
GET https://bugzilla.mozilla.org/extensions/BMO/web/images/background.png [HTTP/1.1 200 OK 225ms]

right-click->copy on "GET" should copy the whole line (with the timestamp & co). This is the current behvior.

right-click->copy on the URL should copy only the URL (currently, the whole line is copied).
Comment 5 Aleksandr Motsjonov 2011-12-06 09:30:53 PST
Created attachment 579338 [details] [diff] [review]
First try

I started with CommandController and it's method doCommand->this.copy(outputNode). I found that there is no information about what actually were clicked. I didn't find how to provide there such info, because of the nature of pop-up creation.
Than I found underline method copySelectedItems, which get only element where it find selected elements with predefined clipboardText property. And again - luck of event object or other information of actual clicked object.
So I made handler in message creation method, that handle all clicks and check's whatever it were "url" box or not. In case of "yes" answer it replaces clipboardText property with URL only and sets little flag for copySelectedItems. Flag needed to identify need for [timestamp] in copied text.

So I guess there are better ways, maybe with a bit refactoring to make it more elegant. I would like to know about it =)

Also multiline select should be considered. What is right way to do in that case. So if I clicked on link, but there were already line selected. Should be only URL selected, or it should work as usual.
Comment 6 Aleksandr Motsjonov 2011-12-06 10:53:19 PST
Created attachment 579377 [details] [diff] [review]
Second try - wrong curly brackets and line numbering fixed.
Comment 7 Mihai Sucan [:msucan] 2011-12-06 11:32:13 PST
Comment on attachment 579377 [details] [diff] [review]
Second try - wrong curly brackets and line numbering fixed.

Review of attachment 579377 [details] [diff] [review]:
-----------------------------------------------------------------

I would suggest a different user experience here: when the user right-clicks on a network message show a context menu item for "copy link location".

This approach makes Ctrl-C, Edit > Copy and right-click copy to copy only the URL after the node has been clicked, which is not ideal, in my opinion.

Rob, any thoughts on this?
Comment 8 Rob Campbell [:rc] (:robcee) 2011-12-06 11:34:12 PST
Comment on attachment 579377 [details] [diff] [review]
Second try - wrong curly brackets and line numbering fixed.

Awesome! Patch applies cleanly on first try.

I had to work out the functionality by looking at the code and then experimenting. What you've got here seems to do the job.

Now for the reviewy bits.

A couple notes about style, in general, take a look around your patch to see how the file does things like spacing around parentheses, braces and operators.

instead of:

if(item.linkClicked){

do:

if (item.linkClicked) {

here:
+        timestampString = "[" + timestampString + "] ";
+        if(item.linkClicked){
+          timestampString = "";
+        }

I think I would write this with an else:

if (item.linkClicked) {
  timestampString = "";
} else {
  timestampString = "[" + timestampString + "] ";
}

in the second chunk:
 
+

extra blank line?

...
+    node.addEventListener('click', function(aEvent){
+      let target = aEvent.rangeParent;
+      if(target && target.classList && target.classList.contains('webconsole-msg-link')){

That if line gets a little long, maybe break the expression into 2 lines like:
       if (target && target.classList &&
           target.classList.contains('webconsole-msg-link')) {
         ...

We generally try to keep lines below 80 characters wide for readability on mxr and small screens.

Other than the styling issues and the if-else, I'd say this is good!
Comment 9 Rob Campbell [:rc] (:robcee) 2011-12-06 11:37:42 PST
(In reply to Mihai Sucan [:msucan] from comment #7)
> Comment on attachment 579377 [details] [diff] [review] [diff] [details] [review]
> Second try - wrong curly brackets and line numbering fixed.
> 
> Review of attachment 579377 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I would suggest a different user experience here: when the user right-clicks
> on a network message show a context menu item for "copy link location".
> 
> This approach makes Ctrl-C, Edit > Copy and right-click copy to copy only
> the URL after the node has been clicked, which is not ideal, in my opinion.
> 
> Rob, any thoughts on this?

I considered adding a separate menu item while reading this, but didn't like that idea. Are you suggesting changing the Copy context menu item to Copy Link Location when the user right clicks on the URL? If so, that might be a good compromise.

Nevertheless, in my testing, this patch does do the job even if it isn't exactly discoverable.

Maybe we should get some UX feedback on the route we should take.
Comment 10 Rob Campbell [:rc] (:robcee) 2011-12-06 11:41:10 PST
actually, this is such a small issue, I'm willing to take the blame for landing this as-is.

Soswow: If you can make the popup menu change based on where the user's clicking, that would be preferable. If not, I'll take an updated patch as you've written it to do the job and a follow-up bug report to make the nicer menu option.
Comment 11 Mihai Sucan [:msucan] 2011-12-06 11:46:19 PST
Rob: sounds good to me. Later a separate patch for changing the context menuitem to "copy link location" when the user right-clicks on the URL can be written.

Thanks for your contribution Aleksandr! Much appreciated!
Comment 12 Aleksandr Motsjonov 2011-12-06 11:56:56 PST
Created attachment 579396 [details] [diff] [review]
Third try. Code Style corrected.

I fixed Code Style and suggested if/else. I'll think about changing Menu Item label according to your suggestions.
Comment 13 Aleksandr Motsjonov 2011-12-07 08:26:46 PST
Created attachment 579698 [details] [diff] [review]
Fourth try. Now with item title change

I moved everything into context-menu opening event handler. I think it's more logical place and here I can change menu title if needed, which I also added. So now URL only is copied only if 1) user click "Copy link location" 2) There is only one line selected. In other cases normal like copied.
Also I added comment there, don't know yet is it ok.

I don't like this whole peace of code placed inside event handler. I eager move in some named function, but I don't know what is best way to do it.

Please comment =)
Comment 14 Mihai Sucan [:msucan] 2011-12-07 10:46:16 PST
Comment on attachment 579698 [details] [diff] [review]
Fourth try. Now with item title change

Review of attachment 579698 [details] [diff] [review]:
-----------------------------------------------------------------

This is a cool fix! Users will be happy with this kind of Web Console polish! Thanks Aleksandr!

Did some quick testing and found that if I dismiss the context menu without using the mouse, the clipboardText of the given output item is not reset back to the original. Can you please look into this? Thanks!
Comment 15 Rob Campbell [:rc] (:robcee) 2011-12-07 14:39:45 PST
Comment on attachment 579698 [details] [diff] [review]
Fourth try. Now with item title change

+    menuPopup.addEventListener("popupshowing", function(event) {
+      // If user invoke context menu by clicking on URL, than:
+      //  1. Show "Copy link location" item
+      //  2. Replace original clipboardText with URL
+      //  3. Set flag urlClicked that copy function doesn't add [time] before URL
+      // In other cases reset everything.
+

I have some suggestions for wording here.

// If the context menu is invoked while clicking on a URL:
//   1. Show "Copy Link Location"
//   2. Replace original clipboardText with URL
//   3. Set the urlClicked flag so the copy function doesn't add 
//      [time] before the URL.
// Otherwise, reset everything.

+      let clickedIsElement = clickedElem.nodeType && clickedElem.nodeType == 1;

maybe change this to:
      let isClickedElement = clickedElem.nodeType &&
            clickedElem.nodeType == Ci.nsIDOMNode.ELEMENT_NODE;

+      copyItem.setAttribute("label", copyLabel); //Reset context item Label

extra space after //

+    copyItem.setAttribute("label", copyLabel);

Looking good!

I think this is going to work.

Last part is going to be writing a unit test for this to verify the menu is doing what we hope it's doing.

r+ with above nits addressed.
Comment 16 Rob Campbell [:rc] (:robcee) 2011-12-07 14:42:30 PST
oh, one other thing: You should add yourself to the list of Contributors in the license section of the file.
Comment 17 Aleksandr Motsjonov 2011-12-08 05:28:05 PST
Created attachment 580014 [details] [diff] [review]
Fifth Try. Correction according to suggestions. No tests yet.
Comment 18 Aleksandr Motsjonov 2011-12-08 05:32:14 PST
Created attachment 580016 [details] [diff] [review]
Fifth Try. Correction according to suggestions. No tests yet.

Sorry, accidentally posted without Obsoletes options.

I added onhidden even handler, which resets everything, corrected wording, added caps letters to Copy Link Location, and made reseting before opening (maybe it's even not needed).

Now, if everything is ok with that, only tests are left.
Comment 19 Mihai Sucan [:msucan] 2011-12-08 07:57:36 PST
Comment on attachment 580016 [details] [diff] [review]
Fifth Try. Correction according to suggestions. No tests yet.

Review of attachment 580016 [details] [diff] [review]:
-----------------------------------------------------------------

Tested the patch and it works like expected! Thanks for your fix Aleksandr! Great stuff!

Yep, you can move to write the test!

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +83,5 @@
>  jsPropertyInspectTitle=Inspect: %S
>  saveBodies.label=Log Request and Response Bodies
>  saveBodies.accesskey=L
>  copyCmd.label=Copy
> +copyLocationCmd.label=Copy Link Location  

This string has trailing whitespace. Please remove it.
Comment 20 Rob Campbell [:rc] (:robcee) 2012-08-30 09:29:31 PDT
no activity. Unassigning.
Comment 21 Anton Kovalyov (:anton) 2012-09-17 15:17:35 PDT
Created attachment 661943 [details] [diff] [review]
Patch with tests.

Attached is a patch that converts 'Copy' item in the context menu into 'Copy Link Location'. I took Aleksandr's patch, rewrote it (there were too many changes throughout the year to apply it directly) and wrote tests for the change.

Note about tests: I tried to reuse test-network.html but console logging tests were failing because an image from that file was being cached.

Also, I am learning the code base by reading applicable parts of it so if something doesn't make sense (redundant, etc.) this means I copied it from somewhere else. :-) Point me in the right direction and I'll fix it.

Tests for webconsole on my computer:

INFO TEST-START | Shutdown
Browser Chrome Test Summary
	Passed: 1440
	Failed: 0
	Todo: 0

*** End BrowserChrome Test Results ***
Comment 22 Anton Kovalyov (:anton) 2012-09-17 17:09:31 PDT
Created attachment 661974 [details] [diff] [review]
Patch with tests (and all files this time).

Oops, I forgot to `hg add` some files with into my previous patch. Sorry, here's the correct version.
Comment 23 Mihai Sucan [:msucan] 2012-09-18 10:04:42 PDT
Comment on attachment 661974 [details] [diff] [review]
Patch with tests (and all files this time).

Review of attachment 661974 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch Anton! It looks good and all tests pass.

One issue: if I right-click on GET and I pick Copy, I get the network message clipboard text (which is expected). If I click on the URL I get "Copy location" and it works as desired (only URL). But now if I right click again on GET and I select Copy, the text that is copied to the clipboard is different (unexpected behavior).

I'm not sure what user experience people want here. I would like:

1. right-click on GET > Copy: copies the full network message.
2. right-click on URL > Copy location: copies the URL only.
3. Ctrl-C with one network message: copies the URL only. (unsure about this, what do you think?)
4. Ctrl-C with multiple messages: copies the full network messages.

Thoughts? We should also ask Heather about this.

::: browser/devtools/webconsole/test/browser_bug_638949_copy_link_location.js
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const TEST_NETWORK_URI = "http://example.com/browser/browser/devtools/" +
> +  "webconsole/test/test-network-message.html?_date=" + Date.now();

You can reuse any of the existing test files. For example just load test-console.html with Date.now(). You don't need any other network messages anyway, you can play with only one.

@@ +46,5 @@
> +      let selected = output.getItemAtIndex(0);
> +      selected.isLink = true;
> +      selected.clipboardText = selected.
> +        querySelector(".webconsole-msg-url").value;
> +      item.doCommand();

Here you do what the popupshowing event handler does. Why not open the context menu and check if cmd_copy works as desired after that?

::: browser/devtools/webconsole/webconsole.js
@@ +387,5 @@
> +          }
> +        }
> +      }
> +
> +      if (selItem.isLink) {

You can avoid the logic that checks if |elem| is what you need. Just check if selItem.category is CATEGORY_NETWORK and use selItem.url.

(selItem.url was just added in the patch I landed in fx-team)
Comment 24 Heather Arthur [:harth] 2012-09-18 11:37:56 PDT
(In reply to Mihai Sucan [:msucan] from comment #23)
> Comment on attachment 661974 [details] [diff] [review]
> Patch with tests (and all files this time).
> 
> Review of attachment 661974 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the patch Anton! It looks good and all tests pass.
> 
> One issue: if I right-click on GET and I pick Copy, I get the network
> message clipboard text (which is expected). If I click on the URL I get
> "Copy location" and it works as desired (only URL). But now if I right click
> again on GET and I select Copy, the text that is copied to the clipboard is
> different (unexpected behavior).
> 
> I'm not sure what user experience people want here. I would like:
> 
> 1. right-click on GET > Copy: copies the full network message.
> 2. right-click on URL > Copy location: copies the URL only.
> 3. Ctrl-C with one network message: copies the URL only. (unsure about this,
> what do you think?)
> 4. Ctrl-C with multiple messages: copies the full network messages.
> 
> Thoughts? We should also ask Heather about this.

Just talked with Anton about this. I think it's best to have a separate menu item for "Copy Location" when you right click on a url, and leave the "Copy" menuitem with its associated Ctrl-C command. So pressing Ctrl-C after opening the context menu will always copy the entire message.

That will keep things consistent, especially because the text of the entire message appears selected in both cases.
Comment 25 Anton Kovalyov (:anton) 2012-09-18 17:20:34 PDT
Created attachment 662367 [details] [diff] [review]
New approach

So I noticed that there is a new context menu item now--"Open URL"--and decided to make "Copy Link Location" to behave exactly the same. Pros:

1. Consistency: users will see the same behavior for two URL-related commands.
2. Since you don't need to right-click on the link itself to copy the URL anymore Windows users will be able to trigger the context menu from keyboard and copy URL without even touching a mouse.
3. This "Copy Link Location" is now more consistent with generic "Copy Link Location" in Firefox (if you select random text on a page and right-click on a link Firefox shows both "Copy" and "Copy Link Location" items).

I rewrote my test file based on the "Open URL" test file (which is pretty cool). I also moved context menu helpers from that file and into head.js.
Comment 26 Mihai Sucan [:msucan] 2012-09-19 12:40:09 PDT
Comment on attachment 662367 [details] [diff] [review]
New approach

Review of attachment 662367 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. Thank you!

r+ provided the minor comments below are addressed.

::: browser/devtools/webconsole/test/head.js
@@ +229,5 @@
> +  EventUtils.synthesizeMouse(targetElement, 2, 2,
> +                             eventDetails, targetElement.ownerDocument.defaultView);
> +}
> +
> +function closeContextMenu (aContextMenu) {

I'm not sure if we really need this here - in head.js. I would not make a function to wrap foo.hidePopup().

::: browser/devtools/webconsole/webconsole.js
@@ +2272,4 @@
>    /**
>     * Copies the selected items to the system clipboard.
>     */
> +  copySelectedItems: function WCF_copySelectedItems(opts)

Please follow the coding style: aOptions for the variable name and add documentation in the comment above.
Comment 27 Anton Kovalyov (:anton) 2012-09-19 14:41:48 PDT
Created attachment 662696 [details] [diff] [review]
Final patch (with Mihai's comments addressed)

Thanks!
Comment 28 Panos Astithas [:past] 2012-09-21 01:00:59 PDT
https://hg.mozilla.org/integration/fx-team/rev/cdc721f3f9c6
Comment 29 Matthias Versen [:Matti] 2012-09-21 14:29:10 PDT
*** Bug 793297 has been marked as a duplicate of this bug. ***
Comment 30 miserlou 2012-09-21 15:13:07 PDT
File a bug, find it has been fix commited earlier that day.. awesome open source win. Go Firefox!
Comment 31 Tim Taubert [:ttaubert] 2012-09-22 00:35:38 PDT
https://hg.mozilla.org/mozilla-central/rev/cdc721f3f9c6

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