Matthew N. [:MattN] Reporter
	Description • 9 years ago

1) Add key icons to the left column of usernames
2) Add a footer after a separator which opens the password manager

	Riadh Chtara[:rchtara]
	Comment 2 • 9 years ago

@ rfeeley: Hi, I started working on the adding the icons to the password autocomplete  And I was wondering  (with mattN) if the footer should match other footers : search box for ex

	Ryan Feeley [:rfeeley]
	Comment 3 • 9 years ago

I was looking at this design today and wondering if we even need the key icons now that the context menu does not have them.

What is the footer of the search box like? Can you post a screen shot?

	Riadh Chtara[:rchtara]
	Comment 5 • 9 years ago

Bug 1189618 - Update the style of the password manager autocomplete popup

	Mike Conley (:mconley) (:⚙️)
	Comment 6 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/64630/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64630/

	Mike Conley (:mconley) (:⚙️)
	Comment 7 • 8 years ago

Comment on attachment 8771511 [details]
Bug 1189618 [WIP] - Restyle password manager autocomplete popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64630/diff/1-2/

	Mike Conley (:mconley) (:⚙️)
	Comment 8 • 8 years ago

Comment on attachment 8771511 [details]
Bug 1189618 [WIP] - Restyle password manager autocomplete popup.

How does this approach look, Matt?

	Mike Conley (:mconley) (:⚙️)
	Comment 9 • 8 years ago

(Note that the styling is super-preliminary right now - only works on OS X).

	Mike Conley (:mconley) (:⚙️)
	Comment 10 • 8 years ago

Comment on attachment 8771511 [details]
Bug 1189618 [WIP] - Restyle password manager autocomplete popup.

Screenshot of what I've got so far on OS X: https://reviewboard.mozilla.org/r/64630/file/320/

	Ryan Feeley [:rfeeley]
	Comment 11 • 8 years ago

Spoke with Shorlander. Looks good. Copy the search box suggestions for style. If we can't open the Saved Logins into it's own native window, then opening them in about:preferences new tab is fine. Don't open a new browser window.

	Mike Conley (:mconley) (:⚙️)
	Comment 12 • 8 years ago

