841480 - Change - Add Sync to Settings charm pane and move sync prefs to a new Sync panel

Status: VERIFIED FIXED

(Tracking Graveyard :: Metro Operations, defect, P1)

Description

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Deeper flyout is acceptable in Settings, but not encouraged for content like Options. I walked through many Options flyout on Windows 8. None of them contains deeper flyouts.

That being said, my viewpoint is
Sync should be leveled up to entry point, since it will definitely contain deeper flyouts.

From Asa:
"I believe that we'll eventually have an "Account" or similar flyout hooked up to that top level in the Settings flyout that will be wired up to our next generation Sync stuff and perhaps that's where other profile actions, like clear private data and import from other browsers, should live. If so, maybe we create that space now and put our profile related features there."

Comment 1

[Jim Mathies [:jimm]]

Brian, I've been reworking layout of Option over in bug 841511. I've pushed sync down to the bottom to get it out of the way. I'll post a patch there, that work might conflict with your browser.xul work here without it.

Comment 2

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Let's add "Sync" after "Options" for now, since "Options" will be the mostly used entry point.
- Options
- Sync
- About
- Help(online)
- Permissions


"Account" is usually the top entry point in Metro apps. Once we are ready to integrate Metro with the next generation Sync, we will change "Sync" to "Account", and promote it on top of "Options".

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Widget changes

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - WeaveGlue fixes so sync setup will work again

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Promote sync to settings panel w/ ability to add/remove items

Comment 6

[Brian R. Bondy [:bbondy]]

Yuan I wanted to pass by some design decisions through you.  Please let me know if you have any objections.
- I put sync where you said after Options.
- The item just says "Sync" in the Windows settings panel.
- When clicked it brings up the setup pairing screen where it gives you 3 sets of numbers to put into another browser.
- After being successfully setup the item disappears from the Windows settings panel.
- The items in Firefox Options remain so you can disconnect your sync settings if you want to.

Mainly the decisions were made because the Windows settings panel can only have single text links only and no other labels or buttons.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 718578 [details] [diff] [review]
Patch v1 - Widget changes

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

::: widget/windows/winrt/nsWinMetroUtils.cpp
@@ +314,5 @@
> +  NS_ENSURE_ARG_POINTER(aOutIndex);
> +  if (!sSettingsArray)
> +    return NS_ERROR_UNEXPECTED;
> +  if (aIndex > sSettingsArray->Length())
> +    return NS_ERROR_UNEXPECTED;

I'd suggest return NS_ERROR_ILLEGAL_VALUE for index out of bounds errors.

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 718579 [details] [diff] [review]
Patch v1 - WeaveGlue fixes so sync setup will work again

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

::: browser/metro/base/content/sync.js
@@ +31,4 @@
>      Services.obs.addObserver(function onReady() {
>        Services.obs.removeObserver(onReady, "weave:service:ready");
> +      self._init();
> +    }, "weave:service:ready", false);

I just landed a fix for this in bug 840643, so you can drop this change.  Sorry for the duplicated work.

