Closed Bug 1098661 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set
normal
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.2

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file, 3 obsolete files)

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+
Here's a really dumb WIP - but it actually works and drives migration to completion!
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
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)
Iteration: --- → 37.1
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 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.
Iteration: 37.1 → 37.2
Duplicate of this bug: 1110336
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
This addresses all previous review comments.
Attachment #8528142 - Attachment is obsolete: true
Attachment #8535270 - Flags: review?(adw)
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+
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
https://hg.mozilla.org/mozilla-central/rev/b09fa41b6428
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Are there UX specs available anywhere? What is "Forget" supposed to do?
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.
(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)
(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)
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.