Update sync in-content preferences UI to match FxA migration flows

VERIFIED FIXED in Firefox 37

Status

()

Firefox
Sync
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

unspecified
Firefox 37
Points:
5
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
The UX flows for migration call for the sync prefs panes to be updated, and I can't find an existing bug for that.
Flags: qe-verify+
Flags: firefox-backlog+
(Assignee)

Comment 1

3 years ago
Created attachment 8525090 [details] [diff] [review]
0009-Bug-1098661-very-rough-wip-for-sync-prefs.patch

Here's a really dumb WIP - but it actually works and drives migration to completion!
(Assignee)

Comment 2

3 years ago
Created attachment 8526560 [details] [diff] [review]
0009-Bug-1098661-very-rough-wip-for-sync-prefs.patch

Updated to reflect the changes in bug 1019985.  It still the "upgrade" button and now also includes the "resend" and "forget" buttons.
Attachment #8525090 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Created attachment 8528142 [details] [diff] [review]
0010-Bug-1098661-very-rough-wip-for-sync-prefs.patch

I've updated this to use localizable strings and css instead of everything being inline.  Still only incontent-prefs, but I figure it's worth getting feedback before I copy-paste it all into the windowed prefs.
Assignee: nobody → mhammond
Attachment #8526560 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8528142 - Flags: feedback?(adw)

Updated

3 years ago
Iteration: --- → 37.1

Comment 4

3 years ago
Comment on attachment 8528142 [details] [diff] [review]
0010-Bug-1098661-very-rough-wip-for-sync-prefs.patch

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

Looks good to me.

