645802 - Move Sharing settings to the pref dialog

Status: RESOLVED FIXED

(Cloud Services :: Share: Firefox Client, defect)

Description

[Philipp von Weitershausen [:philikon]]

Right now Sharing prefs live inside the web content (right now not even inside the doorhanger but the separate settings tabs). We should consider moving those inside Firefox's chrome (e.g. the preference dialog)

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

We have time to convert the existing account settings system into a preferences window but we need a decision if that's the correct place.  Currently accounts and settings for F1 live in a web page we open in a tab.  I'll attach a quick wireframe storyboard of the current setup process.

This change would have effects on the mockup in attachment 522780 [details] from bug 642684 so we should figure that out after we've decided on this.

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: wire frame of current setup process

This is the storyboard of the current F1 setup process from having no accounts to getting a single account setup.

Here are the major pieces:

* Initial load of the Panel (currently we forward them to the accounts/settings page however it would be nice to do something more inline like attachment 522780 [details])
* Users need to be able to add and remove accounts, multiple account types are allowed (i.e. two twitter accounts)
* Adding an account requires opening a web window for authenticating and authorizing F1 to post.  This could be handled in many ways, currently it's a popup window only showing the URL bar.
* After setup of accounts we always bring the user right back to the page they were on and the panel opened, ready to use
* We only have a single "preference", which is [x] Bookmark the links I share

Here are some extra items I think are worth considering.

* Grouping accounts by type might make a lot of sense, currently we list accounts in the order they were created
* A number of users have requested to change the order in which accounts are listed in the panel

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: sharing preferences mockup

Here's the mockup used during our discussion.

Accounts can be added from this dialog which would open up a popup browser window and returns adding the account into this pane.  Mainly this window is for account management, adding of accounts can be optimized to happen in the panel UI as described in attachment 522780 [details] of bug 642684

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 524721 [details]
sharing preferences mockup

Recapping today's phone call with :jrburke and :mixedpuppy where we discussed this mockup:

* There isn't much point in serving up the prefs from chrome if we keep the share panel in web content. All it would do is create another explicit API plug point between the two that would have to be documented, tested and vetted.

* So our current goal is to implement the pref pane in web content with the exception of the bookmarking checkbox. The bookmarking feature happens within Firefox anyway, so it makes a lot of sense to have that separate.

* This means we would move the "Add" button and the "Sharing Accounts" dropdown into the part that's loaded from the web. This doesn't necessarily mean it'd be be visually part of the white scrollable pane, though, but it would make things easier.

