Add filtering on about:passwords

VERIFIED FIXED in Firefox 38

Status

()

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: liuche, Assigned: Margaret)

Tracking

(Blocks 1 bug)

Trunk
Firefox 37
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 verified)

Details

Attachments

(4 attachments, 3 obsolete attachments)

No description provided.
I'm thinking something like in about:config (but with the right resource resolutions), to the right of the "Passwords" title.
Flags: needinfo?(alam)
Agree. Let's chat about this in SF, I touch down Monday morning and will head over :)

Leaving NI on for now.
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.
Posted image prev_pw_mock2.png (obsolete) —
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 :).
Flags: needinfo?(alam) → needinfo?(liuche)
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 on attachment 8541298 [details]
prev_pw_mock2.png

Padding adjusted in _active mock.
Attachment #8541298 - Attachment is obsolete: true
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.
Assignee: nobody → margaret.leibovic
Flags: needinfo?(liuche)
(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 :)
/r/2093 - Bug 1101741 - Add filtering on about:passwords

Pull down this commit:

hg pull review -r a16651d6db1c8f0651f1b27360549765f10b86db
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
Attachment #8544938 - Flags: feedback?(mark.finkle)
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 on attachment 8544938 [details]
MozReview Request: bz://1101741/margaret

Oh, Reviewboard
Attachment #8544938 - Flags: review+
Attachment #8544938 - Flags: feedback?(mark.finkle)
Attachment #8544938 - Flags: feedback+
(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.
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).
Flags: needinfo?(alam)
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
Flags: needinfo?(alam) → needinfo?(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
Flags: needinfo?(margaret.leibovic) → needinfo?(alam)
That should be the same 'x' as the one in our URL bar too - that can be found in bug 1014848.
Flags: needinfo?(alam) → needinfo?(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.
Attachment #8546257 - Flags: review?(mark.finkle)
/r/2237 - Bug 1101741 - Add filtering on about:passwords. r=mfinkle

Pull down this commit:

hg pull review -r d1d71f255db3b15896f22b71f71ec2bd9455882a
Flags: needinfo?(margaret.leibovic)
Attachment #8546257 - Flags: review?(mark.finkle) → review+
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
(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.
https://hg.mozilla.org/mozilla-central/rev/0f784b15c6c7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
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?
Status: RESOLVED → VERIFIED
Flags: needinfo?(margaret.leibovic)
(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?
Depends on: 1123431
(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.
Flags: needinfo?(margaret.leibovic)
No longer depends on: password-ui
Attachment #8544938 - Attachment is obsolete: true
Attachment #8546257 - Attachment is obsolete: true
Attachment #8618653 - Flags: review+
Attachment #8618654 - Flags: review+
You need to log in before you can comment on or make changes to this bug.