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

RESOLVED FIXED in Firefox 10

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: rnewman, Assigned: liuche)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [about-home][verified in services])

Attachments

(3 attachments, 14 obsolete attachments)

2.61 KB, patch
philikon
: review+
Details | Diff | Splinter Review
6.96 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
6.11 KB, patch
philikon
: review+
Details | Diff | Splinter Review
Depends on Home Tab, of course. (No bug for that yet.)
Blocks: 675826
(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).
Depends on: 588230
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.
Depends on: 544819
Summary: "Pair device" link at bottom of home tab: desktop → "Pair device" and "Setup sync" links at bottom of the Firefox home tab
Summary: "Pair device" and "Setup sync" links at bottom of the Firefox home tab → "Pair a Device" and "Set up Sync" links at bottom of the Firefox home tab
Assignee: nobody → ally
Created attachment 567236 [details] [diff] [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
(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.
Created attachment 567585 [details] [diff] [review]
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.
Attachment #567236 - Attachment is obsolete: true
Attachment #567585 - Attachment is obsolete: true
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().
Assignee: ally → liuche
(Assignee)

Comment 7

6 years ago
Created attachment 568316 [details] [diff] [review]
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"
Attachment #568316 - Flags: review?(philipp)
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.
Attachment #568316 - Flags: review?(philipp) → review-
(Assignee)

Comment 9

6 years ago
Created attachment 568456 [details] [diff] [review]
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.
(Assignee)

Updated

6 years ago
Attachment #568456 - Flags: review?(philipp)
Attachment #568316 - Attachment is obsolete: true
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.
Attachment #568456 - Flags: review?(philipp)
Attachment #568456 - Flags: review?(gavin.sharp)
Attachment #568456 - Flags: feedback+
(Assignee)

Comment 11

6 years ago
Created attachment 568494 [details] [diff] [review]
Part 2, v1: show/hide "Set Up Sync" in about:home if user sets up or removes sync
Attachment #568494 - Flags: review?(philipp)
(Assignee)

Comment 12

6 years ago
Created attachment 568514 [details] [diff] [review]
part 1, v3: reversed "pair" and "setup" positions per mockup
Attachment #568456 - Attachment is obsolete: true
Attachment #568456 - Flags: review?(gavin.sharp)
Attachment #568514 - Flags: review?(gavin.sharp)
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.
Attachment #568494 - Flags: review?(philipp) → feedback+
(Assignee)

Comment 14

6 years ago
Created attachment 568535 [details] [diff] [review]
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.
Attachment #568494 - Attachment is obsolete: true
Attachment #568535 - Flags: review?(philipp)
(Assignee)

Comment 15

6 years ago
Created attachment 568541 [details] [diff] [review]
part 2, v2 dupe
Attachment #568535 - Attachment is obsolete: true
Attachment #568535 - Flags: review?(philipp)
Attachment #568541 - Flags: review?(philipp)
Comment on attachment 568541 [details] [diff] [review]
part 2, v2 dupe

This is the same file as v2. Did you forget a qrefresh? :)
Attachment #568541 - Attachment description: part 2, v3: added helper function → part 2, v2 dupe
Attachment #568541 - Attachment is obsolete: true
Attachment #568541 - Flags: review?(philipp)
(Assignee)

Comment 17

6 years ago
Created attachment 569167 [details] [diff] [review]
Part 2, v4: ACTUALLY uploaded helper function (sorry!)
Attachment #569167 - Flags: review?(philipp)
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
Attachment #569167 - Flags: review?(philipp) → review+
(Assignee)

Comment 19

6 years ago
Created attachment 569216 [details] [diff] [review]
Part 2, v5: fixed spacing nit, ready for UX review
Attachment #569167 - Attachment is obsolete: true
Attachment #569216 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #569216 - Flags: review?(gavin.sharp)
(Assignee)

Comment 20

6 years ago
Created attachment 569234 [details] [diff] [review]
part 3, v0.1: tests for sync-links

Failing on link visibility change after sync setup-complete or sync unlink.
Attachment #569234 - Flags: feedback?(philipp)
(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 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
(Assignee)

Comment 23

6 years ago
Created attachment 569261 [details] [diff] [review]
Part 1, v4: fixed UI details
Attachment #568514 - Attachment is obsolete: true
Attachment #568514 - Flags: review?(gavin.sharp)
Attachment #569261 - Flags: review?(philipp)
(Assignee)

Comment 24

6 years ago
Created attachment 569262 [details] [diff] [review]
Part 1, v5: "Set Up" not "Setup" in localization
Attachment #569261 - Attachment is obsolete: true
Attachment #569261 - Flags: review?(philipp)
Attachment #569262 - Flags: review?(philipp)
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!
Attachment #569262 - Flags: review?(philipp)
Attachment #569262 - Flags: review?(gavin.sharp)
Attachment #569262 - Flags: review+
Attachment #569216 - Flags: review+
(Assignee)

Comment 26

6 years ago
Created attachment 569268 [details] [diff] [review]
Part 3, v1: tests for aboutHome sync links layout, sync links behavior after sync setup/unlink
Attachment #569234 - Attachment is obsolete: true
Attachment #569234 - Flags: feedback?(philipp)
Attachment #569268 - Flags: review?(philipp)
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 :).
Attachment #569268 - Flags: review?(philipp) → feedback+
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).
Attachment #568514 - Attachment is obsolete: false
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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!
Attachment #568514 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 697217
(Assignee)

Comment 30

6 years ago
Created attachment 569461 [details] [diff] [review]
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.
Attachment #569262 - Attachment is obsolete: true
Attachment #569262 - Flags: review?(gavin.sharp)
Attachment #569461 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Blocks: 697259
(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);
  });
(Assignee)

Comment 32

6 years ago
Created attachment 569747 [details] [diff] [review]
Part 3, v2: added tests for correct dialog opened on link click
Attachment #569268 - Attachment is obsolete: true
Attachment #569747 - Flags: review?(philipp)
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.
Attachment #569747 - Flags: review?(philipp) → feedback+
(Assignee)

Comment 34

6 years ago
Created attachment 569765 [details] [diff] [review]
Part 3, v3: merged some tests, added helper function
Attachment #569747 - Attachment is obsolete: true
Attachment #569765 - Flags: review?(philipp)
Comment on attachment 569765 [details] [diff] [review]
Part 3, v3: merged some tests, added helper function

nice!
Attachment #569765 - Flags: review?(philipp) → review+
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.
Attachment #569461 - Flags: review?(gavin.sharp) → review+
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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 :))
(Assignee)

Comment 38

6 years ago
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
Whiteboard: [fixed in services]
(Assignee)

Comment 39

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 697750
Depends on: 697874
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.
Whiteboard: [fixed in services] → [verified in services]
No longer blocks: 697259, 697750
Depends on: 697259, 697750

Updated

6 years ago
No longer depends on: 544819, 588230
Summary: "Pair a Device" and "Set up Sync" links at bottom of the Firefox home tab → "Pair a Device" and "Set up Sync" links at bottom of the Firefox home page

Updated

6 years ago
Component: Firefox Sync: UI → General
Product: Mozilla Services → Firefox
QA Contact: sync-ui → general

Updated

6 years ago
Whiteboard: [verified in services] → [about-home][verified in services]
https://hg.mozilla.org/mozilla-central/rev/15eaaaaa8dce
https://hg.mozilla.org/mozilla-central/rev/e6a733288d72
https://hg.mozilla.org/mozilla-central/rev/b1dcdd118676
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10

Updated

5 years ago
Depends on: 728966
You need to log in before you can comment on or make changes to this bug.