* I'd like to raise one flag: on Windows, the pref dialog (like all dialogs) is modal, but we will have to open a new tab or window to do the OAuth dance. I haven't tested it yet, but this will likely mean we'd have to close the prefs dialog, do the OAuth dance and then reopen it. Lots of failure modes lurking here, of course (what if the OAuth dance doesn't complete? etc.).

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #4)
> * I'd like to raise one flag: on Windows, the pref dialog (like all dialogs) is
> modal, but we will have to open a new tab or window to do the OAuth dance. I
> haven't tested it yet, but this will likely mean we'd have to close the prefs
> dialog, do the OAuth dance and then reopen it. Lots of failure modes lurking
> here, of course (what if the OAuth dance doesn't complete? etc.).

Just tested this on windows.  The add-on manager opens up a new window and I did a quick test using dom inspector to replace one of the button oncommand functions with window.open("http://f1.mozillamessaging.com/") which worked in the same way.

Looks like this should work in general, though unclear about what happens after the window closes.  We should probably have a separate bug opened for what happens when the OAuth dance doesn't complete because there are a lot of factors involved in that.

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to comment #5)
> (In reply to comment #4)
> > * I'd like to raise one flag: on Windows, the pref dialog (like all dialogs) is
> > modal, but we will have to open a new tab or window to do the OAuth dance. I
> > haven't tested it yet, but this will likely mean we'd have to close the prefs
> > dialog, do the OAuth dance and then reopen it. Lots of failure modes lurking
> > here, of course (what if the OAuth dance doesn't complete? etc.).
> 
> Just tested this on windows.  The add-on manager opens up a new window and I
> did a quick test using dom inspector to replace one of the button oncommand
> functions with window.open("http://f1.mozillamessaging.com/") which worked in
> the same way.

Ah yes, making a new *window* will work since the pref dialog will only be modal to the other window. Which means we won't be able to do the OAuth dance in a tab. Pity. Anyway, glad this works. I just remembered some funny focusing/modal issues on Windows and wanted to make sure we got that base covered.

Comment 7

[James Burke]

With the select box UI for the "Add Account", how will we surface the "you need to log out of the service before adding another of this type" that we show now for things like Facebook/Yahoo/LinkedIn?

Comment 8

[James Burke]

I created a web UI branch to try out the settings in web content idea here:
https://github.com/mozilla/f1/tree/bug/645802

It requires that the Fx build has its new account storage changes to work. 

I have not matched the UI mock exactly, but do have a dropdown for share services to do an add, and list out already added accounts. This is just to test UI flow.

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #7)
> With the select box UI for the "Add Account", how will we surface the "you need
> to log out of the service before adding another of this type" that we show now
> for things like Facebook/Yahoo/LinkedIn?

Good question.  Because this is a select plus add button we could possibly put that text below the select after those account types are chosen.  Alternatively we could have that text inside the grouping area, it would work almost in the way it does now.

I wonder if we should work on that intermediary page before the OAuth dance.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

>* So our current goal is to implement the pref pane in web content with the
>exception of the bookmarking checkbox.

Just to clarify, you mean Web content placed inside of the options/preferences prefpane?

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #10)
> >* So our current goal is to implement the pref pane in web content with the
> >exception of the bookmarking checkbox.
> 
> Just to clarify, you mean Web content placed inside of the options/preferences
> prefpane?

yes, to handle the display and adding of accounts. In exactly the same way as the panel is in an html iframe right now, the user experience and interface look and feel will be matched to that of the OS preferences panel.  The panel interactions will act much like the Sync panel does now.  The "Bookmark links I share" will be part of the regular preference panel.

Comment 12

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1: Factor out message dispatching

Comment 13

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2: Factor out message dispatching to SecureKeyValueStore

Comment 14

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3: Share prefpane

Comment 15

[Philipp von Weitershausen [:philikon]]

Attachment: Part 4: Tests for Share prefpane

Comment 16

[Philipp von Weitershausen [:philikon]]

Attachment: Part 5: Styling for the Share prefpane

Comment 17

[Philipp von Weitershausen [:philikon]]

I haven't tested this against James's bug/645802 branch yet, but I do have tests that verify the storage API works, so it should Just Work(tm) ;)

In any case I pushed off a try build: http://tbpl.mozilla.org/?tree=MozillaTry&rev=e92b520c56be. Builds and logs will be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-e92b520c56be.

Note: For the styling I've taken the F1 logo for now, we'll have to change the branding anyway.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 527472 [details] [diff] [review]
Part 4: Tests for Share prefpane

># HG changeset patch
># Parent 4875c2816cd5beb56b0180cf7c89a90a2482fd94
># User Philipp von Weitershausen <[e-mail]>
>Bug 645802 - Part 4: Tests for Share prefpane
>
>Reuse existing brwoser_store.js tests by factoring out test functions to a separate file.
>
>diff --git a/services/share/tests/browser/Makefile.in b/services/share/tests/browser/Makefile.in
>--- a/services/share/tests/browser/Makefile.in
>+++ b/services/share/tests/browser/Makefile.in
>@@ -41,22 +41,24 @@ srcdir    = @srcdir@
> VPATH     = @srcdir@
> relativesrcdir = services/share/tests/browser
> 
> include $(DEPTH)/config/autoconf.mk
> 
> _BROWSER_TEST_FILES = \
> 		corpus \
> 		head.js \
>+		keyvaluestore_tests.js \
> 		share.html \
> 		page.html \
> 		browser_aaLoadPanel.js \
> 		browser_bookmarkPage.js \
> 		browser_messageWhitelist.js \
> 		browser_panelReady.js \
>+		browser_prefs.js \
> 		browser_shareOptions.js \
> 		browser_store.js \
> 		browser_togglePanel_button.js \
> 		browser_togglePanel_key.js \
> 		$(NULL)
> 
> libs:: $(_BROWSER_TEST_FILES)
> 	$(PYTHON) $(topsrcdir)/config/nsinstall.py $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/browser/$(relativesrcdir)
>diff --git a/services/share/tests/browser/browser_prefs.js b/services/share/tests/browser/browser_prefs.js
>new file mode 100644
>--- /dev/null
>+++ b/services/share/tests/browser/browser_prefs.js
>@@ -0,0 +1,29 @@
>+/* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+let gPrefWin;
>+function test() {
>+  waitForExplicitFinish();
>+
>+  gPrefWin = window.openPreferences("paneShare");
>+  next(gPrefWin, "load", function () {
>+    // Redefine this global variable. Now all of our helpers will use
>+    // this instead of the share panel's browser.
>+    gShareBrowser = gPrefWin.document.getElementById("share-prefs-browser");
>+
>+    // Trivial test: make sure we got the right URL loaded
>+    let settingsURL = Services.prefs.getCharPref("services.share.settingsURL");
>+    is(gShareWindow.location, settingsURL);
>+
>+    run_next_test();
>+  });
>+}
>+
>+Services.scriptloader.loadSubScript(CHROME_PREFIX + "keyvaluestore_tests.js",
>+                                    this);
>+gTests.push(function tearDown() {
>+  gPrefWin.close();
>+  gPrefWin = null;
>+  gShareBrowser = null;
>+  cleanup(finish);
>+});
>diff --git a/services/share/tests/browser/browser_store.js b/services/share/tests/browser/browser_store.js
>--- a/services/share/tests/browser/browser_store.js
>+++ b/services/share/tests/browser/browser_store.js
>@@ -1,132 +1,17 @@
> /* Any copyright is dedicated to the Public Domain.
>    http://creativecommons.org/publicdomain/zero/1.0/ */
> 
>-let gTestIndex = 0;
>-let gTests = [];
>-
>-gTests.push(function test_storeGet_empty() {
>-  let message = {topic: "storeGet", data: "hypnotoad"};
>-  relayMessage(message, function() {
>-    next(gShareWindow, "message", function(event) {
>-      let message = JSON.parse(event.data);
>-      is(message.topic, "storeGetReturn");
>-      // Store is empty so the data is null.
>-      is(message.data.key, "hypnotoad");
>-      is(message.data.value, null);
>-      run_next_test();
>-    });
>-  });
>-});
>-
>-gTests.push(function test_storeSet() {
>-  let message = {topic: "storeSet",
>-                 data: {key: "hypnotoad",
>-                        value: "Everybody loves Hypnotoad!"}};
>-  relayMessage(message, function() {
>-    // We expect a notification about the change.
>-    next(gShareWindow, "message", function(event) {
>-      let message = JSON.parse(event.data);
>-      is(message.topic, "storeNotifyChange");
>-      is(message.data.key, "hypnotoad");
>-      is(message.data.value, "Everybody loves Hypnotoad!");
>-      run_next_test();
>-    });
>-  });
>-});
>-
>-gTests.push(function test_storeGet_notEmpty() {
>-  let message = {topic: "storeGet", data: "hypnotoad"};
>-  relayMessage(message, function() {
>-    next(gShareWindow, "message", function(event) {
>-      let message = JSON.parse(event.data);
>-      is(message.topic, "storeGetReturn");
>-      is(message.data.key, "hypnotoad");
>-      is(message.data.value, "Everybody loves Hypnotoad!");
>-      run_next_test();
>-    });
>-  });
>-});
>-
>-gTests.push(function test_storeSet_anotherOne() {
>-  let message = {topic: "storeSet",
>-                 data: {key: "calculon",
>-                        value: "I don't do two takes. Amateurs do two takes."}};
>-  relayMessage(message, function() {
>-    // We expect a notification about the change.
>-    next(gShareWindow, "message", function(event) {
>-      let message = JSON.parse(event.data);
>-      is(message.topic, "storeNotifyChange");
>-      is(message.data.key, "calculon");
>-      is(message.data.value, "I don't do two takes. Amateurs do two takes.");
>-      run_next_test();
>-    });
>-  });
>-});
>-
>-gTests.push(function test_storeRemove() {
>-  let message = {topic: "storeRemove", data: "hypnotoad"};
>-  relayMessage(message, function() {
>-    next(gShareWindow, "message", function(event) {
>-      let message = JSON.parse(event.data);
>-      is(message.topic, "storeNotifyChange");
>-      is(message.data.key, "hypnotoad");
>-      is(message.data.value, null);
>-      run_next_test();
>-    });
>-  });
>-});
>-
>-gTests.push(function test_storeGet_removed() {
>-  let message = {topic: "storeGet", data: "hypnotoad"};
>-  relayMessage(message, function() {
>-    next(gShareWindow, "message", function(event) {
>-      let message = JSON.parse(event.data);
>-      is(message.topic, "storeGetReturn");
>-      // Store is empty so the data is null.
>-      is(message.data.key, "hypnotoad");
>-      is(message.data.value, null);
>-      run_next_test();
>-    });
>-  });
>-});
>-
>-gTests.push(function test_storeRemove_nonexistent() {
>-  let message = {topic: "storeRemove", data: "nonexistent"};
>-  relayMessage(message, function() {
>-    next(gShareWindow, "message", function(event) {
>-      let message = JSON.parse(event.data);
>-      is(message.topic, "storeNotifyChange");
>-      is(message.data.key, "nonexistent");
>-      is(message.data.value, null);
>-      run_next_test();
>-    });
>-  });
>-});
>-
>-gTests.push(function test_storeRemoveAll() {
>-  let message = {topic: "storeRemoveAll"};
>-  relayMessage(message, function() {
>-    next(gShareWindow, "message", function(event) {
>-      let message = JSON.parse(event.data);
>-      is(message.topic, "storeNotifyRemoveAll");
>-      run_next_test();
>-    });
>-  });
>-});
>-
>-gTests.push(function tearDown() {
>-  gBrowser.removeCurrentTab();
>-  cleanup(finish);
>-});
>-
>-function run_next_test() {
>-  let test = gTests[gTestIndex++];
>-  window.setTimeout(test, 0);
>-}
>-
> function test() {
>   waitForExplicitFinish();
>   openTab(PAGE_URL, function() {
>     run_next_test();
>   });
> }
>+
>+Services.scriptloader.loadSubScript(CHROME_PREFIX + "keyvaluestore_tests.js",
>+                                    this);
>+
>+gTests.push(function tearDown() {
>+  gBrowser.removeCurrentTab();
>+  cleanup(finish);
>+});
>diff --git a/services/share/tests/browser/head.js b/services/share/tests/browser/head.js
>--- a/services/share/tests/browser/head.js
>+++ b/services/share/tests/browser/head.js
>@@ -1,17 +1,19 @@
>+const CHROME_PREFIX = "chrome://mochitests/content/browser/services/share/tests/browser/";
> const PREFIX = "http://mochi.test:8888/browser/services/share/tests/browser/";
> const SHARE_URL = PREFIX + "share.html";
> const PAGE_URL = PREFIX + "page.html";
> 
> Services.prefs.setCharPref("services.share.shareURL", SHARE_URL);
>+Services.prefs.setCharPref("services.share.settingsURL", SHARE_URL);
> 
> // gShareWindow is gShareBrowser's window object. It's dynamic because
> // it can change.
>-const gShareBrowser = document.getElementById("share-browser");
>+let gShareBrowser = document.getElementById("share-browser");
> this.__defineGetter__("gShareWindow", function() {
>   return gShareBrowser.contentWindow.wrappedJSObject;
> });
> 
> /**
>  * Register a one-time event handler.
>  */
> function next(element, event, callback) {
>diff --git a/services/share/tests/browser/browser_store.js b/services/share/tests/browser/keyvaluestore_tests.js
>copy from services/share/tests/browser/browser_store.js
>copy to services/share/tests/browser/keyvaluestore_tests.js
>--- a/services/share/tests/browser/browser_store.js
>+++ b/services/share/tests/browser/keyvaluestore_tests.js
>@@ -109,24 +109,12 @@ gTests.push(function test_storeRemoveAll
>     next(gShareWindow, "message", function(event) {
>       let message = JSON.parse(event.data);
>       is(message.topic, "storeNotifyRemoveAll");
>       run_next_test();
>     });
>   });
> });
> 
>-gTests.push(function tearDown() {
>-  gBrowser.removeCurrentTab();
>-  cleanup(finish);
>-});
>-
> function run_next_test() {
>   let test = gTests[gTestIndex++];
>   window.setTimeout(test, 0);
> }
>-
>-function test() {
>-  waitForExplicitFinish();
>-  openTab(PAGE_URL, function() {
>-    run_next_test();
>-  });
>-}

Comment 19

[Shane Caraveo (:mixedpuppy)]

please ignore comment 18

Comment 20

[Philipp von Weitershausen [:philikon]]

Part 1: https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/d93ecae862c4
Part 2: https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/b951dfef7085
Part 3: https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/fcc28f8e3a83
Part 4: https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/672e9e123e8a
Part 5: https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/e7dd9b05b873

If there's UI polish to be done --> follow up bugs.

Comment 21

[Philipp von Weitershausen [:philikon]]

Needed a small follow-up to fix mochitests on Windows:
https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/f3fd46677323