1093201 - (password-ui) Mobile passwords management UI (about:passwords)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Logins, Passwords and Form Fill, defect)

Description

[:Margaret Leibovic]

We should add some UI to let users see and manage their passwords on mobile. This will be especially helpful for bug 1078704.

mfinkle created an about:passwords page in his passwords add-on, modeled after some of the other about: pages we have. We should start thinking about what we'll want to do in the product.

From an engineering perspective, I think we should just start by trying to make a list of all the user's stored passwords, with the option to be able to forget some of them, similar to what desktop has.

Comment 1

[Chenxia Liu [:liuche]]

I wonder if we want to actually display the passwords, or just offer to reset/clear them? Should we tie this into Master Password in any way? I guess Desktop doesn't require this, so for a v1, a list of website:username/password where the passwords can be toggled to be shown.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: fennec-passwords-list.png

Example of the main list from my add-on

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: fennec-passwords-list-detail.png

Example of the detail page

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Feel free to steal code from the add-on:
https://github.com/mfinkle/passwords

It's kinda why I made it :)

Comment 5

[:Margaret Leibovic]

Chenxia, do you want to take a stab at this? Robin, have you thought about how we might want to do password management UI for the browser?

I think no matter what, we'll need to get some plumbing in place, so I don't think we need to block on UX to start getting the basics in place here.

Comment 6

[Chenxia Liu [:liuche]]

Yep, I'll take this and get a desktop parity version.

Comment 7

[Chenxia Liu [:liuche]]

Attachment: Patch: WIP of aboutPasswords

Still need to do the following:
- Finish listener for passwordmgr-storage-changed so about:passwords will stay up current
- Add no-passwords message
- Handle removing passwords (long-press context menu as a v1)

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Patch: Nightly build flag patch

Patch to be folded into aboutPasswords patch that will move everything behind the NIGHTLY build flag.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8519344 [details] [diff] [review]
Patch: WIP of aboutPasswords

Review of attachment 8519344 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome! I just made a quick pass with some drive-by feedback.

