Repro: 1) Open a website and display HUD 2) Select some text in the Console output 3) Note: Right click and there is no context menu 4) Note: Edit > Copy is disabled 5) Type Cntl-C Expected: I can copy/paste text Actual: Nothing is added to the System Clipboard

This is my first stab at solving this issue. Here are my thoughts: - The code uses the HUDService to determine the chromeWindow of the contentWindow where the HUD is placed. Then, using the getSelection() method I get the text (toString()). This may pose issues if some other part of the GUI has some selected text (where? I haven't been able to repro). This should be fixable by checking the selection object and seeing where it starts and ends, or is this not a problem? - The code uses the nsIClipboardHelper API which allows only the copying of plain-text strings. For HTML I have to use a transferable object and then define the string to be copied for text/html mime type. I saw the nsIClipboardCommands API: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIClipboardCommands#copySelection.28.29 Is that of any use in this context? How does copySelection() work? Shouldn't it take an argument so I can point it to which selection I want copied? If not, I'll just go for the stuff explained in: https://developer.mozilla.org/en/using_the_clipboard For the next version of the patch I'll work on HTML copy/paste. - The main bar Edit > Copy item still stays disabled when there's text selected in the HUD console. This is because the browser.js code (and assorted functions/methods in other scripts of the Firefox GUI) uses document.commandDispatcher.focusedWindow.getSelection(), where focusedWindow, if I am not mistaken, seems to point to the window of the web page that is loaded in the currently active tab. Making the Edit > Copy menu item enabled would perhaps require (quite) some changes in that code. Do we want to touch it? Additionally, it could become confusing if the user makes two selections: one in the HUD output and one in the page. Which selection is copied by the Edit > Copy menu item? I suggest leaving this as-is. I am not sure if it is correct to make the Edit > Copy item enabled. - The makeHUDNodes() method is now changed such that it also creates a context menu that is used for the HUD output node. I set a dynamically generated ID for the menupopup. Does this change during the lifetime of the HUD? I see that the reattachConsole() method expects that hudId can change. Should I also update the ID of the menupopup? - Is the solution for Ctrl-C support good enough? Would it be better to use XUL <key>?

(In reply to comment #2) > > - The code uses the HUDService to determine the chromeWindow of the > contentWindow where the HUD is placed. Then, using the getSelection() method I > get the text (toString()). This may pose issues if some other part of the GUI > has some selected text (where? I haven't been able to repro). This should be > fixable by checking the selection object and seeing where it starts and ends, > or > is this not a problem? that should not be a problem, as the chrome bits in the output node will be focused separately from anything in content. As long as you have the eventHandler set for copy it will work. > > - The code uses the nsIClipboardHelper API which allows only the copying of > plain-text strings. For HTML I have to use a transferable object and then > define > the string to be copied for text/html mime type. > > I saw the nsIClipboardCommands API: > > https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIClipboardCommands#copySelection.28.29 > > Is that of any use in this context? How does copySelection() work? Shouldn't it > take an argument so I can point it to which selection I want copied? I assume it uses nsIFocusManager to determine what is currently focused. not sure. lemme cc Enn on this bug. > - The main bar Edit > Copy item still stays disabled when there's text selected > in the HUD console. This is because the browser.js code (and assorted > functions/methods in other scripts of the Firefox GUI) uses > document.commandDispatcher.focusedWindow.getSelection(), where focusedWindow, > if > I am not mistaken, seems to point to the window of the web page that is loaded > in the currently active tab. Making the Edit > Copy menu item enabled would > perhaps require (quite) some changes in that code. Do we want to touch it? > I would not worry about that right now. It may end up that the console gets its own toolbar menu with an edit menu, especially since it will need to pop out and be its own window - as an option. > Additionally, it could become confusing if the user makes two selections: one > in > the HUD output and one in the page. Which selection is copied by the Edit > > Copy menu item? I suggest leaving this as-is. > As I understand it the focusManager allows only one point of *focus* at a time. > I am not sure if it is correct to make the Edit > Copy item enabled. > probably only when there is something selected in the console output. > > - The makeHUDNodes() method is now changed such that it also creates a context > menu that is used for the HUD output node. I set a dynamically generated ID for > the menupopup. Does this change during the lifetime of the HUD? no it does not. > I see that the > reattachConsole() method expects that hudId can change. Should I also update > the > ID of the menupopup? no, it does not expect the id to change - and for that tab it never will, it expects that a new contentWindow is generated. Each load of a page destroys the existing contentWindow and creates a new one. > > - Is the solution for Ctrl-C support good enough? Would it be better to use XUL > <key>? hmmm, i'll look into that.

enn: anything to worry about in comment 3? ddahl

Comment on attachment 455494 [details] [diff] [review] initial patch >+ this.chromeWin = HUDService.getChromeWindowFromContentWindow(this.contentWindow); please use this.chromeWindow >+ this.outputNode.setAttribute("context", "hud-" + this.hudId + "-output-contextmenu"); you do not need hud- as a prefix, it already has hud_ >+ this.copyOutputMenuItem = this.makeXULNode("menuitem"); >+ this.copyOutputMenuItem.setAttribute("label", this.getStr("copyCmd.label")); >+ this.copyOutputMenuItem.setAttribute("accesskey", this.getStr("copyCmd.accesskey")); >+ this.copyOutputMenuItem.setAttribute("key", "key_copy"); >+ >+ this.contextMenu = this.makeXULNode("menupopup"); >+ this.contextMenu.setAttribute("id", "hud-" + this.hudId + "-output-contextmenu"); remove "hud-" >+ this.contextMenu.addEventListener("popupshowing", >+ function () { self.contextMenuShowing() }, false); all indents should be 2 spaces. >+ this.contextMenu.addEventListener("command", >+ function () { self.copyOutputSelection() }, false); >+ this.contextMenu.appendChild(this.copyOutputMenuItem); >+ go through all indention and make sure it is 2 spaces:) >+ /** >+ * Retrieve the outputNode selection. >+ */ >+ getOutputSelection: function HUD_getOutputSelection () >+ { >+ var sel = this.chromeWin.getSelection(); use selection = >+ if (!sel || sel.isCollapsed) { >+ return ''; >+ } >+ >+ return sel.toString(); >+ }, >+ >+ /** >+ * The popupshowing event handler for the outputNode context menu. >+ */ >+ contextMenuShowing: function HUD_contextMenuShowing () >+ { >+ var sel = this.getOutputSelection(); use selection = >+ if (sel) { >+ this.copyOutputMenuItem.setAttribute("disabled", "false"); >+ } else { all "else"'s on newline >+ this.copyOutputMenuItem.setAttribute("disabled", "true"); >+ } >+ }, >+ >+ /** >+ * Perform a copy of the selected text into the clipboard. >+ */ >+ copyOutputSelection: function HUD_copyOutputSelection () nit: no space after function name: HUD_copyOutputSelection() - go back through all methods and fix >+ { >+ if (!clipboardHelper) { >+ return; >+ } >+ >+ var sel = this.getOutputSelection(); >+ if (sel) { use "selection" - it is clear what sel is, but verbose is better in most cases >+ clipboardHelper.copyString(sel); >+ } >+ }, >+ >diff -r 732bb261eb4b toolkit/locales/en-US/chrome/global/headsUpDisplay.properties >--- a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties Wed Jun 30 15:00:00 2010 -0700 >+++ b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties Thu Jul 01 21:07:37 2010 +0300 >@@ -30,8 +30,10 @@ tipWarn=Toggle console.warn logging > btnLog=Log > tipLog=Toggle console.log logging > btnGlobal=Global Messages > tipGlobal=Toggle Global Message logging > localConsole=Local Console > btnClear=Clear Console > tipClear=Clear the console output > stringFilterClear=Clear Filter >+copyCmd.label=Copy >+copyCmd.accesskey=C This look pretty good. re-submit with changes - I will review it again. You should write some tests too. The tests are all in hudservice/tests/browser/browser_HUDServiceTestsAll.js search mxr for "synthesizeMouseEvent" for example tests that do right-clicking. Very cool.

Also: the testing guide is here: https://developer.mozilla.org/en/Browser_chrome_tests I just pushed a new patch as well, so you should pull before continuing.

This patch is not the right way to do this. You should not be rolling your own clipboard handling code. Is the selection being made in a separate window? If so, adding editMenuOverlay.xul as an overlay should just make this work (along with the appropriate menus defined). If this is in a child frame, the solution is probably something similar. Otherwise, is there something special about the selection itself?

(In reply to comment #7) > This patch is not the right way to do this. You should not be rolling your own > clipboard handling code. > D'oh! > Is the selection being made in a separate window? If so, adding > editMenuOverlay.xul as an overlay should just make this work (along with the > appropriate menus defined). If this is in a child frame, the solution is > probably something similar. > > Otherwise, is there something special about the selection itself? The console is inserted into the tab's notificationBox, so it is part of the tab itself. There is no child window or frame. Is there a predefined eventHandler function in the overlay to add to the console outputNode?

Is the text being selected defined in a <description>? Have you made it focusable? (using -moz-user-focus: normal) If not, the clipboard commands will be using whatever is focused instead.

(In reply to comment #9) > Is the text being selected defined in a <description>? Have you made it > focusable? (using -moz-user-focus: normal) If not, the clipboard commands will > be using whatever is focused instead. the nodes in question are divs, and are set in css to be selectable

This is the updated patch. I have rebased my local branch and this patch should apply fine on top of the default branch. Thanks ddahl for your review and for the answers to my questions. I made the changes you requested. I hope I haven't missed anything. Regarding the nsIFocusManager: I haven't found any documentation about this. I also haven't found the implementation of cmd_copy - which is associated to key_copy and menu_copy. Thanks to Neil as well. He is right, the output nodes currently have only -moz-user-select: text, which is not sufficient. I have changed the HUD stylesheets to include -moz-user-focus: normal as well. The stylesheet change makes the Edit > Copy menu item to work, and it makes Ctrl-C to work as well. Additionally, now users can't have two simultaneous selections in the content and in the console output, so it's less confusing. My updated patch no longer implements its own keypress event handler for Ctrl-C. Also, the code does no longer need its own clipboard copy implementation. The code now only provides a context menu item, to allow the user quick access to the copy option. Lastly, I have included a new test function in the browser chrome test code for the HUD console. I haven't tested the actual clipboard copy because that's up to other functions outside the HUD console now - I only tested the context menu functionality. Please let me know if that's fine. Thanks!

I think this patch looks pretty decent, but requesting review from Mossop and some feedback from Neil.

Comment on attachment 455766 [details] [diff] [review] patch update 2 This seems ok. The more correct way though is to use <menuitem command="cmd_copy" ...> and the disabled state should update by itself when there is a selection. You'll need to add your own controller if you specifically want to check if the selection is within your block of text. But you might not care, as the content area doesn't check this.

Neil: Before submitting the updated patch I tested with <menuitem command="cmd_copy">, however that did not affect the state. It does not update by itself when there is a selection. It's always enabled. Should I update the patch to use command="cmd_copy"?

This patch is the same as "patch update 2". The only difference is that this patch applies cleanly on the mozilla-central default branch.

(In reply to comment #14) > Neil: Before submitting the updated patch I tested with <menuitem > command="cmd_copy">, however that did not affect the state. It does not update > by itself when there is a selection. It's always enabled. > > Should I update the patch to use command="cmd_copy"? Yes.

Neil: Could you please elaborate? I am new to the Gecko API and XUL. When I put command=cmd_copy the menuitem always stays enabled, even if I have my popupshowing event handler to disable it.

I tried the latest patch and it works ok for me. Why do you need to disable the command manually? + if (this.outputNode == ancestor || + this.outputNode.compareDocumentPosition(ancestor) & 16) { + disabled = "false"; + } As I was looking at the code, I wondered what the 16 was here. Better to use the constant instead. I'm guessing that you want to use Node.DOCUMENT_POSITION_CONTAINED_BY here. As an side note to ddahl, the headsUpDisplay.css files should really be in the 'console' subdirectory.

Neil: if I do not disable the menuitem manually, the item stays enabled at all times, even if there is no selection at all, even if Edit > Copy is disabled. I did not use Node.DOCUMENT_POSITION_CONTAINED_BY because Node is not in the global scope. I could use this.outputNode.DOCUMENT_POSITION_CONTAINED_BY. I'll attach a patch with command=cmd_copy, and with this.outputNode.DOCUMENT_POSITION_CONTAINED_BY instead of 16.

This is the updated patch with the changes requested (applies cleanly on mozilla-central). The menuitem now has command=cmd_copy, and the popupshowing event handler uses the DOCUMENT_POSITION_CONTAINED_BY constant. However, please note that the event handler has no effect now - I don't know why the change from oncommand="..." to command=cmd_copy causes the menuitem to always stay enabled (on my system). Please test and let me know if this is fine. Thanks for your feedback!

> I did not use Node.DOCUMENT_POSITION_CONTAINED_BY because Node is not in the > global scope. I could use this.outputNode.DOCUMENT_POSITION_CONTAINED_BY. Hint. The following should work: Components.interfaces.nsIDOM3Node.DOCUMENT_POSITION_CONTAINED_BY See <http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/core/nsIDOM3Node.idl>

Comment on attachment 457874 [details] [diff] [review] patch update 3 Please review this patch. It includes the changes that were requested. Thanks!

Comment on attachment 457874 [details] [diff] [review] patch update 3 >diff -r 96de199027d7 toolkit/components/console/hudservice/HUDService.jsm >+ contextMenuShowing: function HUD_contextMenuShowing() >+ { >+ var disabled = "true", selection = this.chromeWindow.getSelection(); >+ >+ // make sure the selected range is within the outputNode. >+ if (selection && !selection.isCollapsed && selection.rangeCount) { >+ let ancestor = selection.getRangeAt(0).commonAncestorContainer; >+ if (this.outputNode == ancestor || >+ this.outputNode.compareDocumentPosition(ancestor) & >+ this.outputNode.DOCUMENT_POSITION_CONTAINED_BY) { >+ disabled = "false"; >+ } >+ } >+ >+ this.copyOutputMenuItem.setAttribute("disabled", disabled); Just set this.copyOutputMenuItem.disabled. Also use booleans rather than strings. >diff -r 96de199027d7 toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js >+function testClipboardCopy() >+{ >+ var HUD = HUDService.hudWeakReferences[hudId].get(); >+ var chromeWindow = HUD.chromeWindow; >+ var chromeDocument = HUD.chromeDocument; >+ var selection = chromeWindow.getSelection(); >+ var console = tab.linkedBrowser.contentWindow.wrappedJSObject.console; >+ >+ if (selection.rangeCount > 0) { >+ selection.removeAllRanges(); >+ } >+ >+ // test 1: check that the copyOutputMenuItem is disabled >+ HUD.contextMenuShowing(); >+ is(HUD.copyOutputMenuItem.getAttribute("disabled"), "true", >+ "HUD.copyOutputMenuItem is disabled"); >+ >+ console.log("Hello world!"); >+ >+ executeSoon(function () { >+ var range = chromeDocument.createRange(); >+ range.selectNode(HUD.outputNode.firstChild); >+ selection.addRange(range); >+ >+ // test 2: check that the copyOutputMenuItem is enabled, because a node is >+ // selected. >+ HUD.contextMenuShowing(); >+ is(HUD.copyOutputMenuItem.getAttribute("disabled"), "false", >+ "HUD.copyOutputMenuItem is enabled"); >+ >+ selection.removeAllRanges(); This test isn't actually testing that clipboard copying works.

David: thanks for your review! For me this.copyOutputMenuItem.disabled did not work. I had to use setAttribute("disabled", true) to actually see the menuitem disabled. Anyhow, I'll change the patch to use the .disabled property, as desired. Regarding the testClipboardCopy() - indeed, the function name is misleading. The initial intent was to test clipboard copy, BUT given the solution suggested by Neil, we just reused the global cmd_copy command, which I presume has its own test code in some other place. Thus, the only remaining test code was for the popupmenushowing event handler. What do you suggest I do with the test code? Shall I rename the function to testCopyOutputMenu()? Or something else?

Requesting blocking status for Firefox 4 for this bug, because the Web Console really needs the ability to copy output in order to be useful for webdevs looking to gather information about problems with their applications.

(In reply to comment #24) > What do you suggest I do with the test code? Shall I rename the function to > testCopyOutputMenu()? Or something else? Yeah, rename that test for sure.

Updated patch. Rebased to the latest mozilla-central, and made the changes requested: - the code uses copyOutputMenuItem.disabled - the property, no longer the element attribute. - the test function is renamed.

This is an essential design element of the console, blocking:betaN+. Sooner the better.

(In reply to comment #18) > As an side note to ddahl, the headsUpDisplay.css files should really be in the > 'console' subdirectory. The latest patches establishing where the css files live was created by gavin and dao, can you explain why? Everyone else says they belong in toolkit/themes

Mihai: since you have already had this patch in review, you should continue to do so with gavin or mossop.

(In reply to comment #29) > The latest patches establishing where the css files live was created by gavin > and dao, can you explain why? Everyone else says they belong in toolkit/themes The same place as they are now has a subdirectory called 'console' where other console related theme files live. Not a real issue, but since the content files for both the HUD console and the normal error console are in the same place, I had expected to find the theme files in the same place as well and had to look around for them.

I don't understand why the custom enabling/disabling code is needed - this patch seems to work fine without it. It requires refactoring the test, though, and I had some difficulties with that. The console seems to not be initialized when my test runs (HUD.chromeWindow is null), and I don't understand why. Can one of you fix it?

Comment on attachment 461408 [details] [diff] [review] patch >diff --git a/toolkit/themes/winstripe/global/headsUpDisplay.css b/toolkit/themes/winstripe/global/headsUpDisplay.css > .hud-output-node div { > -moz-user-select: text; > white-space: pre-wrap; >+ -moz-user-focus: normal; > } All of these styles (and the similar ones for ".jsterm-output-node div") should be outside of the theme. Adding a content/ CSS file is a bit of a pain given how this is currently dynamically loaded, though, so maybe we should just put them inline somehow (or set them with script when the elements are added?). Can be a followup.

Comment on attachment 461408 [details] [diff] [review] patch Thanks for looking into my patch, Gavin! Please allow me to explain how my patch came into being, so that we both understand the current situation: - My patch came with a context menu that holds the copy output menu item having the oncommand="goDoCommand('cmd_copy')" attribute. I did that initially instead of command="cmd_copy", because in testing I saw that the menu item is never disabled, irrespective of the selection state (collapsed or not). Neil correctly asked for command="cmd_copy", but I am still at odds *why* the context menuitem always stays enabled. Can anyone confirm? - Another choice I made is to use setAttribute("disabled", "true"/"false") in the popupshowing event handler I had (the contextMenuShowing() method). Again, this was because menuitem.disabled = true did not work for me. Weird. Once again, I was correctly asked to switch to the use of menuitem.disabled property. Maybe it's a problem only on my system. The popupshowing event handler came into being only because I wanted to enable/disable the menuitem when there was a selection (or not) inside the outputNode. The effect of the event handler came to be moot with the above two changes. Hence you are now correct in saying that the event handler is no longer needed. The functionality works fine, but I wanted the menuitem to be enabled/disabled appropriately. Regarding the patch you submitted: - HUD.chromeWindow is null indeed, but it's unused by the test code you submitted. I could not reproduce the error. - HUD.contextMenu is null and it causes failure on execution. This happens because the HUD.reattachConsole() code does not set the contextMenu property to point to the element we want in the test code. I fixed both of these issues, and I'll provide a patch in a few moments. - Another problem that cropped out now is that the EventUtils.sendMouseEvent() method has no effect, which means the popupshowing event handler is never trigger, so ... the test never completes (it times out). I bumped into this issue in bug 580030 - I had no solution for it. It is weird that I can nicely use the EventUtils methods Inspector tests, but not in WebConsole ones - perhaps the state we are in, in the WebConsole tests, is really weird and it makes the methods fail silently?

Attaching the updated patch, as explained in the previous comment. Please note that the patch fails because the popupshowing event handler is never invoked. I don't know why.

(In reply to comment #34) > Hence you are now correct in saying that the event handler is no longer needed. > The functionality works fine, but I wanted the menuitem to be enabled/disabled > appropriately. With my patch applied, testing on Mac, the menuitem is disabled/enabled correctly (enabled when there is a selection, disabled otherwise). Are you saying that you're seeing otherwise?

(In reply to comment #36) > (In reply to comment #34) > > Hence you are now correct in saying that the event handler is no longer needed. > > The functionality works fine, but I wanted the menuitem to be enabled/disabled > > appropriately. > > With my patch applied, testing on Mac, the menuitem is disabled/enabled > correctly (enabled when there is a selection, disabled otherwise). Are you > saying that you're seeing otherwise? I am really glad we are now getting somewhere. :) On Ubuntu 10.04 Linux, Gnome, I see otherwise. The context menuitem is always enabled. That is why I did the popupshowing event handler, and that is why I wanted the oncommand attribute, and that is why I wanted the setAttribute("disabled"). Also, does the EventUtils.sendMouseEvent() method work for you in the patch I provided?

(In reply to comment #37) > On Ubuntu 10.04 Linux, Gnome, I see otherwise. The context menuitem is always > enabled. I do see that it's always enabled if there is a selection anywhere in the window (including in the text box), which may explain what you're seeing? The test ends up stalling with some text in the console text box selected. > Also, does the EventUtils.sendMouseEvent() method work for you in the patch I > provided? The test doesn't get that far: Error: "HUD.copyOutputMenuItem is undefined"

(In reply to comment #38) > (In reply to comment #37) > > On Ubuntu 10.04 Linux, Gnome, I see otherwise. The context menuitem is always > > enabled. > > I do see that it's always enabled if there is a selection anywhere in the > window (including in the text box), which may explain what you're seeing? The > test ends up stalling with some text in the console text box selected. No, that is not the case. I see the context menuitem enabled even when Edit > Copy is disabled, when I have *no* text selected in any place in the whole chrome window (and document window). > > Also, does the EventUtils.sendMouseEvent() method work for you in the patch I > > provided? > > The test doesn't get that far: > Error: "HUD.copyOutputMenuItem is undefined" You got further than I do. HUD.copyOutputMenuItem is inside the first event handler. Hah, that's funny. So, the event handler is invoked on your system. It's not, on mine. The HUD.copyOutputMenuItem is undefined once again due to the reattachConsole() issue. So, shall I update the patch to include the menuitem as well? What do I do about the event handler not being invoked on my system? It's a must, for me, to be able to do WebConsole dev and test work.

(In reply to comment #39) > The HUD.copyOutputMenuItem is undefined once again due to the reattachConsole() > issue. So, shall I update the patch to include the menuitem as well? > > What do I do about the event handler not being invoked on my system? It's a > must, for me, to be able to do WebConsole dev and test work. Perhaps you should mark this bug as dependent on bug 580030 ? or merge them?

sorry, didn't realize that 580030 landed already

mak: do you know how to fix the test on this patch? it keeps timing out. Mouse events!:)

is it possble to programatically select a menuitem instead of using mouse events?

Assuming you are talking about this: > EventUtils.sendMouseEvent({type:'click', button: 2}, HUD.outputNode, > HUD.chromeWindow); This doesn't simulate a mouse click, it just causes any 'click' event listeners to fire. You want to use: synthesizeMouse(element, x, y, { type : "contextmenu", button: 2 }); which fires the 'contextmenu' event that I think this test is really looking for, and it also causes native event handling to occur.

Also, you want to listen for popupshown, not popupshowing: HUD.contextMenu.addEventListener("popupshowing", function () { as the disabled state isn't updated until then.

Given the complications we're seeing with the context menu approach and testing, this patch just adds the -moz-user-focus: normal rule. That's enough to make it possible to copy with Cmd/Ctrl+C, which is a good first step at least, while we try to figure out how to also add the context menu. I took the opportunity to move these rules out of the theme (and attempt to use a single class - "hud-output-node" - to identify all "output nodes").

Oh, I hadn't seen Neil's comments before making mine. If addressing his comments fixes all the issues with the other patch, let's do that.

Comment on attachment 462827 [details] [diff] [review] simpler patch I agree, we can go for the simpler patch, but I think the style changes you made are beyond the purpose of this bug. I just tested Neil's suggestions, and once again he's right. Dude, thank you! I'll attach an updated patch now.

This is the updated patch. Changes: - included the copyOutputMenuItem in reattachConsole, to make the test code work fine when the event handlers are invoked. - made the changes suggested by Neil: synthesizeMouse with type:contextmenu, and the event listeners are now added for the popupshown event. Result: the test code works fine, but it fails on my system. It's the usual story: the menuitem is always enabled on my system.

Comment on attachment 462899 [details] [diff] [review] updated patch I confirmed that this is broken on Linux for some reason. The test also fails. Obviously we can't land it like that - perhaps we should disable the test on linux and file a followup on figuring this out? Or just take my alternate patch that just enables Cmd/Ctrl+C.

David Dahl agreed that we should disable the test on Linux and file a follow-up report on the issue of the menuitem not being disabled on Linux. I will do that tomorrow. Thanks!

As discussed, I updated the patch to skip the failing test on Linux - I asked on #developers how to best do this, and Mak told me to use navigator.platform. I reported a follow-up bug 584972 about the menuitem being always enabled. I also made an adjustment to the test code: the contextmenu is now closed with the synthetic Escape key, such that we are sure to show the contextmenu for the second time. This patch is now rebased on the yesterday's evening mozilla-central default branch. It should merge cleanly.

Comment on attachment 463478 [details] [diff] [review] updated patch + test code Changing reviewers because Gavin is away.

Comment on attachment 463478 [details] [diff] [review] updated patch + test code To see review comments with expandable code context, please see http://reviews.visophyte.org/r/463478/. on file: toolkit/components/console/hudservice/HUDService.jsm line 1856 > consoleWrap.appendChild(this.outputNode); > outerWrap.appendChild(this.contextMenu); > outerWrap.appendChild(consoleWrap); I feel like order matters here, but it's not immediately clear why in the code. Please add a comment to the code explaining this. on file: toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js line 24 > * Mihai Șucan <mihai.sucan@gmail.com> looks like encoding fail somehwere on file: toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js line 683 > // skip the test on Linux, because the menu is always enabled. nit: if you end in a period, please start with a capital letter. In general, I prefer to see complete sentences in comments (helps people who learned English as a second language). With that said, gavin disagrees with me :) r=sdwilsh

Thanks for the r+, Shawn! I have made the styling changes for the comments, as suggested.

This is the rebased patch for mozilla-central checkin. Please first checkin the patch from bug 582201 - to make sure the patch applies cleanly. Thanks!

Comment on attachment 464368 [details] [diff] [review] [checked-in] rebased patch + test code http://hg.mozilla.org/mozilla-central/rev/2d6999995d6c

fix checked in on bug 585951 to disable copy menu test code. Was causing oranges on windows.

It looks like that bug remove the test entirely. Is there a bug filed on adding it back so that we have reliable test coverage here?

Did gavin mean to revert this to UNCO?

I don't think so. Mihai, can you file a follow-up bug to re-enable the failing test or a similar one that works?

I'm removing the dependency on bug 584972 and bug 586386 because those are follow up bugs, not dependencies.