862019 - Allow Ctrl-C to copy the selected header name and value

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Mihai Sucan [:msucan]]

I selected a header in  the request/response view and I want to copy with Ctrl-C: this doesn't work. We should allow such behavior. Since netmonitor uses the variables view it makes sense to allow this in all cases where this widget is used (web console and debugger).

Comment 1

[brian_satchwannabe]

Attachment: Patch Attempt to add Ctrl-C functionality

Hey, I haven't contributed to Mozilla before but I'd like to offer a patch for this bug. Any and all comments and/or advice appreciated.

Comment 2

[Victor Porof [:vporof][:vp]]

Comment on attachment 811747 [details] [diff] [review]
Patch Attempt to add Ctrl-C functionality

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

Hey, thanks for the patch and contributing to Mozilla! \o/

This patch looks good, I only have a few comments below. You should also add a mochitest [0] for this, see the browser_dbg_variables-view* in browser/devtools/debugger/test for some examples and browser/devtools/debugger/test/Makefile.in for where you need to make the build system aware of the new file. Let me know if you need any help with that.

To run mochitests, do something like
> ./mach mochitest-browser browser/devtools/debugger/test/
or 
> ./mach mochitest-browser browser/devtools/debugger/test/a-specific-source-file.js

[0] https://developer.mozilla.org/en/docs/Mochitest

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +79,5 @@
>    this._appendEmptyNotice();
>  
>    this._onSearchboxInput = this._onSearchboxInput.bind(this);
>    this._onSearchboxKeyPress = this._onSearchboxKeyPress.bind(this);
> +  this._onViewKeyDown = this._onViewKeyDown.bind(this);

There are a number of things that might go wrong when switching from keydown to keypress. The previous implementation wasn't designed to handle repeated keypresses for example.

If necessary, add another event listener for keypress.

@@ +823,5 @@
> +      case e.DOM_VK_C:
> +        if (e.ctrlKey) {
> +          clipboardHelper.copyString(
> +            item._name.getAttribute("value")
> +            + item.separatorStr 

Nit: make sure you're not adding trailing whitespace.

@@ +826,5 @@
> +            item._name.getAttribute("value")
> +            + item.separatorStr 
> +            + item._valueLabel.getAttribute("value")
> +          );
> +        }

I'm not entirely sure what the expected behavior should be? Copying the key + value combo, or just the value? What do you think?

Comment 3

[brian_satchwannabe]

Hey,
I made the suggested changes (i.e. created a separate keyDown handler).
In realtion to your question about expected behavior, I think that seeing as both key and value are highlighted in the list, then both should be copied. Also, you can already copy either the individual key or value by entering edit mode for them.

I'm having some trouble creating the test however.
I tried adding the Ctrl-C test to the
/devtools/debugger/test/browser_dbg_variables-view-accessibility.js
You can see what I was trying at the below link:
http://pastebin.mozilla.org/3179202 (line 484 - 493)
However, when I run the tests, the suite doesnt seem to wait for the callbacks to complete before finishing. Is there some mechanism that will force the test to wait till the waitForClipboard callbacks have been completed?

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to brian_satchwannabe from comment #3)

Yup, you're in a generator function that takes promises as yield arguments to handle concurrency. See https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm for more information. 

Basically, you'll need to do something like this:

let copied = promise.defer();

waitForClipboard(gVariablesView.getFocusedItem().name, function setup() {
  EventUtils.synthesizeKey("C", { ctrlKey: true }, gDebugger);
}, copied.resolve, copied.reject);

try {
  yield copied;  
  ok(true, "Ctrl-C should copy the selected item to the clipboard.");
} catch (e) {
  ok(false, "Ctrl-C didn't copy the selected item to the clipboard.");
}

