431011 - Ctrl-Shift-B (Open Library/Places Organizer) has no counterpart in Linux

Status: VERIFIED FIXED

(Firefox :: Keyboard Navigation, defect)

Description

[[:Cww]]

Ctrl-Shift-B opens the Library in Windows, but it seems really weird that it got left out of Linux.  I can't think of anything else that uses Ctrl-Shift-B so it's just annoying that a shortcut that I'm really used to doesn't work.

Comment 1

[Tracy Walker [:tracy]]

the keyboard shortcut for Bookmark all tabs (Ctrl+Shift+D) is also missing.

Comment 3

[David Dahl :ddahl]

After much newbie and advanced moz dev mxr searches the access key code for this was found here:

browser/base/content/browser-sets.inc

line 281: 
# Accel+Shift+A-F are reserved on GTK2
#ifndef MOZ_WIDGET_GTK2
...
    <key id="manBookmarkKb" key="&bookmarksSidebarCmd.commandkey;" command="Browser:ShowAllBookmarks" modifiers="accel,shift"/>
#endif

Looks like we need to pick a new key-command for linux - I have no idea what it should be.

I would like to fix this as I am on linux and working on places:)

Comment 4

[David Dahl :ddahl]

Reed:

Shawn said you might know what to assign to the key command.

Comment 5

[David Dahl :ddahl]

Attachment: added ctrl-shift k as open places command on linux

since ctrl-shift a - f are reserved by GTK (as per the comment in browser-sets.inc), I used "ctrl-shift K" Since I am working on places a lot i would like this fixed:) Any better ideas for the keyboard command?

Comment 6

[Tim Riley [:timr]]

Or maybe ctrl-shift O for _O_rganize Bookmarks.  Or ctrl-shift P for _P_laces?  I don't have  strong feeling about this.  I just don't want it to linger with an unlanded fix for a long time.

Comment 7

[David Dahl :ddahl]

Attachment: Changed patch to ctrl-shift-o to open bookmarks

How about ctrl-shift "o"? I hear some folks don't like "o" for commandkeys.

Comment 8

[David Dahl :ddahl]

Attachment: another tweak to the naming convention

As per gavin:
changed bookmarksSidebarGtkCmd to bookmarksGtkCmd to more accurately describe the open places organizer and not the sidebar.

Comment 9

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

Comment on attachment 367670 [details] [diff] [review]
another tweak to the naming convention

>diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc

> # Accel+Shift+A-F are reserved on GTK2
> #ifndef MOZ_WIDGET_GTK2
>     <key id="bookmarkAllTabsKb" key="&bookmarkThisPageCmd.commandkey;" command="Browser:BookmarkAllTabs" modifiers="accel,shift"/>
>     <key id="manBookmarkKb" key="&bookmarksSidebarCmd.commandkey;" command="Browser:ShowAllBookmarks" modifiers="accel,shift"/>
> #endif
>+#ifdef MOZ_WIDGET_GTK2

Make this an #else instead?

I don't want to scope creep this bug too much, but it would be nice if you could make the following changes to clean up our entity names a bit (make them match their actual use and associated strings), or file a followup bug to do so.

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

> <!ENTITY bookmarksSidebarCmd.accesskey  "B">

Rename this one to bookmarksButton.accesskey, and move it next to bookmarksButton.label.

> <!ENTITY bookmarksSidebarCmd.commandkey "b">

Rename this one to bookmarksCmd.commandkey, since it's used for both the sidebar and the manager.

Comment 10

[David Dahl :ddahl]

Attachment: expanded scope patch

expanded scope - tested on Linux - not sure about other platforms. I pushed the patch to the tryserver just to see how that works. I need to get my windows and mac builds going over here:)

Comment 11

[David Dahl :ddahl]

Also, not sure if all of the name changing is done - did not touch the key="viewBookmarksSidebarKb", or if this should be changed.

Comment 12

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

Comment on attachment 368133 [details] [diff] [review]
expanded scope patch

No need to change the IDs or other attributes, that will potentially affect addons for no real gain. I was really only talking about changing the entity names to better reflect their use.

Comment 13

[David Dahl :ddahl]

Attachment: trunk patch

Comment 14

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

Comment on attachment 368272 [details] [diff] [review]
trunk patch

>diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc

> #ifndef MOZ_WIDGET_GTK2
>     <key id="bookmarkAllTabsKb" key="&bookmarkThisPageCmd.commandkey;" command="Browser:BookmarkAllTabs" modifiers="accel,shift"/>
>-    <key id="manBookmarkKb" key="&bookmarksSidebarCmd.commandkey;" command="Browser:ShowAllBookmarks" modifiers="accel,shift"/>
>+    <key id="manBookmarkKb" key="&bookmarksCmd.commandkey;" command="Browser:ShowAllBookmarks" modifiers="accel,shift"/>
>+#else
>+    <key id="manBookmarkKb" key="&bookmarksGtkCmd.commandkey;" command="Browser:ShowAllBookmarks" modifiers="accel,shift"/>
> #endif

r=me, but I wouldn't mind if you flipped this #ifndef/#else into an #ifdef/#else to avoid the easy-to-miss "n" in #ifndef.

Comment 15

[David Dahl :ddahl]

Comment on attachment 368272 [details] [diff] [review]
trunk patch

string change, will it make it? i hope so.

Comment 16

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

Not sure how common commandkey localizing is, but I suppose we should add an l10n note that indicates that bookmarksGtkCmd.commandkey should not contain the letters A-F.

For the branch, we should perhaps skip the entity renaming. Sorry I didn't mention that earlier.

Comment 17

[David Dahl :ddahl]

Attachment: branch patch without entity renaming - same as previous r+ patch

re-posting the previous approved patch w/o any entity renaming

Comment 18

[David Dahl :ddahl]

Attachment: branch patch without entity renaming and #ifdef #else

Comment 19

[David Dahl :ddahl]

Attachment: branch patch

I know you guys already approve, but I am a noob and flagging your review again.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 368364 [details] [diff] [review]
branch patch

a191=beltzner

Comment 21

[David Dahl :ddahl]

Attachment: trunk patch 2

i need to learn mercurial-queues

Comment 22

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

Comment on attachment 368384 [details] [diff] [review]
trunk patch 2

No need to re-request ui-review, since there have been no functional changes since it was granted.

Comment 23

[[:Cww]]

Adding a user-doc-needed flag to update http://support.mozilla.com/en-US/kb/Keyboard+Shortcuts

Comment 24

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

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/2918de2996cb

Comment 25

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/00f9feba2406

Comment 26

[Tracy Walker [:tracy]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090415 Shiretoko/3.5b4pre

Comment 28

[Chris Ilias [:cilias]]

user-doc-complete
<https://support.mozilla.com/kb/Keyboard+shortcuts?bl=n>

Comment 29

[cody.j.b.scott]

Ctrl+Shift+B does not do anything for me in FF29 on LinuxMint 16.