805217 - Remove duplicate menuitems when Tools is accessed using the keyboard.

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

Before bug 803344, the Tools menu has:

Tools
------
....
....
[ ]&providerName; for &brandShortName;
   &providerName; for &brandShortName; >
....
....

The first menuitem allows for toggling the Social API. The second menuitem has a submenu for toggling Social API subfeatures.

After bug 803344 lands, we'll now have a string that says "Remove from &brandShortName;"

We should then be able to remove the first menuitem if the Tools menu is accessed using a keyboard and show a removal menuitem under the submenu with a straightforward label.

Comment 1

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

Attachment: Patch

Waiting until 803344 lands before requesting review.

Comment 2

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

Attachment: Patch v2

This patch removes the redundant menuitem in the Tools menu, and removes the menuitem in the App -> Options menu. It adds an always-visible submenu in the Tools menu that mirrors the same options as the social toolbar item's menu.

A new menuitem is added for "Turn on" and "Turn off" which toggles the social api, but leaves the toolbar button visible. The user would have to "Remove from Firefox" to remove the toolbar item. This allows for quick toggling and less penalty for misclicks.

Screencast of patch: http://screencast.com/t/Nci2r7gVpGx

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 677248 [details] [diff] [review]
Patch v2

>+    <command id="Social:Toggle" oncommand="Social.toggle();" hidden="true"
>+             turnOffLabel="&social.turnOff.label;" turnOffAccessKey="&social.turnOff.accesskey;"
>+             turnOnLabel="&social.turnOn.label;" turnOnAccessKey="&social.turnOn.accesskey;"/>

>   updateToggleCommand: function SocialUI_updateToggleCommand() {
>     let toggleCommand = this.toggleCommand;
>     // We only need to update the command itself - all our menu items use it.
>-    toggleCommand.setAttribute("checked", Services.prefs.getBoolPref("social.enabled"));
>+    let enabled = Services.prefs.getBoolPref("social.enabled");
>+    let label = toggleCommand.getAttribute(enabled ? "turnOffLabel" : "turnOnLabel");
>+    let accesskey = toggleCommand.getAttribute(enabled ? "turnOffAccessKey" : "turnOnAccessKey");

>+<!ENTITY social.turnOff.label "Turn off">
>+<!ENTITY social.turnOff.accesskey "T">
>+<!ENTITY social.turnOn.label "Turn on">
>+<!ENTITY social.turnOn.accesskey "T">

Put these strings in browser.properties.

Comment 4

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

Comment on attachment 677248 [details] [diff] [review]
Patch v2

Boriss, I think you were concerned when I mentioned this interaction in our meeting yesterday. Can you take a look at this screencast and provide your input on the usability of the approach? http://screencast.com/t/Nci2r7gVpGx

Comment 5

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

Attachment: Patch v2.1

Thanks for the feedback Dao. I talked with Stephen and he said that he wanted the string to say "Turn on Facebook Messenger"/"Turn off Facebook Messenger" so it would be more clear.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 679730 [details] [diff] [review]
Patch v2.1

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

Some comments here, waiting for next patch that I know you're already working on

::: browser/base/content/browser-menubar.inc
@@ +524,3 @@
>                    <menuseparator id="socialAmbientMenuSeparator"
> +                                 hidden="true"
> +                                 class="show-only-for-keyboard"/>

this separator shouldn't have show-only-for-keyboard anymore

@@ -521,5 @@
>                              autocheck="false"
>                              command="Social:ToggleSidebar"
>                              label="&social.toggleSidebar.label;"
>                              accesskey="&social.toggleSidebar.accesskey;"/>
> -                  <menuitem id="social-toggle-notifications-keyboardmenuitem"

this id is also used on a test

::: browser/base/content/browser.css
@@ +218,5 @@
>  
> +#main-menubar[openedwithkey=false] #menu_socialToggle {
> +  display: none;
> +}
> +

nix this

::: browser/base/content/browser.xul
@@ -681,2 @@
>  #endif
> -            <menuitem id="social-toggle-sidebar-menuitem"

keep the ids here and for social-toggle-notifications-menuitem as they are needed for tests

Comment 7

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

Attachment: Patch v2.2

This patch addresses the issues mentioned in your previous review.

Pushed to try, https://tbpl.mozilla.org/?tree=Try&rev=d6d343c8b025

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b0fbcbdf3b

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b1b0fbcbdf3b