Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Display the user's FxA profile image in the Sync Pref pane

RESOLVED FIXED in Firefox 39

Status

()

Firefox
Sync
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: zaach, Assigned: zaach)

Tracking

Trunk
Firefox 39
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
If the signed in user has a FxA profile image we should display it on the preferences pane.
(Assignee)

Comment 1

2 years ago
Created attachment 8572955 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane
Attachment #8572955 - Flags: ui-review?(rfeeley)
(Assignee)

Comment 2

2 years ago
Created attachment 8572958 [details]
Signed in but no profile image
(Assignee)

Comment 3

2 years ago
Created attachment 8572959 [details]
Signed in with a profile image
(Assignee)

Comment 4

2 years ago
I should note that this is pref-ed off. "identity.fxaccounts.profile_image.enabled" must be set to true for the UI to reflect the changes.
(Assignee)

Updated

2 years ago
Attachment #8572955 - Flags: feedback?(mhammond)
Comment on attachment 8572955 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

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

LGTM, but note:

* sadly there are 2 versions of sync.js and sync.xul that need to be updated (starting with just 1 is fine while we nut things out, but both will need to change)
* like my comments in bug 1139657, I wonder if this is to focused on the avatar? eg, promiseAccountsChangeProfileImageURI() could just be promiseAccountsChangeProfile(), which is a generic page used to change *any* part of the profile, including the avatar (even if the avatar is the only thing that can be changed now)
* I don't think the pref is needed.
Attachment #8572955 - Flags: feedback?(mhammond) → feedback+
(Assignee)

Comment 6

