1101741 - Add filtering on about:passwords

Status: VERIFIED FIXED

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

Comment 1

[Chenxia Liu [:liuche]]

I'm thinking something like in about:config (but with the right resource resolutions), to the right of the "Passwords" title.

Comment 2

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

Agree. Let's chat about this in SF, I touch down Monday morning and will head over :)

Leaving NI on for now.

Comment 3

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

From our conversation, we talked about adding a "find" bar.

This is basically "search", just actively filtering the list beneath it. 

Mock ups to follow.

Comment 4

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

Attachment: prev_pw_mock2.png

Mockups!

Magnifying glass icon is the same as in our current toolbar, and the + is the same as the "add new tab" in the tablet UI :).

Comment 5

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

Attachment: specs_pw_actions_padding.png

Simple specs

Comment 6

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

Attachment: prev_pw_mock2_active2.png

For the active state, we should leverage something similar to our find-in-page UX/UI. How about something like this that comes up when the user hits the magnifying glass??

Comment 7

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

Comment on attachment 8541298 [details]
prev_pw_mock2.png

Padding adjusted in _active mock.

Comment 8

[:Margaret Leibovic]

Chenxia is PTO for a while, so I can take a stab at this in the meantime.

I wonder if doing something similar to the find bar might be confusing, since this is part of the page, whereas the find bar is a part of the browser UI separate from the page. Maybe we should try to make a search bar expand from where the magnifying glass is at the top of the page? I guess the "Passwords" header makes this a bit awkward.

In any case, we don't have much room to work with, so I can try this current approach and see how we feel.

Comment 9

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

(In reply to :Margaret Leibovic from comment #8)
> Chenxia is PTO for a while, so I can take a stab at this in the meantime.
> 
> I wonder if doing something similar to the find bar might be confusing,
> since this is part of the page, whereas the find bar is a part of the
> browser UI separate from the page. Maybe we should try to make a search bar
> expand from where the magnifying glass is at the top of the page? I guess
> the "Passwords" header makes this a bit awkward.

Yeah, I have a design for that too. The page header being there is one issue, but another is also having two input bars in the same vicinity... Since our "filtering" works similar to a "find" action, I thought we could give this alternative a try.  

> In any case, we don't have much room to work with, so I can try this current
> approach and see how we feel.

Let's test it out, I'm prepared to go with alternatives if this seems weird :)

Comment 10

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1101741/margaret

Comment 11

[:Margaret Leibovic]

/r/2093 - Bug 1101741 - Add filtering on about:passwords

Pull down this commit:

hg pull review -r a16651d6db1c8f0651f1b27360549765f10b86db

Comment 12

[:Margaret Leibovic]

Comment on attachment 8544938 [details]
MozReview Request: bz://1101741/margaret

This needs some visual love (I need icons), but it's functional. I took the filter logic from desktop's passwordManager.js, but they use a XUL tree for the UI, so there wasn't much I could copy there.

I imagine this approach might not perform well if we have a very large number of passwords, but I felt like re-creating a sub-set of the login list would probably be faster than iterating through all logins on every input change. I think a solution in the future would probably be to just create elements for the first 20 items or so, then add more as the user scrolls.

Here's a test build:
http://people.mozilla.org/~mleibovic/passwords.apk

Comment 13

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

https://reviewboard.mozilla.org/r/2091/#review1321

::: mobile/android/chrome/content/aboutPasswords.js
(Diff revision 1)
>      Services.obs.addObserver(this, "passwordmgr-storage-changed", false);

Side note: I had about:passwords open and in a different tab I added some passwords via the doorhanger. When going back to about:passwords the list was not updated. I kinda expected that to happen via "passwordmgr-storage-changed".

