1168185 - Sync UI

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Sync, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

This bug is to track the design and implementation of the UI bits required for Firefox Sync on FxOS. We need something similar to about:preferences#sync on desktop to allow the user to configure the sync client and access the associated information.

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Attachment: WIP patch for Data Sync configurations in Settings app

In this patch, I add an entry Firefox Sync for Data Sync configurations as the following:
* Enable/Disable Sync
* Sync Status and Last Sync Time
* Collection Selections - Bookmarks, History, Password, etc.

and we need UX team's support to finalize the UI.

Comment 2

[Sean Lee [:seanlee][:weilonge]]

Hi Fernando, Michiel, 

Based on the discussion in bug 1191770, I would like to take Evelyn's advice to implement the switch for enabling/disabling Firefox Sync in Settings app.

Besides, the implementation in this bug is just to provide the skeleton of FxSync UI in Settings app, and we will have to implement the behavior of manipulating the checkboxs mentioned at comment 1 in another bug.

Eventually, we will refine the UI after the UX specification cooked.

Could you give me some suggestions on this?
(Although Fernando is in PTO, he is probably interested in this :) )

Thank you!

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] weilonge:seanlee/DataSync/master/Bug1168185 > mozilla-b2g:master

Comment 4

[Michiel de Jong [:michielbdejong]]

A build-time switch has advantages over a runtime switch: it reduces the size of the code when features are disabled. And even a runtime switch does not need to have any UI in the settings app.

Consider this code:

````js
const SYNCTO_ENABLED = false;
if (SYNCTO_ENABLED) {
  // our code goes here
}
````

That would be pretty safe to land in master, even if we're not launching the feature.
Enabling the feature would then be a one-line PR changing 'false' to 'true'.

So my preference would be:
best option: change the gaia Makefile to support command-line options for enabling/disabling features
second option: have a const in the code which we set to false to disable the feature
third option: do it in the settings app.

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Let's move to bug 1191770 and discuss it. :)

Comment 6

[Michiel de Jong [:michielbdejong]]

Ah yes, sorry, I had both bugs open and commented on the wrong one!

Comment 7

[Sean Lee [:seanlee][:weilonge]]

Hi Michiel,

Thanks for your comments at bug 1191770.
The flag implementation will use the settings way to enable and disable Data Sync feature.

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Attachment: Screen Shot 2015-08-18 at 19.39.37.png

Hi Juwei,

The attachment is the screenshot of Firefox Sync configuration panel, and here is the POC video of Data Sync:
https://www.youtube.com/watch?v=zmFInfOt6Ms

In this bug, I would like to implement the skeleton of Firefox Sync panel, and it's hidden by default.
That would be helpful for us to implement non-UI components in our architecture.
In our near plan, Browser history synchronizing feature will be implemented first.

Could you give some suggestions from UX perspective?

Thank you.

Comment 9

[Juwei Huang[:juwei]]

Looks good to me!
Thank you!

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Sean Lee [:seanlee] from comment #2)
> Hi Fernando, Michiel, 
> 
> Based on the discussion in bug 1191770, I would like to take Evelyn's advice
> to implement the switch for enabling/disabling Firefox Sync in Settings app.

We already discussed about this in person and reopened bug 1191770, so I am clearing the needinfo. Thanks Sean.

Comment 11

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8648760 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1168185 > mozilla-b2g:master

Hi Evelyn,

Could you help to review my patch? Thank you!

Comment 12

[Evelyn Hung]

Comment on attachment 8648760 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1168185 > mozilla-b2g:master

Sorry for my late review. I know this is just a skeleton of Data Sync setting panel, however, I'd like to see more concrete logic of dealing with corresponding settings. Some settings might have complicated logic as its control, but some might be just key/value pairs which we could add now. Please reply on Github to give me more context of each setting. Thanks.

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Per the discussion with Fernando, he will do the integration task following my patch, and I will focus on History Adapter. Thanks a lot Fernando's support! :)

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Alternative design

Hello Juwei, I modified the design a little bit to match Firefox Sync on Android and Firefox Accounts on Firefox OS as much as possible. Please, let me know what do you think. Thanks!

Now we'll have three possible screens (I am describing them from left to right of the screenshot):

1. "Logged out user" screen. This one matches the one we have on Firefox for Android. Here we introduce Sync to the user and allow her to start using it.

2. "Logged in user with unverified account" screen. This will be presented to the user when the account she used to log into Sync is an unverified Firefox Accounts. Note that this screen will not appear on 2.5 because of bug 1205292.

3. "Logged in user" screen. Here we show the user the account she is using with Sync, we allow her to sign out, to request a synchronization on demand and to select the collections she wants to synchronize. We also show the last time a successful synchronization was made and the links to the ToS and the privacy notice.

Comment 15

