1144856 - Change the doorhanger messages to mention "login" instead of "password"

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[:Paolo Amadini]

Change the password save or change request doorhanger message.

From: Would you like to remember the password on example.com?
To: Would you like Firefox to remember this login for example.com?

The action buttons should also mention "login" instead of "password".

Comment 1

[Justin Dolske [:Dolske]]

Do we want to say "login" even for logins where there is only a password? (EG for a PIN code, or for something like Mailman where there's no username at all.)

Comment 2

[:Paolo Amadini]

Well, one of the things I was thinking about is how much we want to make the labels to be "dynamic" when the user edits the username field.

We could have the button change from "Remember Login" to "Update Login" if the user types an existing username. If we treat an empty username differently, we may have the additional combinations "Remember Password" and "Update Password" when the username is empty.

And the question to the top might change as well in these cases, but I'm not sure we want to do this, since a change to an element placed before the currently focused element may be unexpected, and additionally may cause the buttons themselves to move.

Comment 3

[Ryan Feeley [:rfeeley]]

Attachment: remember-update.001.png

Four states of the edited login form. We won't change the headline dynamically to avoid awkwardness of laying out the door hanger again.

Comment 4

[Ryan Feeley [:rfeeley]]

Attachment: remember-update.002.png

Comment 5

[Ryan Feeley [:rfeeley]]

Attachment: remember-update.003.png

Comment 6

[:Paolo Amadini]

Attachment: The patch

Comment 7

[:Paolo Amadini]

Comment on attachment 8584164 [details] [diff] [review]
The patch

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

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties
@@ +9,5 @@
> +# String is the login's hostname.
> +rememberLoginMsg = Remember this login for %S?
> +rememberLoginButtonText = Remember
> +rememberLoginButtonAccessKey = R
> +# LOCALIZATION NOTE (rememberLoginMsg):

Fixed this copy-and-paste error locally.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8584164 [details] [diff] [review]
The patch

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

I would prefer we make this change throughout the UI (page info, prefs, and the dialog) to remain consistent but I'll leave that between you and Ryan for coordination.

Comment 9

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #8)
> I would prefer we make this change throughout the UI (page info, prefs, and
> the dialog) to remain consistent but I'll leave that between you and Ryan
> for coordination.

Yeah, I think the idea was that we should make the doorhanger internally consistent now that we have the new structure, without blocking on all the other string changes. Ryan, is that correct?

I'll land the strings and we can revert their usage if needed.

Comment 10

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/d9c0f2a3a0a6

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/acb7c9364516 for mochitest-5 test failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=2430506&repo=fx-team

Comment 12

[:Paolo Amadini]

Updated the test checking the string and started a tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba05fb1f7053

Comment 13

[Ryan Feeley [:rfeeley]]

If you feel we can make them all in one big change, I'm fine for leaving it at "password(s)" in the new capture doorhanger for now.

Comment 14

[Matthew N. [:MattN]]

(In reply to Ryan Feeley from comment #13)
> If you feel we can make them all in one big change, I'm fine for leaving it
> at "password(s)" in the new capture doorhanger for now.

OK, it doesn't necessarily need to be one change but it should probably be in the same release.

Comment 15

[:Paolo Amadini]

(In reply to Ryan Feeley from comment #13)
> If you feel we can make them all in one big change

We can't change all the strings for the 39 release, however. I remember that when I asked about this during our last meeting, you told us you preferred to have the panel updated in advance of the rest of the user interface, rather than having the new fields released in 39 with the old strings. Do I remember incorrectly, or did we say something different? Or we're just changing our mind?

I ask because I'm fine both ways, I believe Matt leans towards doing this later, but you're the final go-to person for this decision.

Comment 16

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #15)
> We can't change all the strings for the 39 release

Why not? We still have time.

Comment 17

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #16)
> (In reply to :Paolo Amadini from comment #15)
> > We can't change all the strings for the 39 release
> 
> Why not? We still have time.

Well, if you're volunteering to do this or to find someone who could, it would be great :-)

My last working day before uplift is tomorrow and I'd like to prototype the login fill mechanism for the doorhanger while we have the opportunity to talk about it in person.

Comment 18

[Ryan Feeley [:rfeeley]]

I'm fine either way. Long term we want to move to say login everywhere, but I think users can substitute login for password in their heads with minimal effort.

Comment 19

[:Paolo Amadini]

For the record, we're not making this string change for the 39 release.

Comment 20

[:Paolo Amadini]

Attachment: With updated test

Comment 21

[Ryan Feeley [:rfeeley]]

"Would you like Firefox to remember this login?"

and if no username:

"Would you like Firefox to remember this password?"

Comment 22

[Matthew N. [:MattN]]

(In reply to Ryan Feeley from comment #21)
> and if no username:
> 
> "Would you like Firefox to remember this password?"

Do you expect this text to change if a username is added to the field?

Comment 23

[Ryan Feeley [:rfeeley]]

Nice-to-have if low effort. I can live without it, but it's a nice touch.

Comment 24

[Ryan Feeley [:rfeeley]]

In summary:
Would you like Firefox to remember this login?
Would you like Firefox to remember this password?
Would you like to update this login?
Would you like to update this password?

Comment 25

[Bill Maggs (bmaggs)]

I think it is probably unnecessary to have two different strings. The word logins provides enough info, and not that much value to change it in that one case.

Comment 26

[Riadh Chtara[:rchtara]]

Attachment: MozReview Request: Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8631815 [details]
MozReview Request: Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

https://reviewboard.mozilla.org/r/12937/#review11545

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:9
(Diff revision 1)
> +rememberLoginMsg = Would you like Firefox to remember this login?
> +rememberLoginMsgNoUser = Would you like Firefox to remember this password?

Oh, sorry, I forgot to tell you that you can't hard-code Firefox in strings (especially not in Toolkit). You need to substitute brandShortName into the string and then add a localization note saying: "%S is brandShortName."

See https://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=027ffd03bae2#1646 for an example.

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:13
(Diff revision 1)
> +# LOCALIZATION NOTE (updateLoginMsg):

This note is empty so the line should be removed.

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:11
(Diff revision 1)
> +rememberLoginButtonText = Remember
> +rememberLoginButtonAccessKey = R

Did `rememberButtonText` not work due to the ampersand?

::: toolkit/components/passwordmgr/test/test_notifications.html:358
(Diff revision 1)
> -        expectedText = /^Would you like to remember the password on example.org\?$/;
> +        expectedText = /^Would you like Firefox to remember this login\?$/;

This file will need to use brandShortName too.

Comment 28

[Riadh Chtara[:rchtara]]

Comment on attachment 8631815 [details]
MozReview Request: Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

Comment 29

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/12937/#review11545

> Did `rememberButtonText` not work due to the ampersand?

I just added because I wanted to have the msg, button text, button key in the same name space

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8631815 [details]
MozReview Request: Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

https://reviewboard.mozilla.org/r/12937/#review11577

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:797
(Diff revisions 1 - 2)
> +    let brandName = brandBundle.GetStringFromName("brandShortName");
> +    let promptMsg = type == "password-save" ? this._getLocalizedString(saveMsgNames.prompt, [brandName])

s/brandName/brandShortName/ so it doesn't get confused with brandFullName which includes "Mozilla"

::: toolkit/components/passwordmgr/test/test_notifications.html:67
(Diff revisions 1 - 2)
> +const BRAND_BUNDLE = "chrome://branding/locale/brand.properties";

Nit: move this const with the other (below Services)

::: toolkit/components/passwordmgr/test/test_notifications.html:105
(Diff revisions 1 - 2)
> +   let brandName = brandBundle.GetStringFromName("brandShortName");
> +
> +

Nit: Delete the added new lines

::: toolkit/components/passwordmgr/test/test_notifications.html:368
(Diff revisions 1 - 2)
> -        expectedText = /^Would you like Firefox to remember this login\?$/;
> -        ok(expectedText.test(notificationText), "Checking text: " + notificationText);
> +        expectedText = "Would you like " + brandName + " to remember this login?";
> +        is(expectedText, notificationText, "Checking text: " + notificationText);

Good idea to get rid of the regex

::: toolkit/components/passwordmgr/test/test_notifications.html:382
(Diff revisions 1 - 2)
> -        ok(expectedText.test(notificationText), "Checking text: " + notificationText);
> +        is(expectedText, notificationText, "Checking text: " + notificationText+ "Would you like " + brandName + " to remember this login");

Looks like: '+ "Would you like " + brandName + " to remember this login"' was accidentally pasted here?

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:8
(Diff revisions 1 - 2)
>  # LOCALIZATION NOTE (rememberLoginMsg):
> -rememberLoginMsg = Would you like Firefox to remember this login?
> -rememberLoginMsgNoUser = Would you like Firefox to remember this password?
> +# String is the browser name.
> +rememberLoginMsg = Would you like %S to remember this login?
> +# LOCALIZATION NOTE (rememberLoginMsgNoUser):
> +# String is the browser name.
> +rememberLoginMsgNoUser = Would you like %S to remember this password?

Combine these into one localization note and put it on one line with "%S":

\# LOCALIZATION NOTE (rememberLoginMsg, rememberLoginMsgNoUser): %S is brandShortName

Comment 31

[Riadh Chtara[:rchtara]]

Comment on attachment 8631815 [details]
MozReview Request: Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

Comment 32

[Riadh Chtara[:rchtara]]

Comment on attachment 8631815 [details]
MozReview Request: Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8631815 [details]
MozReview Request: Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

https://reviewboard.mozilla.org/r/12937/#review11601

You can mark checkin-needed with a try push with these two small nits fixed. Thanks.

::: toolkit/components/passwordmgr/test/test_notifications.html:100
(Diff revisions 2 - 3)
> +    const BRAND_BUNDLE = "chrome://branding/locale/brand.properties";

Nit: I think you moved the const the wrong way. I was saying to move it to the top of the file with `const { Services }…`

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
(Diff revisions 2 - 3)
> -const BRAND_BUNDLE = "chrome://branding/locale/brand.properties";

Nit: This was fine as-is at the top of the file with the other const.

Comment 34

[Riadh Chtara[:rchtara]]

Comment on attachment 8631815 [details]
MozReview Request: Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8631815 [details]
MozReview Request: Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"

https://reviewboard.mozilla.org/r/12937/#review11605

Ship It!

Comment 36

[Riadh Chtara[:rchtara]]

try job
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77c613f97b3c

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bffb6366654a

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bffb6366654a

Comment 39

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build(s):
- https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-07-24-03-02-10-mozilla-central/

Test Cases Used:

* ensured that the "Remember" button is being displayed when logging in using new credentials that haven't been saved
* ensured that the "Update" button is being displayed when logging into a previously saved username with a new password
* ensured that when using an already saved username, the "Remember" button is changed to the "Update" button
** also tested vise versa, made sure entering a brand new username will change the button from "Update" to "Remember"

Ensured the correct domain was appearing in the doorhanger via the following websites:

- facebook.com
- twitter.com
- linked.com
- amazon.com
- tumblr.com
- pinboard.in
- accounts.mozilla.com
- starbucks.com

Also created a new One & Done task (Making sure the correct domain is displayed in the Password Manager doorhanger)
* https://oneanddone.mozilla.org/en-US/tasks/137/

Quick Questions:
================

* the doorhanger currently looks a bit different than the proposed design that's been attached in comment # 3 (expected?)
** the domain name is being displayed above the proposed string in bold text rather than being included in the string

* the doorhanger doesn't mention the current channel when updating passwords, example:
** Would you like Nightly to remember this login?
** Would you like to update this login? (should the channel be included the same way it's used via "remember" to keep things consistent?)

Ryan, see above questions.

Comment 40

[Ryan Feeley [:rfeeley]]

The capture doorhanger evolved slightly due to some negotiations, and while it still a long way from the original design, it's currently in a good state. The domain appears where it should, although if possible I would prefer it to be grey and normal weight as specified here: http://people.mozilla.org/~rfeeley/capture-increments/

It's also not supposed to mention the current channel when updating (the assummption is that when updating, this capture dialog will be familiar and the current channel is not required).

Comment 41

[Kamil Jozwiak [:kjozwiak]]

Thanks Ryan! Going to make this as verified as per testing done in comment #39.

Comment 42

[Bill Maggs (bmaggs)]

(In reply to Ryan Feeley from comment #40)
> The capture doorhanger evolved slightly due to some negotiations, and while
> it still a long way from the original design, it's currently in a good
> state. The domain appears where it should, although if possible I would
> prefer it to be grey and normal weight as specified here:
> http://people.mozilla.org/~rfeeley/capture-increments/
> 
> It's also not supposed to mention the current channel when updating (the
> assummption is that when updating, this capture dialog will be familiar and
> the current channel is not required).

Then let's fix it. I have no preference at all for  weighting, but I see no reason for the channel-specific reference.