Save password dialog string changes

RESOLVED FIXED

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rfeeley, Assigned: aaronraimist)

Tracking

unspecified
All
iOS

Firefox Tracking Flags

(fxios-v1.1 wontfix, fxios-v4.0 affected, fxios+)

Details

(Whiteboard: [needstrings][5.0])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Currently in iOS Firefox:
https://cloud.githubusercontent.com/assets/68213/9887118/ff018404-5bbb-11e5-80c9-1d9fa4894446.png

CURRENT:
* "Do you want to save the password for user@email.com on domain.org?"
* Yes
* Not now

PROPOSED
* "Save this login for domain.org?"
* Save Login
* Don’t Save

We are moving to a new design in desktop, although not as quickly as you are able to in mobile. It is not recommended to copy it completely (as the UI is more complex), but the copy should be enough
https://cloud.githubusercontent.com/assets/68213/9887073/c4594aa8-5bbb-11e5-9deb-4c0f63bd188f.png

Details:
https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96290118

Fennec currrently does, which we will change to "save":
* "Would you like <Fennec> to remember this login?"
* user@domain.org (clickable to edit dialog)
* Remember
* Never
status-fxios-v1.1: --- → ?
tracking-fxios: --- → ?
Ryan, are we concerned about losing the context of what email/username is being saved? Perhaps "Save user@domain for domain.org?"
Flags: needinfo?(rfeeley)
(Reporter)

Comment 2

3 years ago
Yes, I erred on having too little info perhaps.

Looking at a real world example:

* Save ryanfeeley@gmail.com for tumblr.com?

This still seems too lean.