::: mobile/android/chrome/content/aboutPasswords.js
@@ +26,5 @@
> +    clipboard.copyString(string);
> +  } catch (e) {}
> +}
> +
> +function init() {

I would just put this all in the Passwords.init method, and call that from the "load" listener.

@@ +37,5 @@
> +}
> +
> +function uninit() {}
> +
> +function onPopState(aEvent) {

This function could also be part of the Passwords object.

@@ +52,5 @@
> +}
> +
> +function showList() {
> +  // Hide the detail page and show the list
> +  let details = document.querySelector("#login-details");

Instead of using query selector here, you should just getElementById("login-details").

@@ +55,5 @@
> +  // Hide the detail page and show the list
> +  let details = document.querySelector("#login-details");
> +  details.style.display = "none";
> +  let list = document.querySelector("#logins-list");
> +  list.style.display = "block";

Instead of setting styles in JS like this, we should use class names and/or attributes in CSS (I prefer attributes, like a "hidden" attribute).

Also, if we want to do this every time the page loads, we can just set the appropriate attribute in the markup to make these thing start in hidden state that we want.

@@ +59,5 @@
> +  list.style.display = "block";
> +}
> +
> +var Passwords = {
> +  _createItem: function _createItem(login) {

You don't need to name functions that are properties on objects (we used to need to do this to get more useful stack traces, but now the JS engine is smarter and will use the property name).

@@ +66,5 @@
> +    outer.className = "login-item list-item";
> +    outer.addEventListener("click", function() {
> +      this.showDetails(outer);
> +      history.pushState({ id: login.guid }, document.title);
> +    }.bind(this), true);

Instead of using bind, you should use fat arrow functions.

@@ +118,5 @@
> +
> +    this._loadList();
> +
> +    document.getElementById("copyusername-btn").addEventListener("click", Passwords.copyUsername.bind(this), false);
> +    document.getElementById("copypassword-btn").addEventListener("click", Passwords.copyPassword.bind(this), false);

The implementations of copyUsername and copyPassword don't use the `this` keyword, so you don't need to bind the context.

@@ +135,5 @@
> +    }
> +
> +    logins.forEach(login => login.QueryInterface(Ci.nsILoginMetaInfo));
> +
> +    logins.sort(function(a, b) {

For consistency, you can also use a fat arrow function here.

@@ +165,5 @@
> +    let login = detailItem.login = listItem.login;
> +    let favicon = document.querySelector("#login-details > .login-item .icon");
> +    favicon.setAttribute("src", login.hostname + "/favicon.ico");
> +
> +    detailItem.querySelector(".hostname").textContent = login.hostname;

If you're only going to have one element you're trying to change, I would use an id, not a class name.

@@ +174,5 @@
> +
> +    let userInputs = [];
> +    if (matchedURL) {
> +      let [, , domain] = matchedURL;
> +      userInputs = domain.split(".").filter((part) => part.length > 3);

Nit: I feel like we prefer no parens on single-argument fat arrow function parameters.

@@ +182,5 @@
> +    let days = Math.round((Date.now() - lastChanged) / 1000 / 60 / 60/ 24);
> +    detailItem.querySelector(".password-age").textContent = gStringBundle.formatStringFromName("passwordsDetails.age", [days], 1);
> +
> +    let list = document.querySelector("#logins-list");
> +    list.style.display = "none";

Once again, I think we should use a class name or attribute here to change the style.

@@ +194,5 @@
> +    let login = detailItem.login;
> +    if (!login) {
> +      return;
> +    }
> +    copyString(login.username);

UX idea: should we show a toast when we do this? And maybe show a toast if we fail to get a login for some reason?

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #9)

> > +    if (matchedURL) {
> > +      let [, , domain] = matchedURL;
> > +      userInputs = domain.split(".").filter((part) => part.length > 3);
> 
> Nit: I feel like we prefer no parens on single-argument fat arrow function
> parameters.

O_o

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Jumping in with some visuals.

As an initial pass, let's inherit from aboutBase.css like bug 1075219 to keep it consistent.

Comment 12

[Chenxia Liu [:liuche]]

Attachment: Patch: about:passwords UI

This patch makes a basic about:passwords page.

- Shows list of passwords + details
- Clicking on details will open link to account signin page for the password entry
- Toast on clicking Copy *

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Details page

Screenshot for you, :epiclam!

Feel free to suggest string changes - there will definitely be follow-ups to flesh out what this page can do.

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Looks good! thanks Chenxia!

What does "age: 0" refer to again?

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #14)
> Looks good! thanks Chenxia!
> 
> What does "age: 0" refer to again?

How many days since you last updated the password. We should figure out a better way to show that.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8524285 [details] [diff] [review]
Patch: about:passwords UI

Review of attachment 8524285 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! I have a bunch of feedback, but I think this is close to being able to land behind a Nightly flag.

We should also try making a very simple test using JavascriptTest. Best to do that now while there's a minimal set of features to test! I think the first iteration of this test would look like:

1) Add some logins (at least one) to the login manager
2) Load about:passwords
3) Check that login(s) appear (you can grab the window and poke directly into the content to see this)

And we can hold off on testing things like the copy buttons, since the UX for that might change.

::: mobile/android/chrome/content/aboutPasswords.js
@@ +25,5 @@
> +    let clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].getService(Ci.nsIClipboardHelper);
> +    clipboard.copyString(string);
> +    gChromeWin.NativeWindow.toast.show(notifyString, "short");
> +  } catch (e) {
> +    debug("Error copying from about:passwords");

We should probably have an error toast if there copyString method can fail.

@@ +31,5 @@
> +}
> +
> +function openLink(aEvent) {
> +  try {
> +    let formatter = Cc["@mozilla.org/toolkit/URLFormatterService;1"].getService(Ci.nsIURLFormatter);

Unused variable.

@@ +37,5 @@
> +    let url = aEvent.currentTarget.getAttribute("link");
> +    let BrowserApp = gChromeWin.BrowserApp;
> +    BrowserApp.addTab(url, { selected: true, parentId: BrowserApp.selectedTab.id });
> +  } catch (ex) {
> +    debug("Error fetching and opening account link: " + url);

When would this throw? I don't think we need a try/catch here.

@@ +41,5 @@
> +    debug("Error fetching and opening account link: " + url);
> +  }
> +}
> +
> +function uninit() {}

