1205047 - Save password dialog string changes

Status: RESOLVED FIXED

(Firefox for iOS :: Browser, defect)

Description

[Ryan Feeley [:rfeeley]]

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 [e-mail] 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?"
* [e-mail] (clickable to edit dialog)
* Remember
* Never

Comment 1

[Darrin Henein [:darrin]]

Ryan, are we concerned about losing the context of what email/username is being saved? Perhaps "Save user@domain for domain.org?"

Comment 2

[Ryan Feeley [:rfeeley]]

Yes, I erred on having too little info perhaps.

Looking at a real world example:

* Save [e-mail] 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 [e-mail] for tumblr.com?

Quite an eyeful.

Your turn!

Comment 3

[Ryan Feeley [:rfeeley]]

Or…
* Save [e-mail] login for tumblr.com?
* Save Login
* Don’t Save

Comment 4

[Darrin Henein [:darrin]]

What about (subtle change)…
* Save login [e-mail] for tumblr.com?
* Save Login
* Don’t Save

Comment 5

[Ryan Feeley [:rfeeley]]

Yes.

Comment 6

[Aaron Raimist [:aaronraimist]]

Attachment: 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 [e-mail] for tumblr.com?
and
* Update login for tumblr.com?
with buttons
* Don't Save
* Update

Comment 7

[Aaron Raimist [:aaronraimist]]

Attachment: With Patch: Simulator Screen Shot Oct 10, 2015, 8.39.55 PM.png

Comment 8

[Ryan Feeley [:rfeeley]]

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?

Comment 9

[Aaron Raimist [:aaronraimist]]

(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 [e-mail] for tumblr.com?

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

The word "login" could also be replaced with "password".

Comment 10

[Ryan Feeley [:rfeeley]]

Yeah, ideally we use "password" whenever username is not present (or removed).

Comment 11

[Ryan Feeley [:rfeeley]]

In conclusion:
* Save login [e-mail] for tumblr.com?
* Save password for tumblr.com?
* Update login [e-mail] for tumblr.com?
* Update password for tumblr.com?

Comment 12

[Ryan Feeley [:rfeeley]]

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

There shouldn't be a "Not now", just a "Don’t save". No?

Comment 13

[Stephan Leroux [:sleroux]]

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?

Comment 14

[u567866]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1684

Sorry about the delay. How does this patch look?

Comment 15

[Aaron Raimist [:aaronraimist]]

(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.

Comment 16

[Stephan Leroux [:sleroux]]

Brian, would it make sense to change to this use the new value-based localization stuff?

Comment 17

[Brian Nicholson (: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).

Comment 18

[Aaron Raimist [:aaronraimist]]

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.

Comment 19

[Stephan Leroux [:sleroux]]

Thanks for updating the PR Aaron! Looks good!

Comment 20

[Stephan Leroux [:sleroux]]

master 7320ae27304577f27451c182266cb4ec52324103

Comment 21

[Francesco Lodolo [:flod]]

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

Comment 22

[Stephan Leroux [:sleroux]]

: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?

Comment 23

[Francesco Lodolo [:flod]]

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.

Comment 24

[Stefan Plewako [:stef]]

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.

Comment 25

[Stephan Leroux [:sleroux]]

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.

Comment 26

[Francesco Lodolo [:flod]]

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$@?

Comment 27

[Stephan Leroux [:sleroux]]

Attachment: 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.

Comment 28

[Aaron Raimist [:aaronraimist]]

Attachment: 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.

Comment 29

[Francesco Lodolo [:flod]]

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

Comment 30

[Stefan Plewako [:stef]]

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.