Tooltip for Reconnect to Sync

VERIFIED FIXED in Firefox 41

Status

()

Firefox
Sync
--
trivial
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: rfeeley, Assigned: Jessie Hildebrandt)

Tracking

unspecified
Firefox 41
Points:
---

Firefox Tracking Flags

(firefox41 verified)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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)
(Reporter)

Updated

3 years ago
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good first bug]
(Reporter)

Comment 1

3 years ago
Created attachment 8611254 [details]
Example of what the tooltip looks like
(Assignee)

Comment 2

3 years ago
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)
(Reporter)

Comment 3

3 years ago
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)
(Assignee)

Comment 5

3 years ago
Alright, thanks for the help guys! I'll get to making the changes right away.
(Assignee)

Comment 6

3 years ago
Created attachment 8613259 [details] [diff] [review]
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 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+
(Assignee)

Comment 8

3 years ago
Created attachment 8613348 [details] [diff] [review]
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!
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)
(Assignee)

Comment 10

3 years ago
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. :)
(Assignee)

Comment 11

3 years ago
Created attachment 8615527 [details] [diff] [review]
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.
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+

Updated

3 years ago
Assignee: nobody → jessieh

Comment 13

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/243d64120baa
https://hg.mozilla.org/mozilla-central/rev/243d64120baa
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
https://hg.mozilla.org/mozilla-central/rev/243d64120baa
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]

Updated

2 years ago
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
status-firefox41: fixed → verified
You need to log in before you can comment on or make changes to this bug.