1208145 - Revert bug 1121291 to bring back the button to toggle all passwords

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Ryan Feeley [:rfeeley]]

Attachment: show-hide.gif

So that I can safer in some situations, as a Firefox user, I want to show only the passwords I want to show and have selected.

See attached animation.

Acceptance criteria:
* When the user selects one or more rows, the Show button is enabled
* When the user clicks to Show password, password is shown, the button converts to become a Hide button, and the row remains selected
* When the user has the password column hidden, and presses Show, the password column appears with the selected password shown
* With passwords shown, when the user makes a different selection, or leaves the password section, any shown passwords become hidden again and the Hide button becomes a Show button again
* When no rows are selected, the Show button is disabled
* When no passwords exist, the Show button is disabled

Comment 1

[Ryan Feeley [:rfeeley]]

"So that I can safer" >> "So that I can feel safer"

(I'm so spoiled by the non-Bugzilla editable internet)

Comment 2

[Matthew N. [:MattN]]

The import button is already on the right for Windows. Does that change the position of Show?

Comment 3

[Matthew N. [:MattN]]

Also, I guess we should add Show to the context menu too.

Comment 4

[Matthew N. [:MattN]]

[Tracking Requested - why for this release]: Follow-up from bug 1121291. Users aren't able to discover showing passwords as easily.

Comment 5

[Ryan Feeley [:rfeeley]]

Good idea Matt. I think all CRUD-related options should remain on left (Remove, Import… and hopefully soon Add) and that the others (Show, Copy and eventually Fill) should appear on the right. I'll mock it up and attach.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 44 as it's a follow up to changes already made in passwords manager.

Comment 7

[Ryan Feeley [:rfeeley]]

Attachment: saved-logins-window.png

What do you think about this layout? If we can't get a doorhanger, we can certainly get an explicit "Copy Password" button in the window. "Fill Login" could perhaps come in the future?

Comment 8

[Matthew N. [:MattN]]

Dolske, I'm not sure how to deal with bug 1121291 and its follow-ups in Fx44 as implementing the ideal solution here and backing out the string removals of bug 1121291 both would break string freeze. (A stright backout would also break profiles who've used this new code due to XUL persistence but that is easily fixed by changing the column ID). I've been thinking about this for a while and I have some workarounds but there are no ideal solutions. I also don't have time to implement them this week (while beta is still taking fixes to functionality) since I'm on vacation. Can you help figure out a path forward for Fx44 while I'm away?

Options for Fx44:
a) Backout 1121291 while also changing the column ID to prevent the pw column from showing by default in profiles that used the new code. This breaks string freezes by bringing 4 string (2 labels + accesskeys) back from the dead.
b) Implement this bug with new strings. This obviously breaks string freeze.
c) Implement this bug by reusing existing Android strings (e.g. via build system magic)
d) Implement this bug by reusing other strings e.g. showPasswordPlaceholder +  btnHide (plus some case transformation)
e) Ship this regression (discoverability of viewing passwords and not allowing showing multiple at once) in 44 and 45 and fix it for 46 with proper strings.
f) Use an "eye" icon open/close for string frozen branches to avoid new strings. Don't provide accessible text descriptions or else re-use showPasswordPlaceholder + btnHide for them.

Thanks.

Comment 9

[[:philipp]]

for an issue in a beta version, the current behavior is causing an unusual amount of user questions/confusion on sumo: https://support.mozilla.org/questions/firefox?tagged=bug1208145&show=all

Comment 10

[Justin Dolske [:Dolske]]

Well, shit. There are no good options here. My preferences, in order:

1) Backout bug 1121291 and use the Android strings, if feasible
2) Backout bug 1121291 and restore old strings, breaking the string freeze.
3) Live with it for a release.

The 5 strings that were removed from toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:

hidePasswords=Hide Passwords
hidePasswordsAccessKey=P
showPasswords=Show Passwords
showPasswordsAccessKey=P
noMasterPasswordPrompt=Are you sure you wish to show your passwords?


I don't think we should attempt to fix this by implementing the new show/hide button. I'd rather we get back to a known-good point, and not risk needing yet more followups (with no time to do them). So that means a backout of bug 1121291.

