804258 - The Facebook sidebar doesn't show up under View / Sidebar

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Doug Turner (:dougt)]

I installed the Facebook messenger social api thing.  It looks like a sidebar.  Facebook's documentation refers to the integration as a sidebar.  However, when I go to the Firefox menu bar, i do not see "Facebook" under View / Sidebar/

Comment 1

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

Attachment: Add a new menu item for View->Sidebar

Adds a new menu item (and a new string).  Note that this only works correctly after the patch to bug 804442 is applied (so the command this uses is updated correctly)

Comment 2

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

Comment on attachment 674157 [details] [diff] [review]
Add a new menu item for View->Sidebar

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

Please include 8 lines of context in your patches. Only not granting feedback because I haven't spent enough time to determine if this is all that is needed.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +654,5 @@
>  <!ENTITY social.toggleSidebar.label "Show sidebar">
>  <!ENTITY social.toggleSidebar.accesskey "s">
>  
> +<!ENTITY social.socialSidebar.label "Social">
> +<!ENTITY social.socialSidebar.accesskey "s">

I think this should be the social provider name and we don't necessarily need an accesskey. Also, if we do it this way we can uplift since there won't be any string additions.

Comment 3

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

Attachment: Just use the provider name, so no strings

Comment 4

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

Comment on attachment 674485 [details] [diff] [review]
Just use the provider name, so no strings

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

Overall I don't think the comments are necessary but I'll let you make the call on them.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-social.js#111 needs to be updated, and we need to make sure not to show this menuitem if social is not enabled or activated.

Comment 5

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

(In reply to Jared Wein [:jaws] from comment #4)
> Overall I don't think the comments are necessary but I'll let you make the
> call on them.

Yeah, I think you are correct.

> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> social.js#111 needs to be updated, and we need to make sure not to show this
> menuitem if social is not enabled or activated.

Sorry, I should have been clearer - bug 804442 has landed on inbound and will make that unnecessary.  The fact the new menuitem uses the command Social:ToggleSidebar means it is hidden when disabled.

I'll re-ask for review once 804442 lands on m-c.

Comment 6

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

Comment on attachment 674485 [details] [diff] [review]
Just use the provider name, so no strings

And it just landed :)  I'm confident the visibility thing is also working correctly - was there anything else?

Comment 7

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

Attachment: patch for nightly

comments removed - sorry for the bugspam!

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 674510 [details] [diff] [review]
patch for nightly

