Closed Bug 1120716 Opened 5 years ago Closed 5 years ago

Offer a "learn more" link somewhere in the migration UI

Categories

(Firefox :: Sync, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox37 --- verified
firefox38 --- verified

People

(Reporter: markh, Assigned: adw)

References

Details

Attachments

(4 files, 2 obsolete files)

I've noticed that nowhere in the migration flow is a "learn more" link for people who are offered a migration.  There may be a number of users who want to understand exactly why this migration is being offered and how the new system works, but there's no way for them to learn about it.

I was thinking the infobar, or sync prefs, or something, should have a "learn more" link which takes the user to an MDN article.

Ryan, do you agree?  If not, we can close this as WONTFIX.  If so, I guess we need to know exactly where such a link would be added and a new bug for someone to create that content.
Flags: needinfo?(rfeeley)
A link seems to be the most compatible element given that there are already 2 buttons in some contexts
Flags: needinfo?(rfeeley)
Duplicate of this bug: 1127097
From bug 1127097, the link is https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/sync-upgrade
Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
Attached patch patch (obsolete) — Splinter Review
* Adds the link in the main infobar and the two prefs infobars in the needs-user state.  The link isn't offered if the user already upgraded on a different device because presumably they already have learned more.

* Added a link param to Weave's Notification class.

* The link is inserted into its xul:notification using the node.ownerDocument.getAnonymousElementByAttribute trick, which is how the other links in xul:notifications work.  Ideally there'd be an API...

* Links in xul:notifications get the default hyperlink color, which looks terrible at least in OS X notifications with INFO priorities.  And not great elsewhere IMO.  I took the liberty of changing the color globally to be the same as the notification text, and then I added an underline.  It looks better.

* I stuck the URL of the support page in a pref.  Not sure why these have to be in prefs, but that seems to be the standard.  Not sure whether the pref name I chose is OK.
Attachment #8558222 - Flags: review?(mhammond)
Comment on attachment 8558222 [details] [diff] [review]
patch

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

r- just for the pref and string, but otherwise looks good.  That said though, (a) can we get a screen-shot attached do we can run it past Ryan, and (b) I'm not particularly comfortable with the CSS change - I don't have a problem with it, but my knowledge is such that I don't know what I'd be looking out for.  Blair, are you able to sanity check the CSS changes here, or suggest someone else?  See comment 4 for the rationale.

::: browser/app/profile/firefox.js
@@ +1222,5 @@
>  
>  #ifdef MOZ_SERVICES_SYNC
>  // The sync engines to use.
>  pref("services.sync.registerEngines", "Bookmarks,Form,History,Password,Prefs,Tab,Addons");
> +pref("services.sync.fxaMigration.learnMoreURL", "https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/sync-upgrade");

I don't think we want this pref - see later.

::: services/sync/modules/FxaMigrator.jsm
@@ +468,5 @@
>    }),
>  
> +  get learnMoreLink() {
> +    try {
> +      var url = Services.prefs.getCharPref(LEARN_MORE_URL_PREF);

I think this should just be: Services.urlFormatter.formatURLPref("app.support.baseURL") + "sync-upgrade" (so no need for a new pref - hard-coding the "tail" portion is typical)

@@ +474,5 @@
> +      return null;
> +    }
> +    let sb = Services.strings.createBundle("chrome://weave/locale/services/sync.properties");
> +    return {
> +      text: sb.GetStringFromName("sync.eol.learnMore.label"),

I can't see where this string is set in this patch, but can we investigate reusing another string just for uplift (eg, maybe we *do* add a new string for this patch to land in central, but abuse things in a patch for Aurora that reuses?)
Attachment #8558222 - Flags: review?(mhammond)
Attachment #8558222 - Flags: review-
Attachment #8558222 - Flags: feedback?(bmcbride)
Attached image notification-links.png
FYI here are the three xul:notification priorities on OS X with the patch applied.
Attached image learn-more.png
Top: Infobar in the main window
Middle: In-content prefs
Bottom: Dialog prefs
Attachment #8558243 - Flags: feedback?(rfeeley)
(In reply to Mark Hammond [:markh] from comment #5)
> @@ +474,5 @@
> > +      return null;
> > +    }
> > +    let sb = Services.strings.createBundle("chrome://weave/locale/services/sync.properties");
> > +    return {
> > +      text: sb.GetStringFromName("sync.eol.learnMore.label"),
> 
> I can't see where this string is set in this patch, but can we investigate
> reusing another string just for uplift (eg, maybe we *do* add a new string
> for this patch to land in central, but abuse things in a patch for Aurora
> that reuses?)

That's the string created by this changeset: http://hg.mozilla.org/mozilla-central/rev/9e834d0decc2  I really don't think this needs a new string.  This existing string is exactly what we want.
(In reply to Drew Willcoxon :adw from comment #8)
> That's the string created by this changeset:
> http://hg.mozilla.org/mozilla-central/rev/9e834d0decc2  I really don't think
> this needs a new string.  This existing string is exactly what we want.

Ack - sorry about that - I don't know how I managed to screw up my mxr search for it :)
I'm hoping we can use the native prefs style for the native prefs and the in-content style defined by Maslaney for the in-content prefs.
(In reply to Ryan Feeley from comment #10)
> I'm hoping we can use the native prefs style for the native prefs and the
> in-content style defined by Maslaney for the in-content prefs.

You mean for the entire infobar, right?  Sadly this bug is just about the link, and we probably need a new bug for the general look of the bar.

Or maybe I'm missing what you are saying and just the link in the infobar needs different styling?

Ryan, can you please clarify?
Flags: needinfo?(rfeeley)
Comment on attachment 8558222 [details] [diff] [review]
patch

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

Given my mistake about the string and the fact that changing the pref is a trivial change I don't need to see again.  Assuming rfeeley and unfocused like it, go for it!
Attachment #8558222 - Flags: review- → review+
Link looks great, but yeah, infobar doesn't match.
Flags: needinfo?(rfeeley)
Comment on attachment 8558222 [details] [diff] [review]
patch

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

As discussed on IRC.
Attachment #8558222 - Flags: feedback?(bmcbride) → feedback+
Attached patch patch v2 (obsolete) — Splinter Review
Blair, would you mind double checking my CSS?
Attachment #8558222 - Attachment is obsolete: true
Attachment #8558840 - Flags: review?(bmcbride)
Comment on attachment 8558840 [details] [diff] [review]
patch v2

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

::: toolkit/themes/linux/global/notification.css
@@ +5,5 @@
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
> +notification,
> +.messageText > .text-link {
> +  color: InfoText !important;

Can you double check usage of !important is needed for these? Generally best to avoid them, if possible.
Attachment #8558840 - Flags: review?(bmcbride) → review+
Yeah, the !importants are necessary it seems.  I tested on all three platforms.

To complement the OS X screenshots I posted earlier, here are before and after screenshots on Windows and Linux, Windows on top, Linux on bottom.  The only notification that looks bad with blue links is the red high-priority one, on both platforms.

I'll land this patch now, but I can land a follow-up that retains the current blue link color for info- and medium-priority notifications on Windows and Linux, using the main text color only for high-priority notifications, if anyone thinks that's a good idea.
Er, I forgot to remove the text-decoration: underline in the before shots, so imagine the underlines not being there. :-/
https://hg.mozilla.org/mozilla-central/rev/618b9a0b2941
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Comment on attachment 8559401 [details] [diff] [review]
landed unbitrotted patch

This patch must land after bug 1097406 lands on Aurora.

Approval Request Comment
[Feature/regressing bug #]: Not a regression, but part of legacy-Sync-to-FxA migration.

[User impact if declined]: This patch adds a "Learn more" link to messsages asking the user to upgrade from old Sync to FxA. Without that link, the user still sees the messages but doesn't have an easy way to understand what they mean.

[Describe test coverage new/current, TreeHerder]: Manual testing only.

[Risks and why]: Low risk, simply adding a link to existing messages in an infobar and messages in the Sync preferences pane.

[String/UUID change made/needed]: None. (The appropriate "Learn more" string already exists in our string files.)
Attachment #8559401 - Flags: approval-mozilla-aurora?
Attachment #8559401 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1130850
Verified that the 'Learn more' link is present in the migration UI and redirects to https://support.mozilla.org/en-US/kb/firefox-sync-upgrade-frequently-asked-questions?as=u&utm_source=inproduct. I tested on Windows 7 64-bit, Mac OS X 10.10 and Ubuntu 14.04 32-bit using Firefox 37beta4 and latest Developer Edition.
Status: RESOLVED → VERIFIED
Attachment #8558243 - Flags: feedback?(rfeeley)
You need to log in before you can comment on or make changes to this bug.