Remove this empty function.

@@ +44,5 @@
> +
> +function uninit() {}
> +
> +let Passwords = {
> +  init: function() {

Style nit: Space between "function" and "(".

@@ +47,5 @@
> +let Passwords = {
> +  init: function() {
> +    window.addEventListener("popstate", this , false);
> +
> +    Services.obs.addObserver(this, "passwordmgr-storage-changed", false);

You should remove this observer on window unload.

@@ +53,5 @@
> +    this._loadList();
> +
> +    document.getElementById("copyusername-btn").addEventListener("click", this.copyUsername.bind(this), false);
> +    document.getElementById("copypassword-btn").addEventListener("click", this.copyPassword.bind(this), false);
> +    document.getElementById("details-header").addEventListener("click", openLink, false);

For consistency, it would be nice if openLink was just a helper method inside the Passwords object.

@@ +65,5 @@
> +      logins = Services.logins.getAllLogins();
> +    } catch(e) {
> +      // Master password was not entered
> +      debug("Master password permissions error: " + e);
> +      logins = [];

You don't need to set the logins variable if you're just returning early. Or if you want to try to load an empty list, you should remove the return statement.

@@ +69,5 @@
> +      logins = [];
> +      return;
> +    }
> +
> +    logins.forEach(login => login.QueryInterface(Ci.nsILoginMetaInfo));

Does this actually change the value of the logins in the array? Did you mean to use map instead?

And if this doesn't change the value, is that QI needed?

@@ +79,5 @@
> +    list.innerHTML = "";
> +    for (let i = 0; i < logins.length; i++) {
> +      let item = this._createItemForLogin(logins[i]);
> +      list.appendChild(item);
> +    }

This could be simplified to use forEach, like this:

logins.forEach(login => {
  let item = this._createItemForLogin(login);
  list.appendChild(item);
});

@@ +82,5 @@
> +      list.appendChild(item);
> +    }
> +  },
> +
> +  showList: function() {

General style nit: We usually prefix methods with an underscore if they're only called from within the object where they're declared (JS version of "private"), and this is an example of a method that could be "private".

@@ +90,5 @@
> +    let list = document.getElementById("logins-list");
> +    list.removeAttribute("hidden");
> +  },
> +
> +  _onPopState: function (aEvent) {

Global nit: Don't use the "a" prefix for parameters. We've been moving away from that in our JS style.

@@ +109,5 @@
> +    let outer = document.createElement("div");
> +    outer.setAttribute("loginID", login.guid);
> +    outer.className = "login-item list-item";
> +    outer.addEventListener("click", function() {
> +      Passwords.showDetails(outer);

Use an arrow function here, and then you can just call `this.showDetails`.

@@ +115,5 @@
> +    }, true);
> +
> +    let img = document.createElement("img");
> +    img.className = "icon";
> +    img.setAttribute("src", login.hostname + "/favicon.ico");

This seems like a fine solution for now, but this won't necessarily work for all sites, since favicons can be included in <link> tags. Also, I don't know that we want to be making all these network requests in a chrome page... we can file a follow-up to see if there's any way to use the favicon cache, but it's annoying that there isn't an easy way to get at this in JS right now :/

@@ +145,5 @@
> +    return outer;
> +  },
> +
> +  _createItemForLogin: function (login) {
> +    let item = this._createItem(login);

I think we should just combine _createItem and _createItemFromLogin into one method.

@@ +150,5 @@
> +    item.login = login;
> +    return item;
> +  },
> +
> +  _getElementForLogin: function (key) {

s/key/login/

@@ +178,5 @@
> +
> +  showDetails: function (listItem) {
> +    let detailItem = document.querySelector("#login-details > .login-item");
> +    let login = detailItem.login = listItem.login;
> +    let favicon = document.querySelector("#login-details > .login-item .icon");

You can do `detailItem.querySelector(".icon")` if you want the icon element that's a child of the detail item.

@@ +185,5 @@
> +    document.getElementById("details-header").setAttribute("link", login.hostname);
> +
> +    document.getElementById("detail-hostname").textContent = login.hostname;
> +    document.getElementById("detail-realm").textContent = login.httpRealm;
> +    document.getElementById("detail-username").textContent = login.username;

I wonder if there's any sort of basic HTML template support in the tree...

@@ +187,5 @@
> +    document.getElementById("detail-hostname").textContent = login.hostname;
> +    document.getElementById("detail-realm").textContent = login.httpRealm;
> +    document.getElementById("detail-username").textContent = login.username;
> +
> +    let matchedURL = login.hostname.match(/^((?:[a-z]+:\/\/)?(?:[^\/]+@)?)(.+?)(?::\d+)?(?:\/|$)/);

Can you document what this regex does? (Or where you copied it from? :)

::: mobile/android/themes/core/aboutPasswords.css
@@ +27,5 @@
> +}
> +
> +.realm {
> +  /* hostname is not localized, so keep the margin on the left side */
> +  margin-left: .67em;

We should use -moz-margin-start in case we ever ship an RTL locale.

Also, why .67em? Seems like an odd value?

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Part 1: about:passwords

Nits/comments fixed in patch, or addressed below.

> We should also try making a very simple test using JavascriptTest. Best to
> do that now while there's a minimal set of features to test! I think the
> first iteration of this test would look like:
> 
> 1) Add some logins (at least one) to the login manager
> 2) Load about:passwords
> 3) Check that login(s) appear (you can grab the window and poke directly
> into the content to see this)
> 
> And we can hold off on testing things like the copy buttons, since the UX
> for that might change.