Your suggestion (c) is interesting (using the Android strings from mobile/android/locales/en-US/chrome/aboutLogins.properties)... But I'm not sure how that's going to work on the L10N side. It's easy enough to hack up for a normal a en-US build (reach into /mobile, package that .properties file, and use it in the code), but the L10N repacks are what really need to work. That's a black-box I don't understand. Axel, is this reasonably feasible? Are Android L10N files available in a desktop repack environment?

However... That would still mean not having an accesskey or the noMasterPasswordPrompt string, so that code would have to be disabled. Not a dealbreaker (given the alternatives), but is yet another complication.

Oh, we _could_ hardcode the english strings, but that seems worse than the "live with it" option for users who don't know english.


I really don't want to go with the "live with it" option for Release -- but the others have pretty significant costs, the clock has run out, and for better or worse we've been living with it as-is on Nightly/Dev/Beta.

If we do go with "live with it" for Firefox 44, I think we should still do a backout on 45/46.

Comment 11

[Axel Hecht [:Pike]]

I'm tentatively assuming that a change here on beta would fail the hardened tree rules that Ritu announced in https://groups.google.com/forum/#!topic/mozilla.dev.planning/jDiniugtwgU for 44.

I don't think the "use Android strings" idea flies, for the same reason that dolske mentioned.

I'm far from convinced that this should trigger a string change on aurora. The underlying assumption would be that at this point, 60% of our users would/could see an English string they don't understand.

We're still in the world where the majority of our localizations don't land fixes at this point in time (or should). The single-repo story I'm proposing wouldn't have let this problem arise, funnily enough. As the strings would still be in l10n repos at least as good as we ship them on release right now.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Dolske, Axel: While we do have a higher bar for uplifting fixes to Beta44, the criteria clearly allows us to accept critical regressions. Based on Philipp's comment 9 and so many users providing feedback that they do want to be able to see all previous passwords, I would consider this a key blocking scenario. 

How easy would it be to back out bug 1121291? I can work with SV to get them to verify on a try build to minimize risk. Will that help? I know we are in a string freeze mode but we must make exceptions where needed and this sounds like that kind of an exceptional issue. :(

Comment 13

[Axel Hecht [:Pike]]

Practically speaking, we can't back out a bug on the l10n side of things. There's just no commit that's relating to that change.

Note, comment 9 talks about "unusual", I don't know in which metric, and how this problem maps between beta and release audience. I'd like to see a metric in which we should succeed before taking a change.

Comment 14

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #10)
> Oh, we _could_ hardcode the english strings, but that seems worse than the
> "live with it" option for users who don't know english.

We could hardcode the English strings and only use them in English builds…

Comment 15

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1208145 - Back out changeset e4e3a8b66db4 (bug 1121291). r=dolske

Review commit: https://reviewboard.mozilla.org/r/30467/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30467/

Comment 16

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske

Bug 1121291 made @hidden persist but we are reverting that change so we need to remove the values so the password column doesn't always show by default.

Review commit: https://reviewboard.mozilla.org/r/30469/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30469/

Comment 17

[Matthew N. [:MattN]]

Morphing this bug to cover reverting bug 1121291 since the discussion about it is here.

Comment 18

[Francesco Lodolo [:flod]]

Do we have any telemetry data about how many people are using this dialog, and how many were using those buttons? We have seen complaints in the past when the UI changed, what makes this case so different?

I'm also missing another point: is the UI in 45+ any better than what we have right now in 44 (I believe it's the same)? What do we gain in 'fixing' 44?

Unless I'm wrong, tomorrow is the last day to sign-off on Beta for localizations, which means almost nobody will be able to fix this. Which also means English strings will surface in the UI, and make our localizers look sloppy. It sounds like a no-win to me.

Comment 19

[[:philipp]]

