675821 - "Pair a Device" and "Set up Sync" links at bottom of the Firefox home page

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Richard Newman [:rnewman]]

Depends on Home Tab, of course. (No bug for that yet.)

Comment 1

[Philipp von Weitershausen [:philikon]]

(In reply to comment #0)
> Depends on Home Tab, of course. (No bug for that yet.)

No bug and thousands. Bug 551849 tracks the general App and Home tab project, bug 588230 tracks Home Tab snippets which is what we want (for the Sync Key at least).

Comment 2

[Philipp von Weitershausen [:philikon]]

Bug 544819 is probably the closest bug we have on file for the kind of Home tab we need for this. Adding it as a dependency.

Comment 3

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v-1) about:home page

currently broken. for starters.
JavaScript error: chrome://browser/content/aboutHome.js, line 40: Permission denied for <moz-safe-about:home> to get property XPCComponents.classes

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to :Ally Naaktgeboren from comment #3)
> Created attachment 567236 [details] [diff] [review] [diff] [details] [review]
> Part 1 (v-1) about:home page
> 
> currently broken. for starters.
> JavaScript error: chrome://browser/content/aboutHome.js, line 40: Permission
> denied for <moz-safe-about:home> to get property XPCComponents.classes

Yup, because about:home currently doesn't run with chrome privileges. We would need to turn them on in AboutRedirector, just like we did for about:sync-progress, and get a security review for that.

Comment 5

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v0) about:home tab, scrapped

since raising privs on the page breaks the local storage, we will not be pursuing this compromise for ffx10. work uploaded for posterity.

Comment 6

[Philipp von Weitershausen [:philikon]]

We're going to do the same hack that we do for the session restore button (thanks for the pointer, Margaret!) . Here's what should happen in this bug:

Part 1: Duplicate openSetup() and openAddDevice() from browser/components/preferences/sync.js to browser/base/content/browser-syncui.js so that we can use them in browser.js. Note that an older version of openSetup() already exists there. (The code duplication sucks, we should fix this at some point, but not now... see bug 583366.)

Part 2: Add two links to the bottom of the about:home page:

  "Set Up Sync"    "Pair a Device"

Little bit like in this mockup (only shows one link, but you get the idea): https://people.mozilla.com/~faaborg/files/projects/sync/setup-i3/n-m-d%204%20-%20activate%20page.html

There are two cases to consider:

(a) Sync is not set up: Show both links. "Set Up Sync" launches openSetup(null), "Pair a Device" launches openSetup("pair").

(b) Sync is set up: Show only "Pair a Device" which launches openAddDevice().

Comment 7

[Chenxia Liu [:liuche]]

Attachment: Part 1, v1: adds sync setup/pair links to about:home, removes "About Mozilla link"

adds sync setup/pair links to about:home, removes "About Mozilla link"

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 568316 [details] [diff] [review]
Part 1, v1: adds sync setup/pair links to about:home, removes "About Mozilla link"

Very nice work! There's a few nits here and there and the links don't do exactly what they need to do yet, but this is easily fixed. See below for details.


>+/* Hack to make buttons appear as links.
>+ * Necessary to piggyback on browser.js OnBrowserClick listener,
>+ * which checks that click originates from a button*/

Nit: punctuation and space after the sentence ends. Also, we typically wrap at 80 characters :)