Part 3 will be tests!

> 
> @@ +69,5 @@
> > +      logins = [];
> > +      return;
> > +    }
> > +
> > +    logins.forEach(login => login.QueryInterface(Ci.nsILoginMetaInfo));
> 
> Does this actually change the value of the logins in the array? Did you mean
> to use map instead?
> 
> And if this doesn't change the value, is that QI needed?

I dug around and found out that .QueryInterface is actually a reflexive operator, so it's exposing the LoginMetaInfo interface of login without needing to return anything. (I also tried removing it and it's necessary.)

> 
> @@ +115,5 @@
> > +    }, true);
> > +
> > +    let img = document.createElement("img");
> > +    img.className = "icon";
> > +    img.setAttribute("src", login.hostname + "/favicon.ico");
> 
> This seems like a fine solution for now, but this won't necessarily work for
> all sites, since favicons can be included in <link> tags. Also, I don't know
> that we want to be making all these network requests in a chrome page... we
> can file a follow-up to see if there's any way to use the favicon cache, but
> it's annoying that there isn't an easy way to get at this in JS right now :/
> 
Filed bug 1103267.

> 
> @@ +185,5 @@
> > +    document.getElementById("details-header").setAttribute("link", login.hostname);
> > +
> > +    document.getElementById("detail-hostname").textContent = login.hostname;
> > +    document.getElementById("detail-realm").textContent = login.httpRealm;
> > +    document.getElementById("detail-username").textContent = login.username;
> 
> I wonder if there's any sort of basic HTML template support in the tree...
> 
Filed bug 1103262.

