1110336 - Update sync old-dialog-based preferences UI to match FxA migration flows

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Drew Willcoxon :adw]

+++ This bug was initially created as a clone of Bug #1016825 +++

We should show legacy-Sync-to-Firefox-Accounts migration notifications in the preferences window and in-content preferences.

Comment 1

[Drew Willcoxon :adw]

Oops, this is bug 1098661.

Comment 2

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

Let's make this about the old-school dialog-based prefs.

Ryan, the "info bar" at the top of the sync pane doesn't fit in the old-school dialog window - the text "The sync account system is being discontinued. A new Firefox Account is required to sync." alone means the "upgrade" button isn't visible.  If we consider the "needs verification" state, the fact we include the user's email address and *two* buttons means that doesn't fit either.

So I'm guessing we will want the buttons on a second row?  Can you please let us know what should happen here?

Comment 3

[Drew Willcoxon :adw]

Mark, the specs show the dialog prefs infobar looking basically like the main window infobar.  The WIP patch you gave me is very helpful, but it draws the infobar like the in-content prefs infobar.  I think we should try to make it look more like main window infobar to match the spec.  It also shouldn't be a problem to make the text wrap.  This PDF in particular shows the text wrapping: https://www.dropbox.com/s/qvhgcv2u9tdltpn/Sync%20Urgent%20Upgrade.pdf

Comment 4

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

(In reply to Drew Willcoxon :adw from comment #3)
> Mark, the specs show the dialog prefs infobar looking basically like the
> main window infobar.  The WIP patch you gave me is very helpful, but it
> draws the infobar like the in-content prefs infobar.  I think we should try
> to make it look more like main window infobar to match the spec.  It also
> shouldn't be a problem to make the text wrap.  This PDF in particular shows
> the text wrapping:
> https://www.dropbox.com/s/qvhgcv2u9tdltpn/Sync%20Urgent%20Upgrade.pdf

Sounds fine to me!  Note that the incontent patch which just landed also doesn't wrap when the window is too small, so we probably need a fix there too.

Comment 5

[Ryan Feeley [:rfeeley]]

Here is the yield/caution/warning icon you are looking for! http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/identity-icons-https-mixed-display.png

Comment 6

[Drew Willcoxon :adw]

Ryan, thanks, but as I said, we do have images on OS X that look like the images in your spec, but they're low-DPI only.  I don't think it's acceptable to not have 2x DPI images on 2x DPI Mac screens and other 2x DPI screens.

And my needinfo for this is on bug 1110009 comment 3, not this bug, but I'll mark needinfo for you again here so hopefully you will see it...

Comment 7

[Drew Willcoxon :adw]

Attachment: WIP patch, adds notificationbox to preferences.xul

Here's a WIP.  You can't add an infobar to a prefpane because it looks wrong.  The infobar is in the pane, surrounded by the pane's padding.  So this adds it -- a notificationbox precisely -- to preferences.xul and makes prefwindow specially recognize and position notificationboxes.  That's a little icky but not too much because it tailors prefwindow to this bug but not too closely IMO.  Other panes could use it too if they want.

I need to make it so that the infobar appears only when the sync prefpane is selected, so for that, I'll have to add some kind of paneselected event to prefwindow I think.  The Sync pane would listen for it and show and hide the infobar appropriately.  And an infobar property getter on prefwindow.

Also, the text label inside the infobar is not centered vertically it looks like.  Don't know what's up with that.

This patch builds on Mark's WIP patch that I'll post next.

Comment 8

[Drew Willcoxon :adw]

Attachment: Mark's WIP patch, uses same styling as in-content prefs infobar

Comment 9

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

Comment on attachment 8539604 [details] [diff] [review]
WIP patch, adds notificationbox to preferences.xul

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

I don't really understand why we really need a notificationbox here?  Can't we just stick with some elements which are hidden by default, the same as used by incontent prefs?

Comment 10

[Drew Willcoxon :adw]

In comment 4 we agreed, I thought, that we would try to match Ryan's spec, which shows an infobar that looks like the main window's infobar.  Given that, I'd prefer to have the infobar in the Sync prefpane instead of in the prefwindow, but as I said in comment 7, it looks wrong.

Comment 11

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

Comment on attachment 8539604 [details] [diff] [review]
WIP patch, adds notificationbox to preferences.xul

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

Yeah, I can see the simpler option I was advocating isn't going to look anything like the mockups

Comment 12

[Drew Willcoxon :adw]

Attachment: patch on top of Mark's WIP patch

Mark, this is based off your WIP patch that I posted earlier.  I'd like to land your patch and then land this on top so that you get credit for the fact that you wrote most of this.  Next, I'll post a complete diff containing both patches as if they were one, in case that helps you review this.

As we discussed, this simply ports your in-content patch to the dialog-based prefs and forgets about using a real notificationbox or anything that looks like it, and we can follow up with Ryan later about maybe simplifying his UI proposal so it's simpler to implement.

I tried to match your in-content changes as closely as possible, including the recent changes you made in recent bugs.  A lot is copied and pasted from your in-content patch.

Some other things:

The window has to be resized to account for the warning appearing and disappearing.

I didn't find that the <description> needs a min-width in order to wrap.  Maybe it's because I'm on OS X, or because this is in a prefwindow and not in content?

The `#sync-migration description` CSS is to make the description not hug the warning box, which looks bad.  I just eyeballed 8px.

Comment 13

[Drew Willcoxon :adw]

Attachment: combined patch (Mark's + Drew's)

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 8539605 [details] [diff] [review]
Mark's WIP patch, uses same styling as in-content prefs infobar

r+ to this with the reasoning in comment 12.

Comment 15

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

Comment on attachment 8545590 [details] [diff] [review]
combined patch (Mark's + Drew's)

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

I haven't tried it, but LGTM!

::: browser/components/preferences/sync.js
@@ +109,5 @@
>        topics.forEach(function (topic) {
>          Weave.Svc.Obs.remove(topic, this.updateWeavePrefs, this);
>        }, gSyncPane);
> +      Weave.Svc.Obs.remove(migrateTopic, this.updateMigrationState, this);
> +    }.bind(this), false);