@@ +123,5 @@
>        onComplete: function onComplete(aCredentials) {
>          self.jpake = null;
>  
> +        if (self._progressBar) {
> +          self._progressBar.mode = "determined";

Rather than putting if statements everywhere, let's just make WeaveGlue._progressBar into a lazy getter so it's guaranteed to work.

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Widget changes

Applied widget nit

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 718580 [details] [diff] [review]
Patch v1 - Promote sync to settings panel w/ ability to add/remove items

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

::: browser/metro/base/content/browser-ui.js
@@ +1704,5 @@
> +    switch (aTopic) {
> +      case "metro-settings-entry-selected":
> +        let entry = this._entries[parseInt(aData, 10)];
> +        if (entry)
> +          entry.onselected();

There's a possibility of a race condition here.  If we display the Settings charm, then we add or remove an entry, and then the user clicks on one of the already-displayed entries, we'll receive the wrong index.

Perhaps we should match by label rather than by index.  Or we could force a refresh of the Settings pane if it's open when an entry changes.  I think it's worth doing something here, since Sync setup/connection happens asynchronously it seems likely that some users will hit this bug.

@@ +1708,5 @@
> +          entry.onselected();
> +      break;
> +#ifdef MOZ_SERVICES_SYNC
> +      case "weave:service:setup-complete":
> +        this.removeEntryAt(this._syncSettingIndex);

Just to be safe, we should check 'if ("_syncSettingIndex" in this)' before calling removeEntryAt, and we should "delete this._syncSettingIndex;" afterwards.

Comment 11

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

(In reply to Brian R. Bondy [:bbondy] from comment #6)
> Yuan I wanted to pass by some design decisions through you.  Please let me
> know if you have any objections.
> - I put sync where you said after Options.
> - The item just says "Sync" in the Windows settings panel.
> - When clicked it brings up the setup pairing screen where it gives you 3
> sets of numbers to put into another browser.
> - After being successfully setup the item disappears from the Windows
> settings panel.
> - The items in Firefox Options remain so you can disconnect your sync
> settings if you want to.
> 
> Mainly the decisions were made because the Windows settings panel can only
> have single text links only and no other labels or buttons.

Thanks for the comment, Brian.

I understand the limits of texts and links only on settings panel. And you are right, the item should just say "Sync". 

We need to move everything related to Sync out of Options flyout if we are going to promote Sync. 

That means we need a flyout page that is titled "Sync". It opens after the users tap on "Sync" from setting panel. At this stage, on this flyout page, the Sync UI can remain the same as the current Sync UI in options flyout. (a separate bug has been filed for using Win8 UI elements for Sync)

Note: After the users have set up Sync, the "Sync" entry point should still appear on setting panel. Tapping on it should open that "Sync" flyout page that shows the option to pair a device, and other syncing status.
 
Let me know if you have any question. Feel free to attach screenshot and ask for info you need.

Comment 12

[Brian R. Bondy [:bbondy]]

Well that's a completely different task then what I understood this bug to be and spent the day doing. OK.

I don't think I understand the "deeper flyout" comment in Comment 0 because it's my understanding that having a sync flyout panel would make it at the same depth level. It's just moved from one flyout into another.

Comment 13

[Brian R. Bondy [:bbondy]]

To avoid more unneeded work blocking on Comment 12 to understand the deeper flyout comment in case I've misunderstood some part of this work.

Comment 14

[Asa Dotzler [:asa]]

I think the work Bryan did is good enough for v1. We can re-visit more sync work in v2. Let's take this and push the rest off.

Brian, let's revert this back to what you signed up for and land that. We can do more later.

Comment 15

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Based on Asa's comment, (In reply to Brian R. Bondy [:bbondy] from comment #12)
> Well that's a completely different task then what I understood this bug to
> be and spent the day doing. OK.
> 
> I don't think I understand the "deeper flyout" comment in Comment 0 because
> it's my understanding that having a sync flyout panel would make it at the
> same depth level. It's just moved from one flyout into another.

Brian, sorry about the confusion.
In my proposal, this bug was supposed to be dependent on another Bug 845468. 
You are correct. If the new layout will not get implemented, this Sync flyout panel is going to be at the same depth level as before.

I still think Sync related options should eventually move out of the options flyout, even though, some flows, such as "pairing a device" will be on the same depth level. Options are supposed to be a flat sheet that contains simple controls. Sync contains too much page transitions. It's better to create its own flyout to handle them.

I'm fine with landing what you have implemented for v1, if there is no time to revisit and complete Bug 845468.

Comment 16

[Brian R. Bondy [:bbondy]]

Since the end game result is bug 845468 and since I have unfinished review comments to implement, I'll just do the new flyout panel. I think it will be only slightly more work than the unfinished review comments anyway.

Comment 17

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Sync pane

The restyling will be done in a different bug.

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: Settings charm Pane

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Sync pane - not connected

This is changing in another bug, so this is just for the context of this bug.

Comment 20

[Brian R. Bondy [:bbondy]]

Attachment: Sync panel connected w/ details expanded

Ditto this is just the temporary look until the other bug is finished

Comment 21

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Comment on attachment 718784 [details]
Sync panel connected w/ details expanded

Looks good to me.

2 small suggestions
Since now Sync has it own flyout, I think we can remove the Details toggle button. Just display all the information by default.

And the bold title "Sync" can be removed, as there is a header "Sync" already. 

I know those changes are out of scope in this bug, but it'll be great to have them fixed together :) I will give Review+ anyway.

Comment 22

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #19)
> Created attachment 718782 [details]
> Sync pane - not connected

This really needs a description of what sync is in it. Maybe that's in the other bug?

Comment 23

[Brian R. Bondy [:bbondy]]

I'll land this since it's reviewed and do a followup for the title removing and details button removal.

Comment 24

[Brian R. Bondy [:bbondy]]

Actually I have to wait because my patch doesn't apply to m-i because of jimm's patch, so I'll rebase it and test on m-c once that gets merged.  Thanks for the late night review by the way Matt!  I'll post Yuan's couple changes into his bug, just finishing them up.

Comment 25

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1. Remove bold title, remove details button, expand all

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Sync pane - Rebased to m-c tip

Rebased, carrying forward r+.

Comment 27

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Remove bold title, remove details button, expand all - Rebased to m-c tip

Comment 28

[Matt Brubeck (:mbrubeck)]

Comment on attachment 719000 [details] [diff] [review]
Patch v2 - Remove bold title, remove details button, expand all - Rebased to m-c tip

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

::: browser/metro/locales/en-US/chrome/sync.dtd
@@ -8,4 @@
>  <!ENTITY sync.notconnected          "Not connected">
>  <!ENTITY sync.connect               "Connect">
>  <!ENTITY sync.connected             "Connected">
>  <!ENTITY sync.details               "Details">

I think you can remove sync.details too.

Comment 29

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/026f901ab417
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c769a2741a

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5c769a2741a
https://hg.mozilla.org/mozilla-central/rev/026f901ab417

Comment 31

[juan becerra [:juanb]]

Tested on 2013-02-28 with a nightly build from http://hg.mozilla.org/mozilla-central/rev/c65d59d33aa8
- The Sync panel looks like third attachment with the revisions from the description in the patch in attachment 5 [details].
- It has a main Sync panel heading, followed with four entries: Connected, Last sync, This device, Account and their respective elements (button or text field).

Comment 32

[Samvedana (:Samvedana)]

Attachment: sync.png

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130812030209
Built from http://hg.mozilla.org/mozilla-central/rev/87c1796bc46c

WFM
Tested on windows 8 using latest nightly for iteration-11. I saw the screen attached here.