::: mobile/android/chrome/content/aboutPasswords.js
(Diff revision 1)
> +    while (list.firstChild) {

It's faster to clear the list like this:
list.innerHTML = "";
Instead of removing each one. Kinda defeats using the fragment.

I think we need to iterate on the behavior a little too. Like, when closing the find bar should we show the unfiltered list again?

Nice start!

Comment 14

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

Comment on attachment 8544938 [details]
MozReview Request: bz://1101741/margaret

Oh, Reviewboard

Comment 15

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #13)

> ::: mobile/android/chrome/content/aboutPasswords.js
> (Diff revision 1)
> > +    while (list.firstChild) {
> 
> It's faster to clear the list like this:
> list.innerHTML = "";
> Instead of removing each one. Kinda defeats using the fragment.

I did some internet research to come to this decision, and the internet seemed to agree that iterating and calling removeChild is much faster than innerHTML. However, But I agree that iterating through the nodes to remove them defeats the purpose of the fragment. I also just looked at the implementation for jQuery's 'empty' method, and they use textContent = "", so maybe that is actually fine:
https://github.com/jquery/jquery/blob/master/src/manipulation.js#L380

Or maybe we instead we should just blow away the whole list node and create a new one.

> I think we need to iterate on the behavior a little too. Like, when closing
> the find bar should we show the unfiltered list again?

Yes, good call.

Comment 16

[:Margaret Leibovic]

antlam, can I get assets for the search icon and the close icon? There's already a search icon in the tree for about:config, but it looks blurry on my phone, so I think we need multiple icon sizes (I can just update the icon in about:config with whatever new assets you give me).

Comment 17

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

Sorry - bug 1019045 is the one being referred to in comment 4 (where the icon's are!). Let me know if those don't work for you

Comment 18

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #17)
> Sorry - bug 1019045 is the one being referred to in comment 4 (where the
> icon's are!). Let me know if those don't work for you

Oh, okay, I'll just need to make copies of those where they can be accessed by a content page.

I still need an icon for the find bar at the bottom. The one that's in the tree for our normal find bar isn't the correct color:
http://hg.mozilla.org/mozilla-central/raw-file/d480b3542cc2/mobile/android/base/resources/drawable-mdpi/find_close.png

Comment 19

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

That should be the same 'x' as the one in our URL bar too - that can be found in bug 1014848.

Comment 20

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/2091/#review1429

> Side note: I had about:passwords open and in a different tab I added some passwords via the doorhanger. When going back to about:passwords the list was not updated. I kinda expected that to happen via "passwordmgr-storage-changed".

Looks like this was caused by a bug in the change I made to the observer logic.

Comment 21

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1101741/margaret

Comment 22

[:Margaret Leibovic]

/r/2237 - Bug 1101741 - Add filtering on about:passwords. r=mfinkle

Pull down this commit:

hg pull review -r d1d71f255db3b15896f22b71f71ec2bd9455882a

Comment 23

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

https://reviewboard.mozilla.org/r/2235/#review1437

::: mobile/android/themes/core/aboutPasswords.css
(Diff revision 1)
> +  background: @color_about_background@;

I didn't realize we still used these pseudo-variables for colors. Do we use various color schemes? We might be able to use real moz-var in CSS now. No big deal for now.

Nice work

Comment 24

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #23)
> https://reviewboard.mozilla.org/r/2235/#review1437
> 
> ::: mobile/android/themes/core/aboutPasswords.css
> (Diff revision 1)
> > +  background: @color_about_background@;
> 
> I didn't realize we still used these pseudo-variables for colors. Do we use
> various color schemes? We might be able to use real moz-var in CSS now. No
> big deal for now.

I think we just use this as a way to keep colors consistent across files (similar to colors.xml for Java). We can file a separate bug to investigate updating to use CSS variables. I wonder what desktop does nowadays.

Comment 25

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/0f784b15c6c7

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f784b15c6c7

Comment 27

[Cristina Madaras, QA [:CristinaM]]

The magnifying glass icon is displayed to the right of the "Passwords" title and the search works fine so I will mark this as Verified fixed.

Suggestion: The search field is case sensitive, so I'm asking if the auto-capitalize attribute can be turned off like in the search filed from about:config page?

Comment 28

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

(In reply to Cristina Madaras, QA [:CristinaM] from comment #27)

> Suggestion: The search field is case sensitive, so I'm asking if the
> auto-capitalize attribute can be turned off like in the search filed from
> about:config page?

Yes, the auto-capitalization should be turned off. I think we should also make the search case insensitive.

File a new bug?

Comment 29

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #28)
> (In reply to Cristina Madaras, QA [:CristinaM] from comment #27)
> 
> > Suggestion: The search field is case sensitive, so I'm asking if the
> > auto-capitalize attribute can be turned off like in the search filed from
> > about:config page?
> 
> Yes, the auto-capitalization should be turned off. I think we should also
> make the search case insensitive.
> 
> File a new bug?

I filed bug 1123431.

Comment 32

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1101741 - Add filtering on about:passwords. r=mfinkle

Comment 33

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1101741 - Add filtering on about:passwords