Closed
Bug 1120716
Opened 10 years ago
Closed 10 years ago
Offer a "learn more" link somewhere in the migration UI
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
People
(Reporter: markh, Assigned: adw)
References
Details
Attachments
(4 files, 2 obsolete files)
36.34 KB,
image/png
|
Details | |
227.51 KB,
image/png
|
Details | |
58.58 KB,
image/png
|
Details | |
13.47 KB,
patch
|
Gavin
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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)
Comment 1•10 years ago
|
||
A link seems to be the most compatible element given that there are already 2 buttons in some contexts
Flags: needinfo?(rfeeley)
Assignee | ||
Comment 3•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Flags: qe-verify+
Flags: firefox-backlog+
Assignee | ||
Comment 4•10 years ago
|
||
* 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)
Reporter | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
FYI here are the three xul:notification priorities on OS X with the patch applied.
Assignee | ||
Comment 7•10 years ago
|
||
Top: Infobar in the main window
Middle: In-content prefs
Bottom: Dialog prefs
Attachment #8558243 -
Flags: feedback?(rfeeley)
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Reporter | ||
Comment 9•10 years ago
|
||
(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 :)
Comment 10•10 years ago
|
||
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.
Reporter | ||
Comment 11•10 years ago
|
||
(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)
Reporter | ||
Comment 12•10 years ago
|
||
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+
Comment 13•10 years ago
|
||
Link looks great, but yeah, infobar doesn't match.
Flags: needinfo?(rfeeley)
Comment 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
Blair, would you mind double checking my CSS?
Attachment #8558222 -
Attachment is obsolete: true
Attachment #8558840 -
Flags: review?(bmcbride)
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
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.
Assignee | ||
Comment 18•10 years ago
|
||
Er, I forgot to remove the text-decoration: underline in the before shots, so imagine the underlines not being there. :-/
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8558840 -
Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Assignee | ||
Comment 21•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8559401 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 22•10 years ago
|
||
status-firefox37:
--- → fixed
status-firefox38:
--- → fixed
Comment 23•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8558243 -
Flags: feedback?(rfeeley)
You need to log in
before you can comment on or make changes to this bug.
Description
•