1265485 - (Devtools) Browser Console keyboard shortcut does not work

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[kevink9876543]

OS: Linux x86_64 (but Windows users see this as well)

Current SeaMonkey Nightly sets Ctrl-Shift-J for both Browser Console & Error Console, but Error Console takes precedence, leaving Browser Console without a keyboard shortcut.

Prior version had set Ctrl-Shift-Z for Browser Console, but that keyboard shortcut did not work either (appears to did nothing), probably because that's the keyboard shortcut for "Redo".

Comment 1

[Philip Chee]

A recent change in the devtools code is that all menu items are dynamically generated so the keyboard short cut keys are overriding our XUL defined shortcuts so we need to override /devtools/client/locales/en-US/menus.properties with our own.

Plan of action:
1. Create a menus.properties file and put it in suite/locales/en-US/chrome/browser/
2. In suite/locales/jar.mn add an override:
   override chrome://devtools/locale/menus.properties chrome://navigator/locale/menus.properties
3. You also need to add a line in that jar.mn file like:
   locale/@AB_CD@/navigator/menus.properties (%chrome/browser/menus.properties)

Comment 2

[kevink9876543]

Attachment: bug1265485.patch

Couple notes about this patch:
1) the menus.properties was copied from mozilla/devtools/client/locales/en-US then edited
2) apparently we can't use just any unassigned hotkey here - I had initially tried to use Ctrl-Shift-Y, but it did nothing.  So went with Ctrl-Shift-O which WFM.

Comment 3

[Masahiko Imanaka [:marsf]]

http://hg.mozilla.org/releases/comm-aurora/rev/70c2c9d7851e#l8.20
> <!ENTITY browserConsoleCmd.commandkey    "Ζ">

I have noticed the "Ζ"(0x396) is the Greek alphabet, is not latin "Z" (0x5a).
It shouldn't be used in en-US locale.

Comment 4

[Philip Chee]

(In reply to Masahiko Imanaka [:marsf] from comment #3)
> http://hg.mozilla.org/releases/comm-aurora/rev/70c2c9d7851e#l8.20
> > <!ENTITY browserConsoleCmd.commandkey    "Ζ">
> 
> I have noticed the "Ζ"(0x396) is the Greek alphabet, is not latin "Z" (0x5a).
> It shouldn't be used in en-US locale.

Congratulations! You win a no-prize!
http://goodcomics.comicbookresources.com/wp-content/uploads/2016/02/no-prize.jpg

Comment 5

[Filipus Klutiero]

Ctrl+Shift+J, which worked with Firefox 47, opens page source starting from version 48.
This should not be assigned to SeaMonkey.

Comment 6

[kevink9876543]

Attachment: bug1265485.patch 2

With Error Console being part of comm-* now, here's a patch that instead makes *its* keyboard shortcut Ctrl-Shift-O.  This frees up Ctrl-Shift-J for the Browser Console.

Tested in 2.46 and current comm-central.

Comment 7

[Ian Neal]

Comment on attachment 8802736 [details] [diff] [review]
bug1265485.patch 2

r/a=me

Comment 8

[:aceman]

https://hg.mozilla.org/comm-central/rev/a1ac8eae4234ff0280aab2ee5fe8175598d1ef57

Comment 9

[kevink9876543]

Comment on attachment 8802736 [details] [diff] [review]
bug1265485.patch 2

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Can't access Browser Console by keyboard shortcut
Testing completed (on m-c, etc.): landed on c-c, tested locally w/ 2.46
Risk to taking this patch (and alternatives if risky): low risk
String changes made by this patch: none

Comment 10

[Philip Chee]

(In reply to kevink9876543 from comment #9)
> Comment on attachment 8802736 [details] [diff] [review]
> bug1265485.patch 2
> 
> [Approval Request Comment]
> Regression caused by (bug #): 
> User impact if declined: Can't access Browser Console by keyboard shortcut
> Testing completed (on m-c, etc.): landed on c-c, tested locally w/ 2.46
> Risk to taking this patch (and alternatives if risky): low risk
> String changes made by this patch: one string change. Only an accelerator key
> which each locale translates differently anyway.

Comment 11

[kevink9876543]

Comment on attachment 8802736 [details] [diff] [review]
bug1265485.patch 2

Moving approval request to comm-beta now that the 52 cycle has become aurora.

Comment 12

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-beta/rev/a2dfa6e67d4f714f0e6f7ac3aabde1505f4bbb61

f- for IanN :) Tabs in the file and patch at the exact location. But already in c-c so...

Comment 13

[Rainer Bielefeld]

Fix causes "Bug 1326258 - Paste As Quotation shortcut conflicts with Error Console shortcut"

Comment 14

[Rainer Bielefeld]

Has impact at "Bug 1284288 - shortcuts.xhtml: adapt 'Error Console', 'DOM Inspector' and new 'FF DevTools' related contents"

Comment 15

[kevink9876543]

Oops, sorry about that!  What keyboard shortcut would you suggest using for Error Console?

Comment 16

[kevink9876543]

Attachment: bug1265485-fix.patch

Per discussion on irc with RattyAway, shortcut is changed to Accel-Alt-J

Comment 17

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8823060 [details] [diff] [review]
bug1265485-fix.patch

Kevin,

it works but by changing the modifiers you created a change impacting all languages. Localizer have no way to actually find this change.

Could you rename commandkey to commandkey2 and check if this still works. I will r+ it then.

Comment 18

[kevink9876543]

Attachment: addressed review comments

Thanks.

Comment 19

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8824633 [details] [diff] [review]
addressed review comments

Looks good. Thanks. Will check it in later if you don't have access.

Comment 20

[Frank-Rainer Grahl (:frg)]

Because I was trusted with reviewing it I just added a+ too.

https://hg.mozilla.org/comm-central/rev/78c415cf613ecfd5cbedcb580d1cf5273981f9a9

Should this go into a possible 2.49esr?

Comment 21

[kevink9876543]

Comment on attachment 8824633 [details] [diff] [review]
addressed review comments

[Approval Request Comment]

Regression caused by (bug #): this bug

User impact if declined: Cannot use "Paste as Quotation" keyboard shortcut

Testing completed (on m-c, etc.): landed in c-c, locally tested on c-a

Risk to taking this patch (and alternatives if risky): low risk

String changes made by this patch:
1) errorConsoleCmd.commandkey is renamed to errorConsoleCmd.commandkey2
2) keyboard shortcut modifiers for Error Console were changed, affecting all locales

Comment 23

[Philip Chee]

Comment on attachment 8824633 [details] [diff] [review]
addressed review comments

> [Approval Request Comment]

> String changes made by this patch:
> 1) errorConsoleCmd.commandkey is renamed to errorConsoleCmd.commandkey2
> 2) keyboard shortcut modifiers for Error Console were changed, affecting all
> locales
String changes are a  big NO-NO for comm-aurora and comm-beta and comm-release. Localizers really hate being blindsided by last late breaking changes. So given that the Browser Console can still be open via the menu this is a r- a-.