(that's one way of doing things, alternatively you can try others once you grasp the power of Task.jsm).

You'll also want to use the metaKey instead of ctrlKey, because on OS X it'll be Cmd+C instead of Ctrl+C. Read all about this here: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent

Comment 5

[brian_satchwannabe]

Attachment: Patch for Bug 862019 to add Variables View Ctrl-C

Comment 6

[brian_satchwannabe]

(In reply to Victor Porof [:vp] from comment #4)

Ok sweet, cheers for the links.
I've attached a revised patch. Hopefully it's a little better. :)
Appreciate your help on this.

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 813385 [details] [diff] [review]
Patch for Bug 862019 to add Variables View Ctrl-C

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

Path looks good, thanks! I only have a few comments below, but other than that I think this is ready!

r+ with these addressed:

::: browser/devtools/debugger/test/browser_dbg_variables-view-accessibility.js
@@ +482,5 @@
>        "The 'someProp7' variable should be hidden.");
> +	
> +	// Part 11: Test that Ctrl-C copies the current item to the system clipboard
> +
> +	gVariablesView.focusFirstVisibleItem();

Nit: there's some funky indentation here, but again, I can take care of it before landing.

@@ +487,5 @@
> +    let copied = promise.defer();
> +    var isMac = window.navigator.platform.indexOf("Mac") !== -1;
> +    var expectedValue = gVariablesView.getFocusedItem().name
> +      + gVariablesView.getFocusedItem().separatorStr
> +      + gVariablesView.getFocusedItem().value;

You don't have yo go through all this trouble. metaKey is magically correct on all platforms (ctrl on Win/Linux and cmd on Mac).

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +23,5 @@
>  Cu.import("resource:///modules/devtools/shared/event-emitter.js");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "devtools",
>    "resource://gre/modules/devtools/Loader.jsm");
> +  

Nit: trailing whitespace.

I usually set my editor settings to trim them whenever I save, but don't worry about it, I'll handle it before landing.

@@ +830,5 @@
>    /**
> +   * Listener handling a key down event on the view.
> +   */
> +  _onViewKeyDown: function(e) {
> +    let item = this.getFocusedItem();

Might want to call getFocusedItem() inside the switch statement, for a little bit of optimization.

@@ +833,5 @@
> +  _onViewKeyDown: function(e) {
> +    let item = this.getFocusedItem();
> +
> +    switch (e.keyCode) {
> +      case e.DOM_VK_C:

Nit: This should really be better off as a simple conditional for now:
> if (e.keyCode == e.DOM_VK_C)

@@ +837,5 @@
> +      case e.DOM_VK_C:
> +        // Copy current selection to clipboard.
> +        if (e.ctrlKey || e.metaKey) {
> +          clipboardHelper.copyString(
> +            item._name.getAttribute("value")

You can use item._nameString instead of querying an attribute.

@@ +839,5 @@
> +        if (e.ctrlKey || e.metaKey) {
> +          clipboardHelper.copyString(
> +            item._name.getAttribute("value")
> +            + item.separatorStr
> +            + item._valueLabel.getAttribute("value")

Idem here, use item._valueString.

Comment 8

[brian_satchwannabe]

Hey, so should I re-submit a patch to fix up the latest comments or leave it as is. I got a separate mail saying that the patch had been approved so I'm not sure if I should re-submit or not. I'll be on #introduction today (irishshagua) if you get a change maybe you can ping me.

Comment 9

[Victor Porof [:vporof][:vp]]

"r+ with comments addressed" usually means the patch is fine, but there are a few small things that should be addressed before landing. Once you upload a new patch, it'll be marked for landing. The two emails you received are a bugzilla artifact, meaning the same thing :)

Comment 10

[brian_satchwannabe]

Ok sweet, I'll try and resubmit after work tonight.
Thanks again for all the guidance on this...

Comment 11

[Victor Porof [:vporof][:vp]]

Thank *you* for all the work! This was a much needed feature, and you nailed it pretty much instantly and without much help from me ;)

Comment 12

[brian_satchwannabe]

Attachment: Ctrl-C to copy selected item in VariablesView

Resubmitting the patch to address comments. Hopefully third times a charm.

Comment 13

[Victor Porof [:vporof][:vp]]

Comment on attachment 813808 [details] [diff] [review]
Ctrl-C to copy selected item in VariablesView

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

Thanks! I'll push to try to see if the test passes on all platforms, and then land it!

Congrats!

Comment 14

[Victor Porof [:vporof][:vp]]

https://tbpl.mozilla.org/?tree=Try&rev=b3021f4fc2e6

Comment 15

[Victor Porof [:vporof][:vp]]

Looks good! Landing.

Comment 16

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/22a5355a3460

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/22a5355a3460