>+
>+button.sync-link {

No need to qualify the tag name here. Just doing ".sync-link" is enough and actually preferred (cf https://developer.mozilla.org/en/Writing_Efficient_CSS#Don.E2.80.99t_qualify_Class_Rules_with_tag_names)

>+  -moz-appearance: none;
>+  outline: none;
>+  border: 0;

border: none;

>     <div id="bottomSection">
>-      <div id="aboutMozilla">
>-        <a href="http://www.mozilla.com/about/">&abouthome.aboutMozilla;</a>

Yeeeeaaaah we probably don't want to mess with this. I realize the mockup I mentioned in comment 6 doesn't show this link, so I'm not blaming you for removing it. But I bet the Engangement would love to have a word with us if we remove this. :)

Just keep the "aboutMozilla" <div> and add the buttons in the "syncLinksContainer" <div> below. If you look at https://people.mozilla.com/~faaborg/files/projects/sync/allFlows-i2.png, that's how it's arranged. (Yes, the "About Mozilla" link says "Firefox answers to no-one but you. Learn more." but that's a change that's outside the scope of this bug. :))

>diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js
>--- a/browser/base/content/browser-syncui.js
>+++ b/browser/base/content/browser-syncui.js
>@@ -286,26 +286,39 @@ let gSyncUI = {
>     if (this._needsSetup())
>       this.openSetup();
>     else
>       this.doSync();
>   },
> 
>   //XXXzpao should be part of syncCommon.js - which we might want to make a module...
>   //        To be fixed in a followup (bug 583366)
>-  openSetup: function SUI_openSetup() {
>+  openSetup: function SUI_openSetup(wizardType) {

Please also copy over the doc comment explaining the `wizardType` parameter from browser/components/preferences/sync.js.

>     let win = Services.wm.getMostRecentWindow("Weave:AccountSetup");
>     if (win)
>       win.focus();
>     else {
>       window.openDialog("chrome://browser/content/syncSetup.xul",
>-                        "weaveSetup", "centerscreen,chrome,resizable=no");
>+                        "weaveSetup", "centerscreen,chrome,resizable=no",
>+                        wizardType);

Nit: align arguments.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -403,17 +403,17 @@ function findChildShell(aDocument, aDocS
>     if (docShell)
>       return docShell;
>   }
>   return null;
> }
> 
> var gPopupBlockerObserver = {
>   _reportButton: null,
>-  
>+

Unrelated whitespace change. Please remove.

>@@ -2661,16 +2661,22 @@ function PageProxyClickHandler(aEvent)
>  *  to the DOM for unprivileged pages.
>  */
> function BrowserOnAboutPageLoad(document) {
>   if (/^about:home$/i.test(document.documentURI)) {
>     let ss = Components.classes["@mozilla.org/browser/sessionstore;1"].
>              getService(Components.interfaces.nsISessionStore);
>     if (!ss.canRestoreLastSession)
>       document.getElementById("sessionRestoreContainer").hidden = true;
>+    // Sync-related links
>+    if (Services.prefs.prefHasUserValue("services.sync.username")) {
>+      document.getElementById("setupSyncLink").hidden = true;
>+    } else {
>+      document.getElementById("pairDeviceLink").hidden = true;
>+    }

We want to show the "Pair a Device" link always, I guess I could've been clearer in comment 6. So please get rid of the `else` clause here.

> /**
>  * Handle command events bubbling up from error page content
>  */
> function BrowserOnClick(event) {
>     // Don't trust synthetic events
>@@ -2799,16 +2805,22 @@ function BrowserOnClick(event) {
>     else if (/^about:home$/i.test(errorDoc.documentURI)) {
>       if (ot == errorDoc.getElementById("restorePreviousSession")) {
>         let ss = Cc["@mozilla.org/browser/sessionstore;1"].
>                  getService(Ci.nsISessionStore);
>         if (ss.canRestoreLastSession)
>           ss.restoreLastSession();
>         errorDoc.getElementById("sessionRestoreContainer").hidden = true;
>       }
>+      else if (ot == errorDoc.getElementById("pairDeviceLink")) {
>+        gSyncUI.openAddDevice();
>+      }

This is not entirely correct. As I said in comment 6:

(a) When Sync is set up, this calls gSyncUI.openAddDevice()
(b) When Sync is not set up, this calls gSyncUI.openSetup("pair");


>diff --git a/browser/locales/en-US/chrome/browser/syncSetup.dtd b/browser/locales/en-US/chrome/browser/syncSetup.dtd
>--- a/browser/locales/en-US/chrome/browser/syncSetup.dtd
>+++ b/browser/locales/en-US/chrome/browser/syncSetup.dtd
>@@ -1,9 +1,10 @@
> <!ENTITY accountSetupTitle.label    "&syncBrand.fullName.label; Setup">
>+<!ENTITY aboutHomeSyncSetup.label   "Set Up Sync">

We already have this string as `syncSetup.label` in browser.dtd. aboutHome.xhtml already included browser.dtd, so you can just use it.

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Part 1, v2: fixed nits, returned "aboutMozilla", add "Pair Device" to sho up always

wizardType argument alignment is already aligned with the " of the other arguments - is the alignment convention to align with the text inside the quotes? If so, I will fix to reflect that.

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 568456 [details] [diff] [review]
Part 1, v2: fixed nits, returned "aboutMozilla", add "Pair Device" to sho up always

Awesomesauce. Over to :gavin for browser peer review.

Note: this is only the first step for this bug. Two more parts to come:

* Update about:home, if it's open, on weave:service:setup-complete and weave:service:start-over observer notifications
* Tests! See browser/base/content/test/browser_aboutHome.js for the existing tests. Can easily add test cases to that file.

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Part 2, v1: show/hide "Set Up Sync" in about:home if user sets up or removes sync

Comment 12

[Chenxia Liu [:liuche]]

Attachment: part 1, v3: reversed "pair" and "setup" positions per mockup

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 568494 [details] [diff] [review]
Part 2, v1: show/hide "Set Up Sync" in about:home if user sets up or removes sync

>   onLoginFinish: function SUI_onLoginFinish() {
>     // Clear out any login failure notifications
>     let title = this._stringBundle.GetStringFromName("error.login.title");
>     this.clearError(title);
>+
>+    // Remove "setup sync" link in about:home if it is open. 

Oh hah, onLoginFinish() is also called on "weave:service:setup-complete". Yeah, let's separate those cases out. Let's create an onSetupComplete() method that calls onLoginFinish() and does this work.

>+    let browsers = gBrowser.browsers;
>+    let numBrowsers = browsers.length;
>+    for (let i = 0; i < numBrowsers; i++) {
>+      let b = gBrowser.getBrowserAtIndex(i);

Simpler:

  let browsers = gBrowser.browers;
  for (let i = 0; i < browsers.length; i++) {
    let b = browsers[i];

>+      try {
>+        if (/^about:home$/i.test(b.currentURI.spec)) {

Unless I'm mistaken, the following is equivalent:

  if (b.currentURI.spec == "about:home")

(Death to regular expressions!)

>+          b.contentDocument.getElementById("setupSyncLink").hidden = true;
>+        }
>+      } catch(e) {
>+        Cu.reportError(e);
>+      }

Any particular reason you're catching errors and then reporting them to the error console? I *think* the observer service does that automatically already.


Same comments apply to onStartOver(). In fact, you could even factor the whole "find about:home and show or hide this button" logic out into a helper showHideSetupSyncAboutHome() that takes a single bool argument for the .hidden attribute of the button.

Comment 14

[Chenxia Liu [:liuche]]

Attachment: part 2, v2: made code less inefficient

I didn't make the helper function - to change visibility requires the browser object, and I guess I could have passed that and a bool to the helper, and then gotten the button from the browser dom, but that's a lot more code to replace just one line.

Comment 15

[Chenxia Liu [:liuche]]

Attachment: part 2, v2 dupe

Comment 16

[Philipp von Weitershausen [:philikon]]

Comment on attachment 568541 [details] [diff] [review]
part 2, v2 dupe

This is the same file as v2. Did you forget a qrefresh? :)

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Part 2, v4: ACTUALLY uploaded helper function (sorry!)

Comment 18

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569167 [details] [diff] [review]
Part 2, v4: ACTUALLY uploaded helper function (sorry!)

>+  // Set visibility of "Setup Sync" link
>+  showSetupSyncAboutHome : function SUI_showSetupSyncAboutHome(toShow) {

Nit: no space before the colon.

r=me with that

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Part 2, v5: fixed spacing nit, ready for UX review

Comment 20

[Chenxia Liu [:liuche]]

Attachment: part 3, v0.1: tests for sync-links

Failing on link visibility change after sync setup-complete or sync unlink.

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> >diff --git a/browser/locales/en-US/chrome/browser/syncSetup.dtd b/browser/locales/en-US/chrome/browser/syncSetup.dtd
> >--- a/browser/locales/en-US/chrome/browser/syncSetup.dtd
> >+++ b/browser/locales/en-US/chrome/browser/syncSetup.dtd
> >@@ -1,9 +1,10 @@
> > <!ENTITY accountSetupTitle.label    "&syncBrand.fullName.label; Setup">
> >+<!ENTITY aboutHomeSyncSetup.label   "Set Up Sync">
> 
> We already have this string as `syncSetup.label` in browser.dtd.
> aboutHome.xhtml already included browser.dtd, so you can just use it.

Turns out I was lying. We have that string *with the ellipsis*. But the link shouldn't have the ellipsis. Sorry about that. Can you bring it back?

Comment 22

[Philipp von Weitershausen [:philikon]]

Comment on attachment 568514 [details] [diff] [review]
part 1, v3: reversed "pair" and "setup" positions per mockup

I just gave this a spin. Some feedback besides the string thing I just mentioned:

* It seems the default colour for links is not 'blue' but #0000ee.
* The links need more top margin so that the distance to "About Mozilla" is a bit higher. margin-top: 1em seems to look ok to me.
* When you click the buttons, the text shifts slightly to the right. That's because buttons have this 3D effect. You can see the default CSS in [1]. So we want to add something like this:

  .sync-link:active {
    padding: 0px 6px 0px 6px;
  }

[1] https://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#564

Comment 23

[Chenxia Liu [:liuche]]

Attachment: Part 1, v4: fixed UI details

Comment 24

[Chenxia Liu [:liuche]]

Attachment: Part 1, v5: "Set Up" not "Setup" in localization

Comment 25

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569262 [details] [diff] [review]
Part 1, v5: "Set Up" not "Setup" in localization

Yup, that looks good to me! Once again, over to Gavin!

Comment 26

[Chenxia Liu [:liuche]]

Attachment: Part 3, v1: tests for aboutHome sync links layout, sync links behavior after sync setup/unlink

Comment 27

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569268 [details] [diff] [review]
Part 3, v1: tests for aboutHome sync links layout, sync links behavior after sync setup/unlink

Looks great. It occurred to me that we can and should also add tests to verify that clicking on the buttons open the right dialogs. You can use

  EventUtils.sendMouseEvent({type: "click"}, button, win);

to synthesize a click event (`win` is the window that `button` is in, in this case the contentWindow of the about:home page). To observe the dialog window being opened, do something like this:

  let expectedDialog = "Weave:AccountSetup";
  Services.ww.registerNotification(function onWindow(subject, topic) {
    let wintype = subject.QueryInterface(Ci.nsIDOMWindow)
                         .document.documentElement.getAttribute("windowtype");
    if (topic == "domwindowopened" && wintype == expectedDialog) {
      Services.ww.unregisterNotification(onWindow);
      executeSoon(runNextTest);
    }    
  });

Of course, this needs to be done before you send the synthesized click event, and you want to change `expectedDialog` depending on which dialog we're expecting :).

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 568514 [details] [diff] [review]
part 1, v3: reversed "pair" and "setup" positions per mockup

>diff --git a/browser/base/content/aboutHome.css b/browser/base/content/aboutHome.css

>+.sync-link {

>+  color: blue;

color: -moz-hyperlinktext;

>+.sync-link:hover {

color: -moz-activehyperlinktext, perhaps?

>diff --git a/browser/base/content/aboutHome.xhtml b/browser/base/content/aboutHome.xhtml

>+        <button class="sync-link" id="pairDeviceLink">&pairDevice.title.label;</button>

String re-use in different contexts is usually frowned upon, it's easy to run into issues with different locales wanting different strings despite en-US working fine with the same (due to e.g. length issues or contextual differences in language). It might be a good idea to run this through dev.l10n as a followup (I think it's already the case that these strings are re-used in different places).

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> function BrowserOnAboutPageLoad(document) {

>+    // Sync-related links
>+    if (Services.prefs.prefHasUserValue("services.sync.username")) {
>+      document.getElementById("setupSyncLink").hidden = true;
>+    }

This works fine for subsequent loads, but it won't hide the link on the page that you clicked it on, after setup is complete. Is there a "sync-setup-complete" notification (or something like that) that you can use to hide the link in already-loaded tabs? See Browser:HideSessionRestoreButton for similar situation (bug 644998).

Comment 29

[Philipp von Weitershausen [:philikon]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #28)
> Comment on attachment 568514 [details] [diff] [review] [diff] [details] [review]
> part 1, v3: reversed "pair" and "setup" positions per mockup

Slightly old revision of the patch, but not too different.

> > function BrowserOnAboutPageLoad(document) {
> 
> >+    // Sync-related links
> >+    if (Services.prefs.prefHasUserValue("services.sync.username")) {
> >+      document.getElementById("setupSyncLink").hidden = true;
> >+    }
> 
> This works fine for subsequent loads, but it won't hide the link on the page
> that you clicked it on, after setup is complete. Is there a
> "sync-setup-complete" notification (or something like that) that you can use
> to hide the link in already-loaded tabs? See
> Browser:HideSessionRestoreButton for similar situation (bug 644998).

Yuuup. Already taken care of in part 2!

Comment 30

[Chenxia Liu [:liuche]]

Attachment: Part 1, v6: css color attrs

also filed follow-up bug to localization for part 2 of comment (bug 697217). Insta-refresh of links is addressed in patch Part 2, but will be modify to use e10s-friendly approach.

Comment 31

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #27)
> to synthesize a click event (`win` is the window that `button` is in, in
> this case the contentWindow of the about:home page). To observe the dialog
> window being opened, do something like this:

Here's a better version, one that will hopefully work:

  let expectedDialog = "Weave:AccountSetup";
  Services.ww.registerNotification(function onWindow(subject, topic) {
    let win = subject.QueryInterface(Ci.nsIDOMWindow);
    win.addEventListener("load", function onLoad() {
      win.removeEventListener("load", onLoad, false);
      let wintype = win.document.documentElement.getAttribute("windowtype");
      if (topic == "domwindowopened" && wintype == expectedDialog) {
        Services.ww.unregisterNotification(onWindow);
        executeSoon(runNextTest);
      }    
    }, false);
  });

Comment 32

[Chenxia Liu [:liuche]]

Attachment: Part 3, v2: added tests for correct dialog opened on link click

Comment 33

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569747 [details] [diff] [review]
Part 3, v2: added tests for correct dialog opened on link click

This looks great, but you could avoid a lot of code duplication by creating a helper function, `expectDialogWindow(expectedDialog)` that contains basically the following lines:

>+    Services.ww.registerNotification(function onWindow(subject, topic) {
>+      let win = subject.QueryInterface(Components.interfaces.nsIDOMWindow);
>+      win.addEventListener("load", function onLoad() {
>+        win.removeEventListener("load", onLoad, false);
>+        let wintype = win.document.documentElement.getAttribute("windowtype");
>+        if (topic == "domwindowopened" && wintype == expectedDialog) {
>+          win.close();
>+          Services.ww.unregisterNotification(onWindow);
>+          executeSoon(runNextTest);
>+        }
>+      }, false);

And then just call it in the tests with `expectDialogWindow("Weave:AccountSetup")` etc.

Comment 34

[Chenxia Liu [:liuche]]

Attachment: Part 3, v3: merged some tests, added helper function

Comment 35

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569765 [details] [diff] [review]
Part 3, v3: merged some tests, added helper function

nice!

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 569461 [details] [diff] [review]
Part 1, v6: css color attrs

>diff --git a/browser/base/content/aboutHome.css b/browser/base/content/aboutHome.css

>+/* Hack to make buttons appear as links. Necessary to piggyback on browser.js
>+ * OnBrowserClick listener, which checks that click originates from a button.

Can't we just add a || event.target.className == "sync-link" or somesuch to BrowserOnClick to avoid the need for this hackery? Please at least file a followup for this.

Comment 37

[Philipp von Weitershausen [:philikon]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #36)
> >+/* Hack to make buttons appear as links. Necessary to piggyback on browser.js
> >+ * OnBrowserClick listener, which checks that click originates from a button.
> 
> Can't we just add a || event.target.className == "sync-link" or somesuch to
> BrowserOnClick to avoid the need for this hackery? Please at least file a
> followup for this.

+1 for follow-up! Chenxia, can you file? (And land this on s-c please :))

Comment 38

[Chenxia Liu [:liuche]]

Changeset urls:

https://hg.mozilla.org/services/services-central/rev/15eaaaaa8dce
https://hg.mozilla.org/services/services-central/rev/e6a733288d72
https://hg.mozilla.org/services/services-central/rev/b1dcdd118676

Comment 39

[Chenxia Liu [:liuche]]

STR: Behavior for sync links ("Set Up Sync" and "Pair Device") on about:home

On load of about:home, if Sync has not been set up, both links should appear below the "About Mozilla" link.
After setting up sync successfully, the "Set Up Sync" link should no longer be visible on about:home, without refreshing the page.

On load of about:home, if Sync is already set up, only "Pair Device" should be visible.
Unlinking Sync (Pref->Sync->unlink) successfully should cause the "Set Up Sync" link to be visible on about:home, without refresh.

Comment 40

[Tracy Walker [:tracy]]

looks good on s-c nightly.  

Note last line of STR's in comment 39.  Statement doesn't exclude Pair Device, which should always be present.

Comment 41

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/15eaaaaa8dce
https://hg.mozilla.org/mozilla-central/rev/e6a733288d72
https://hg.mozilla.org/mozilla-central/rev/b1dcdd118676