971266 - Sync tooltip in Australis menu for signed-in users should say “Open Sync Preferences”

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Ryan Feeley [:rfeeley]]

When the user is known, the Sync menu item tooltip currently repeats the email address for the signed-in user: https://www.dropbox.com/s/gchlu315syulu5j/what-should-tooltip-say.png

It should instead say “Open Sync Preferences” (not in quotes).

Comment 1

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

Can't fix this in 29, no string. Let's fix it on trunk (30) though.

Comment 2

[vedavidh]

 have tried finding the file where the exact code has been written but i didn't find the file all i got is the UI for different things and the function for the sing in and sync

Comment 3

[vedavidh]

 have tried finding the file where the exact code has been written but i didn't find the file all i got is the UI for different things and the function for the sing in and sync

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Asking Ryan for details.

Comment 5

[Ryan Feeley [:rfeeley]]

I can't find it myself, Gavin, know of this strings whereabouts?

Comment 6

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

That label's tooltip text is set here:

http://hg.mozilla.org/mozilla-central/annotate/0e19655e93df/browser/base/content/browser-fxaccounts.js#l194

It should be set to a new string rather than userData.email. That requires adding the string in question from comment 0 to sync.properties and accessing it via a string bundle here.

Comment 7

[Shubham Jindal [:shubham]]

Attachment: bug971266.diff

Comment 8

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

Comment on attachment 8537334 [details] [diff] [review]
bug971266.diff

Looks good to me! I think you can use gSyncUI._stringBundle instead of calling createBundle again. If you did that, it would probably also make sense to change it to just gSyncUI.stringBundle ("_" prefix suggests and internal property, and there's no reason for this to be).

But those things are not necessary changes, this looks good as-is.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8537334 [details] [diff] [review]
bug971266.diff

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

::: browser/base/content/browser-fxaccounts.js
@@ +191,5 @@
>            this.button.setAttribute("signedin", "true");
>            this.button.setAttribute("label", userData.email);
> +          this.button.setAttribute("tooltiptext", Services.strings.createBundle(
> +            "chrome://browser/locale/sync.properties")
> +            .GetStringFromName("client.tooltiptext.email"));

I agree with Gavin, but rather than exposing gSyncUI._stringBundle as an API, I'd prefer a gSyncUI.getString("name-of-string") using _stringBundle internally, it would read better.

::: services/sync/locales/en-US/sync.properties
@@ +45,5 @@
>  # %1: the app name (Firefox)
>  error.sync.eol.description = Your Firefox Sync service is no longer available. You need to upgrade %1$S to keep syncing.
>  sync.eol.learnMore.label = Learn more
>  sync.eol.learnMore.accesskey = L
> +client.tooltiptext.email = Open Sync Preferences

We should have a better name for this, client and email here don't point out anything useful for the localizer.
something like: menuwidget.signedin.tooltip would do.
A LOCALIZAZION NOTE() explaining where it is used, would also we welcome.

Comment 10

[Shubham Jindal [:shubham]]

I will change the name and submit the patch. I didn't get the part of A LOCALIZATION NOTE().

Comment 11

[Tim Nguyen :ntim]

(In reply to Shubham Jindal [:shubham] from comment #10)
> I will change the name and submit the patch. I didn't get the part of A
> LOCALIZATION NOTE().


A note explaining where the string is used. Something like this :
# LOCALIZATION NOTE : [...]

Comment 12

[Shubham Jindal [:shubham]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #9)

> I agree with Gavin, but rather than exposing gSyncUI._stringBundle as an
> API, I'd prefer a gSyncUI.getString("name-of-string") using _stringBundle
> internally, it would read better.

I didn't get this part. I used DXR to search gSyncUI and see if there is anything like gSyncUI.getString or gSyncUI.getStringFromName, but I was not able to find such function. How should I use this to get the String?

Comment 13

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

(In reply to Shubham Jindal [:shubham] from comment #12)
> I didn't get this part. I used DXR to search gSyncUI and see if there is
> anything like gSyncUI.getString or gSyncUI.getStringFromName, but I was not
> able to find such function. How should I use this to get the String?

You'll have to add it. That involves adding a function somewhere around here:
https://hg.mozilla.org/mozilla-central/annotate/67872ce17918/browser/base/content/browser-syncui.js#l98

That looks like this:

getString: function (stringName) {
  return this._stringBundle.GetStringFromName(stringName);
}

Then you can have your newly added code just call gSyncUI.getString("client.tooltiptext.email") instead of .createBundle().getStringFromName() etc.

You could also then change other callers of this._stringBundle.GetStringFromName in browser-syncui.js to use the new function.

Comment 14

[Chris Karlof [:ckarlof]]

This is fixed in bug 1139698.

Comment 15

[Nazir Ahmed Sabbir [:NaSb]]

Reproduced this bug in Nightly 30.0a1 (2014-02-11) on Linux x64 by following comment 0's instruction!

Bug is now fixed on Latest Nightly 42.0a1 (2015-07-21) 

Build ID: 20150721030212
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0

Comment 16

[Hossain Al Ikram [:ikram] (QA Contact)]

I have seen the bug windows 7, 64 bit  with Nightly 36.0a1 (2014-11-04)!

This Bugs fix is verified now on Latest Nightly 42.0a1 (2015-07-21)

Build ID : 20150721030212
User Agent : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0