Also, I'd indent "Password:" by about 6px.

Layout guidance: https://raw.github.com/gnome-design-team/gnome-mockups/master/system-lock-login-boot/login-dissect.png

When you redesign it like this it would be good if you could also make it so that by default it could start with the password entry form of the last person loggen in visible and focused, so that when you start up you can just type in the password, rather than having to press enter and wait half a second whilst it does an animation, which, as pointed out here https://bugzilla.gnome.org/show_bug.cgi?id=685304 is slightly glitchy. I think combining the current design for the lock screen (under the visor with the time and notifications) with elements of the lightdm design would be good. As with your mockup, a vertical list of the users, that can be navigated using the up and down buttons, but without the need to press enter - the central one (initially focused on the last user) would be expanded as with the bottom part of your mockup, and above and below it the other users (including not listed), which would expand as part of the transition of moving to the centre of the screen, when you navigate to their user with the arrows. As well as being particularly useful for people who use one main account, currently the design doesn't look great if you have more than one user, and this could allow a more consistent design and a more smooth and lightweight feel to the logging in experience.

Created attachment 248861 [details] [review]
loginDialog: pre-allocate prompt message height

Right now things jump around if a message comes in.
This commit makes sure there's room for a message to start.

Created attachment 248862 [details] [review]
loginDialog: drop padding between buttons and entry

Now that we preallocate space for the prompt message there is
a lot of loose space between the entry and the buttons.

This commit helps tighten things up by getting rid
of the large top padding set above the login buttons.

Created attachment 248863 [details] [review]
loginDialog: make prompt entry wider

This makes it match mock ups better and looks more visually
pleasing.

Created attachment 248864 [details] [review]
loginDialog: force user list and prompt to be the same width

Created attachment 248865 [details] [review]
loginDialog: handle typing at screenshield

If the screenshield is up and the user starts typing, we need to
keep track of it and replay it later, otherwise what they type
will be ignored.

This is useful when the user has disable-user-list set to TRUE,
and will also be useful to bring feature parity between the
login screen and unlock screen.

Created attachment 248866 [details] [review]
loginDialog: conditionalize out gdm specific bits

The unlock dialog and login dialog are very similar and should probably
be derived from a common codebase.

This commit moves toward that goal by passing in the session mode to the
login dialog constructor, and then checking if the mode is 'gdm' before
doing login dialog specific stuff.

A subsequent commit will add in unlock-dialog specific bits.

Created attachment 248867 [details] [review]
loginDialog: add unlock-dialog code paths

Now that we've conditionalized out the gdm bits, we can conditionalize
in the unlock-dialog bits.

Created attachment 248868 [details] [review]
sessionMode: drop unlock dialog

Now that the login dialog can do the bits that the unlock dialog
could do before we can drop the unlock dialog completely.

Review of attachment 248861 [details] [review]:

Hm, how will this work with multiline messages? Should we preallocate two lines worth of text?