Most verbose (and so the user knows it's Firefox talking and not the website)

* Would you like Firefox to save the login of ryanfeeley@gmail.com for tumblr.com?

Quite an eyeful.

Your turn!
Flags: needinfo?(rfeeley) → needinfo?(dhenein)
(Reporter)

Comment 3

3 years ago
Or…
* Save ryanfeeley@gmail.com login for tumblr.com?
* Save Login
* Don’t Save
What about (subtle change)…
* Save login ryanfeeley@gmail.com for tumblr.com?
* Save Login
* Don’t Save
Flags: needinfo?(dhenein) → needinfo?(rfeeley)
Whiteboard: [needs strings]
(Reporter)

Comment 5

3 years ago
Yes.
Flags: needinfo?(rfeeley)
status-fxios-v1.1: ? → wontfix
tracking-fxios: ? → 2.0+
(Assignee)

Comment 6

3 years ago
Created attachment 8672346 [details] [diff] [review]
Patch for Bug 1205047

Here is a patch for this bug. Let me know if this looks correct.

The strings here: 
https://github.com/mozilla/firefox-ios/blob/1844b83c55a2e182f0fabde5e8a32963677d61cd/Client/Frontend/Browser/LoginsHelper.swift#L158
should also be updated. Maybe:
* Update login ryanfeeley@gmail.com for tumblr.com?
and
* Update login for tumblr.com?
with buttons
* Don't Save
* Update
Attachment #8672346 - Flags: feedback?(rfeeley)
Attachment #8672346 - Flags: feedback?(dhenein)
(Assignee)

Comment 7

3 years ago
Created attachment 8672347 [details]
With Patch: Simulator Screen Shot Oct 10, 2015, 8.39.55 PM.png
(Reporter)

Comment 8

3 years ago
Sure. I'd like to see a curly quote on "Don’t" if possible (am thinking Bugzilla is gonna eat that character).

What does it say when there is no username and it's just a password?
(Assignee)

Comment 9

3 years ago
(In reply to Ryan Feeley [:rfeeley] from comment #8)
> Sure. I'd like to see a curly quote on "Don’t" if possible (am thinking
> Bugzilla is gonna eat that character).
> 
> What does it say when there is no username and it's just a password?

I'll update that button to say "Don’t Save".


With a username the text could be:
* Update login ryanfeeley@gmail.com for tumblr.com?

Without a username the text could be:
* Update login for tumblr.com?

The word "login" could also be replaced with "password".
Flags: needinfo?(rfeeley)
(Reporter)

Comment 10

3 years ago
Yeah, ideally we use "password" whenever username is not present (or removed).
Flags: needinfo?(rfeeley)
(Reporter)

Comment 11

3 years ago
In conclusion:
* Save login ryanfeeley@gmail.com for tumblr.com?
* Save password for tumblr.com?
* Update login ryanfeeley@gmail.com for tumblr.com?
* Update password for tumblr.com?
Flags: needinfo?(dhenein)
(Reporter)

Comment 12

3 years ago
Comment on attachment 8672346 [details] [diff] [review]
Patch for Bug 1205047

There shouldn't be a "Not now", just a "Don’t save". No?
Attachment #8672346 - Flags: feedback?(rfeeley)
Whiteboard: [needs strings] → [needstrings]
Rank: 1
tracking-fxios: 2.0+ → +
Hardware: Other → All
Hey Aaron,

Looks like there was a patch for this but it never got reviewed. Are you still interested in contributing the patch back into the mainline repo? If so, could you open a pull request to https://github.com/mozilla/firefox-ios?
Flags: needinfo?(dhenein) → needinfo?(aaronraimist)

Comment 14

3 years ago
Created attachment 8737526 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1684

Sorry about the delay. How does this patch look?
Attachment #8737526 - Flags: review?(sleroux)
(Assignee)

Comment 15

3 years ago
(In reply to aaronraimist from comment #14)
> Created attachment 8737526 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1684
> 
> Sorry about the delay. How does this patch look?

Also I accidentally created a new Bugzilla account. This patch is from me.
Assignee: nobody → aaronraimist
Status: NEW → ASSIGNED
Flags: needinfo?(aaronraimist)
(Assignee)

Updated

3 years ago
Attachment #8672346 - Attachment is obsolete: true
Attachment #8672346 - Flags: feedback?(dhenein)
Brian, would it make sense to change to this use the new value-based localization stuff?
Flags: needinfo?(bnicholson)
Yeah, we might as well add IDs if we're changing the strings anyway.

Aaron, do you mind adding the "value" parameter to each of the NSLocalizedStrings you changed here? See https://mail.mozilla.org/pipermail/mobile-firefox-dev/2016-March/001899.html for details/background (I'll eventually add this to the README once we have all the details sorted out).
Flags: needinfo?(bnicholson)
(Assignee)

Comment 18

3 years ago
OK, I have updated the pull request to add keys to all of the strings in LoginsHelper.swift. Let me know if there is anything else that should be changed.
Flags: needinfo?(sleroux)
Thanks for updating the PR Aaron! Looks good!
Flags: needinfo?(sleroux)
Attachment #8737526 - Flags: review?(sleroux) → review+
master 7320ae27304577f27451c182266cb4ec52324103
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-fxios-v4.0: --- → affected
Resolution: --- → FIXED
Whiteboard: [needstrings] → [needstrings][5.0]
This bug removes several strings (7 if I counted them correctly) on master, which will become v5.0

If these strings are used in v4.0 as I think, we can't remove them until v5.x has branched.

Basically we need to keep strings around for one more cycle when we remove a feature, and periodically clean up these extra strings.
https://github.com/mozilla/firefox-ios/commit/fcdc159687b8eaec0b46be60469231bbebda3315
Flags: needinfo?(aaronraimist)
:flod, is this because we export our strings from master branch and not v4.x? If so, is there a reason why we need to export from master (which should be containing 5.0 work anyways) instead of v4.x?
It doesn't matter from where we extract strings, the problem is that we only have one repository for strings, and that's "by design" (we're even trying to move Firefox* to that kind of structure). 

We don't want to start messing with branches (no external tools support), or tags/releases (basically no chance to improve your localization within a cycle).

Hopefully a practical example can be clearer.

Current situation: master is 5.0, we have a branch with 4.0, l10n-repo is 4.0. Current release in App Store is 3.0.

Fast forward 6 weeks: master is 6.0, we have a branch with 5.0, l10n-repo is still 4.0 because we haven't extracted strings yet. Current release in App Store is 4.0, localizers start working on 5.0.

Fast forward 2 more weeks: we extract strings for 5.0. Master is still 6.0, l10n-repo now has 5.0 strings, current release in App Store is still 4.0. How do you release a 4.1 (if needed), given that the l10n-repo is missing a bunch of strings you removed in the 5.x development cycle?

The only alternative to keeping strings around is to "freeze" l10n for 6 weeks: once you release v4.0, you store somewhere else the strings for that release and use them instead of the standard l10n-repo. The downside is that we lose any chance to improve localization in a minor release, and it complicates the build system.
https://github.com/splewako/mozilla-l10n-en/commit/63d174aaa55136a486084a9bb31dbf5bbe166de2
> + <trans-unit id="LoginsHelper.PromptUpdateLogin.Title">
> +  <source>Update login for %1$@ on %2$@?</source>
> +  <target>Update login for %1$@ on %2$@?</target>
> +  <note>Prompt for updating a login. The first parameter is the username being saved. The second parameter is the hostname of the site.</note>
> + </trans-unit>

Isn't that misleading? From reading the string I would expect old username to be presented and updated with just entered data (not listed) or password to be updated for presented login.
Yes it does seem to be a bit misleading. Only the password for the given username will be updated, not the username which the comment implies. I think something along the lines of:

'Prompt for updating a login. The first parameter is the username for which the password will be updated for. The second parameter is the hostname of the site.
I would also point out a bit of inconsistency in the new strings (didn't look at them yet)

Save login %1$@ for %2$@?
Update login for %1$@ on %2$@?
Created attachment 8740541 [details] [review]
Github PR https://github.com/mozilla/firefox-ios/pull/1710

Here's the patch to re-add the old strings to OldStrings.swift so they are included as part of the l10n export.
Attachment #8740541 - Flags: review?(sarentz)
(Assignee)

Comment 28

3 years ago
Created attachment 8740732 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1713

:flod, Fixed that issue with the for vs on.

:stef, Does this look better? I used :sleroux's wording.
Flags: needinfo?(aaronraimist)
Attachment #8740732 - Flags: review?(sleroux)
Attachment #8740732 - Flags: feedback?(splewako)
Attachment #8740732 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8740732 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1713

The change in this case is minimal. Since we've started using proper string IDs, for bigger changes we'll need to start following these guidelines
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings
Attachment #8740732 - Flags: feedback?(francesco.lodolo) → feedback+
Comment on attachment 8740732 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1713

(In reply to Aaron Raimist [:aaronraimist] from comment #28)
> Created attachment 8740732 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1713
> 
> :flod, Fixed that issue with the for vs on.
> 
> :stef, Does this look better? I used :sleroux's wording.

Yes and the "for vs on" change makes it even better. Thank you.
Attachment #8740732 - Flags: feedback?(splewako) → feedback+
Attachment #8740541 - Flags: review?(sarentz)
Attachment #8740732 - Flags: review?(sleroux) → review+
You need to log in before you can comment on or make changes to this bug.