808422 - Expose the keyboard shortcut for focusing the chat bar

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Bug 801035 added a keyboard shortcut (ctrl+shift+c) for focusing the chat bar of the Social API, but didn't expose the shortcut in the UI of the program due to string freezes.

We should expose this shortcut in a keyboard-only type way for Windows+Linux users, and figure out a different way for Mac since https://bugzilla.mozilla.org/show_bug.cgi?id=801035#c11 says that the keyboard-only approach doesn't work for Mac.

Comment 1

[Dão Gottwald [:dao]]

I don't think we need to figure out anything for Mac; the item and shortcut being displayed in the "Facebook Messenger for Firefox" me for all users doesn't seem like a problem.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Add 'Focus chat window' to the social menu

Note that this menu item is always shown while social is enabled, even if there are no chat windows focusable.  Fixing that would involve moving the command attribute setting into socialchat.xml and dealing with the fact that the new separator also would need to be hidden (at least until we have other such commands in that section).

Hence request just for f= at this stage, but feel free to upgrade to r= if you think it is OK :)

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 684228 [details] [diff] [review]
Add 'Focus chat window' to the social menu

Review of attachment 684228 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-sets.inc
@@ +415,5 @@
>      <key id="viewBookmarksSidebarWinKb" key="&bookmarksWinCmd.commandkey;" command="viewBookmarksSidebar" modifiers="accel"/>
>  #endif
>      
>      <key id="sharePage" key="&sharePageCmd.commandkey;" command="Social:SharePage" modifiers="accel,shift"/>
> +    <key id="focusChatBar" key="&social.chatBar.commandkey;" command="Social:FocusChat" modifiers="accel,shift"/>

This <key> (and the one above it) should have disabled="true" on it which gets removed when the feature becomes available and likewise added-back when the feature becomes unavailable. See bug 814269.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 684228 [details] [diff] [review]
Add 'Focus chat window' to the social menu

>+<!ENTITY social.chatBar.label "Focus chat window">

"chat window" is the term we want to use for this?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Dão Gottwald [:dao] from comment #4)
> "chat window" is the term we want to use for this?

Gavin and I had a very brief discussion and while we didn't feel it was perfect, we failed to come up with anything better.

Do you (or Jaws, or anyone ;) have other suggestions?

Comment 6

[Dão Gottwald [:dao]]

I have no other suggestion.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated to disable key (via the command)

The command now also has a 'disabled' attribute and the code updates that along with hidden (but note that the attribute must be used for 'disabled' rather than a property, and that keys explicitly check for disabled="true")

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 684997 [details] [diff] [review]
Updated to disable key (via the command)

Review of attachment 684997 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-menubar.inc
@@ +541,5 @@
> +                            accesskey="&social.chatBar.accesskey;"
> +                            key="focusChatBar"
> +                            command="Social:FocusChat"
> +                            class="show-only-for-keyboard"/>
> +                  <menuseparator class="show-only-for-keyboard social-statusarea-separator"/>

I don't think we need another menuseparator here. It gets further complicated because if the Social:FocusChat command is hidden, users on OS X will see two adjacent menuseparators. If we just move the menu_focusChatBar to be above the social-statusarea-separator and remove this new one, then we won't have this complexity and it won't look awkward if chats aren't available.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +658,5 @@
>  <!ENTITY social.activated.undobutton.label "Undo">
>  <!ENTITY social.activated.undobutton.accesskey "U">
>  
>  <!ENTITY social.chatBar.commandkey "c">
> +<!ENTITY social.chatBar.label "Focus chat window">

These aren't in a window per-se. Let's go with "Focus chats"

Comment 9

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27ddfe913b0c

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/27ddfe913b0c