Closed Bug 1208145 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Password Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox43 --- unaffected
firefox44 + verified
firefox45 --- fixed
firefox46 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: rfeeley, Assigned: MattN)

References

(Depends on 1 open bug)

Details

Attachments

(6 files, 3 obsolete files)

58 bytes, text/x-review-board-request
Dolske
: review+
Details
58 bytes, text/x-review-board-request
Dolske
: review+
Details
51.93 KB, application/x-gzip
Details
58 bytes, text/x-review-board-request
Dolske
: review+
Details
58 bytes, text/x-review-board-request
Dolske
: review+
Details
31.45 KB, application/javascript;charset=utf-8
MattN
: feedback+
Details
Attached image show-hide.gif (obsolete) —
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
"So that I can safer" >> "So that I can feel safer"

(I'm so spoiled by the non-Bugzilla editable internet)
The import button is already on the right for Windows. Does that change the position of Show?
Flags: needinfo?(rfeeley)
Also, I guess we should add Show to the context menu too.
[Tracking Requested - why for this release]: Follow-up from bug 1121291. Users aren't able to discover showing passwords as easily.
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.
Tracked for 44 as it's a follow up to changes already made in passwords manager.
Attached image saved-logins-window.png (obsolete) —
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?
Flags: needinfo?(rfeeley) → needinfo?(MattN+bmo)
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.
Flags: needinfo?(dolske)
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
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.
Flags: needinfo?(dolske) → needinfo?(l10n)
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.
Flags: needinfo?(l10n)
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. :(
Flags: needinfo?(l10n)
Flags: needinfo?(dolske)
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.
Flags: needinfo?(l10n)
(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…
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/
Attachment #8706722 - Flags: review?(dolske)
Morphing this bug to cover reverting bug 1121291 since the discussion about it is here.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)
Summary: Allow user to show selected passwords with a button → Revert bug 1121291 to bring back the button to toggle all passwords
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.
(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.
(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?
Flags: needinfo?(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.
Attachment #8706722 - Flags: review?(dolske) → review+
(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.
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"
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.
Attachment #8707165 - Attachment mime type: application/javascript → application/javascript;charset=utf-8
Comment on attachment 8707165 [details]
5 strings for each locale in JS objects

Looks good to me, thanks
Attachment #8707165 - Flags: feedback+
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/
Attachment #8707178 - Flags: review?(dolske)
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.
Attachment #8707177 - Flags: review?(dolske)
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/
Attachment #8707177 - Flags: review?(dolske)
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/
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
Attachment #8707165 - Attachment is obsolete: true
Attachment #8707214 - Flags: feedback+
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.
(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 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 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 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.
Attachment #8707177 - Flags: approval-mozilla-beta?
Attachment #8707177 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(MattN+bmo)
Attachment #8707178 - Flags: approval-mozilla-beta?
Attachment #8707178 - Flags: approval-mozilla-aurora?
Attachment #8707177 - Flags: review?(dolske) → review+
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?
Attachment #8707178 - Flags: review?(dolske) → review+
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 on attachment 8706721 [details]
MozReview Request: Bug 1208145 - Back out changeset e4e3a8b66db4 (bug 1121291). r=dolske

https://reviewboard.mozilla.org/r/30467/#review27431
Attachment #8706721 - Flags: review?(dolske) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/b0083022f6c3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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!
Forgot to NI Matt.
(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.
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 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 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 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+
Attachment #8707177 - Flags: approval-mozilla-beta?
Attachment #8707177 - Flags: approval-mozilla-beta+
Attachment #8707177 - Flags: approval-mozilla-aurora?
Attachment #8707177 - Flags: approval-mozilla-aurora+
Comment on attachment 8707178 [details]
MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu

Aurora45+, Beta44+
Attachment #8707178 - Flags: approval-mozilla-beta?
Attachment #8707178 - Flags: approval-mozilla-beta+
Attachment #8707178 - Flags: approval-mozilla-aurora?
Attachment #8707178 - Flags: approval-mozilla-aurora+
(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!
Flags: needinfo?(florin.mezei)
Attachment #8707177 - Attachment description: MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a= → MozReview Request: Bug 1208145 - Revert bug 1121291 using hard-coded strings from Fx43. r=dolske a=ritu
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/
Attachment #8707178 - Attachment description: MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a= → MozReview Request: Bug 1208145 - Clear XUL persistence data for passwordManager.xul passwordCol.hidden. r=dolske a=ritu
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 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/
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.
(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.
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
(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.
Thanks for testing Tony
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.
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
QA Whiteboard: [verify? Fx44+, Fx45?, Fx46± see comment 48, comment 63]
Blocks: 1508165
You need to log in before you can comment on or make changes to this bug.