366479 - [mac] Copy command is broken in the JS console

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P2)

Description

[Shawn Wilsher :sdwilsh]

When trying to paste with the keyboard shortcut, I just get a beep.  Pasting from the context menu works just fine.

Comment 1

[Phil Ringnalda (:philor)]

Works: my last build from before Cocoa widgets were turned on.
Fails: my first build after Cocoa widgets were turned on.
Works: a build from ten minutes ago, with --enable-default-toolkit=mac

To Cocoa with you, bug!

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> Works: my last build from before Cocoa widgets were turned on.
> Fails: my first build after Cocoa widgets were turned on.
> Works: a build from ten minutes ago, with --enable-default-toolkit=mac
> 
> To Cocoa with you, bug!

The strange thing is that it works in other places just fine.

Comment 3

[Asaf Romano (gone)]

So the fix for bug 291276 doesn't play nice with cocoa widget, sigh.

The edit menu items was were always disabled in the JS-console (and few other windows which only have menus on mac), but the key still worked.

Comment 4

[Asaf Romano (gone)]

Attachment: patch

Comment 5

[Asaf Romano (gone)]

Attachment: patch

easier now that bug 366992 is fixed.

Comment 6

[Asaf Romano (gone)]

Attachment: patch

Comment 7

[Asaf Romano (gone)]

Comment on attachment 255710 [details] [diff] [review]
patch

Hrm, the js console should have a cmd_copy command regardless of macBrowserOverlay (we still need the browserMountPoints.inc for other windows).

Comment 8

[Asaf Romano (gone)]

Attachment: patch

- Don't remove cmd_copy from console.xul (add it via editMenuOverlay). I did keep the macBrowserOverlay part though for the reason mentioned in the previous comment.
- Remove the keypress handler, we get one for free from browser-base.inc.

Comment 9

[Asaf Romano (gone)]

Attachment: patch

see above :-/

Comment 10

[Asaf Romano (gone)]

Attachment: don't break suiterunner

Comment 11

[[e-mail]]

Comment on attachment 255741 [details] [diff] [review]
don't break suiterunner

Note that for some unknown reason I get an xpconnect scope assertion when I close the console window with these patches applied.

Comment 13

[Shawn Wilsher :sdwilsh]

hm, this seems to be working now - not sure what fixed it.

Comment 14

[Asaf Romano (gone)]

I incidentally fixed the original issue in bug 368918.

This broke the copy command in the js console (on mac), thus morphing this bug to fix just that.

Comment 15

[Asaf Romano (gone)]

Attachment: patch

Beltzner: this removes Accel+C from the context menu of items in the error console. I figured we should do so regardless of this bug, unless you disagree ;)

Comment 16

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

Comment on attachment 256396 [details] [diff] [review]
patch

>Index: toolkit/components/console/content/console.js

> function copyItemToClipboard()
> {
>   gConsole.copySelectedItem();
> }

Looks like this function is now unused (assuming you also land the suite patch).

>Index: toolkit/components/console/content/consoleBindings.xml

>       <property name="selectedItem">
>         <getter>return this.mSelectedItem</getter>
>         <setter><![CDATA[

>+          // Update edit commands
>+          window.updateCommands("focus");

I'm not sure I understand why this is needed. Won't the global focus controller take care of updating the focus commands automatically? I guess it might only do that when the window itself is focused and has no current focused element. Is that not sufficient in this case?

Comment 17

[Asaf Romano (gone)]

No focus or blur event is triggered when the hbox is already "focused", i.e. when clicking on the empty area before selecting a row.

Comment 18

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

Comment on attachment 256396 [details] [diff] [review]
patch

Ok, r=me with or without the updateCommands call, if you file a followup about the edgecases we talked about yesterday (command not being updated properly when tabbing out of the textfield, clearing items from the console, switching panes, etc). Don't forget the first part of comment 16, either.

I don't think this needs ui-review, for what it's worth, since we're just making this menu consistent with all the other edit context menus.

Comment 19

[Asaf Romano (gone)]

Attachment: as checked in

mozilla/toolkit/components/console/content/console.js 1.6
mozilla/toolkit/components/console/content/console.xul 1.11
mozilla/toolkit/components/console/content/consoleBindings.xml 1.18