Closed Bug 972567 Opened 6 years ago Closed 5 years ago

Tooltip for Reconnect to Sync

Categories

(Firefox :: Sync, defect, trivial)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox41 --- verified

People

(Reporter: rfeeley, Assigned: jessieh)

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 2 obsolete files)

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

Reconnect user@domain.com (substitute user@domain.com for the user’s email address)
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good first bug]
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!
Flags: needinfo?(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 ;)
Flags: needinfo?(rfeeley) → needinfo?(markh)
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 :)
Flags: needinfo?(markh)
Alright, thanks for the help guys! I'll get to making the changes right away.
Attached patch syncTooltip.patch (obsolete) — Splinter Review
Alright, the changes have been made! The code builds and it works as intended; hopefully I've put this patch together properly.
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.
Attachment #8613259 - Flags: feedback+
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!
Attachment #8613259 - Attachment is obsolete: true
Attachment #8613348 - Flags: review?(markh)
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.
Attachment #8613348 - Flags: review?(markh)
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. :)
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.
Attachment #8613348 - Attachment is obsolete: true
Attachment #8615527 - Flags: review?(markh)
Comment on attachment 8615527 [details] [diff] [review]
syncTooltip.patch - Third revision

Awesome, thank you!
Attachment #8615527 - Flags: review?(markh) → review+
Assignee: nobody → jessieh
https://hg.mozilla.org/mozilla-central/rev/243d64120baa
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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]
QA Whiteboard: [bugday-20150729]
(In reply to Saheda Reza [:Antora] from comment #16)
> It's oke Latest Nightly and Firefox Beta.

Awesome, thank you!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.