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

VERIFIED FIXED in Firefox 37

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

unspecified
Firefox 37
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

+++ 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.
Flags: qe-verify+
Flags: firefox-backlog+
Oops, this is bug 1098661.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1098661
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?
Status: RESOLVED → REOPENED
Flags: needinfo?(rfeeley)
Resolution: DUPLICATE → ---
Summary: Show legacy-Sync-to-Firefox-Accounts migration notifications in the preferences window and in-content preferences → Update sync old-dialog-based preferences UI to match FxA migration flows
Assignee: nobody → adw
Status: REOPENED → ASSIGNED
Iteration: --- → 37.2
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
(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.
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
Flags: needinfo?(rfeeley)
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...
Flags: needinfo?(rfeeley)
Flags: needinfo?(rfeeley)
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.
Attachment #8539604 - Flags: feedback?(mhammond)
Iteration: 37.2 → 37.3
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?
Attachment #8539604 - Flags: feedback?(mhammond)
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 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
Attachment #8539604 - Flags: feedback+
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.
Attachment #8539604 - Attachment is obsolete: true
Attachment #8545589 - Flags: review?(mhammond)
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.
Attachment #8539605 - Flags: review+
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
Attachment #8545590 - Flags: review+
(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 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.
Attachment #8545590 - Flags: review+
(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.
Attachment #8545589 - Attachment is obsolete: true
Attachment #8545590 - Attachment is obsolete: true
Attachment #8545589 - Flags: review?(mhammond)
Attachment #8546106 - Flags: review?(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!
Attachment #8546106 - Flags: review?(mhammond) → review+
https://hg.mozilla.org/mozilla-central/rev/df2c68a00ffb
https://hg.mozilla.org/mozilla-central/rev/0d113455b05c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
QA Contact: twalker
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.