1166935 - Show the user's Firefox Account display name when signed in to Sync

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Zachary Carter [:zaach]]

We should display a user's Firefox Account "display name" in the preferences and toolbar menu if the have one set.

Comment 1

[Edouard Oger [:eoger]]

Attachment: fxa_display_name_in_preferences.patch

Simple enough patch, we only show the display name when the account is verified and working.

Comment 2

[Zachary Carter [:zaach]]

Hi Ryan, I recall an old mockup where the email address AND the display name were visible. Do we want to only show the display name if it's set and hide the email?

Comment 3

[Zachary Carter [:zaach]]

Comment on attachment 8614282 [details] [diff] [review]
fxa_display_name_in_preferences.patch

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

Simple enough. Let's wait on rfeeley's feedback on whether we'll show both the email and name or not.

::: browser/components/preferences/in-content/sync.js
@@ +324,5 @@
> +            };
> +            img.src = data.avatar;
> +          }
> +          if (data.displayName) {
> +            document.getElementById("fxaEmailAddress1").textContent = data.displayName;

Perhaps we should change the element ID if it could show a name or email address.

Comment 4

[Ryan Feeley [:rfeeley]]

Good idea. For expediency’s sake, let's have one area to show either email address or display name. Not both.

Comment 5

[Edouard Oger [:eoger]]

Attachment: 0002-Bug-1166935-Show-fxa-user-s-display-name-instead-of-.patch

Updated to use two separate containers (and fixed a bug where it wouldn't show the Display Name when avatar preference was disabled).

Comment 6

[Zachary Carter [:zaach]]

Comment on attachment 8616809 [details] [diff] [review]
0002-Bug-1166935-Show-fxa-user-s-display-name-instead-of-.patch

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

Just one adjustment and I think this should be good to go.

::: browser/components/preferences/in-content/sync.js
@@ +321,2 @@
>            try {
> +            avatarEnabled = Services.prefs.getBoolPref("identity.fxaccounts.profile_image.enabled");

Our use of this pref grew broader than its original intention when I named it. Because of reading/writing conflicts with profile data storage, we're using the pref to enabled/disable showing profile information in general. So, we can put both display name and avatar behind this flag.

Comment 7

[Edouard Oger [:eoger]]

I discussed this with markh yesterday, he thinks we should live with this misnamed pref until we remove it.

Comment 8

[Zachary Carter [:zaach]]

Yeah, I'm wondering if we still need that pref at all since Mark has fixed the issue with profile storage. Mark? If we do keep it, we should probably treat it as "profileInfoEnabled" like in your hamburger menu patch and only show the display name and avatar if it's true.

Comment 9

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

(In reply to Zachary Carter [:zaach] from comment #8)
> Yeah, I'm wondering if we still need that pref at all since Mark has fixed
> the issue with profile storage. Mark? If we do keep it, we should probably
> treat it as "profileInfoEnabled" like in your hamburger menu patch and only
> show the display name and avatar if it's true.

Agreed - we should treat it as though it applies to all profile info and delete the pref entirely in a few weeks - maybe directly after the next merge (keeping it for now gives us an easy way to disable it if things hit the fan)

Comment 10

[Edouard Oger [:eoger]]

Attachment: bug-1166935.patch

Renamed the preference variable to profileInfoEnabled and made sure avatar/displayName are shown only when the pref is true, to be consistent with the Hamburger menu patch.

Comment 11

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5da54c35d85b

Comment 12

[Edouard Oger [:eoger]]

Attachment: bug-1166935.patch

Added r=markh, commit message was written before Whistler ;)

Comment 13

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

Comment on attachment 8630240 [details] [diff] [review]
bug-1166935.patch

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

LGTM - change those vars to lets and re-upload the patch carrying my r+ forward and set checkin-needed.

::: browser/components/preferences/in-content/sync.js
@@ +278,5 @@
>                    .wrappedJSObject;
>      // service.fxAccountsEnabled is false iff sync is already configured for
>      // the legacy provider.
>      if (service.fxAccountsEnabled) {
> +      var displayNameLabel = document.getElementById("fxaDisplayName");

use let here and below

Comment 14

[Edouard Oger [:eoger]]

Attachment: bug-1166935.patch

Updated with let and rebased.

Comment 15

[Pulsebot]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb1ee4dc8dc9