This should match bug 1144385.

Update, this should match the mocks in bug 1128526. This will bring about:passwords to, as ckarlof states, "feature parity as a password manager with a text file."

Bill and I setting as P2

Not actively working on this atm.

antlam confirms that the url/hostname/domain should be editable.

I need a way to handle errors should I be unable to save the users changes to the password db (for what ever reason). I propose a modal dialog with the string "Unfortunately $shortBrand was unable to save your changes at this time." Anthony & Bill, what are your thoughts?

loads the add edit login screen, push buttons! Not written: - getting out of edit login screen - actually writing to the db - formatting/layout/sizing - button handler for show on the edit login page is missing a password to show - favicon missing from page note: the header class ignores hidden="true" unless display="flex" is removed. I scratch my aching head.

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #8) > I need a way to handle errors should I be unable to save the users changes > to the password db (for what ever reason). > > I propose a modal dialog with the string "Unfortunately $shortBrand was > unable to save your changes at this time." > > Anthony & Bill, what are your thoughts? Hm, I think we typically use our toasts/super toasts for this kind of feedback right? Let's stick to those :) Can we have a "Try again" as the action? "Changes could not be saved | Try again"

- wip - buttons are fixed, header hides, can tell if the user has changed something & it needs to be saved. working through problems with login service and its exceptions. detailed comments in patch.

core logic present. needs clean up in a dire manner, proper show password, and a lot of polish.

thoughts?

The toast was not in your mock, but it felt really wrong to hit the save button and get no ui affordance to indicate something happened.

Comment on attachment 8630145 [details] screenshot "Add Logins" Almost there! Some notes: - Background should be white - Input fields height 36px, corner radius 4px, border 1px, border color #AFB1B3, Padded 20px inbetween (vertically) - Input field type is Roboto light, color #222222, 15px, padded 10px from the left - 30px margins on left and right edges of screen - Action button: 30px margin from bottom of last input field, 60px tall, padded 30px left and right from edge, corner radius 4 px, no border, type: Roboto regular 20px, white - First item: 30px margin from orange divider

Comment on attachment 8630147 [details] screenshot "Add Logins" after hitting 'show' button & save button Interesting! + for the toast. I think it works. We should probably kick the user back to the list after they successfully added this login though. Also, can we change the copy on to "Saved login"? Since they are adding a login, this makes more sense.