2 years ago
(In reply to Mark Hammond [:markh] from comment #5)
> Comment on attachment 8572955 [details] [diff] [review]
> Display the user's FxA profile image in the Sync Pref pane
> 
> Review of attachment 8572955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, but note:
> 
> * sadly there are 2 versions of sync.js and sync.xul that need to be updated
> (starting with just 1 is fine while we nut things out, but both will need to
> change)
> * like my comments in bug 1139657, I wonder if this is to focused on the
> avatar? eg, promiseAccountsChangeProfileImageURI() could just be
> promiseAccountsChangeProfile(), which is a generic page used to change *any*
> part of the profile, including the avatar (even if the avatar is the only
> thing that can be changed now)

I think we'd still want some distinction in the URL so that the settings page could highlight the field that the user intends to edit (or show a different page, as is the current case). There is a unified settings page planned in the future, but avatars might still be on there own page for the first version.

> * I don't think the pref is needed.

Let's keep it in for now, at least until the support for WebChannel makes it to production on the content server.
(In reply to Zachary Carter [:zaach] from comment #6)

> I think we'd still want some distinction in the URL so that the settings
> page could highlight the field that the user intends to edit (or show a
> different page, as is the current case).

The profile field name in the query string might work?

> > * I don't think the pref is needed.
> 
> Let's keep it in for now, at least until the support for WebChannel makes it
> to production on the content server.

No worries - although maybe you can still remove it from firefox.js (which just means catching an exception when you get the value).  But whatevs :)
(Assignee)

Comment 8

2 years ago
Created attachment 8573999 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane
Attachment #8572955 - Attachment is obsolete: true
Attachment #8572955 - Flags: ui-review?(rfeeley)
Attachment #8573999 - Flags: ui-review?
(Assignee)

Comment 9

2 years ago
Created attachment 8574396 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane
Attachment #8573999 - Attachment is obsolete: true
Attachment #8573999 - Flags: ui-review?
Attachment #8574396 - Flags: ui-review?
It works! Can the profile picture in the menu be encircled with adjusted padding like this? http://people.mozilla.org/~rfeeley/images/avatar-circle-padding.gif
Attachment #8574396 - Flags: ui-review? → ui-review?(rfeeley)
Comment on attachment 8574396 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

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

Circle crop for a plus.
Attachment #8574396 - Flags: ui-review?(rfeeley) → ui-review-
(Assignee)

Comment 12

2 years ago
Comment on attachment 8574396 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

The hamburger menu has its own issue here: https://bugzilla.mozilla.org/show_bug.cgi?id=1139698. I'll make a note that we should use circles there, too.

As far as the pref pane is concerned, what do you think, Ryan?
Attachment #8574396 - Flags: ui-review- → ui-review?(rfeeley)
(Assignee)

Updated

2 years ago
Attachment #8574396 - Flags: review?(mhammond)
Pref pane looks great!
Comment on attachment 8574396 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

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

This looks pretty good, but the "old" style prefs aren't handled.  Personally I can live with that, but there is a (small IIUC) risk that in-content prefs will not make the next train, so we'd want approval from someone that we can live without the profile image for the entire next release train if that happens, or we'd need similar treatment to that dialog-based code.  So only f+ until we've a plan there.

(I also added ui-review+ given comment 13 - rfeeley is still learning his way around these flags ;)

::: browser/components/preferences/in-content/sync.js
@@ +294,5 @@
> +        document.getElementById("fxaProfileImage").style.backgroundImage = "";
> +
> +        if (data.avatar) {
> +          let img = new Image();
> +          // Make sure the image is available before displaying it

please extend this comment with something like ", as we don't want to overwrite the default profile image with a broken/unavailable image"

@@ +303,5 @@
> +          img.src = data.avatar;
> +        }
> +      };
> +
> +      fxAccounts.getSignedInUserProfile().then(data => {

getSignedInUserProfile() isn't part of this patch - I recall it flying past before, but the name seems unfortunate given its use in this context - it *sounds* like it will just return profile data but it seems to be returning the user data *and* the profile data - any chance of renaming that to something like getSignedInUserDataWithProfile() or similar?

@@ +305,5 @@
> +      };
> +
> +      fxAccounts.getSignedInUserProfile().then(data => {
> +        doUpdate(data);
> +      },

I think this should ready:
fxAccounts.getSignedInUserProfile().then(data => {
  doUpdate(data);
}.catch(err) {
  log.error(err); // (actually, make this log.error("Failed to update profile", err);
});

As IIUC, the way you have written it will only catch errors in getSignedInUserProfile, not in doUpdate, whereas the above will catch both.

::: services/fxaccounts/FxAccounts.jsm
@@ +972,5 @@
>    },
>  
> +  // Returns a promise that resolves with the URL to use to change
> +  // the current account's profile image.
> +  promiseAccountsChangeProfileURI: function(editSetting) {

it looks like you expect editSetting to be not specified (maybe just in the future) - if so, you might as well add a default (eg, editSetting = null) - and also add a comment for what this is for.  If you don't expect it to ever be called without editSetting then I wouldn't bother checking it is truthy below - just let it blow up if someone calls it incorrectly.
Attachment #8574396 - Flags: ui-review?(rfeeley)
Attachment #8574396 - Flags: ui-review+
Attachment #8574396 - Flags: review?(mhammond)
Attachment #8574396 - Flags: feedback+
(Assignee)

Comment 15

2 years ago
Since this is more experimental I think we'd be okay if in-content prefs don't make it, but let's ask rfk.
Flags: needinfo?(rfkelly)
(Assignee)

Updated

2 years ago
Blocks: 1147047
(Assignee)

Comment 16

2 years ago
(In reply to Mark Hammond [:markh] from comment #14)
> Comment on attachment 8574396 [details] [diff] [review]
> Display the user's FxA profile image in the Sync Pref pane
> 
> Review of attachment 8574396 [details] [diff] [review]:
> -----------------------------------------------------------------
...
> 
> @@ +303,5 @@
> > +          img.src = data.avatar;
> > +        }
> > +      };
> > +
> > +      fxAccounts.getSignedInUserProfile().then(data => {
> 
> getSignedInUserProfile() isn't part of this patch - I recall it flying past
> before, but the name seems unfortunate given its use in this context - it
> *sounds* like it will just return profile data but it seems to be returning
> the user data *and* the profile data - any chance of renaming that to
> something like getSignedInUserDataWithProfile() or similar?

Yeah, "verified" is the odd attribute out– "uid" and "email" appear in both profile and account data (although  the value of "email" could potentially diverge once/if we support multiple email addresses). I feel like we should perhaps refactor so that getSignedInUserProfile rejects with the appropriate error (NO_ACCOUNT or UNVERIFIED_ACCOUNT) and let the consumer handle it (the Sync pref UI code, in this case) instead of relying on the account data being null or verified being false. If the account is unverified, the consumer can a separate call to retrieve the account data and display that instead.
> This looks pretty good, but the "old" style prefs aren't handled.  Personally I can live with that, but there is a (small IIUC) risk that in-content prefs will not make the next train, so we'd want approval from someone that we can live without the profile image for the entire next release train if that happens, or we'd need similar treatment to that dialog-based code.

What release is in-content prefs targeting? I heard 38.

This avatar stuff would probably be in 39, correct?
Flags: needinfo?(mhammond)
(In reply to Chris Karlof [:ckarlof] from comment #17)
> What release is in-content prefs targeting? I heard 38.
> 
> This avatar stuff would probably be in 39, correct?

That's my understanding too, but I've been told that for stuff like readinglist I must keep the old prefs current "just incase".  We probably don't have to be quite as strict for this given it's just fluff.
Flags: needinfo?(mhammond)
As long as we're not *breaking* the old-style prefs pane, I don't see a problem with shipping it avatar-less for a cycle if we need to.
Flags: needinfo?(rfkelly)
(Assignee)

Comment 20

2 years ago
Created attachment 8582815 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

This version refactors getSignedInUserProfile to only return profile data. It will
reject with an error that the UI code handles when there's no account
or it's unverified.
Attachment #8574396 - Attachment is obsolete: true
Attachment #8582815 - Flags: ui-review+
(Assignee)

Updated

2 years ago
Attachment #8582815 - Flags: review?(mhammond)
Comment on attachment 8582815 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

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

This looks good, apart from the sync.js changes which have crossed a (vague, subjective) complexity threshold and I think my suggestion will make it simpler to reason about (but let me know if I've missed something there)

::: browser/components/preferences/in-content/sync.js
@@ +284,5 @@
>            checkbox.disabled = enginesListDisabled;
>          }
> +
> +        // Clear the profile image (if any) of the previously logged in account.
> +        document.getElementById("fxaProfileImage").style.backgroundImage = "";

I think document.getElementById("fxaProfileImage").style.removeProperty("background-image") is the preferred way of doing that.

@@ +304,5 @@
> +        doUpdate(data);
> +      }).catch(err => {
> +        FxAccountsCommon.log.error(err);
> +
> +        if (err.message === FxAccountsCommon.ERROR_NO_ACCOUNT) {

TBH I'm not that keen on these changes and the "retry" of doUpdate.  Can we rejig this to something like:

      fxAccounts.getSignedInUser().then(data => {
        ... all the existing code.
      }).then(() => {
        return fxAccounts.getSignedInUserProfile();
      }).then(profileData => {
        ... just do the profile thing
      }, err => {
         ... handle getSignedInUserProfile() failing (ie, no account, not verified)
      }).catch(err => {
        ... just log the error - it should be "impossible" to get here - it could even be left off as the existing code has no such .catch (but I think it makes sense to have it - maybe just Cu.reportError() rather than logging, as the error may not be FxA related at all.
      });
Attachment #8582815 - Flags: review?(mhammond) → review-
> We probably don't have to be quite as strict for this given it's just fluff.

I'm just going to assume that "fluff" is Australian for "really fantastic engineering".
> I'm just going to assume that "fluff" is Australian for "really fantastic engineering".

Am Australian, can confirm...
(In reply to Chris Karlof [:ckarlof] from comment #22)

> I'm just going to assume that "fluff" is Australian for "really fantastic
> engineering".

:) Seriously though, I hope I haven't offended Zaach here - I meant "visual awesomeness rather than new required functionality" :)
> :) Seriously though, I hope I haven't offended Zaach here - I meant "visual awesomeness rather than new required functionality" :)

I doubt it.

I was just trying to de-escalate to avoid anyone going all "Mark Hammond" on the bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1139166#c27). :)
(Assignee)

Comment 26

2 years ago
No worries, Mark. I knew what you meant :)

(In reply to Mark Hammond [:markh] from comment #21)
> Comment on attachment 8582815 [details] [diff] [review]
> Display the user's FxA profile image in the Sync Pref pane
> 
> Review of attachment 8582815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, apart from the sync.js changes which have crossed a (vague,
> subjective) complexity threshold and I think my suggestion will make it
> simpler to reason about (but let me know if I've missed something there)
> 
> ::: browser/components/preferences/in-content/sync.js
> @@ +284,5 @@
> >            checkbox.disabled = enginesListDisabled;
> >          }
> > +
> > +        // Clear the profile image (if any) of the previously logged in account.
> > +        document.getElementById("fxaProfileImage").style.backgroundImage = "";
> 
> I think
> document.getElementById("fxaProfileImage").style.removeProperty("background-
> image") is the preferred way of doing that.
> 
> @@ +304,5 @@
> > +        doUpdate(data);
> > +      }).catch(err => {
> > +        FxAccountsCommon.log.error(err);
> > +
> > +        if (err.message === FxAccountsCommon.ERROR_NO_ACCOUNT) {
> 
> TBH I'm not that keen on these changes and the "retry" of doUpdate.  Can we
> rejig this to something like:
> 
>       fxAccounts.getSignedInUser().then(data => {
>         ... all the existing code.

Hm, which existing code do you mean?

>       }).then(() => {
>         return fxAccounts.getSignedInUserProfile();
>       }).then(profileData => {
>         ... just do the profile thing
>       }, err => {
>          ... handle getSignedInUserProfile() failing (ie, no account, not
> verified)

getSignedInUser above would tell us whether the account was absent or unverified, so if we handle those states there this could just log whatever error occured. Or we can defer that to the handler below.

>       }).catch(err => {
>         ... just log the error - it should be "impossible" to get here - it
> could even be left off as the existing code has no such .catch (but I think
> it makes sense to have it - maybe just Cu.reportError() rather than logging,
> as the error may not be FxA related at all.
>       });
(Assignee)

Comment 27

2 years ago
Created attachment 8583683 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

Implemented markh's suggestions
Attachment #8582815 - Attachment is obsolete: true
Attachment #8583683 - Flags: ui-review+
(Assignee)

Updated

2 years ago
Attachment #8583683 - Flags: review?(mhammond)
Comment on attachment 8583683 [details] [diff] [review]
Display the user's FxA profile image in the Sync Pref pane

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

Yeah, I think that's much better - thanks!
Attachment #8583683 - Flags: review?(mhammond) → review+
https://hg.mozilla.org/mozilla-central/rev/ae08455b713c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
The default-profile-image.svg isn't copied to skin/classic/aero/browser/preferences/in-content/
(Assignee)

Updated

2 years ago
Blocks: 1148565
You need to log in before you can comment on or make changes to this bug.