972567 - Tooltip for Reconnect to Sync

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Ryan Feeley [:rfeeley]]

The Reconnect to Sync Australis menu item could use a nice tooltip that says:

Reconnect [e-mail] (substitute [e-mail] for the user’s email address)

Comment 1

[Ryan Feeley [:rfeeley]]

Attachment: Example of what the tooltip looks like

Comment 2

[Jessie Hildebrandt]

Hello! I'd like to tackle this as my first bug while I acquaint myself with the system here.

Could you point me towards the appropriate files/directories that I should be looking at?
I'm currently digging through the source tree, but even after reading the Mozilla Source Code Directory Structure guide I'm unsure where to start looking. I'm assuming it's somewhere in the browser folder, but I'm not 100% certain.

Thanks!

Comment 3

[Ryan Feeley [:rfeeley]]

Thanks Jessie! Let's get help from the Mark who worked on Sync originally and knows the code base more than I, a mere UX designer ;)

Comment 4

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

In browser-fxaccounts, you will find a line:

          this.button.setAttribute("tooltiptext", userData.email);

which is where the tooltip is set to just the user email address. You'll need to edit accounts.properties to add a new string - something like "reconnectDescription = Reconnect %S" (and with a "LOCALIZATION NOTE" comment documenting the %S, as you'll see already in the file for other strings.  Then browser-fxaccounts.js will change to something like:

  let description = this.strings.formatStringFromName("reconnectDescription",
          [userData.email], 1);
  this.button.setAttribute("tooltiptext", description);

ping me if you need more info - I'm "markh" on IRC, but note I'm in Australia, so timezones :)

Comment 5

[Jessie Hildebrandt]

Alright, thanks for the help guys! I'll get to making the changes right away.

Comment 6

[Jessie Hildebrandt]

Attachment: syncTooltip.patch

Alright, the changes have been made! The code builds and it works as intended; hopefully I've put this patch together properly.

Comment 7

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

Comment on attachment 8613259 [details] [diff] [review]
syncTooltip.patch

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

This looks good, thanks! Please fix the issue below and upload a new version and request review from me.

::: browser/base/content/browser-fxaccounts.js
@@ +241,5 @@
>        this.button.setAttribute("label", defaultLabel);
>        this.button.removeAttribute("tooltiptext");
>        this.button.removeAttribute("fxastatus");
>  
> +      let tooltipDescription = this.strings.formatStringFromName("reconnectDescription", [userData.email], 1);

Please move this line to just above the setAttribute call that uses it - otherwise we are formatting the variable even when it's not used.

Comment 8

[Jessie Hildebrandt]

Attachment: syncTooltip.patch - Second revision

Good catch! I didn't take notice of that when I was writing it out; I just thought it looked awfully nice nestled between those two blocks of code. ;)

Here's a revised version of the patch with the variable definition in a more appropriate place, as suggested.

Thanks again for all of the help!

Comment 9

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

Comment on attachment 8613348 [details] [diff] [review]
syncTooltip.patch - Second revision

Ack - I'm very sorry, but I misled you :(  

> this.button.setAttribute("tooltiptext", userData.email);

is actually correct - that's the "successfully logged in" case - it's the block above - |if (this.loginFailed)| that needs changing - so you want your 2 new lines in the block above, and this block where the tooltip is set to the email address should be unchanged.

Sorry about that.  Note that to test the "login failed" case you probably want to create a new bool preference "services.sync.debug.ignoreCachedAuthCredentials" set to true, then change your FxA password, then force a sync - an info bar should appear at the bottom of the window asking you to reconnect, and the tooltip in the menu should adjust to this new "Reconnect you@somewhere" string.

Comment 10

[Jessie Hildebrandt]

Whoops! Sorry about that.

I've swapped the code around and restored the previous tooltip for the "successfully logged in" case, and I'm now waiting for it to build. I'm going to be awfully busy tonight so I probably won't have the time to finish it up; I'll have the patch ready and submitted for review tomorrow afternoon. :)

Comment 11

[Jessie Hildebrandt]

Attachment: syncTooltip.patch - Third revision

Alright, I tested out the build I made last night and the tooltip changes properly when Sync is unable to login successfully. Here's the new revision of the patch with the proper changes.

Comment 12

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

Comment on attachment 8615527 [details] [diff] [review]
syncTooltip.patch - Third revision

Awesome, thank you!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/243d64120baa

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/243d64120baa

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/243d64120baa

Comment 16

[Saheda Reza [:Antora]]

I found same issue on nightly & also Latest Developer Edition.There didn't show tooltip on sync.

Firefox nightly:

Build ID	20140213030201
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:30.0) Gecko/20100101 Firefox/30.0

Firefox Developer Edition:

Build ID 	20150731004008
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:41.0) Gecko/20100101 Firefox/41.0

It's oke Latest Nightly and Firefox Beta.

[buday-20150729]

Comment 17

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

(In reply to Saheda Reza [:Antora] from comment #16)
> It's oke Latest Nightly and Firefox Beta.

Awesome, thank you!