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

VERIFIED FIXED in Firefox 42

Status

()

defect
P1
normal
Rank:
15
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Paolo, Assigned: rchtara)

Tracking

Trunk
mozilla42
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox42 verified)

Details

(Whiteboard: pwmgr42)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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".
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.)
(Reporter)

Comment 2

4 years ago
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.
Posted image remember-update.001.png (obsolete) —
Four states of the edited login form. We won't change the headline dynamically to avoid awkwardness of laying out the door hanger again.
Posted image remember-update.002.png (obsolete) —
Attachment #8582568 - Attachment is obsolete: true
Attachment #8583824 - Attachment is obsolete: true
(Reporter)

Comment 6

4 years ago
Posted patch The patch (obsolete) — Splinter Review
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8584164 - Flags: review?(MattN+bmo)
(Reporter)

Updated

4 years ago
Iteration: --- → 39.3 - 30 Mar
Points: --- → 1
Flags: qe-verify+
Flags: firefox-backlog+
(Reporter)

Comment 7

4 years ago
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 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.
Attachment #8584164 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 9

4 years ago
(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.
Flags: needinfo?(rfeeley)
(Reporter)

Comment 12

4 years ago
Updated the test checking the string and started a tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba05fb1f7053
Flags: needinfo?(paolo.mozmail)
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.
Flags: needinfo?(rfeeley)
(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.
(Reporter)

Comment 15

4 years ago
(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.
Flags: needinfo?(rfeeley)
(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.
(Reporter)

Comment 17

4 years ago
(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.
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.
Flags: needinfo?(rfeeley)
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
(Reporter)

Comment 19

4 years ago
For the record, we're not making this string change for the 39 release.
Assignee: paolo.mozmail → nobody
Iteration: 40.1 - 13 Apr → ---
Status: ASSIGNED → NEW
(Reporter)

Comment 20

4 years ago
Attachment #8584164 - Attachment is obsolete: true
Attachment #8586680 - Flags: review+
Blocks: 1175279
Rank: 15
Priority: -- → P1
"Would you like Firefox to remember this login?"

and if no username:

"Would you like Firefox to remember this password?"
(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?
Nice-to-have if low effort. I can live without it, but it's a nice touch.
(Assignee)

Updated

4 years ago
Assignee: nobody → rchtara
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?
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.
(Assignee)

Comment 26

4 years ago
Bug 1144856 - Change the doorhanger messages to mention "login" instead of "password"
Attachment #8631815 - Flags: review?(MattN+bmo)
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.
Attachment #8631815 - Flags: review?(MattN+bmo)
(Assignee)

Comment 28

4 years ago
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"
Attachment #8631815 - Flags: review?(MattN+bmo)
(Assignee)

Comment 29

4 years ago
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
Attachment #8631815 - Flags: review?(MattN+bmo) → review+
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
(Assignee)

Comment 31

4 years ago
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"
Attachment #8631815 - Flags: review+ → review?(MattN+bmo)
(Assignee)

Comment 32

4 years ago
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 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.
Attachment #8631815 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 34

4 years ago
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"
Attachment #8631815 - Flags: review+ → review?(MattN+bmo)
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!
Attachment #8631815 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/bffb6366654a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Updated

4 years ago
Iteration: --- → 42.2 - Jul 27
QA Contact: 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.
Flags: needinfo?(rfeeley)
Whiteboard: pwmgr42
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).
Flags: needinfo?(rfeeley)
Thanks Ryan! Going to make this as verified as per testing done in comment #39.
Status: RESOLVED → VERIFIED
(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.
You need to log in before you can comment on or make changes to this bug.