Closed
Bug 1208145
Opened 9 years ago
Closed 9 years ago
Revert bug 1121291 to bring back the button to toggle all passwords
Categories
(Toolkit :: Password Manager, defect)
Toolkit
Password Manager
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: rfeeley, Assigned: MattN)
References
(Depends on 1 open bug)
Details
Attachments
(6 files, 3 obsolete files)
(deleted),
text/x-review-board-request
|
Dolske
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
Dolske
:
review+
|
Details |
(deleted),
application/x-gzip
|
Details | |
(deleted),
text/x-review-board-request
|
Dolske
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
Dolske
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
(deleted),
application/javascript;charset=utf-8
|
MattN
:
feedback+
|
Details |
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
Reporter | ||
Comment 1•9 years ago
|
||
"So that I can safer" >> "So that I can feel safer"
(I'm so spoiled by the non-Bugzilla editable internet)
Assignee | ||
Comment 2•9 years ago
|
||
The import button is already on the right for Windows. Does that change the position of Show?
Flags: needinfo?(rfeeley)
Assignee | ||
Comment 3•9 years ago
|
||
Also, I guess we should add Show to the context menu too.
Assignee | ||
Comment 4•9 years ago
|
||
[Tracking Requested - why for this release]: Follow-up from bug 1121291. Users aren't able to discover showing passwords as easily.
status-firefox43:
--- → unaffected
status-firefox44:
--- → affected
status-firefox45:
--- → affected
tracking-firefox44:
--- → ?
Reporter | ||
Comment 5•9 years ago
|
||
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.
Reporter | ||
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
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)
Comment 9•9 years ago
|
||
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•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
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)
Comment 13•9 years ago
|
||
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)
Assignee | ||
Comment 14•9 years ago
|
||
(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…
Assignee | ||
Comment 15•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/30467/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30467/
Attachment #8706721 -
Flags: review?(dolske)
Assignee | ||
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
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
Comment 18•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
(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 21•9 years ago
|
||
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+
Assignee | ||
Comment 22•9 years ago
|
||
(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•9 years ago
|
||
Comment 24•9 years ago
|
||
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"
Assignee | ||
Comment 25•9 years ago
|
||
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 hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8707165 -
Attachment mime type: application/javascript → application/javascript;charset=utf-8
Comment 27•9 years ago
|
||
Comment on attachment 8707165 [details]
5 strings for each locale in JS objects
Looks good to me, thanks
Attachment #8707165 -
Flags: feedback+
Assignee | ||
Comment 28•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/30595/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30595/
Attachment #8707177 -
Flags: review?(dolske)
Assignee | ||
Comment 29•9 years ago
|
||
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)
Assignee | ||
Comment 30•9 years ago
|
||
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)
Assignee | ||
Comment 31•9 years ago
|
||
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)
Assignee | ||
Comment 32•9 years ago
|
||
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/
Assignee | ||
Comment 33•9 years ago
|
||
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+
Assignee | ||
Comment 34•9 years ago
|
||
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.
Assignee | ||
Comment 35•9 years ago
|
||
(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.
Assignee | ||
Comment 36•9 years ago
|
||
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/
Assignee | ||
Comment 37•9 years ago
|
||
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/
Assignee | ||
Comment 38•9 years ago
|
||
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?
Assignee | ||
Comment 39•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8707177 -
Flags: review?(dolske) → review+
Comment 40•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8707178 -
Flags: review?(dolske) → review+
Comment 41•9 years ago
|
||
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•9 years ago
|
||
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+
Comment 43•9 years ago
|
||
(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•9 years ago
|
||
(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•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
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.
Assignee | ||
Comment 48•9 years ago
|
||
(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.
Assignee | ||
Comment 49•9 years ago
|
||
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
Assignee | ||
Comment 50•9 years ago
|
||
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/
Assignee | ||
Comment 51•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Comment 55•9 years ago
|
||
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/
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Comment 56•9 years ago
|
||
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/
Assignee | ||
Comment 57•9 years ago
|
||
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/
Assignee | ||
Comment 58•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/639150c94a1c
https://hg.mozilla.org/releases/mozilla-beta/rev/39c8c5085c6b
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 59•9 years ago
|
||
Assignee | ||
Comment 60•9 years ago
|
||
verification-steps |
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•9 years ago
|
||
(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)
Comment 62•9 years ago
|
||
bugherder uplift |
status-b2g-v2.5:
--- → fixed
Comment 63•9 years ago
|
||
(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.
Assignee | ||
Comment 64•9 years ago
|
||
Thanks for testing Tony
Comment 65•9 years ago
|
||
The Italian builds look fine
https://ftp.mozilla.org/pub/firefox/candidates/44.0b9-candidates/build1/mac/it/
Comment 66•9 years ago
|
||
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.
Updated•9 years ago
|
QA Whiteboard: [verify? Fx44+, Fx45?, Fx46± see comment 48, comment 63]
You need to log in
before you can comment on or make changes to this bug.
Description
•