Closed Bug 1184663 Opened 9 years ago Closed 8 years ago

Remove the Sync promotion in the password doorhanger

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: rfeeley, Assigned: eoger)

Details

Attachments

(2 files, 2 obsolete files)

When users are logging in to a site, they are often asked to save a password in what's called a door hanger.

If they have not set up Sync, at the bottom of the door hanger appears a panel that promotes Sync with a link to SUMO.

Clicking the link closes the door hanger, and the password is not saved.

We recommend removing the link, and instead providing instruction on what benefit Sync brings, and where it can be enabled.

Roughly we had something like: "Want your passwords on other devices?
Sign up for Sync in the Firefox preferences." but could use your help to improve it.
How about this?

Access your passwords everywhere you use Firefox. Open Preferences and select Sync.
> Access your passwords everywhere you use Firefox. Open Preferences and
> select Sync.

Great! I will assign the bug to Edouard who is on a string-changing roll lately.
Assignee: matej → edouard.oger
Attached patch bug-1184663.patch (obsolete) — Splinter Review
I'm not very happy about this patch, we introduce a lot of edge cases in the XBL. What do you think Mark?
Flags: needinfo?(markh)
Comment on attachment 8638747 [details] [diff] [review]
bug-1184663.patch

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

I think we can make this OK without making it much uglier than it already is - shame about the stringid change though :(

::: browser/base/content/urlbarBindings.xml
@@ +3071,5 @@
>        </property>
>        <property name="_notificationMessage">
>          <getter><![CDATA[
> +          if (this._notificationType === "passwords") {
> +            return gNavigatorBundle.getFormattedString(

Best I can tell, %S will be replaced with the *first* string in the array - so I think it would probably be OK to always pass the 2 strings to getFormattedString if you reverse them (ie, to ensure syncBrandShortName is used as %1$S).

@@ +3146,5 @@
>              else {
>                this._viewsLeft = viewsLeft - 1;
>              }
>  
> +            if (this._notificationType !== "passwords") {

I'd be inclined to change _notificationLink to return null for "passwords" (probably making it a switch/case at the same time) and treating null specially - probably by explicitly hiding this._promolink - I suspect that there's a risk the way it's written here that it might have a href from a previous popup?

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +465,5 @@
>  syncPromoNotification.bookmarks.description=You can access your bookmarks on all your devices with %S.\u0020
>  # LOCALIZATION NOTE (syncPromoNotification.passwords.label): This appears in
> +# the remember password panel. %1$S will be replaced by brandShortName (e.g. Firefox).
> +# %2$S will be replaced by syncBrandShortName
> +syncPromoNotification.passwords.label=Access your passwords everywhere you use %1$S. Open Preferences and select %2$S.

sadly we aren't allowed to keep the same string id when changing the value - so you are probably going to be forced to change this to something like "syncPromoNotificatio.sync-passwords.label" or something - which is going to have an unfortunate knock-on effect
Attachment #8638747 - Flags: feedback+
Flags: needinfo?(markh)
Maybe we could improve all the copies for sync promotion? We have it in bookmarks and addons too (and it would avoid making edge cases too). Here they are:

>syncPromoNotification.bookmarks.description=You can access your bookmarks on all your devices with %S.\u0020
>syncPromoNotification.passwords.description=You can access your passwords on all your devices with %S.\u0020
>syncPromoNotification.addons.description=You can access your add-ons on all your devices with %S.\u0020
>syncPromoNotification.addons-sync-disabled.description=You can use your %S account to synchronize add-ons across multiple devices.\u0020
Flags: needinfo?(rfeeley)
Yes!
Flags: needinfo?(rfeeley)
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Priority: -- → P1
:eoger, are you still planning on landing this, or should we have someone else pick it up?
Flags: needinfo?(edouard.oger)
I'm still working on the synced tabs bug in the little time I got, I can't work on this one sorry :/
Flags: needinfo?(edouard.oger)
I haven't given this patch much attention until now. I acknowledge the problem, and I'm not that pleased with the solution, either. How do folks feel about just removing it entirely? Saying "oh btw, in the future you might want to set up Sync" feels pretty awkward and unactionable to me.
Assignee: edouard.oger → nobody
Priority: P1 → P3
(In reply to Chris Karlof [:ckarlof] from comment #9)
> I haven't given this patch much attention until now. I acknowledge the
> problem, and I'm not that pleased with the solution, either. How do folks
> feel about just removing it entirely? Saying "oh btw, in the future you
> might want to set up Sync" feels pretty awkward and unactionable to me.

I agree that would be better than the status quo, which is clumsy and somewhat "hostile" in that it basically invites the user to not be able to save their password. For this reason I think we should do something sooner rather than later - it's just a matter of exactly what :) So ni? rfeeley...
Flags: needinfo?(rfeeley)
Ni! Future designs will replace the current collapse/defer behavior with "don't save"/discard so removing it entirely is best.
Flags: needinfo?(rfeeley)
Summary: Improve the copy of the Sync promotion in the password → Remove the Sync promotion in the password doorhanger
The sync promotion is distracting in the context of saving a login. Let's prioritize this for removal.
Flags: needinfo?(edwong)
Priority: P3 → P2
I'm tracking this
Flags: needinfo?(edwong)
Attached patch bug-1184663.patch (obsolete) — Splinter Review
Taking this bug back
Assignee: nobody → edouard.oger
Attachment #8638747 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8740091 - Flags: review?(markh)
Comment on attachment 8740091 [details] [diff] [review]
bug-1184663.patch

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

I think we also want to remove the block at https://dxr.mozilla.org/mozilla-central/rev/e847cfcb315f511f4928b03fd47dcf57aad05e1e/browser/base/content/urlbarBindings.xml#2590. Matt, is there anything else here you think we need to undo for this bug? See comment 9 and later for less-noisy context.
Attachment #8740091 - Flags: review?(markh) → feedback?(MattN+bmo)
Here's an updated patch in the meantime.
Attachment #8740091 - Attachment is obsolete: true
Attachment #8740091 - Flags: feedback?(MattN+bmo)
Attachment #8741152 - Flags: feedback?(MattN+bmo)
Comment on attachment 8741152 [details] [diff] [review]
bug-1184663.patch

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

I have a lot going on and a backlog of reviews so I'll delegate back to Mark for a thorough review.

We could perhaps cleanup the stored pref values but that just seems like it adds risk for almost no gain.
Attachment #8741152 - Flags: review?(markh)
Attachment #8741152 - Flags: feedback?(MattN+bmo)
Attachment #8741152 - Flags: feedback+
It's possible there are tests testing the promo that you may need to change/remove so a try push will be good.
Thanks Matt,

> We could perhaps cleanup the stored pref values but that just seems like it adds risk for almost no gain.

Yeah, agreed.

I had a decent look before, so Edouard, can you please remove the block I mentioned in comment 15, push it to try and when it's green re-request review? Thanks!
Attachment #8741152 - Flags: review?(markh)
Comment on attachment 8741152 [details] [diff] [review]
bug-1184663.patch

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

double r+ :)
Attachment #8741152 - Flags: review?(markh)
Attachment #8741152 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4bc2899f5d53
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
I have reproduced this bug with nightly 42.0a1 (2015-07-16) on windows 10, 32 bit.

The bug’s fix is now verified on Latest beta 48.0b10.

Build ID:20160721144529
User Agent:Mozilla/5.0 (Windows NT 10.0; rv:48.0) Gecko/20100101 Firefox/48.0

[testday-20160722]
You need to log in before you can comment on or make changes to this bug.