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

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

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

The UX flows for migration call for the sync prefs panes to be updated, and I can't find an existing bug for that.

Comment 1

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

Attachment: 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!

Comment 2

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

Attachment: 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.

Comment 3

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

Attachment: 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.

Comment 4

[Drew Willcoxon :adw]

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.

Comment 5

[Drew Willcoxon :adw]

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.

Comment 7

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

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.

Comment 8

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

Attachment: 0001-Bug-1098661-Update-sync-in-content-preferences-UI-to.patch

This addresses all previous review comments.

Comment 9

[Drew Willcoxon :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?

Comment 10

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

Pushed with de-duplication of strings.

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

Comment 11

[Phil Ringnalda (:philor)]

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

Comment 12

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

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

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Francesco Lodolo [:flod]]

Are there UX specs available anywhere? What is "Forget" supposed to do?

Comment 15

[Drew Willcoxon :adw]

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

Comment 16

[Francesco Lodolo [:flod]]

Thanks, this definitely helps. I wonder if the action for "Forget" is clear enough, but the available space is also limited.

Comment 17

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

(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.

Comment 18

[Ryan Feeley [:rfeeley]]

Mark and I just discussed that “Reset” is a good option as it reverts to the pre-migratory state.

Comment 19

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

(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.

Comment 20

[Camelia Badau [:cbadau], Desktop Test Engineering]

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?

Comment 21

[Mark Hammond [:markh] [: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.