::: browser/components/preferences/in-content/sync.js
@@ +10,5 @@
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "fxaMigrator", function () {
> +  let {fxaMigrator} = Components.utils.import("resource://services-sync/FxaMigrator.jsm");
> +  return fxaMigrator;

defineLazyModuleGetter?

@@ +105,5 @@
>      topics.forEach(function (topic) {
>        Weave.Svc.Obs.add(topic, this.updateWeavePrefs, this);
>      }, this);
> +    // The FxA migration observer is a special case.
> +    Weave.Svc.Obs.add(migrateTopic, this.updateMigrationState, this);

You might want to pass in updateMigrationState bound to `this` to avoid surprises in the future when changing updateMigrationState, even though you don't use `this` in updateMigrationState right now.

@@ +293,5 @@
> +  // *sob* - Sync's Svc.obs wrapper omits |topic| from the args!
> +  updateMigrationState: function(subject, state) {
> +    if (!state) {
> +      // no migration action necessary.
> +      document.getElementById("sync-migration").setAttribute("hidden", "true");

.hidden = true

@@ +304,5 @@
> +        selIndex = 0;
> +        initPromise = Promise.resolve();
> +        break;
> +      case fxaMigrator.STATE_USER_FXA_VERIFIED:
> +        Cu.import("resource://gre/modules/FxAccounts.jsm");

I'd prefer a defineLazyModuleGetter up top.

@@ +314,5 @@
> +        );
> +        selIndex = 1;
> +        break;
> +      default:
> +        Cu.reportError("updateMigrationState has unknown state!");

Nit: You could catch the !state case here too to avoid the special case at the top.  You could still log an error when !!state.

@@ +315,5 @@
> +        selIndex = 1;
> +        break;
> +      default:
> +        Cu.reportError("updateMigrationState has unknown state!");
> +        // I guess hiding everything is the right thing here?

I think so.

@@ +320,5 @@
> +        document.getElementById("sync-migration").setAttribute("hidden", "true");
> +        return;
> +    }
> +    initPromise.then(() => {
> +      document.getElementById("sync-migration").removeAttribute("hidden");

.hidden = false

::: browser/components/preferences/in-content/sync.xul
@@ +54,5 @@
> +
> +      <!-- When we are in the "need the user to be verified" state -->
> +      <hbox align="center">
> +        <description>
> +          &migrate.verifyNeeded.before;<label id="sync-migration-address"/>&migrate.verifyNeeded.after;

Hmm, a property string + formatStringFromName would be cleaner.
Attachment #8528142 - Flags: feedback?(adw) → feedback+

Comment 5

3 years ago
Comment on attachment 8528142 [details] [diff] [review]
0010-Bug-1098661-very-rough-wip-for-sync-prefs.patch

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

::: browser/components/preferences/in-content/sync.js
@@ +314,5 @@
> +        );
> +        selIndex = 1;
> +        break;
> +      default:
> +        Cu.reportError("updateMigrationState has unknown state!");

Should included the unknown state in the message, too.

Updated

3 years ago
Iteration: 37.1 → 37.2

Updated

3 years ago
Duplicate of this bug: 1110336
(Assignee)

Comment 7

3 years ago
I'm going to make this just in-content preferences as the "old" preferences UI will need additional UX love before it can be landed.
Summary: Update sync preferences to match migration flows → Update sync in-content preferences UI to match FxA migration flows
(Assignee)

Comment 8

3 years ago
Created attachment 8535270 [details] [diff] [review]
0001-Bug-1098661-Update-sync-in-content-preferences-UI-to.patch

This addresses all previous review comments.
Attachment #8528142 - Attachment is obsolete: true
Attachment #8535270 - Flags: review?(adw)

Comment 9

3 years ago
Comment on attachment 8535270 [details] [diff] [review]
0001-Bug-1098661-Update-sync-in-content-preferences-UI-to.patch

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

Looks fine, the only thing is that ideally we would share strings between this bug and bug 1016825 instead of duplicating them.  Do you think that would be possible?
Attachment #8535270 - Flags: review?(adw) → review+
(Assignee)

Comment 10

3 years ago
Pushed with de-duplication of strings.

https://hg.mozilla.org/integration/fx-team/rev/069da43ec3be
Backed out in https://hg.mozilla.org/integration/fx-team/rev/a76b8d5ee8dc - one of the two things in that push, or both, was leaking prefs windows in bc1 and bc3
(Assignee)

Comment 12

3 years ago
Repushed with a fix to the observer removal - this was causing the leak.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90ea0683e9e5

https://hg.mozilla.org/integration/fx-team/rev/b09fa41b6428
https://hg.mozilla.org/mozilla-central/rev/b09fa41b6428
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Are there UX specs available anywhere? What is "Forget" supposed to do?

Comment 15

3 years ago
Here's the spec for the migration state with the Forget button: https://www.dropbox.com/s/0l8aj6sjgjz0xbw/Sync%20Preverified.pdf

Here's the spec for the state before that, where the user needs to upgrade to an FxA account: https://www.dropbox.com/s/qvhgcv2u9tdltpn/Sync%20Urgent%20Upgrade.pdf

The "existing Forget this Email feature" the first one mentions is the link in the Sync pref pane that effectively cancels FxA account creation while Firefox is waiting for the user verify his account.  That link's string is here: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/sync.dtd#74
Thanks, this definitely helps. I wonder if the action for "Forget" is clear enough, but the available space is also limited.
(Assignee)

Comment 17

3 years ago
(In reply to Francesco Lodolo [:flod] from comment #14)
> Are there UX specs available anywhere? What is "Forget" supposed to do?
...
> Thanks, this definitely helps. I wonder if the action for "Forget" is clear
> enough, but the available space is also limited.

I agree "Forget" isn't obvious.  "Logout" seems the most logical as this is only used in the context of an FxAccount and only after being logged in (not verified, but still logged in).  Even worse though, after a "forget", the previous account name is still used as the default - so "Forget" is actually inaccurate.

needinfo Ryan for his consideration.
Flags: needinfo?(rfeeley)
Mark and I just discussed that “Reset” is a good option as it reverts to the pre-migratory state.
Flags: needinfo?(rfeeley)
(Assignee)

Comment 19

3 years ago
(In reply to Ryan Feeley from comment #18)
> Mark and I just discussed that “Reset” is a good option as it reverts to the
> pre-migratory state.

But then we became less sure :)  I think we agreed to let it slide for now and make a decision based on further feedback as the migration UX is seen by more eyes.
QA Contact: camelia.badau
Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 37.0a1 (buildID: 20141228030216) and the following mentions should be done here: 
- I've created a new account (which was not verified) -> on about:preferences#sync, I have "Verify Email" and "Forget this Email", but I don't see this inline notification: "A verification email awaits at user@domain" (with "Start Over" and "Resend" buttons) - it is expected? 
- I've logged in with an old account -> I don't see this inline notification : "We’ve rebuilt Sync to make it easier for everyone. Please upgrade to a Firefox Account to continue syncing." (with "Unlink Sync" and "Upgrade" buttons) - it is ok? when should I see this notification? 

Is there something I'm missing here?
Flags: needinfo?(mhammond)
(Assignee)

Comment 21

3 years ago
This really can't be tested until more of the sync migration stuff lands and sync is configured with special test servers, so I believe everything mentioned above is expected.  I'm verifying this and we will ensure there is a QA process so the entire migration flow can be effectively tested.
Status: RESOLVED → VERIFIED
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(mhammond)
You need to log in before you can comment on or make changes to this bug.