> ::: mobile/android/themes/core/aboutPasswords.css
> @@ +27,5 @@
> > +}
> > +
> > +.realm {
> > +  /* hostname is not localized, so keep the margin on the left side */
> > +  margin-left: .67em;
> 
> We should use -moz-margin-start in case we ever ship an RTL locale.
> 
> Also, why .67em? Seems like an odd value?

This is from the padding in about:addons - http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutAddons.css#16

Comment 18

[Chenxia Liu [:liuche]]

Attachment: Part 1: about:passwords

(just folded the nightly flags in here)

Comment 19

[:Margaret Leibovic]

Comment on attachment 8527091 [details] [diff] [review]
Part 1: about:passwords

Review of attachment 8527091 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

::: mobile/android/chrome/content/aboutPasswords.js
@@ +69,5 @@
> +    let list = document.getElementById("logins-list");
> +    list.innerHTML = "";
> +    logins.forEach(login => {
> +      let item = this._createItemForLogin(login);
> +      list.appendChild(item);

Depending on how many passwords you have, we might see performance problems with this, and it could be better to use a document fragment, rather than appending the items individually. But we can do this in a follow-up if it's a problem.

@@ +81,5 @@
> +    let list = document.getElementById("logins-list");
> +    list.removeAttribute("hidden");
> +  },
> +
> +  _onPopState: function (aEvent) {

Nit: Drop the a prefix.

@@ +153,5 @@
> +      }
> +    }
> +  },
> +
> +  observe: function (aSubject, aTopic, aData) {

Nit: Drop the a prefixes.

@@ +198,5 @@
> +
> +  _copyUsername: function() {
> +    let detailItem = document.querySelector("#login-details > .login-item");
> +    let login = detailItem.login;
> +    if (!login) {

When would there not be a login here? If this isn't expected, I would rather just throw an error than eat it here.

::: mobile/android/chrome/content/aboutPasswords.xhtml
@@ +12,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/. -->
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +  <head>
> +    <title>Passwords</title>

Use the localized entity here (I see there's already one in aboutPasswords.dtd :).

@@ +21,5 @@
> +    <script type="application/javascript;version=1.8" src="chrome://browser/content/aboutPasswords.js"></script>
> +  </head>
> +  <body dir="&locale.dir;">
> +    <div id="passwords-header" class="header">
> +      <div>Passwords</div>

Same here.

Comment 20

[Chenxia Liu [:liuche]]

Attachment: Part 1: about:passwords

Carrying over r+ and fixes.

Comment 21

[Chenxia Liu [:liuche]]

Attachment: Part 2: Tests

I'm having trouble registering a listener for the "load" event on a tab that gets opened by a "click" event - nesting eventListeners doesn't seem to work, but maybe I'm adding listeners to the wrong thing?

Comment 22

[:Margaret Leibovic]

Comment on attachment 8533966 [details] [diff] [review]
Part 2: Tests

Review of attachment 8533966 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good. I think I found the source of your problem.

::: mobile/android/base/tests/testAboutPasswords.java
@@ +13,5 @@
> +
> +    @Override
> +    public void testJavascript() throws Exception {
> +        super.testJavascript();
> +    }

Do you need to declare this method if it's just calling the super method?

::: mobile/android/base/tests/testAboutPasswords.js
@@ +54,5 @@
> +});
> +
> +add_test(function test_passwords_list() {
> +  // Test that the (single) entry added in setup is correct.
> +  let logins_list = browser.contentDocument.querySelector("#logins-list");

Nit: Use getElementById.

@@ +82,5 @@
> +  do_check_eq(username.textContent, LOGIN_FIELDS.username);
> +
> +  // Check that details page opens link to host.
> +  BrowserApp.deck.addEventListener("TabOpen", () => {
> +    let browsertab = BrowserApp.selectedTab.browser;

Oh, the problem here is that the selected browser hasn't changed by the time the "TabOpen" event has fired. You should define an event argument in this function, and then use event.target as the browser.

@@ +90,5 @@
> +      do_check_eq(BrowserApp.selectedTab.browser.currentURI.spec, LOGIN_FIELDS.hostname);
> +      Services.tm.mainThread.dispatch(run_next_test, Ci.nsIThread.DISPATCH_NORMAL);
> +    }, false);
> +
> +    BrowserApp.deck.removeEventListener("load", this, false);

I think you meant "TabOpen" here.

::: mobile/android/chrome/content/aboutPasswords.js
@@ +196,5 @@
>      loginDetails.removeAttribute("hidden");
> +
> +    let loadEvent = document.createEvent("Events");
> +    loadEvent.initEvent("PasswordsDetailsLoad", true, false);
> +    document.dispatchEvent(loadEvent);

I think you may want window.dispatchEvent here, or at least that would be consistent with about:addons:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutAddons.js#349

Comment 23

[Chenxia Liu [:liuche]]

Attachment: Part 2: Tests

So "load" doesn't normally bubble:
https://developer.mozilla.org/en-US/docs/Web/Events/load

Comment 24

[:Margaret Leibovic]

Comment on attachment 8534089 [details] [diff] [review]
Part 2: Tests

Review of attachment 8534089 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! r+ with comments addressed.

Let's get a try run and land this!

::: mobile/android/base/tests/testAboutPasswords.js
@@ +10,5 @@
> +Cu.import("resource://gre/modules/AndroidLog.jsm");
> +
> +function ok(passed, text) {
> +  do_report_result(passed, text, Components.stack.caller, false);
> +}

This is unused, you can get rid of it.

@@ +12,5 @@
> +function ok(passed, text) {
> +  do_report_result(passed, text, Components.stack.caller, false);
> +}
> +
> +const LOGIN_FIELDS = {

Nit: Maybe name this TEST_LOGIN to be more explicit about what it is?

@@ +63,5 @@
> +  let username = logins_list.querySelector(".username");
> +  do_check_eq(username.textContent, LOGIN_FIELDS.username);
> +
> +  let login_item = browser.contentDocument.querySelector("#logins-list > .login-item");
> +  browser.addEventListener("PasswordsDetailsLoad", function() {

Nit: Use an arrow function to be consistent with the other event listeners?

@@ +73,5 @@
> +  login_item.click();
> +});
> +
> +add_test(function test_passwords_details() {
> +  let login_details = browser.contentDocument.getElementById("login-details");

Global nit: Use camelCase for JS variables. The underscores in some function names are an xpchsell test-specific thing (maybe to help distinguish the magical function names that exist as part of the test context?), but we still use our normal camelCase styles for local variables/functions.

@@ +81,5 @@
> +  let username = login_details.querySelector(".username");
> +  do_check_eq(username.textContent, LOGIN_FIELDS.username);
> +
> +  // Check that details page opens link to host.
> +  BrowserApp.deck.addEventListener("TabOpen", (tabevent) => {

Nit: We usually just use `event` as the parameter name for event listeners.

@@ +83,5 @@
> +
> +  // Check that details page opens link to host.
> +  BrowserApp.deck.addEventListener("TabOpen", (tabevent) => {
> +    // Wait for tab to finish loading.
> +    let browser_target = tabevent.target;

I would just call this `browser`. Or maybe `selectedBrowser`, since it's the browser in the new selected tab.

@@ +87,5 @@
> +    let browser_target = tabevent.target;
> +    browser_target.addEventListener("load", () => {
> +      browser_target.removeEventListener("load", this, true);
> +
> +      do_check_eq(BrowserApp.selectedTab.browser.currentURI.spec, LOGIN_FIELDS.hostname);

I think you should just use our browser local variable here, instead of poking into the selected tab. But you could add another check to make sure the selected browser is indeed that browser.

Comment 25

[Chenxia Liu [:liuche]]

https://tbpl.mozilla.org/?tree=Try&rev=814b6115a3e0

Comment 26

[Chenxia Liu [:liuche]]

Ignore the previous try push, didn't manage to update my try patch correctly.

PSA, from gbrown's blog: https://gbrownmozilla.wordpress.com/2014/12/07/new-android-job-names-on-treeherder-update-your-try-pushes/

Comment 27

[Chenxia Liu [:liuche]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=841c8a906e8f

Comment 28

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/4f270128536a
https://hg.mozilla.org/integration/fx-team/rev/64f427666f49

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4f270128536a
https://hg.mozilla.org/mozilla-central/rev/64f427666f49

Comment 30

[Francesco Lodolo [:flod]]

Comment on attachment 8533964 [details] [diff] [review]
Part 1: about:passwords

Review of attachment 8533964 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/locales/en-US/chrome/aboutPasswords.properties
@@ +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/.
> +
> +passwordsDetails.age=Age: %S days

This is broken even for English (1 days), we need a proper plural form here.
Also a comment explaining what "Age" means in this context would definitely help.

Comment 31

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #30)
> This is broken even for English (1 days), we need a proper plural form here.
> Also a comment explaining what "Age" means in this context would definitely
> help.

Filed bug 1110693, didn't realize this one was fixed (fooled by the "part 1" in the commit message).

Comment 32

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_pw_actions_mock1.png

Here's a go at the UI we could trigger, standard Android style drop down of actions that would show when a user taps on the items.

Comment 33

[Chenxia Liu [:liuche]]

Comment on attachment 8542207 [details]
prev_pw_actions_mock1.png

Moving this to bug 1122225