(In reply to Francesco Lodolo [:flod] from comment #18)
> Do we have any telemetry data about how many people are using this dialog,
> and how many were using those buttons? We have seen complaints in the past
> when the UI changed, what makes this case so different?

there is some in https://bugzilla.mozilla.org/show_bug.cgi?id=1121291#c9:
> There are 155K samples on FF40 release, 87% for "show", 13% for "hide". As a
> rough comparison, PWMGR_NUM_SAVED_PASSWORDS has 19.4M samples (16% have no
> passwords, or 62% having 0-5 passwords). 155K is fairly small relative to
> 19M.
> 
> But... PWMGR_MANAGE_OPENED, which measures how many times this UI is shown,
> has 222K samples. 155K is very large relative to that. In fact, it implies
> ~60% of the time people open this dialog they clock Show Passwords.

Comment 20

[Justin Dolske [:Dolske]]

(In reply to Francesco Lodolo [:flod] from comment #18)
> Do we have any telemetry data about how many people are using this dialog,
> and how many were using those buttons? We have seen complaints in the past
> when the UI changed, what makes this case so different?

Comment 19 copied over the relevant data (thanks). The thing that's different here is that the current UX (from 1121291) isn't something we consider acceptable. The intent (See comments 9&19 there) was to have done a followup fix (this bug!), but that never happened. So this isn't a case of "we like the new UI, but some grumpy users do not".

> I'm also missing another point: is the UI in 45+ any better than what we
> have right now in 44 (I believe it's the same)? What do we gain in 'fixing'
> 44?

The current UI exists on 44+. Matt and I discussed this earlier, and I'd like to do this back out of 1121291 everywhere (including 46, since that's about to become Aurora). If we take another shot at it, it will be in 47 or beyond so there's time to fix it properly in Nightly.

> Unless I'm wrong, tomorrow is the last day to sign-off on Beta for
> localizations, which means almost nobody will be able to fix this. Which
> also means English strings will surface in the UI, and make our localizers
> look sloppy. It sounds like a no-win to me.

I agree the current situation sucks. If we do nothing, we have bad UX for everyone. If we do the backout, some users get a good (previous) UX, while it's bad for others. I'm trying to find the least-worst option.

Is there a way we can stretch the L10N signoff, perhaps for just a few top-N locales? Or have someone restore the strings for top-N locales manually? (We don't actually need localizers to do any translations, since we have the existing strings in http://mxr.mozilla.org/l10n-mozilla-release/find?text=&string=passwordmgr.properties).

Hmm... I suppose that means we could hard-code the translations. We did that with Pocket (and undid it in bug 1161138). We could do the same thing here? It's more code work/risk, but not hugely so. Maybe a promising option?

Comment 21

[Justin Dolske [:Dolske]]

Comment on attachment 8706722 [details]
MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske

https://reviewboard.mozilla.org/r/30469/#review27221

rs=me on the backout, I assume this was a straight-up backout that doesn't need manual review?

(and r=me on the persist fix)

Obviously will need approval-aurora/beta before landing there, which involves a decision on the L10N impact too.

Comment 22

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #21)
> rs=me on the backout, I assume this was a straight-up backout that doesn't
> need manual review?

Correct.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/56d1bb97d672
https://hg.mozilla.org/integration/fx-team/rev/b0083022f6c3

Comment 24

[Axel Hecht [:Pike]]

Attachment: passwordmgr.properties for all shipping locales, in the version they're shipping

Sadly, restoring the previous content is tricky with sign-offs etc.

I've went ahead and created a tarball of all the passwordmanager.properties files that we ship, in the revision that we ship them in in 43, merged against en-US. I.e., there shouldn't be missing strings in them.

If we can use that to a stunt like we did for pocket, that'd be great.

I have a really hard time estimating the risk of landing this in 43, 'cause that's a mix of "we need to fix 'cause they updated" and "we shouldn't touch it cause they didn't update"

Comment 25

[Matthew N. [:MattN]]

Thanks! I guess I mid-aired with you but it seems you prefer hard-coding in m-a and m-b over bringing the strings back so I'll implement that with your helpful tarball.

Comment 26

[Matthew N. [:MattN]]

Attachment: 5 strings for each locale in JS objects

I generated this file using the following commands in a directory with the extracted tar file from attachment 8707116 [details]:

> echo "const STRINGS = {};" > ../1208145.js
> grep -R -E "^(noMasterPasswordPrompt|(hide|show)Passwords(AccessKey)?)" | sed 's/\/passwordmgr.properties:/\"]["/' | sed 's/^/STRINGS["/' | sed 's/=/"] = "/' | sed 's/$/";/' | sed 's/\(STRINGS\["[^"]\+"]\)/\1 = {};\n\1/' | sort -u >> ../1208145.js

Let me know if you see any encoding or escaping issues. There are some JS unicode escape sequences but those should work fine in the double quotes.

Comment 27

[Axel Hecht [:Pike]]

Comment on attachment 8707165 [details]
5 strings for each locale in JS objects

Looks good to me, thanks

Comment 28

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu

Review commit: https://reviewboard.mozilla.org/r/30595/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30595/

Comment 29

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu

Bug 1121291 made @hidden persist but we are reverting that change so we need to remove the values so the password column doesn't always show by default.

Review commit: https://reviewboard.mozilla.org/r/30597/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30597/

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8707177 [details]
MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu

This isn't implemented yet but I published this first so you will be able to review the interdiff between the m-c version and the m-a/m-b version.

Comment 31

[Matthew N. [:MattN]]

Comment on attachment 8707177 [details]
MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30595/diff/1-2/

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8707178 [details]
MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30597/diff/1-2/

Comment 33

[Matthew N. [:MattN]]

Attachment: 5 strings for each locale in JS objects v.2

Revised file and commands since a few locales e.g. ja-JP had extra whitespace before the `=`:

> echo "const STRINGS = {};" > ../1208145.js
> grep -R -E "^(noMasterPasswordPrompt|(hide|show)Passwords(AccessKey)?)" | sed 's/\/passwordmgr.properties:/\"]["/' | sed 's/^/STRINGS["/' | sed 's/\s*=/"] = "/' | sed 's/$/";/' | sed 's/\(STRINGS\["[^"]\+"]\)/\1 = {};\n\1/' | sort -u >> ../1208145.js

Comment 34

[Matthew N. [:MattN]]

Comment on attachment 8707177 [details]
MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu

I still need to test this as my attempts to do so by modifying default prefs and localized prefs for general.useragent.locale didn't work.

Comment 35

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (vacation Jan. 1 – 10) from comment #34)
> I still need to test this as my attempts to do so by modifying default prefs
> and localized prefs for general.useragent.locale didn't work.

I just did a ja beta build with this patch and it worked fine but the fallback didn't work if the locale existed in the object but the string didn't so I've fixed that now. The ja beta repo had already deleted the string and the hard-coded one was used.

Comment 36

[Matthew N. [:MattN]]

Comment on attachment 8707177 [details]
MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30595/diff/2-3/

Comment 37

[Matthew N. [:MattN]]

Comment on attachment 8707178 [details]
MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30597/diff/2-3/

Comment 38

[Matthew N. [:MattN]]

Comment on attachment 8707177 [details]
MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu

Approval Request Comment
[Feature/regressing bug #]: A follow-up to bug 1121291 required a new string so couldn't be implemented on m-a/m-b so instead we're reverting bug 1121291 and re-using the old strings.
[User impact if declined]: Users can't figure out how to see their password in plaintext anymore due to a UI change that needed follow-up work.
[Describe test coverage new/current, TreeHerder]: Existing m-bc tests cover the code. This is a revert to old code plus a change to re-use the old strings without breaking string freeze on the .properties file.
[Risks and why]: Low risk reverting to old behaviour. Small chance that we fallback to an en-US string.
[String/UUID change made/needed]: No changes to .properties files since we are packaging the old strings into the repo instead to avoid breaking string freeze.

Comment 39

[Matthew N. [:MattN]]

Comment on attachment 8707178 [details]
MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu

Approval Request Comment: See comment 38

This is just like the m-c version but with the UI_VERSION changed so the password column isn't always visible by default with plaintext.

I just realized that I will need to modify migrateUI on later versions to ensure we don't miss a migration due to the change in numbering.

Comment 40

[Justin Dolske [:Dolske]]

Comment on attachment 8707177 [details]
MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu

https://reviewboard.mozilla.org/r/30595/#review27417

As we chatted about, I probably would have split this into 2 changesets (pure backout in one, remove .properties and add localized strings in the other), but doesn't really matter here since the string stuff is fairly simple and self-contained.

::: toolkit/components/passwordmgr/content/passwordManager.js:291
(Diff revision 3)
> +STRINGS["it"]["hidePasswords"] = " Nascondi password";

Shouldn't matter, but is the leading space (within the quotes) in the original string file? Or was this possible glitch when converting into this patch?

::: toolkit/components/passwordmgr/content/passwordManager.js:1039
(Diff revision 3)
> +               getSelectedLocale("passwordmgr");

"global" seems to be preferred usage in Toolkit, seems like it shouldn't matter but worth being consistent?

Comment 41

[Justin Dolske [:Dolske]]

Comment on attachment 8707178 [details]
MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu

https://reviewboard.mozilla.org/r/30597/#review27429

Comment 42

[Justin Dolske [:Dolske]]

Comment on attachment 8706721 [details]
MozReview Request: Bug 1208145 - Back out changeset e4e3a8b66db4 (bug 1121291). r=dolske

https://reviewboard.mozilla.org/r/30467/#review27431

Comment 43

[Justin Dolske [:Dolske]]

(In reply to Matthew N. [:MattN] (vacation Jan. 1 – 10) from comment #39)

> I just realized that I will need to modify migrateUI on later versions to
> ensure we don't miss a migration due to the change in numbering.

Good catch. We need to remember to update all channels consistently when we change this code.

Comment 44

[Francesco Lodolo [:flod]]

(In reply to Justin Dolske [:Dolske] from comment #40)
> ::: toolkit/components/passwordmgr/content/passwordManager.js:291
> (Diff revision 3)
> > +STRINGS["it"]["hidePasswords"] = " Nascondi password";
> 
> Shouldn't matter, but is the leading space (within the quotes) in the
> original string file? Or was this possible glitch when converting into this
> patch?

As long as it doesn't have any effect I'm fine, but that's an error. 

I use a space before and after the separator, you probably assumed there's no space before and after '=' when converting .properties.
I'd suggest to trim those strings before landing, it involves only Italian and Russian.

Comment 45

[Carsten Book [:Tomcat]]

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

Comment 46

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Matt, did you do a local build with your fix and manually verify that the "Show passwords" comes back? And all functionality related to clicking that button also works as expected? If I take this fix in Beta9, it will be the last Beta before we enter RC and I really want to avoid any regressions. Thanks!

Comment 47

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Forgot to NI Matt.

Comment 48

[Matthew N. [:MattN]]

(In reply to Ritu Kothari (:ritu) from comment #46)
> Matt, did you do a local build with your fix and manually verify that the
> "Show passwords" comes back? And all functionality related to clicking that
> button also works as expected? If I take this fix in Beta9, it will be the
> last Beta before we enter RC and I really want to avoid any regressions.
> Thanks!

I did a local Japanese build and manually verified that the button works and the strings are in Japanese.

Comment 49

[Matthew N. [:MattN]]

Revised script to fix whitepace. Btw. it affected more than the two languages.

> echo "const STRINGS = {};" > ../1208145.js
> grep -R -E "^(noMasterPasswordPrompt|(hide|show)Passwords(AccessKey)?)" | sed 's/\/passwordmgr.properties:/\"]["/' | sed 's/^/STRINGS["/' | sed 's/\s*=\s*/"] = "/' | sed 's/$/";/' | sed 's/\(STRINGS\["[^\
"]\+"]\)/\1 = {};\n\1/' | sort -u > ../1208145.js

Comment 50

[Matthew N. [:MattN]]

Comment on attachment 8707177 [details]
MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30595/diff/3-4/

Comment 51

[Matthew N. [:MattN]]

Comment on attachment 8707178 [details]
MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30597/diff/3-4/

Comment 52

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8707177 [details]
MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu

There is overwhelming feedback from end-users and on SUMO that they want "show passwords" button back, this patch is mostly a back out that brings back the Fx43 behavior with some string changes (could not be avoided). Beta44+, Aurora45+

Comment 53

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8707178 [details]
MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu

Aurora45+, Beta44+

Comment 54

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Matthew N. [:MattN] from comment #48)
> (In reply to Ritu Kothari (:ritu) from comment #46)
> > Matt, did you do a local build with your fix and manually verify that the
> > "Show passwords" comes back? And all functionality related to clicking that
> > button also works as expected? If I take this fix in Beta9, it will be the
> > last Beta before we enter RC and I really want to avoid any regressions.
> > Thanks!
> 
> I did a local Japanese build and manually verified that the button works and
> the strings are in Japanese.

Thanks Matt! Florin, could you please get some focused testing on this fix for 44.0b9? Thanks!

Comment 55

[Matthew N. [:MattN]]

Comment on attachment 8707177 [details]
MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30595/diff/4-5/

Comment 56

[Matthew N. [:MattN]]

Comment on attachment 8707178 [details]
MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30597/diff/4-5/

Comment 57

[Matthew N. [:MattN]]

Comment on attachment 8707178 [details]
MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30597/diff/5-6/

Comment 58

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/639150c94a1c
https://hg.mozilla.org/releases/mozilla-beta/rev/39c8c5085c6b

Comment 59

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0db1f9106ce6
https://hg.mozilla.org/releases/mozilla-aurora/rev/3384695f473b

Comment 60

[Matthew N. [:MattN]]

Verification instructions:

This bug reverts the password management UI to how it was in Fx33 so that can be used as a baseline in behaviour.

Preferences/Options > Security > Saved Logins/Passwords…

The Show/Hide passwords button should be present and localized and should toggle the visibility of the password column. The column should always be hidden by default when the password manager is opened.

Comment 61

[Andrei Vaida [:avaida]]

(In reply to Ritu Kothari (:ritu) from comment #54)
> (In reply to Matthew N. [:MattN] from comment #48)
> > (In reply to Ritu Kothari (:ritu) from comment #46)
> > > Matt, did you do a local build with your fix and manually verify that the
> > > "Show passwords" comes back? And all functionality related to clicking that
> > > button also works as expected? If I take this fix in Beta9, it will be the
> > > last Beta before we enter RC and I really want to avoid any regressions.
> > > Thanks!
> > 
> > I did a local Japanese build and manually verified that the button works and
> > the strings are in Japanese.
> 
> Thanks Matt! Florin, could you please get some focused testing on this fix
> for 44.0b9? Thanks!

We'll make sure this gets verified on 44.0b9.

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/39c8c5085c6b

Comment 63

[Tony Mechelynck [:tonymec]]

(In reply to Matthew N. [:MattN] from comment #60)
> Verification instructions:
> 
> This bug reverts the password management UI to how it was in Fx33 so that
> can be used as a baseline in behaviour.
> 
> Preferences/Options > Security > Saved Logins/Passwords…

(On Linux: Edit → Preferences → Security → Saved Logins)
> 
> The Show/Hide passwords button should be present and localized and should
> toggle the visibility of the password column. The column should always be
> hidden by default when the password manager is opened.

Button present: OK
Localized: Didn't test, I'm on en-US
Toggle visibility: OK, with are-you-sure dialog when showing but not when hiding.
Hidden by default: OK (after show - close either tab or dialog - reopen: the Password column is hidden again).

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0" (en-US) ID:20160114030246 CSet:6fa2ab99f52feb1b6ead5581b8f5d398546a55a5

I'm not setting VERIFIED because I haven't tested any UI language other than en-US.

Comment 64

[Matthew N. [:MattN]]

Thanks for testing Tony

Comment 65

[Francesco Lodolo [:flod]]

The Italian builds look fine
https://ftp.mozilla.org/pub/firefox/candidates/44.0b9-candidates/build1/mac/it/

Comment 66

[Bogdan Maris, Desktop QA]

We tested using Firefox 44 beta 9 l10n builds (en, ar, de, es-ES, fr, it, ja, ko, pl, pt-PT, ru, tr, vi, zh-CN) across platforms (Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 64-bit) and the show/hide passwords button works as expected and it's localized on all builds we tested.