chromeos: Make WebUI auth dialog handle Enter and Escape.

chromeos: Make WebUI auth dialog handle Enter and Escape.

This adds a form to the HTTP auth dialog to make Enter work
and adds a handler to listen for Escape.  It also improves
many small aspects of the dialog's appearance (more padding
around the edges and between items; narrower, centered input
fields; disabled text selection; <label> tags).

BUG=chromium:89833
TEST=manual: went to a page requiring auth; confirmed that enter submits and escape cancels, and that clicking the buttons still works too

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97959

[Daniel Erat, 2011-08-22 22:14:17]

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
File chrome/browser/resources/http_auth.html (right):

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:68: document.onkeydown = handleKeyDown;
Should I be setting the body element's handler instead, or does it not really
matter?

[xiyuan, 2011-08-22 22:29:05]

LGTM with nits

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
File chrome/browser/resources/http_auth.html (right):

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:55: if (event.keyCode == 27) {
nit: use 'e' instead of 'event'

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:55: if (event.keyCode == 27) {
nit: add a comment of what key this is.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:57: return false;
nit: you probably don't need to return anything.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:59: return true;
same here.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:68: document.onkeydown = handleKeyDown;
On 2011/08/22 22:14:18, Daniel Erat wrote:
> Should I be setting the body element's handler instead, or does it not really
> matter?

Think it's fine to use document.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:68: document.onkeydown = handleKeyDown;
nit: suggest to use document.addEventListener('keydown', handleKeyDown)

[arv (Not doing code reviews), 2011-08-23 00:14:59]

LGTM with nits

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
File chrome/browser/resources/http_auth.html (right):

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:46: chrome.send('DialogClose', ['']);
chrome.send('DialogClose')

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:51: return false;
e.preventDefault()

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:55: if (event.keyCode == 27) {
// Esc

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:57: return false;
e.preventDefault();

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:82: <form id="form">
Add this to your css

form {
  margin: 0;
}

[Daniel Erat, 2011-08-23 00:48:03]

Thanks, another look?  I made a bunch of changes to the dialog's appearance. 
You can see the updated version at
http://www/~derat/no_crawl/20110822/screenshot-20110822-173703.png.

There's still a previously-existing issue with the background selection color in
the input fields being white.  The text is also white, so it becomes invisible
when highlighted.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
File chrome/browser/resources/http_auth.html (right):

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:46: chrome.send('DialogClose', ['']);
On 2011/08/23 00:14:59, arv wrote:
> chrome.send('DialogClose')
> 

Done.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:51: return false;
On 2011/08/23 00:14:59, arv wrote:
> e.preventDefault()

Done.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:55: if (event.keyCode == 27) {
On 2011/08/22 22:29:05, xiyuan wrote:
> nit: use 'e' instead of 'event'

Done.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:55: if (event.keyCode == 27) {
On 2011/08/22 22:29:05, xiyuan wrote:
> nit: add a comment of what key this is.

Done.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:57: return false;
On 2011/08/23 00:14:59, arv wrote:
> e.preventDefault();

Done.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:68: document.onkeydown = handleKeyDown;
On 2011/08/22 22:29:05, xiyuan wrote:
> nit: suggest to use document.addEventListener('keydown', handleKeyDown)

Done.

http://codereview.chromium.org/7709013/diff/1/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:82: <form id="form">
On 2011/08/23 00:14:59, arv wrote:
> Add this to your css
> 
> form {
>   margin: 0;
> }

Done.

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:26: 
This isn't RTL-friendly, but with the exception of the alignment of the buttons
div, neither is the rest of the dialog. :-/

http://codereview.chromium.org/7709013/diff/4/chrome/browser/ui/login/login_p...
File chrome/browser/ui/login/login_prompt_ui.cc (right):

http://codereview.chromium.org/7709013/diff/4/chrome/browser/ui/login/login_p...
chrome/browser/ui/login/login_prompt_ui.cc:153: static const int kDialogHeight =
160;
This is gross, but I think that fixing it would be outside the scope of this
change. :-(

[arv (Not doing code reviews), 2011-08-23 01:25:33]

Some more nits

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
File chrome/browser/resources/http_auth.html (right):

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:6: margin: 10px 10px 0px 10px;
s/0px/0/

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:26: text-align: right;
RTL

text-align: end;

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:27: padding-right: 5px;
-webkit-padding-end: 5px;

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:36: text-align: right;
text-align: end;

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:40: text-align: left;
with text-alugn: end this can be removed

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:44: <script type="text/javascript">
remove type

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:106: <td id="usernameLabelCell">
username-label-cell

https://sites.google.com/a/chromium.org/dev/developers/web-development-style-...

[Daniel Erat, 2011-08-23 21:45:09]

Thanks!

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
File chrome/browser/resources/http_auth.html (right):

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:6: margin: 10px 10px 0px 10px;
On 2011/08/23 01:25:33, arv wrote:
> s/0px/0/

Done.

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:26: text-align: right;
On 2011/08/23 01:25:33, arv wrote:
> RTL
> 
> text-align: end;

Done.

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:27: padding-right: 5px;
On 2011/08/23 01:25:33, arv wrote:
> -webkit-padding-end: 5px;

Done.

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:36: text-align: right;
On 2011/08/23 01:25:33, arv wrote:
> text-align: end;

Done.

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:40: text-align: left;
On 2011/08/23 01:25:33, arv wrote:
> with text-alugn: end this can be removed

Done.

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:44: <script type="text/javascript">
On 2011/08/23 01:25:33, arv wrote:
> remove type 

Done.

http://codereview.chromium.org/7709013/diff/4/chrome/browser/resources/http_a...
chrome/browser/resources/http_auth.html:106: <td id="usernameLabelCell">
On 2011/08/23 01:25:33, arv wrote:
> username-label-cell
> 
>
https://sites.google.com/a/chromium.org/dev/developers/web-development-style-...

Done.

[Daniel Erat, 2011-08-23 22:15:44]

FYI Kuan: I'm rolling back your changes from r97651; Arv says that onclick()
calls should be avoided.  (I started this change locally yesterday before your
change went in and wasn't aware that they conflicted until I synced today.)