Remove duplicate menuitems when Tools is accessed using the keyboard.

RESOLVED FIXED in Firefox 19

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
Firefox 19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Created attachment 674852 [details] [diff] [review]
Patch

Waiting until 803344 lands before requesting review.
Created attachment 677248 [details] [diff] [review]
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
Attachment #674852 - Attachment is obsolete: true
Attachment #677248 - Flags: review?(felipc)
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.
Attachment #677248 - Flags: feedback-
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
Attachment #677248 - Flags: ui-review?(jboriss)
Created attachment 679730 [details] [diff] [review]
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.
Attachment #677248 - Attachment is obsolete: true
Attachment #677248 - Flags: ui-review?(jboriss)
Attachment #677248 - Flags: review?(felipc)
Attachment #679730 - Flags: review?(felipc)
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
Attachment #679730 - Flags: review?(felipc)
Created attachment 681875 [details] [diff] [review]
Patch v2.2

This patch addresses the issues mentioned in your previous review.

Pushed to try, https://tbpl.mozilla.org/?tree=Try&rev=d6d343c8b025
Attachment #679730 - Attachment is obsolete: true
Attachment #681875 - Flags: review?(felipc)
Attachment #681875 - Flags: review?(felipc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b0fbcbdf3b
Flags: in-testsuite+
Summary: Use the strings from bug 803344 to remove the duplicate menus under Tools → Remove duplicate menuitems when Tools is accessed using the keyboard.

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/b1b0fbcbdf3b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Depends on: 813316

Updated

5 years ago
Blocks: 813436
You need to log in before you can comment on or make changes to this bug.