Closed
Bug 862019
Opened 12 years ago
Closed 11 years ago
Allow Ctrl-C to copy the selected header name and value
Categories
(DevTools :: Netmonitor, defect, P3)
DevTools
Netmonitor
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 27
People
(Reporter: msucan, Assigned: brian_satchwannabe)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
5.79 KB,
patch
|
vporof
:
review+
|
Details | Diff | Splinter Review |
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).
Updated•12 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•11 years ago
|
||
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.
Attachment #811747 -
Flags: review?(vporof)
Comment 2•11 years ago
|
||
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?
Attachment #811747 -
Flags: review?(vporof) → feedback+
Assignee | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
(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
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #811747 -
Attachment is obsolete: true
Attachment #813385 -
Flags: review?(vporof)
Assignee | ||
Comment 6•11 years ago
|
||
(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•11 years ago
|
||
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.
Attachment #813385 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
"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 :)
Assignee | ||
Comment 10•11 years ago
|
||
Ok sweet, I'll try and resubmit after work tonight. Thanks again for all the guidance on this...
Comment 11•11 years ago
|
||
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 ;)
Assignee | ||
Comment 12•11 years ago
|
||
Resubmitting the patch to address comments. Hopefully third times a charm.
Attachment #813385 -
Attachment is obsolete: true
Attachment #813808 -
Flags: review?(vporof)
Comment 13•11 years ago
|
||
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!
Attachment #813808 -
Flags: review?(vporof) → review+
Comment 16•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/22a5355a3460
Assignee: nobody → brian_satchwannabe
Whiteboard: [fixed-in-fx-team]
Updated•11 years ago
|
Status: NEW → ASSIGNED
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/22a5355a3460
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•