574036 - HUD Console should allow copying text

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Austin King [:ozten]]

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

Comment 2

[Mihai Sucan [:msucan]]

Attachment: initial patch

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>?

Comment 3

[David Dahl :ddahl]

(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.

Comment 4

[David Dahl :ddahl]

enn:

anything to worry about in comment 3?

ddahl

Comment 5

[David Dahl :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.

Comment 6

[David Dahl :ddahl]

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.

Comment 7

[Neil Deakin]

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?

Comment 8

[David Dahl :ddahl]

(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?

Comment 9

[Neil Deakin]

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.

Comment 10

[David Dahl :ddahl]

(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

Comment 11

[Mihai Sucan [:msucan]]

Attachment: patch update 2

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!

Comment 12

[Rob Campbell [:rc] (:robcee)]

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

Comment 13

[Neil Deakin]

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.

Comment 14

[Mihai Sucan [:msucan]]

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"?

Comment 15

[Mihai Sucan [:msucan]]

Attachment: patch update 2 for mozilla-central

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

Comment 16

[Rob Campbell [:rc] (:robcee)]

(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.

Comment 17

[Mihai Sucan [:msucan]]

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.

Comment 18

[Neil Deakin]

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.

Comment 19

[Mihai Sucan [:msucan]]

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.

Comment 20

[Mihai Sucan [:msucan]]

Attachment: patch update 3

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!

Comment 21

[Philip Chee]

> 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 22

[Mihai Sucan [:msucan]]

Comment on attachment 457874 [details] [diff] [review]
patch update 3

Please review this patch. It includes the changes that were requested. Thanks!

Comment 23

[Dave Townsend [:mossop]]

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.

Comment 24

[Mihai Sucan [:msucan]]

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?

Comment 25

[Kevin Dangoor]

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.

Comment 26

[David Dahl :ddahl]

(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.

Comment 27

[Mihai Sucan [:msucan]]

Attachment: patch update 4

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.

Comment 28

[Johnathan Nightingale [:johnath]]

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

Comment 29

[David Dahl :ddahl]

(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

Comment 30

[David Dahl :ddahl]

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

Comment 31

[Neil Deakin]

(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.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

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 33

[:Gavin Sharp [email: [e-mail]]]

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 34

[Mihai Sucan [:msucan]]

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?

Comment 35

[Mihai Sucan [:msucan]]

Attachment: updated patch

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.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

(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?

Comment 37

[Mihai Sucan [:msucan]]

(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?

Comment 38

[:Gavin Sharp [email: [e-mail]]]

(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"

Comment 39

[Mihai Sucan [:msucan]]

(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.

Comment 40

[David Dahl :ddahl]

(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?

Comment 41

[David Dahl :ddahl]

sorry, didn't realize that 580030 landed already

Comment 42

[David Dahl :ddahl]

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

Comment 43

[David Dahl :ddahl]

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

Comment 44

[Neil Deakin]

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.

Comment 45

[Neil Deakin]

Also, you want to listen for popupshown, not popupshowing:

HUD.contextMenu.addEventListener("popupshowing", function () {

as the disabled state isn't updated until then.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Attachment: simpler patch

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").

Comment 47

[:Gavin Sharp [email: [e-mail]]]

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 48

[Mihai Sucan [:msucan]]

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.

Comment 49

[Mihai Sucan [:msucan]]

Attachment: updated patch

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 50

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 51

[Mihai Sucan [:msucan]]

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!

Comment 52

[Mihai Sucan [:msucan]]

Attachment: updated patch + test code

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 53

[Kevin Dangoor]

Comment on attachment 463478 [details] [diff] [review]
updated patch + test code

Changing reviewers because Gavin is away.

Comment 54

[Shawn Wilsher :sdwilsh]

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 <[e-mail]>

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

Comment 55

[Mihai Sucan [:msucan]]

Attachment: updated patch + test code

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

Comment 56

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch + test code

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 57

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 464368 [details] [diff] [review]
[checked-in] rebased patch + test code

http://hg.mozilla.org/mozilla-central/rev/2d6999995d6c

Comment 58

[Rob Campbell [:rc] (:robcee)]

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

Comment 59

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 60

[Johnathan Nightingale [:johnath]]

Did gavin mean to revert this to UNCO?

Comment 61

[Rob Campbell [:rc] (:robcee)]

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?

Comment 62

[Kevin Dangoor]

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