(In reply to comment #12)
> Review of attachment 248861 [details] [review]:
> 
> Hm, how will this work with multiline messages? Should we preallocate two lines
> worth of text?

Multiline messages a pretty rare, and the additional whitespace wouldn't look very nice.  I think it's better to just grow for that corner case.

Review of attachment 248861 [details] [review]:

OK, then.

Review of attachment 248862 [details] [review]:

OK.

Review of attachment 248863 [details] [review]:

OK.

Review of attachment 248864 [details] [review]:

I'd personally use a container that allocates the maximum width to both, but OK.

Attachment 248861 [details] pushed as 6e00b6e - loginDialog: pre-allocate prompt message height
Attachment 248862 [details] pushed as bd5c04b - loginDialog: drop padding between buttons and entry
Attachment 248863 [details] pushed as 2431b8e - loginDialog: make prompt entry wider

So we're going to consolidate the login dialog and unlock dialog in a different way.
marking some patches obsolete.

Created attachment 249311 [details] [review]
loginDialog: handle typing at screenshield

If the screenshield is up and the user starts typing, we need to
keep track of it and replay it later, otherwise what they type
will be ignored.

This is useful when the user has disable-user-list set to TRUE,
and will also be useful to bring feature parity between the
login screen and unlock screen.

Created attachment 249312 [details] [review]
loginDialog: factor auth prompt code out to utils

Right now there is a lot of duplicated code between the unlock
dialog and the login dialog.

This commit moves the login dialog's auth prompt to a separate
class, so that it can (in a subsequent commit) be used by the
unlock dialog.

Created attachment 249313 [details] [review]
unlockDialog: Use GdmUtil.AuthPrompt instead of ModalDialog

commit ea02380c1524c28e6538ffedb789a12c298742ab made the login
screen stop using ModalDialog. It makes sense for the unlock
code to also stop using ModalDialog, too (for similar reasons).

Now that the login screen's auth prompt code has been separated
out, the unlock dialog can use it to get the buttons and spinners
etc, that it was previously getting from ModalDialog.

This commit drops the ModalDialog usage in the unlock dialog, and
makes the unlock dialog use GdmUtil.AuthPrompt instead.

Review of attachment 249311 [details] [review]:

I guess I don't understand this. If the screen shield is up, and the user types, we'll raise the shield but key presses should be properly sent to the entry.

What is this solving?

(In reply to comment #23)
> Review of attachment 249311 [details] [review]:
> 
> I guess I don't understand this. If the screen shield is up, and the user
> types, we'll raise the shield but key presses should be properly sent to the
> entry.
> 
> What is this solving?

It's just bringing feature parity between the unlock dialog and the login dialog in preparation for them to share code.

Use case is user has user list disabled and types their username and hits enter really fast before the shield comes up.

I don't see how that's broken. As soon as the first character is typed, we create the dialog, raise the shield, call addCharacter, and give the entry key focus.

All subsequent characters should be handled by the entry, not the shield.

Review of attachment 249312 [details] [review]:

::: js/gdm/util.js
@@ +474,3 @@
+        params = Params.parse(params,
+                              { style_class: 'login-dialog-prompt-layout',
+                                vertical: true }, true);

ugh, not a big fan of using Params like this.

Just have the login dialog code set visible to false afterwards.

@@ +478,3 @@
+        this.actor.connect('button-press-event',
+                           Lang.bind(this, function(actor, event) {
+                               if (event.get_key_symbol() == Clutter.KEY_Escape) {

Well, this is old code, sure, but there's no way get_key_symbol() and button-press-event will match up...

@@ +542,3 @@
+    },
+
+    setActorInDefaultButtonWell: function(actor, immediately) {

I don't see you using "immediately" here with anything but true.

@@ +554,3 @@
+        let isWorkSpinner;
+        if (actor == this._spinner.actor)
+            isWorkSpinner = true;

yuck.

@@ +652,3 @@
+    },
+
+    resetButtons: function(cancelLabel, nextLabel) {

Do we really need support for a separate cancel label?

@@ +659,3 @@
+
+        this._buttonBox.visible = true;
+        this._buttonBox.remove_all_children();

Why are we rebuilding the buttons instead of just pre-making two and changing the labels around? That would also mean that resetButtons could go away, and be replaced with this._authPrompt.cancelButton.hide();, and this._authPrompt.nextButton.label = _("Sign In");

@@ +694,3 @@
+                                     this.emit('next');
+                                 }));
+        this._nextButton.add_style_pseudo_class('default');

pseudo class? this ain't a psuedo class.

@@ +717,3 @@
+
+    _updateNextButtonSensitivity: function(sensitive) {
+        if (this._nextButton) {

I'd like to ensure that we always have a next button, we just show / hide it.

Review of attachment 249313 [details] [review]:

Looks OK.

::: js/ui/unlockDialog.js
@@ +32,2 @@
     _init: function(parentActor) {
+        this.actor = new St.Widget({ accessible_role: Atk.Role.WINDOW,

um? This is meant to be Role.DIALOG, right?

@@ +42,3 @@
         this._user = this._userManager.get_user(this._userName);
 
         this._failCounter = 0;

This is unused.

(In reply to comment #23)
> I guess I don't understand this. If the screen shield is up, and the user
> types, we'll raise the shield but key presses should be properly sent to the
> entry.
The dialog doesn't listen for activate on the entry until the user manager loads, which is an asynchronous operation. Granted,

1) the race is small. the user manager loads quickly, and we only show the shield in weird corner cases anyway that probably happen after the user manager is loaded
2) We could restructure the code so we didn't wait for the user manager in the disable-user-list case.

But, the main point of the commit is to just have the code there and waiting, so it can get moved into AuthPrompt in the falling commit, so that it's ready for the unlock code to make use of in the commit after that. The unlock code does need it after all since the PAM query could be slow to come in, and it's easier to read the diffs if you can see how the code is moving around.

If it's bothers you, though, I can just drop that commit, and add that stuff in the unlock commit at the end.

Yeah, I think it would be better to add it at the end. The unlock dialog already has code for an initial answer, and I'd be happier if you took it from there and put it into the auth dialog.

(In reply to comment #26)
> Review of attachment 249312 [details] [review]:
> 
> ::: js/gdm/util.js
> @@ +474,3 @@
> +        params = Params.parse(params,
> +                              { style_class: 'login-dialog-prompt-layout',
> +                                vertical: true }, true);
> 
> ugh, not a big fan of using Params like this.
> 
> Just have the login dialog code set visible to false afterwards.

sure

> 
> @@ +478,3 @@
> +        this.actor.connect('button-press-event',
> +                           Lang.bind(this, function(actor, event) {
> +                               if (event.get_key_symbol() ==
> Clutter.KEY_Escape) {
> 
> Well, this is old code, sure, but there's no way get_key_symbol() and
> button-press-event will match up...
okay i'll fix that in a preliminary commit.

> 
> @@ +542,3 @@
> +    },
> +
> +    setActorInDefaultButtonWell: function(actor, immediately) {
> 
> I don't see you using "immediately" here with anything but true.
It's supposed to be false in startSpinning, i'll fix it.
 
> @@ +554,3 @@
> +        let isWorkSpinner;
> +        if (actor == this._spinner.actor)
> +            isWorkSpinner = true;
> 
> yuck.
well the function was originally called setDefaultButtonWellMode before it was moved over and one of those modes is SESSION_MENU_BUTTON.  I thought about keeping it that way and moving the sessionMenu button handling over at the same time.  I decided against that approach since the unlock dialog will never use a session menu button.

> 
> @@ +652,3 @@
> +    },
> +
> +    resetButtons: function(cancelLabel, nextLabel) {
> 
> Do we really need support for a separate cancel label?

I originally the argument as a canCancel boolean, but unmarked booleans make for hard to read code. I could go to Params, or just leave it.
> 
> @@ +659,3 @@
> +
> +        this._buttonBox.visible = true;
> +        this._buttonBox.remove_all_children();
> 
> Why are we rebuilding the buttons instead of just pre-making two and changing
> the labels around? 

It's just the _prepareDialog function moved over to util.js with a new name.  We rebuilt the buttons before, so this code just maintains that behavior.  I can change it, I don't feel strongly.

> 
> @@ +694,3 @@
> +                                     this.emit('next');
> +                                 }));
> +        this._nextButton.add_style_pseudo_class('default');
> 
> pseudo class? this ain't a psuedo class.

sure it is:

[rstrode@dhcp-191-61] (/srv/sources/gnome/gnome-shell/data/theme) <05:00 PM>
$ git grep default |grep login
gnome-shell.css:.login-dialog .modal-dialog-button:default {
gnome-shell.css:.login-dialog .modal-dialog-button:default:focus {
gnome-shell.css:.login-dialog .modal-dialog-button:default:hover {
gnome-shell.css:.login-dialog .modal-dialog-button:default:active,
gnome-shell.css:.login-dialog .modal-dialog-button:default:pressed {
gnome-shell.css:.login-dialog .modal-dialog-button:default:insensitive {

or are you asserting it doesn't satisfy your notion of what a pseudoclass should be? If so, that's debatable, but really orthogonal to this bug.
 
> @@ +717,3 @@
> +
> +    _updateNextButtonSensitivity: function(sensitive) {
> +        if (this._nextButton) {
> 
> I'd like to ensure that we always have a next button, we just show / hide it.
ok

Hi,
> ::: js/ui/unlockDialog.js
> @@ +32,2 @@
>      _init: function(parentActor) {
> +        this.actor = new St.Widget({ accessible_role: Atk.Role.WINDOW,
> 
> um? This is meant to be Role.DIALOG, right?

Well, Atk.Role.DIALOG has this description:

| A top level window with title bar and a border

which ModalDialog almost fits, sort of, but the unlock dialog doesn't really at all.  Atk.Role.WINDOW has this description:

| A top level window with no title or border.

which seems like a better fit, so I changed it when moving it over from ModalDialog.  I also did the same for LoginDialog a while back.  I don't really care which one we use to be honest. I doubt the a11y tools make use of the destinction (but don't really know).  I think we should be consistent though.

> 
> @@ +42,3 @@
>          this._user = this._userManager.get_user(this._userName);
> 
>          this._failCounter = 0;
> 
> This is unused.

Created attachment 249517 [details] [review]
unlockDialog: drop unused variable

Created attachment 249518 [details] [review]
loginDialog: s/button-press-event/key-press-event/

A bug got introduced when moving the login dialog away from modal
dialog, such that it listens for escape key presses in a mouse
event handler instead of a keyboard event handler.

This commit fixes that code to correctly listen for key-press-event
instead of button-press-event.

Created attachment 249519 [details] [review]
loginDialog: factor auth prompt code out to utils

Right now there is a lot of duplicated code between the unlock
dialog and the login dialog.

This commit moves the login dialog's auth prompt to a separate
class, so that it can (in a subsequent commit) be used by the
unlock dialog.

Created attachment 249520 [details] [review]
unlockDialog: Use GdmUtil.AuthPrompt instead of ModalDialog

commit ea02380c1524c28e6538ffedb789a12c298742ab made the login
screen stop using ModalDialog. It makes sense for the unlock
code to also stop using ModalDialog, too (for similar reasons).

Now that the login screen's auth prompt code has been separated
out, the unlock dialog can use it to get the buttons and spinners
etc, that it was previously getting from ModalDialog.

This commit drops the ModalDialog usage in the unlock dialog, and
makes the unlock dialog use GdmUtil.AuthPrompt instead.

Review of attachment 249518 [details] [review]:

Yes.

Review of attachment 249517 [details] [review]:

OK.

Review of attachment 249519 [details] [review]:

::: js/gdm/util.js
@@ +471,3 @@
+
+    _init: function(params) {
+        this.actor = new St.BoxLayout(params);

I was imagining this.actor = new St.BoxLayout({ style_class: ..., vertical: true });

and then set extra properties from whatever uses AuthPrompt.

@@ +635,3 @@
+    },
+
+    stopSpinning: function() {

Seems to never be used?

Though now that I see this, I think I like isSpinner better. Sorry :(

Review of attachment 249520 [details] [review]:

OK, assuming everything else is.

Attachment 249517 [details] pushed as 09d34a2 - unlockDialog: drop unused variable
Attachment 249518 [details] pushed as 3c31908 - loginDialog: s/button-press-event/key-press-event/

Created attachment 249553 [details] [review]
loginDialog: factor auth prompt code out to utils

Right now there is a lot of duplicated code between the unlock
dialog and the login dialog.

This commit moves the login dialog's auth prompt to a separate
class, so that it can (in a subsequent commit) be used by the
unlock dialog.

Created attachment 249554 [details] [review]
unlockDialog: Use GdmUtil.AuthPrompt instead of ModalDialog

commit ea02380c1524c28e6538ffedb789a12c298742ab made the login
screen stop using ModalDialog. It makes sense for the unlock
code to also stop using ModalDialog, too (for similar reasons).

Now that the login screen's auth prompt code has been separated
out, the unlock dialog can use it to get the buttons and spinners
etc, that it was previously getting from ModalDialog.

This commit drops the ModalDialog usage in the unlock dialog, and
makes the unlock dialog use GdmUtil.AuthPrompt instead.

Review of attachment 249553 [details] [review]:

OK.

Review of attachment 249554 [details] [review]:

::: js/gdm/util.js
@@ +497,3 @@
         this._entry = new St.Entry({ style_class: 'login-dialog-prompt-entry',
                                      can_focus: true });
+        ShellEntry.addContextMenu(this._entry, { isPassword: true });

Separate commit please.

@@ +664,2 @@
     setQuestion: function(question) {
+        if (!this._initialAnswer) {

Put all this initialAnswer stuff in another patch so I can review it separately, please.

Comment on attachment 249553 [details] [review]
loginDialog: factor auth prompt code out to utils

Attachment 249553 [details] pushed as d715110 - loginDialog: factor auth prompt code out to utils

Created attachment 249556 [details] [review]
util: add shell entry menu to auth prompt

This brings us parity with the unlock dialog, and is a prerequisite
for eventually moving the unlock dialog over to using the auth
prompt.

Created attachment 249557 [details] [review]
util: save answers typed early on in the auth prompt

The unlock dialog has a feature where a user can start
typing their password right away, before the dialog is fully
shown.

This commit copies the bones of that functionality over to
GdmUtil.AuthPrompt, so we can subsequently move the unlock
dialog code over to using it.

Created attachment 249558 [details] [review]
unlockDialog: Use GdmUtil.AuthPrompt instead of ModalDialog

commit ea02380c1524c28e6538ffedb789a12c298742ab made the login
screen stop using ModalDialog. It makes sense for the unlock
code to also stop using ModalDialog, too (for similar reasons).

Now that the login screen's auth prompt code has been separated
out, the unlock dialog can use it to get the buttons and spinners
etc, that it was previously getting from ModalDialog.

This commit drops the ModalDialog usage in the unlock dialog, and
makes the unlock dialog use GdmUtil.AuthPrompt instead.

Review of attachment 249556 [details] [review]:

OK.

Comment on attachment 249556 [details] [review]
util: add shell entry menu to auth prompt

Attachment 249556 [details] pushed as be4f259 - util: add shell entry menu to auth prompt

Created attachment 249567 [details] [review]
unlockDialog: Use GdmUtil.AuthPrompt instead of ModalDialog

commit ea02380c1524c28e6538ffedb789a12c298742ab made the login
screen stop using ModalDialog. It makes sense for the unlock
code to also stop using ModalDialog, too (for similar reasons).

Now that the login screen's auth prompt code has been separated
out, the unlock dialog can use it to get the buttons and spinners
etc, that it was previously getting from ModalDialog.

This commit drops the ModalDialog usage in the unlock dialog, and
makes the unlock dialog use GdmUtil.AuthPrompt instead.

Review of attachment 249567 [details] [review]:

OK.

Attachment 249567 [details] pushed as b4567ac - unlockDialog: Use GdmUtil.AuthPrompt instead of ModalDialog