I think that the addition of the .bind(this) means the second param (gSyncPane) to the .forEach above redundant - I don't really care if you remove it or not though

Comment 16

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

(In reply to Drew Willcoxon :adw from comment #12)
> Mark, this is based off your WIP patch that I posted earlier.  I'd like to
> land your patch and then land this on top so that you get credit for the
> fact that you wrote most of this.

credit or blame? ;)

That's fine with me, but it's also fine with me if you land them rolled up with you as the author.  So r=me on whatever parts need it!

Comment 17

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

Comment on attachment 8545590 [details] [diff] [review]
combined patch (Mark's + Drew's)

hrm - actually, I spoke a little too soon :(

I think we do need the FxaMigrator change, so we can still call this.openContentInBrowser from sync.js.  This is important for 2 reasons:

* On Mac, the prefs window is not modal, so there is a small chance this is the last window open - so the getMostRecentWindow() in migrateUpgrade will fail.

* On Windows, the prefs window *is* modal - openContentInBrowser explicitly closes the prefs dialog as the user is unable to interact with the tab browser until is closes.  I guess migrateUpgrade could just do this too, but that still leaves the problem above.

Assuming I'm not missing anything, it *might* also make sense to remove createFxAccount entirely, and have the existing callers just call getFxAccountOptions() and open it however they see fit - but that could be done in a followup.  Also, now I look at it again, getFxAccountOptions() might be better named as getCreateFxAccountOptions() or something.

Comment 18

[Drew Willcoxon :adw]

Attachment: patch on top of Mark's WIP patch v2

(In reply to Mark Hammond [:markh] from comment #15)
> ::: browser/components/preferences/sync.js
> @@ +109,5 @@
> >        topics.forEach(function (topic) {
> >          Weave.Svc.Obs.remove(topic, this.updateWeavePrefs, this);
> >        }, gSyncPane);
> > +      Weave.Svc.Obs.remove(migrateTopic, this.updateMigrationState, this);
> > +    }.bind(this), false);
> 
> I think that the addition of the .bind(this) means the second param
> (gSyncPane) to the .forEach above redundant

Good point, it's probably obvious, but I added the bind because I noticed your WIP references `this` without binding.  Using bind seems more likely to avoid bugs like that than having to remember to use gSyncPane, so I left the bind and changed the forEach to use an arrow function so we don't have to pass `this` or gSyncPane to forEach at all.

(In reply to Mark Hammond [:markh] from comment #16)
> credit or blame? ;)

:-) Actually most of "my" patch is copied and pasted from your in-content patch, so I feel guilty even having my name on that.

(In reply to Mark Hammond [:markh] from comment #17)
> hrm - actually, I spoke a little too soon :(

Ah, good catch.

> Assuming I'm not missing anything, it *might* also make sense to remove
> createFxAccount entirely

Hmm, I left createFxAccount for now.  I think it's fine to keep it as a convenience function given a window, and also it's nice to be able to call one function instead of getFxAccountOptions().then(...).  But I'm definitely not opposed to removing it either, especially if it ends up not getting called in many places... which looks to be true.

I noticed all the log.warn()s mentioned "createFxAccount", so I fixed those.

> Also, now I look at it again, getFxAccountOptions() might be better
> named as getCreateFxAccountOptions() or something.

Renamed it getFxAccountCreationOptions.

Comment 19

[Drew Willcoxon :adw]

Attachment: combined patch (Mark's + Drew's) v2

Comment 20

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

Comment on attachment 8546106 [details] [diff] [review]
patch on top of Mark's WIP patch v2

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

awesome!

Comment 21

[Drew Willcoxon :adw]

Mark's patch (part 1): https://hg.mozilla.org/integration/fx-team/rev/df2c68a00ffb
My patch (part2): https://hg.mozilla.org/integration/fx-team/rev/0d113455b05c

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df2c68a00ffb
https://hg.mozilla.org/mozilla-central/rev/0d113455b05c

Comment 23

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 37.0b6
Build Id: 20150316202753
OS: Win 7 x64, Ubuntu 14.04 x86, Mac Os X 10.7.5

Only a minor display issue was found specific to Ubuntu, bug 1144090 was logged for that issue.