(In reply to Ryan Feeley [:rfeeley] from comment #11)
> If we can't open the Saved Logins into it's own native window, then
> opening them in about:preferences new tab is fine. Don't open a new browser
> window.

Just so we're clear, my initial patch _will_ open the old-school Saved Logins into its own native window. That's the same mechanism that the context "Fill Login" submenu uses.

I assume we want to eventually make it so that we open about:preferences to #security and show the Saved Logins... dialog inside it. That's currently not possible with how about:preferences works, but I think we can make it happen with some tweaks, but I think I might want to do that in a follow-up.

This assumes that opening about:preferences should be the eventual goal. Is that a correct assumption?

	Ryan Feeley [:rfeeley]
	Comment 13 • 8 years ago

The eventual goal is not to bring the user deeper into prefs, but to surface the passwords in new UI (a doorhanger, a toolbar panel and/or a sidebar). For today, the native window is much preferred to Saved Logins in about:preferences#security so this is great news to me.

	Matthew N. [:MattN] Reporter
	Comment 14 • 8 years ago

Ryan, last time we talked I think you weren't sure about whether UX wanted the icons. What's the current opinion?

Mike, I mentioned the other day to focus on the footer as I wasn't sure if UX wanted the icons at this time. I'm fine with this bug just being about adding the footer then another can be about the icons. I agree with Ryan that it doesn't make sense to send people to a preferences tab while they're in the middle of a task (logging in).

	Ryan Feeley [:rfeeley]
	Comment 15 • 8 years ago

I think Mike is going to be able to make the icons work, though may require a second SVG. The styling will match how search history in the search box has a history icon (except this will be a key). I think it's a nice signal for the user that it's just just text that will be filled but the password too.

	Mike Conley (:mconley) (:⚙️)
	Comment 16 • 8 years ago

Comment on attachment 8771511 [details]
Bug 1189618 [WIP] - Restyle password manager autocomplete popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64630/diff/2-3/

	Matthew N. [:MattN] Reporter
	Comment 17 • 8 years ago

Comment on attachment 8771511 [details]
Bug 1189618 [WIP] - Restyle password manager autocomplete popup.

https://reviewboard.mozilla.org/r/64630/#review62406

The big question (ignoring icon stuff) is how we want keyboard accessibilty to work, specifically up/down arrows and tab. I personally think it should be accessible by keyboard as it's an essential part of performing the task of logging in if the saved login doesn't show (e.g. if it's on a subdomain).
Having it keyboard accessible could lead to an alternative (possibly simpler) approach which would be to have the footer as the last result in getResultAt, getStyleAt, getCommentAt, etc. and use getStyleAt to style it differently. I think Riadh looked into both approaches but I'm not sure why his patch didn't go with that approach. Did you consider that approach?

::: browser/base/content/browser.xul:147
(Diff revision 3)
>  
>      <!-- for search and content formfill/pw manager -->
> -    <panel type="autocomplete" id="PopupAutoComplete" noautofocus="true" hidden="true"/>
> +    <panel type="autocomplete" id="PopupAutoComplete" noautofocus="true" hidden="true">
> +      <footer id="LoginAutoCompleteFooter">
> +        <button class="plain" label="&savedLogins.label;"
> +                oncommand="LoginHelper.openPasswordManager(window);"/>

This doesn't seem to be working for me btw.

::: browser/themes/osx/browser.css:1837
(Diff revision 3)
> +.autocomplete-treebody::-moz-tree-image(login) {
> +  padding-inline-start: 8px;
> +  padding-inline-end: 8px;
> +  list-style-image: url(chrome://browser/skin/permissions.svg#login-detailed);
> +}

Lets move the icon stuff to a separate bug (or at least separate commit) if it's not going to slow down this landing

::: browser/themes/osx/browser.css:1843
(Diff revision 3)
> +.autocomplete-treebody::-moz-tree-row(login) {
> +  height: 28px; /* <-- this totally doesn't work, unless I remove
> +                 * the login selector from -moz-tree-row, but then

The approach of the footer being a real row may help?

::: toolkit/components/satchel/AutoCompleteE10S.jsm:146
(Diff revision 3)
>    },
>  
>    // This function is used by the login manager, which uses a single message
>    // to fill in the autocomplete results. See
>    // "RemoteLogins:autoCompleteLogins".
> -  showPopupWithResults: function(browserWindow, rect, results) {
> +  showPopupWithResults: function(browserWindow, rect, results, forLogin=false) {

Nit: spaces around operators

	Mike Conley (:mconley) (:⚙️)
	Comment 19 • 8 years ago

I've filed bug 1294502 to combine the nsFormAutoComplete implementations. This is so that they can share the same AutoCompletePopup logic. Then I'm going to re-jig that AutoCompletePopup to use a richlist instead, and try to figure out a nice story for the footer in this spec.

	Tanvi Vyas[:tanvi]
	Comment 20 • 8 years ago

Part of the design for this is in bug 1307316 - https://bug1307316.bmoattachments.org/attachment.cgi?id=8799132

I'm concerned that the lock with the strikethrough and the key icons are confusing when placed next to each other.

	Matthew N. [:MattN] Reporter
	Comment 21 • 7 years ago

Mike, I'm unassigning you for now due to lack of activity but please feel free to take this again if you'd like.

	Mike Conley (:mconley) (:⚙️)
	Comment 22 • 7 years ago

Totally fair - thanks. :)

	Matthew N. [:MattN] Reporter
	Comment 23 • 7 years ago

We can use the same approach as the insecure password warning (bug 1217162) using a specially styled richlistitem.

	Pulsebot
	Comment 26 • 5 years ago

Pushed by prathikshaprasadsuman@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/271d947b5c4b
Add a 'View Saved Logins' footer to the password manager autocomplete popup. r=MattN