Sorry but this will have to ride the trains.

Comment 24

[Frank-Rainer Grahl (:frg)]

As an alternative because the current key clashes with another key in mail composition how about removing it from c-a and c-b?

Comment 25

[Philip Chee]

(In reply to Frank-Rainer Grahl from comment #24)
> As an alternative because the current key clashes with another key in mail
> composition how about removing it from c-a and c-b?
Yes Please.

Comment 26

[kevink9876543]

Attachment: comm-aurora & comm-beta remove Error Console keyboard shortcut

(In reply to Philip Chee from comment #25)
> (In reply to Frank-Rainer Grahl from comment #24)
> > As an alternative because the current key clashes with another key in mail
> > composition how about removing it from c-a and c-b?
> Yes Please.

If I'm understanding this correctly, here's a patch for that.

Comment 27

[Frank-Rainer Grahl (:frg)]

Attachment: 1265485-whitespace-cleanup.patch

Followup whitespace chleanup for c-c as discussed on irc (or new bug?)

Comment 28

[Frank-Rainer Grahl (:frg)]

Attachment: bug1265485-aurora-beta-fix-V2.patch

Kevin,

just had time to look at the patch. Sorry it took so long.

You missed a line with the original key so that the assigned key wouldn't interfere anywhere. Because of the removal tasksOverlay.dtd does not need a change. The unused old key can ride the train.

Becuase it was a small trivial change I patched it up myself and redirecting to Ratty also for approval. Hope this is ok with you. Tested with 2.48.

Comment 29

[kevink9876543]

Sorry, I'm still confused.  Why remove this line? -
>  accesskey="&errorConsoleCmd.accesskey;"

The key that was changed in this bug is errorConsoleCmd.*commandkey*, not errorConsoleCmd.*accesskey*.  The latter is untouched by the patches in this bug.

What am I missing?

Comment 30

[Frank-Rainer Grahl (:frg)]

Attachment: bug1265485-aurora-beta-fix-V2a.patch

Sorry you are right. Need to concentrate more... Confused accesskey with the other key. r+ because it essentially your first patch with the dtd part removed.

IanN or Ratty need to approve it.

Comment 31

[Philip Chee]

Comment on attachment 8829157 [details] [diff] [review]
bug1265485-aurora-beta-fix-V2a.patch

a=me for comm-aurora and comm-beta

Comment 32

[Philip Chee]

Comment on attachment 8829103 [details] [diff] [review]
1265485-whitespace-cleanup.patch

> Followup whitespace chleanup for c-c as discussed on irc (or new bug?)
A new bug would be nice.

> -<!ENTITY editorCmd.label					"Composer">
> -<!ENTITY editorCmd.accesskey				"c">
> -<!ENTITY editorCmd.commandkey				"4">
> +<!ENTITY editorCmd.label "Composer">
> +<!ENTITY editorCmd.accesskey "c">
> +<!ENTITY editorCmd.commandkey "4">
I would have accepted vertically aligning the "values" but this works too.

Comment 33

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-aurora/rev/4b730951b1e5c23e0065fbd19b0e5d740007425b
https://hg.mozilla.org/releases/comm-beta/rev/a762765080b9b6a6123db4f979c6fe2726504a19