Closed Bug 858321 Opened 7 years ago Closed 7 years ago

fix disabled state of toolbarbutton

Categories

(Firefox Graveyard :: SocialAPI, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 2 obsolete files)

Attached image services disabled menu
boriss and I discussed using the service icon for the button when we're in disabled state, as well as having the provider selection available in disalbed state.

- re-enforces that the provider is disabled by removing provider logo from button
- allows user to re-enable with a different provider than previously selected
- "unchecking" current provider would disable services, though this has potential for confusion, other checked menu lists dont allow unchecking the current checked menu.
- one issue might be that we should now say "turn off services" rather than "turn off provider-name".

images and prototype patch on their way.
Attachment #733622 - Flags: ui-review?(jboriss)
Attached image services enabled
Attachment #733624 - Flags: ui-review?(jboriss)
Attached patch providermenu.patch (obsolete) — Splinter Review
Attachment #733625 - Flags: feedback?(mhammond)
Comment on attachment 733625 [details] [diff] [review]
providermenu.patch

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

Having "Turn on {provider}" along with the list of providers seems wrong.  IMO, there should be an item "Turn off Social Services", and *no* "Turn on ..." item (ie, "Turn off ..." should be hidden when social is already disabled.  But I guess this is a call for UX.

Looks good in general, but untested at the moment.

::: browser/base/content/browser-social.js
@@ +783,5 @@
>        this.button.setAttribute("label", provider.name);
>        this.button.setAttribute("tooltiptext", provider.name);
>        this.button.style.listStyleImage = "url(" + provider.iconURL + ")";
>  
>        this.updateProfile();

I think with this change we can remove the check for Social.provider in updateProfile (ie, every call to this function is now already guarded by a check for Social.provider)

@@ +787,5 @@
>        this.updateProfile();
> +    } else {
> +      this.button.setAttribute("label", gNavigatorBundle.getString("service.toolbarbutton.label"));
> +      this.button.setAttribute("tooltiptext", gNavigatorBundle.getString("service.toolbarbutton.tooltiptext"));
> +      this.button.style.listStyleImage = "";

as discussed on IRC, style.removeProperty might be better here.

@@ +1099,5 @@
>        menuitem.setAttribute("origin", provider.origin);
>        if (provider == Social.provider) {
>          menuitem.setAttribute("checked", "true");
>        } else {
> +        menuitem.setAttribute("oncommand", "Social.setProviderByOrigin(this.getAttribute('origin')); Social.enabled = true;");

no need to set Social.enabled - setProviderByOrigin will do that.

::: browser/base/content/browser.xul
@@ +719,5 @@
>                        command="Social:ToggleNotifications"
>                        label="&social.toggleNotifications.label;"
>                        accesskey="&social.toggleNotifications.accesskey;"/>
>              <menuitem class="social-toggle-menuitem" command="Social:Toggle"/>
> +            <menuseparator class="social-provider-separator"/>

I think you should just remove the new class to keep things simple - otherwise people will assume the new class name is relevant to something, but it's not (the point is to *not* have the old class name)

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +387,5 @@
>  fullscreen.entered=%S is now fullscreen.
>  # LOCALIZATION NOTE (fullscreen.rememberDecision): displayed when we enter HTML5 fullscreen mode, %S is the domain name of the focused website (e.g. mozilla.com).
>  fullscreen.rememberDecision=Remember decision for %S
>  
> +service.toolbarbutton.label=Services

With my ill-fitting UX hat on, I'm not sure a label of "Services" and tooltip of "Social Services" is ideal.  I'd think the label should be "Social Services" and the tooltip even more descriptive.  So I'll just flag this for a real UX person to consider :)

::: browser/themes/linux/browser.css
@@ +2122,5 @@
>  .social-notification-container > .toolbarbutton-1 > .toolbarbutton-icon {
>    margin: 5px 3px;
>  }
>  
> +#social-provider-button {

It'd be nice to fix that other bug where we call for all social styles to be placed in their own file (but I guess there's no need to block this bug on that being fixed)
Attachment #733625 - Flags: feedback?(mhammond) → feedback+
Summary: use service icon when disabled → fix disabled state of toolbarbutton
Duplicate of this bug: 860148
Comment on attachment 733622 [details]
services disabled menu

Mark raises a good point in Comment 3, that "Turn on {provider}" as an option in disabled state is redundant when allowing all social networks to be both viewable and toggle-able in the disabled mode menu.  However, I think there's good reason to keep it nonetheless, because the the pro of allowing users to quickly toggle between the on and off state for one network - without having to re-find it in the list - outweighs the con of redundancy.  This is particularly true as we expect users to have one main social network that they use more of the time.  

In addition, removing the "turn on" item would create an additional inconsistency in that the language to turn off a network says "turn off" in the menu, yet the language to turn on is to select the item from the list of networks.  I'm predicting that this case would be very confusing to users looking for an identical "turn on" option and not seeing one in the menu.

However, if a user only has one social network installed, the benefit of quick switching does not exist because users no longer need to find one social network in a list.  In addition, the negative is even worse, because now the user is only seeing two social-network-related items and they both are for the same network and do the same thing.  I was all ready to file a new bug for the single-network-case to fix this problem, but the behavior I'm seeing in this patch actually handles the single-network case as it should - by not showing the checked list.  Bravo!
Attachment #733622 - Flags: ui-review?(jboriss) → ui-review+
Attachment #733624 - Flags: ui-review?(jboriss) → ui-review+
(In reply to Mark Hammond (:markh) from comment #3)
> Comment on attachment 733625 [details] [diff] [review]
> providermenu.patch
> 
> Review of attachment 733625 [details] [diff] [review]:

> ::: browser/base/content/browser-social.js
> @@ +783,5 @@
> >        this.button.setAttribute("label", provider.name);
> >        this.button.setAttribute("tooltiptext", provider.name);
> >        this.button.style.listStyleImage = "url(" + provider.iconURL + ")";
> >  
> >        this.updateProfile();
> 
> I think with this change we can remove the check for Social.provider in
> updateProfile (ie, every call to this function is now already guarded by a
> check for Social.provider)

That is called with a null provider via the social:provider-set notification, which is how the button is getting updated to the default icon.

Updating with all the other feedback.
Attached patch providermenu.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=d98356d3b4a0
Assignee: nobody → mixedpuppy
Attachment #733625 - Attachment is obsolete: true
Attachment #740419 - Flags: review?(mhammond)
Comment on attachment 740419 [details] [diff] [review]
providermenu.patch

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

LGTM

::: browser/base/content/test/social/head.js
@@ +189,5 @@
>    isbool(!win.SocialShareButton.shareButton.hidden, canShare, "share button visible?");
>    isbool(!doc.getElementById("social-toolbar-item").hidden, active, "toolbar items visible?");
> +  if (active) {
> +    if (!enabled) {
> +      ok(!!!win.SocialToolbar.button.style.listStyleImage, "toolbar button is default icon");

!!!??? :):):) - surely just "!win.SocialToolbar.button.style.listStyleImage" works here?
Attachment #740419 - Flags: review?(mhammond) → review+
updated with comment.  carry forward r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f086ab97aa
Attachment #740419 - Attachment is obsolete: true
Attachment #745975 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b8f086ab97aa
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Depends on: 872455
Whiteboard: [qa-]
Attachment #745975 - Attachment is patch: true
Attachment #745975 - Attachment mime type: text/x-patch → text/plain
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.