(In reply to Jared Wein [:jaws] from comment #2)
> Comment on attachment 674157 [details] [diff] [review]
> Add a new menu item for View->Sidebar
> 
> Review of attachment 674157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please include 8 lines of context in your patches. Only not granting
> feedback because I haven't spent enough time to determine if this is all
> that is needed.
> 
> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +654,5 @@
> >  <!ENTITY social.toggleSidebar.label "Show sidebar">
> >  <!ENTITY social.toggleSidebar.accesskey "s">
> >  
> > +<!ENTITY social.socialSidebar.label "Social">
> > +<!ENTITY social.socialSidebar.accesskey "s">
> 
> I think this should be the social provider name and we don't necessarily
> need an accesskey.

All items in any given menu should either consistently get an access key or consistently get no access key. You need the latter if some items have user- or content-generated labels (see the Bookmarks and History menus for example), otherwise you need the former. Is the provider name user-/content-generated or hardcoded in Firefox? Is it locale dependent?

Comment 9

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

(In reply to Dão Gottwald [:dao] from comment #8)
> Is the provider name user-/content-generated or hardcoded in Firefox?

It is supplied in the provider's manifest (IIRC)

> Is it locale dependent?

No

Comment 10

[Dão Gottwald [:dao]]

(In reply to Mark Hammond (:markh) from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Is the provider name user-/content-generated or hardcoded in Firefox?
> 
> It is supplied in the provider's manifest (IIRC)

... which is hardcoded in Firefox?

> > Is it locale dependent?
> 
> No

Is this a bug?

Comment 11

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

(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to Mark Hammond (:markh) from comment #9)
> > (In reply to Dão Gottwald [:dao] from comment #8)
> > > Is the provider name user-/content-generated or hardcoded in Firefox?
> > 
> > It is supplied in the provider's manifest (IIRC)
> 
> ... which is hardcoded in Firefox?

Right, yes it is for facebook, although IIUC, that is not the intention for any other providers.

> > > Is it locale dependent?
> > 
> > No
> 
> Is this a bug?

I guess it is, but I believe there is no bug on file.

Comment 12

[Dão Gottwald [:dao]]

So it seems like we need a bug filed on getting the Facebook label in shape (either Facebook-provided or localized on our end) and remove all access keys in this menu.

Comment 13

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

The localization of "Facebook" is a wider issue than just this bug, but yeah.

Removing all access keys seems like a step backwards to me, and TBH, I preferred the generic "Social" string as it remains constant in a multi-provider world - so going back to that remains an option that allows us to keep access keys.

Anyway, I'll let you guys decide on the best course of action here...

Comment 14

[Dão Gottwald [:dao]]

(In reply to Mark Hammond (:markh) from comment #13)
> The localization of "Facebook" is a wider issue than just this bug, but yeah.

More precisely, the localization of "Facebook Messenger".

> I preferred the generic "Social" string as it remains constant in a
> multi-provider world

I have no preference here.

Comment 15

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

The other thing we can do here is use "Facebook Messenger _for &brandShortName;", where the 'f' in "for" is the access key. This is what we do for the Tools menuitem.

Comment 16

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #15)
> The other thing we can do here is use "Facebook Messenger _for &brandShortName;"

This seems redundant and doesn't really make sense to me. We could similarly add "Firefox" to all kinds of features and tools we provide as part of Firefox... "New Firefox Tab", "Firefox Options", "Show All Bookmarks for Firefox", "Web Console for Firefox", whatever. Obviously we're not going to do that.

Comment 17

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

Let's add an entry that has only the provider name, with no access key, for v1. We will revisit when multi-providers comes, or we decide to tackle provider name localization.

Comment 18

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

Comment on attachment 674510 [details] [diff] [review]
patch for nightly

markh pointed out that we need a new patch for beta

Comment 19

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

Attachment: Patch against beta but should also apply on aurora

Same as the patch for nightly, but with the addition of explicitly adjusting the "hidden" attribute on the view->sidebar menu item instead of relying on the command's attribute from working.

Comment 20

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

Comment on attachment 676428 [details] [diff] [review]
Patch against beta but should also apply on aurora

[Triage Comment]
r+a=me for 17/18

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a31c31f5e6c

Comment 22

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

https://hg.mozilla.org/releases/mozilla-beta/rev/cf37cd026cb6

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 674510 [details] [diff] [review]
patch for nightly

Why did you not remove access keys from all menu items which as I pointed out would be the right thing to do if you don't want an access key for one item? This is needed such that all menu items can be reached reliably by pressing a letter.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a31c31f5e6c

Comment 25

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

(In reply to Dão Gottwald [:dao] from comment #23)
> Why did you not remove access keys from all menu items which as I pointed
> out would be the right thing to do if you don't want an access key for one
> item? This is needed such that all menu items can be reached reliably by
> pressing a letter.

Why is removing all access keys better than having a single one without an access key? Is the concern that the social provider's name's first letter will interfere with another existing access key?

Comment 26

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #25)
> (In reply to Dão Gottwald [:dao] from comment #23)
> > Why did you not remove access keys from all menu items which as I pointed
> > out would be the right thing to do if you don't want an access key for one
> > item? This is needed such that all menu items can be reached reliably by
> > pressing a letter.
> 
> Why is removing all access keys better than having a single one without an
> access key? Is the concern that the social provider's name's first letter
> will interfere with another existing access key?

Yes. If one of the access keys is F, you can't reach the facebook item by pressing F anymore. Again, this isn't new -- the History and Bookmark menus work this way. If this wasn't clear earlier, I'm really curious why nobody asked for clarification and people decided it would be appropriate to disregard comment 8 and below.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/223c1ce20876

Comment 28

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

(In reply to Dão Gottwald [:dao] from comment #26)
> Yes. If one of the access keys is F, you can't reach the facebook item by
> pressing F anymore.

OK. That's a problem, but I don't think it's a serious one. And it's not as big of a problem as not having this sidebar show up in the menu at all.

> I'm really curious why nobody asked for clarification and people decided it would
> be appropriate to disregard comment 8 and below.

We're on a pretty tight deadline, so I hope you'll forgive the mistakes we've made in the interest of actually fixing these bugs for beta 4 (go to build is tonight/tomorrow morning).

We can followup and remove the access keys for this menu, but I'm not sure that's worth it.

Comment 29

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #28)
> And it's not as
> big of a problem as not having this sidebar show up in the menu at all.

Nobody said that. My r- didn't mean "please don't fix this" but "please fix this properly."

> We can followup and remove the access keys for this menu, but I'm not sure
> that's worth it.

Why would it not be worth it?

Comment 30

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

(In reply to Dão Gottwald [:dao] from comment #29)
> > We can followup and remove the access keys for this menu, but I'm not sure
> > that's worth it.
> 
> Why would it not be worth it?

Because it's churn. And because it changes the behavior for all accesskey users, who might be used to using their accesskeys to access these items, and might be upset if we remove them.

The menu item does not conflict in en-US (there is no "F" access key) - I only see a conflict in "is" and "sq":
http://mxr.mozilla.org/l10n-central/search?string=historySidebarCmd.accesskey
http://mxr.mozilla.org/l10n-central/search?string=bookmarksButton.accesskey

So the tradeoff is "is" and "sq" accesskey users not being able to access the Facebook menu item directly (they can still arrow down, right? This menu isn't long...) vs. access key users for all locales having their access keys switched up on them.

Comment 31

[Dão Gottwald [:dao]]

I'm not sure what you mean by churn.

The fear of behavior change seems like a red herring. People toggling sidebars often are more likely to use the shortcut key. On top of that, the access key should usually match the first letter here, but even if it didn't: This isn't the first time we've updated access keys. It's one of those things that are needed to maintain an evolving UI. Leaving this half-broken just doesn't make sense and would lead to a mess if we did it consequently.

Comment 32

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

"churn" is unnecessary change. Feel free to file a followup. As you say, the use of these access keys is probably almost non-existent, so it doesn't seem worth it to worry about this - particularly if we're going to re-visit when it comes time to deal with multiple provider support or provider name localization.

Comment 33

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #32)
> As you say, the use of these access keys is probably almost non-existent,

That's not what I said. Access keys aren't only useful for learning them by heart. They're also useful for infrequently used menu items or menu items without command keys. If you're used to it, activating menu items by pressing access keys can be generally quicker than using arrow keys -- even for short menus, where you wouldn't want to change your habit ad hoc.

That said, access keys are generally not used much. This is true for many accessibility features we traditionally care about.

Comment 34

[u279076]

Verified fixed across all branches.