[Michiel de Jong [:michielbdejong]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #14)
> we allow her to sign out

Would that also sign her out of Marketplace and FindMyDevice? Maybe replace this with 'Disable sync', and leave signing out of FxA to the FxA screens?

Maybe missing (assuming we launch Sync as opt-in instead of opt-out):
"Logged in to FxA but Sync disabled"
This screen would also be needed after a user disables sync in screen 3.
Of course, it can be just screen 3 but with 'Enable sync' instead of the current 'Sign out', and the rest of the inputs disabled / greyed out. 'Disable sync'/'Enable sync' can also be a switch input, like in https://www.youtube.com/watch?v=BEYgx5chun0&feature=youtu.be&t=180

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Michiel de Jong [:michielbdejong] from comment #15)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #14)
> > we allow her to sign out
> 
> Would that also sign her out of Marketplace and FindMyDevice? Maybe replace
> this with 'Disable sync', and leave signing out of FxA to the FxA screens?

Good point. I'll change it for "Disable Sync"

> 
> Maybe missing (assuming we launch Sync as opt-in instead of opt-out):
> "Logged in to FxA but Sync disabled"

That's the first screen.

> This screen would also be needed after a user disables sync in screen 3.
> Of course, it can be just screen 3 but with 'Enable sync' instead of the
> current 'Sign out', and the rest of the inputs disabled / greyed out.
> 'Disable sync'/'Enable sync' can also be a switch input, like in
> https://www.youtube.com/watch?v=BEYgx5chun0&feature=youtu.be&t=180

I'd prefer not to show screen 3 if the user is not logged into Sync. Just like Android does. I think a bit of common UX between products would be good for the user.

Comment 17

[GitHub Autolander Bot]

Attachment: Pull request

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8662538 [details] [review]
Pull request

I still need to fix the SyncManager tests and write tests for the UI before asking for review, but I could use some feedback in the meantime.

I recorded a video [1] to show you how the UI works with this patch.

There you can see how the user can log into Sync with a verified account ([e-mail]) and play with the collections switches. You'll see how disabling all collection switches should disable the 'Sync now' option and how the user can request a sync on demand. This sync is emulated by one of the apps that I uploaded to bug 1196096. Once the sync is "done" the UI shows the time of this last sync. This time is updated every minute. You can also see on the video that login out from FxA also logs the user out of Sync. Then you can see how an unverified account login looks like and what happens when the user verifies the account from her email inbox. All these states (login, logout, unverified) are kept through reboots.

I took the images from Firefox for Android. I know that I need to provide different sizes for each image, but I'd like the visual design team to provide them. I'll open a bug to request the required visual design work.

Thanks in advance for any feedback!

[1] https://vimeo.com/139901397

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8662538 [details] [review]
Pull request

Etienne, could you take a look to the System app changes, please?

The main change is that I added a way to handle unverified accounts. This won't be happening on 2.5 because of bug 1205292, but we need to be ready when we allow new sync users.

When a user is using an unverified email, we disable the Sync UI actions and show a warning indicating that she needs to verify her account. Once the account is verified we get that as a chrome event coming from the FxA implementation and we move Sync to the enable state. Because the user can verify the account while the device is offline, we need to check the account state on every reboot.

Thanks in advance for any feedback!

Comment 20

[Etienne Segonzac (:etienne)]

Comment on attachment 8662538 [details] [review]
Pull request

r=me for the system part, made a few comments on github but nothing major

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8662538 [details] [review]
Pull request

Review race :)

Comment 22

[[e-mail]]

I've not involved in previous discussion, so could you help point out why we need to add `preprocessor` in settings' build process? We need a very good reason to introduce any 3rd party compiler.

Comment 23

[[e-mail]]

If you want to hide the panel in build time, maybe you can use existing way and refer how the developer menu hide before pref-on the related settings.

Comment 24

[[e-mail]]

Comment on attachment 8662538 [details] [review]
Pull request

Thanks for the patch, please address comments in github and set review again.

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fred Lin [:gasolin] from comment #22)
> I've not involved in previous discussion, so could you help point out why we
> need to add `preprocessor` in settings' build process? We need a very good
> reason to introduce any 3rd party compiler.

The preprocessor was introduced in bug 1202383 and already used in System in bug 1191770

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8662538 [details] [review]
Pull request

Thank you for the feedback, Fred. I updated the PR addressing all the comments you added including the one about splitting the logic of the view in two, although I still disagree it's the right way to do it.

Comment 27

[Evelyn Hung]

Comment on attachment 8662538 [details] [review]
Pull request

I took a quick glance of the PR and didn't see any issues so far. However, I'm working on other high priority bugs, so to avoid blocking your progress, I will leave this review to Fred. Thank you, Fred.

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Thank you Evelyn.

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8662538 [details] [review]
Pull request

This is blocking Firefox Sync testing, so let's try with another reviewer.

Yura, could you take a look at this patch, please?

For some background you can check [1] and [2].

You can test the patch locally by running: FIREFOX_SYNC=1 make install-gaia

I recommend installing the test_synchronizer app that I added at [3] if you want to see an emulated sync request. Do not install test_sync as it's just a mock of the settings panel.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1196096
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1168160
[3] https://bugzilla.mozilla.org/attachment.cgi?id=8661685

Thanks!

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Video

Comment 31

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi Fernando, I just wanted to get a quick suggestion from you in terms of verifying that it works. I am trying to enable sync when already logged in into Firefox Accounts, but I can not get further than 'enabling' state. Am I missing a step in terms of getting to 'enabled'? Thanks.

Comment 32

[Michiel de Jong [:michielbdejong]]

I can reproduce the problem (or at least a similar one) so I'll do some debugging on this tomorrow morning.

Comment 33

[Fernando Jiménez Moreno [:ferjm]]

I am clearing the needinfo as we talked about this over IRC and it seems that the problem is probably an old Gecko version where Bug 1202342 is missing.

Thanks for your help Yura!

Comment 34

[Fernando Jiménez Moreno [:ferjm]]

Hey Yura, now that you are able to make this work locally, could you take a look at the patch, please? Sorry for been such pain but we really need to unblock QA testing as soon as possible. Thank you so much for your help!

Comment 35

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8662538 [details] [review]
Pull request

Settings part looks good to me granted comments are addressed. Thanks

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

Thank you Yura!

https://github.com/mozilla-b2g/gaia/commit/61f30bca846932a89f844a5f8a653f493eaaa067