Comment on attachment 8630180 [details] [diff] [review] addEditLoginToAboutPasswords Review of attachment 8630180 [details] [diff] [review]: ----------------------------------------------------------------- Sweet! I'm excited to try this out. Here's a first pass of review comments, but I also want to try this out before granting review. ::: mobile/android/chrome/content/aboutLogins.js @@ +138,5 @@ > + Cu.reportError("event.state.id: "+ event.state.id); > + this._onEditLogin(event.state.id); > + } else { > + this._selectedLogin = null; > + this._showList(); Nit: 2-space indentation. @@ +141,5 @@ > + this._selectedLogin = null; > + this._showList(); > + } > + }, > + _onEditLogin: function (login) { I would rename this `_showEditLoginDialog` or something like that, to indicate this is about showing the UI, rather than actually submitting an edit. @@ +146,5 @@ > + let list = document.getElementById("logins-list"); > + let header = document.getElementById("logins-header"); > + > + list.setAttribute("hidden", true); > + header.setAttribute("class", "hidden"); This is a bit confusing... I think we should be consistent and either use only "hidden" attributes or only "hidden" class names. @@ +164,5 @@ > + > + _onSaveEditLogin: function() { > + let currUsername = document.getElementById("username").value; > + let currPassword = document.getElementById("password").value; > + let currDomain = document.getElementById("hostname").value; I would call these variables `newFoo` rather than `currFoo`, since they're not actually written yet as the "current" values. @@ +170,5 @@ > + let origPassword = this._selectedLogin.password; > + let origDomain = this._selectedLogin.hostname; > + > + try { > + let loginsManager = Components.classes["@mozilla.org/login-manager;1"].getService(Components.interfaces.nsILoginManager); You could just use Services.logins. @@ +175,5 @@ > + if (( currUsername === origUsername) && > + ( currPassword === origPassword) && > + ( currDomain === origDomain) ) { > + return; > + } Nit: Align the closing brace with the if statement, and use 2-space indentation for the return. Also, no spaces between parens and variable names. @@ +177,5 @@ > + ( currDomain === origDomain) ) { > + return; > + } > + > + let logins = loginsManager.findLogins({}, origDomain, origDomain, null); Instead of making a findLogins call like this, and then looking for the right username, is it possible to just hold onto the login object that we're trying to edit? I see that we don't actually save the login objects on the DOM elements right now, but we could cache them there if it makes editing easier. @@ +184,5 @@ > + if (logins[i].username == origUsername) { > + // clone old record > + let clone; > + clone = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo); > + clone = logins[i].clone(); I'm a bit confused by this... it looks like you just override the previous value of this variable without using it. Why not just `let clone = logins[i].clone();` ? @@ +191,5 @@ > + clone.password = currPassword; > + clone.hostname = currDomain; > + // remove old record > + loginsManager.removeLogin(logins[i]); > + // add new record You can get rid of these comments, since the code they correspond to is easy to read. @@ +197,5 @@ > + break; > + } > + } > + } catch (e) { > + gChromeWin.NativeWindow.toast.show( gStringBundle.GetStringFromName("editLogins.couldNotSave"), "short"); We should return here, otherwise we would show a "saved" toast immediately after this. @@ +207,5 @@ > + let passwordField = document.getElementById("password"); > + let button = document.getElementById("password-btn"); > + let show = gStringBundle.GetStringFromName("password-btn.show"); > + let hide = gStringBundle.GetStringFromName("password-btn.hide"); > + if (button.textContent == show) { I think it would be more robust to check the class name here, rather than comparing the text content of the button directly. You can use button.classList.contains for this. @@ +210,5 @@ > + let hide = gStringBundle.GetStringFromName("password-btn.hide"); > + if (button.textContent == show) { > + passwordField.type = "text"; > + button.textContent= hide; > + button.setAttribute("class", "password-btn-hide"); I also think it's more robust to use button.classList.add/remove to interact with classes, rather than setting/removing an attribute. ::: mobile/android/themes/core/aboutLogins.css @@ +85,5 @@ > visibility: hidden; > } > > +.edit-login-icon { > + background-image: url("resource://android/res/drawable-mdpi-v4/favicon_globe.png"); I see this resourece:// scheme is used elsewhere in this CSS file to get these icons, but rather than explicitly trying to get the MDPI resource, I think it should work to just use "drawable://favicon_globe" to automatically get the right resolution. Also, I imagine this wouldn't work on the API 11+ builds, since I imagine these MDPI assets should only be used for the resource constrained builds. @@ +91,5 @@ > + background-size: 32px 32px; > + background-repeat: no-repeat; > + height: 32px; > + width: 32px; > + padding: 5px; Why do you need this padding on the icon? @@ +101,5 @@ > + > +#save-btn { > + flex: 1; > + height: 50px; > + background-color: #E66000; /*matched to action_orange in java codebase*/ Nice use of the color palette :)

(In reply to Anthony Lam (:antlam) from comment #17) > Comment on attachment 8630145 [details] > screenshot "Add Logins" > > Almost there! Some notes: > - Background should be white > - Input fields height 36px, corner radius 4px, border 1px, border color > #AFB1B3, Padded 20px inbetween (vertically) > - Input field type is Roboto light, color #222222, 15px, padded 10px from > the left > - 30px margins on left and right edges of screen > - Action button: 30px margin from bottom of last input field, 60px tall, > padded 30px left and right from edge, corner radius 4 px, no border, type: > Roboto regular 20px, white > - First item: 30px margin from orange divider Roboto isn't available to html/css. Is there a similiar font-family you would like instead? > Also, can we change the copy on to "Saved login"? Since they are adding a login, this makes more sense. Er, I couldn't parse this sentence. Which string do you want changed?

> > > Also, can we change the copy on to "Saved login"? Since they are adding a login, this makes more sense. > > Er, I couldn't parse this sentence. Which string do you want changed? String for the toast.

Sorry, not Roboto - I meant Clear sans! That's what we use in the about pages

(In reply to Anthony Lam (:antlam) from comment #23) > Sorry, not Roboto - I meant Clear sans! That's what we use in the about pages Clear Sans is the default font defined in aboutBase.css, so hopefully this just automatically works: http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutBase.css#9

(In reply to :Margaret Leibovic from comment #19) > Comment on attachment 8630180 [details] [diff] [review] > addEditLoginToAboutPasswords > > Review of attachment 8630180 [details] [diff] [review]: > ----------------------------------------------------------------- @@ +177,5 @@ > + ( currDomain === origDomain) ) { > + return; > + } > + > + let logins = loginsManager.findLogins({}, origDomain, origDomain, null); > Instead of making a findLogins call like this, and then looking for the right username, is it possible to > just hold onto the login object that we're trying to edit? I see that we don't actually save the login > objects on the DOM elements right now, but we could cache them there if it makes editing easier. I did initially hold onto it, but it ended up with it throwing most of the time because the copy of the record needs to match the one in sql exactly or it throws. (I think this API could use refactoring to be less fragile to consume, but that's out of scope here). In general this call only returns one record, but multiple records are legal (missing user names, multiple accounts on the same site) and so you need to check the username to know you have the exact right record for correctness, but it does not incur much of a perf cost. If you beleive this to be important, we can file a followup to make it so. @@ +91,5 @@ > + background-size: 32px 32px; > + background-repeat: no-repeat; > + height: 32px; > + width: 32px; > + padding: 5px; > Why do you need this padding on the icon? added to make it look like antlam's mock, where the icon is not quite laid out as it is on the list screen. If you find this manner particularly distasteful, I can look for other ways to get the same effect.

- all of antlam's requested changes -- font is already correct - all of margaret's - margaret's classList request required a bit of a refactor of hidden attr/hidden class

Looks good! :) One comment though: the favicon looks really fuzzy. Can we get the right favicon size from the cache?

I intended to use the URL as a reliable fallback in case something happened when the URL field was being edited. This has the added benefit of providing context for the user. But, since we know the URL doesn't always get saved automatically, Chenxia pointed out that we might need a fallback. So, if there's no URL saved, let's use "Edit login" in the top title.

Comment on attachment 8630785 [details] [diff] [review] addEditLoginToAboutPasswords Review of attachment 8630785 [details] [diff] [review]: ----------------------------------------------------------------- This is generally looking good, but I have some feedback I want you to address before we land this. ::: mobile/android/chrome/content/aboutLogins.js @@ +129,5 @@ > _showList: function () { > let list = document.getElementById("logins-list"); > + let header = document.getElementById("logins-header"); > + list.classList.remove("hidden"); > + header.classList.remove("hidden"); Do we need to hide/show this header element explicitly? Shouldn't that happen automatically since it's a child of #edit-logins-page. @@ +135,5 @@ > + let editLoginsPage = document.getElementById("edit-logins-page"); > + editLoginsPage.classList.add("hidden"); > + }, > + > + Nit: trailing whitespace (in a bunch of other places as well). I wonder if there's a linter rule we could add for this. @@ +139,5 @@ > + > + _onPopState: function (event) { > + // Called when back/forward is used to change the state of the page > + if (event.state) { > + Cu.reportError("event.state.id: "+ event.state.id); Remove this logging, since this isn't an error. @@ +179,5 @@ > + if ((newUsername === origUsername) && > + (newPassword === origPassword) && > + (newDomain === origDomain) ) { > + gChromeWin.NativeWindow.toast.show( gStringBundle.GetStringFromName("editLogins.noChanges"), "short"); > + return; This is probably a UX question, but if I hit "save", even if I didn't change anything, I probably just want to go back to the password list, not see an error toast. We don't provide a "Cancel" button, so this could be confusing for users if they don't know to hit the back button. @@ +199,5 @@ > + } catch (e) { > + gChromeWin.NativeWindow.toast.show( gStringBundle.GetStringFromName("editLogins.couldNotSave"), "short"); > + return; > + } > + gChromeWin.NativeWindow.toast.show( gStringBundle.GetStringFromName("editLogins.saved"), "short"); Nit: No space between ( and variable names (this applies in a bunch of places). @@ +217,5 @@ > + } else { > + passwordField.type = "text"; > + button.textContent= hide; > + button.classList.add("password-btn-hide"); > + } I found that if you show your password, then hit back, then go to another login item, the password field is still visible. We should reset this when we hide the edit login page. ::: mobile/android/chrome/content/aboutLogins.xhtml @@ +26,5 @@ > <ul class="toolbar-buttons"> > <li id="filter-button"></li> > </ul> > </div> > + <div id="logins-list" class="list, hidden"> Class lists are space-separated, not comma-separated. This will prevent the "list" class from applying properly. @@ +32,5 @@ > <div id="filter-input-container" hidden="true"> > <input id="filter-input" type="search"/> > <div id="filter-clear"></div> > </div> > + <div id="edit-logins-page" class="hidden"> Nit: "edit-login-page" would be more consistent with the other class names, and also this is about editing a single login. ::: mobile/android/locales/en-US/chrome/aboutLogins.dtd @@ +2,5 @@ > - License, v. 2.0. If a copy of the MPL was not distributed with this > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > > <!ENTITY aboutLogins.title "Logins"> > +<!ENTITY aboutLogins.editLoginsTitle "Add Login"> This should say "Edit Login". ::: mobile/android/themes/core/aboutLogins.css @@ +83,5 @@ > margin: 0 5px; > } > > +.edit-login-icon { > + background-image: url("drawable://favicon_globe"); I'm not seeing a globe appear in an example where I don't have a favicon: https://www.dropbox.com/s/r8s5w67aqilnble/2015-07-09%2000.26.56.png?dl=0 I also found that when I edited one login that did have a favicon, then switched back to this one, the icon didn't update: https://www.dropbox.com/s/siorfax2hwe8cio/2015-07-09%2000.30.03.png?dl=0 So I'm not sure that this actually works... @@ +134,5 @@ > +} > + > +#password-btn { > + margin: none; > + padding:none; `none` is an invalid property value for margin/padding. I think you mean to use `0` here instead, although if this looks fine with these invalid property values, you should just remove these rules.

(In reply to :Margaret Leibovic from comment #29) > Comment on attachment 8630785 [details] [diff] [review] > addEditLoginToAboutPasswords > > Review of attachment 8630785 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: mobile/android/chrome/content/aboutLogins.js > @@ +129,5 @@ > > _showList: function () { > > let list = document.getElementById("logins-list"); > > + let header = document.getElementById("logins-header"); > > + list.classList.remove("hidden"); > > + header.classList.remove("hidden"); > > Do we need to hide/show this header element explicitly? Shouldn't that > happen automatically since it's a child of #edit-logins-page. It's not the header on the edit login view, it's the header on the list view. So if I don't unhide it, showList() will not show the correct header. > > @@ +139,5 @@ > > + > > + _onPopState: function (event) { > > + // Called when back/forward is used to change the state of the page > > + if (event.state) { > > + Cu.reportError("event.state.id: "+ event.state.id); > > Remove this logging, since this isn't an error. This was part of the original PopupState(), removed as part of Bug 1144413, so when I returned PopupState() to the file, I left that as is. Easy enough to remove. > > @@ +179,5 @@ > > + if ((newUsername === origUsername) && > > + (newPassword === origPassword) && > > + (newDomain === origDomain) ) { > > + gChromeWin.NativeWindow.toast.show( gStringBundle.GetStringFromName("editLogins.noChanges"), "short"); > > + return; > > This is probably a UX question, but if I hit "save", even if I didn't change > anything, I probably just want to go back to the password list, not see an > error toast. We don't provide a "Cancel" button, so this could be confusing > for users if they don't know to hit the back button. I can see the reasoning both ways, so let's ask :antlam. Antlam, what would you like to happen if the user hits "save" and there are no changes to save? Currently a toast pops up saying there were no changes to save. Would you also want it to kick them back to the list? To do both?

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #30) > (In reply to :Margaret Leibovic from comment #29) > > Comment on attachment 8630785 [details] [diff] [review] > > addEditLoginToAboutPasswords > > > > Review of attachment 8630785 [details] [diff] [review]: > > ----------------------------------------------------------------- > > ::: mobile/android/chrome/content/aboutLogins.js > > @@ +129,5 @@ > > > _showList: function () { > > > let list = document.getElementById("logins-list"); > > > + let header = document.getElementById("logins-header"); > > > + list.classList.remove("hidden"); > > > + header.classList.remove("hidden"); > > > > Do we need to hide/show this header element explicitly? Shouldn't that > > happen automatically since it's a child of #edit-logins-page. > > It's not the header on the edit login view, it's the header on the list > view. So if I don't unhide it, showList() will not show the correct header. In that case, I think we should just make a div that wraps both the main view header and list, then show/hide that, rather than showing/hiding the individual elements. Looking at the markup, we probably also want to include #filter-input-container in that, since we would never want that to show up on the edit login page.

(In reply to :Margaret Leibovic from comment #31) > (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #30) > > (In reply to :Margaret Leibovic from comment #29) > > > Comment on attachment 8630785 [details] [diff] [review] > > > addEditLoginToAboutPasswords > > > > > > Review of attachment 8630785 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > ::: mobile/android/chrome/content/aboutLogins.js > > > @@ +129,5 @@ > > > > _showList: function () { > > > > let list = document.getElementById("logins-list"); > > > > + let header = document.getElementById("logins-header"); > > > > + list.classList.remove("hidden"); > > > > + header.classList.remove("hidden"); > > > > > > Do we need to hide/show this header element explicitly? Shouldn't that > > > happen automatically since it's a child of #edit-logins-page. > > > > It's not the header on the edit login view, it's the header on the list > > view. So if I don't unhide it, showList() will not show the correct header. > > In that case, I think we should just make a div that wraps both the main > view header and list, then show/hide that, rather than showing/hiding the > individual elements. Looking at the markup, we probably also want to include > #filter-input-container in that, since we would never want that to show up > on the edit login page. I have done just that in the patch on deck for Bug 1155345, but left it out of this patch for the sake of avoiding scope creep. I am happy enough to bring it over to this one.

waiting on Anthony's feedback

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #30) > Antlam, what would you like to happen if the user hits "save" and there are > no changes to save? Currently a toast pops up saying there were no changes > to save. Would you also want it to kick them back to the list? To do both? Yes, we should be booting the user out and back to the list here. As for the toast, we can keep the same message ("Saved login") to reassure the user that everything's been kept. I'm not sure it's vital to explain to the user that they didn't change anything when all they want to do is "save".

Once more, with feeling!

Comment on attachment 8631889 [details] [diff] [review] addEditLoginToAboutPasswords Review of attachment 8631889 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/aboutLogins.js @@ +135,5 @@ > + let button = document.getElementById("password-btn"); > + if (button.classList.contains("password-btn-hide")) { > + document.getElementById("password").type = "password"; > + button.textContent = gStringBundle.GetStringFromName("password-btn.show"); > + button.removeAttribute("class"); I don't think you need to remove the class attribute, since you're removing the "password-btn-hide" class below. I also think it would probably be better to split this logic to toggle the button state into a helper method, since this is duplicated below. But you can do that in a separate bug if you just want to land this. ::: mobile/android/locales/en-US/chrome/aboutLogins.dtd @@ +1,5 @@ > <!-- This Source Code Form is subject to the terms of the Mozilla Public > - License, v. 2.0. If a copy of the MPL was not distributed with this > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > <!ENTITY aboutLogins.title "Logins"> > +<!ENTITY aboutLogins.editLoginsTitle "Edit Login"> This string is unused.

The tree is finally open again! \o/ https://hg.mozilla.org/integration/fx-team/rev/1106b99de434 toggling password button in helper class(es).

Nice! I'm really excited to see this land! :) The toast for the login seems inconsistent with our other strings - I don't think it should be "Saved your login". Also, should the button say "Update", like the mock, or did it get switched to "Save"? Other than that, it looks nice! A few other notes: - We should disable saving if you delete the entire password - Form suggestions should be disabled for the username field - The Show/Hide button changes sizes on click, which is weird

(In reply to Chenxia Liu [:liuche] from comment #39) > A few other notes: > - We should disable saving if you delete the entire password > - Form suggestions should be disabled for the username field > - The Show/Hide button changes sizes on click, which is weird Although this is marked as fixed now, Liuche makes some good points and I just wanted to get this on your radar too Ally :)

Barbara, what do you think of Chenxia's suggestions? I agree with Anthony that she has some good points and I think they could be contenders for v2. What do you think? If so, I can file bugs for each item and attach them to the tracking bug for v2.

I would say that half of these are just follow-ups that should be done for the v1, especially since they are small and don't require much work (the string changes, and maybe removing the form-suggestion if it's just a line or two). I understand landing something is important, and I'm glad we did that here, but at the same time, we don't want to have something that is just not consistent with the mock - v1 should mean feature-limited, not a sloppy experience. I would say the rest of these could fall under v2: > > A few other notes: > > - We should disable saving if you delete the entire password > > - The Show/Hide button changes sizes on click, which is weird

Thanks Chenxia for flagging those. I agree that these are not show-stoppers, however small enough to fix, and big enough to annoy users, e.g. the "disable save when removing password" is a good example, where myself as a user would think, oh this is so obvious why didn't UX/Eng think about it, and rather making me have to think (i.e. don't make the user think too much when something doesn't work :)) Sorry, I wasn't sure what the issue was with the form suggestions? So +1 for getting those in before we land this, assuming those are quick fixes to make. Ally, please file bugs for each. Thanks :)

Barbara: fyi, to answer your question about form (field) suggestions, I'm referring to Form Autofill where Firefox will remember things that you've typed into a field (e.g., a username field) and suggest it in a dropdown in the future. We should disable this for the password edit form because the use case for Form Autofill is to suggest text that you type often so you can just select it instead of typing all of it, and that's not the use case for editing your login (you don't change your username/password often, and when you change it, you don't want to select something that you've used in the past).

Thanks, Chenxia, now that makes sense.

Since these changes have already landed in Nightly, let's keep this bug resolved. Ally, can you file new ones? Since a lot of these are small, maybe we can lump the follow-ups into one bug, and then the v2 into another bug (or two). Talking to rfeeley, he mentioned that the strings for "Show" and "Hide" can be radically different lengths in other languages so it's best to just use one string and toggle the state.

Tested using: Device: Nexus 4 (Android 5.1) Build: Firefox for Android 42.0a1 (2015-07-19)

filed