Fix ChromeOS users list multiple X problem.

Fix ChromeOS users list multiple X problem.

- Use 'raw-button' for X button so that it fix the X image;
- Add a wrapper block to email and name text that fills up the space
  between icon and X button and has a max-width to avoid push X button
  out of view;

BUG=chromium-os:11220, chromium-os:10944
TEST=Verify fix for chromium-os:11220

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

[James Hawkins, 2011-01-25 23:10:33]

LGTM with nit.

http://codereview.chromium.org/6380015/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/6380015/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/chromeos_accounts_user_list.js:186:
removeButton.className = 'raw-button remove-user-button';
nit: Please use two classList.add(X) instead.

[xiyuan, 2011-01-25 23:27:29]

http://codereview.chromium.org/6380015/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/6380015/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/chromeos_accounts_user_list.js:186:
removeButton.className = 'raw-button remove-user-button';
On 2011/01/25 23:10:33, James Hawkins wrote:
> nit: Please use two classList.add(X) instead.

Done.

[kochi, 2011-01-26 02:55:02]

LGTM

Thanks for the fix.
This addresses James's comment on
http://codereview.chromium.org/6282014/

[kochi, 2011-01-26 03:50:19]

Wait a moment...

With this patch applied to the ToT (FYI r72596),
the X button doesn't work.  Without the patch, the X button
works.
I'll be looking into it.

In addition, could you add BUG=chromium-os:10944 line in the
description?


On 2011/01/26 02:55:02, Takayoshi Kochi wrote:
> LGTM
> 
> Thanks for the fix.
> This addresses James's comment on
> http://codereview.chromium.org/6282014/

[kochi, 2011-01-26 04:42:10]

This should fix the X button event handler.

diff --git a/chrome/browser/resources/options/chromeos_accounts_user_list.js b/c
index ef4dbcd..38e6c43 100644
--- a/chrome/browser/resources/options/chromeos_accounts_user_list.js
+++ b/chrome/browser/resources/options/chromeos_accounts_user_list.js
@@ -103,7 +103,7 @@ cr.define('options.accounts', function() {
       // Handle left button click
       if (e.button == 0) {
         var el = e.target;
-        if (el.className == 'remove-user-button') {
+        if (el.classList.contains('remove-user-button')) {
           this.removeUser(el.parentNode.user);
         }
       }

On 2011/01/26 03:50:19, Takayoshi Kochi wrote:
> Wait a moment...
> 
> With this patch applied to the ToT (FYI r72596),
> the X button doesn't work.  Without the patch, the X button
> works.
> I'll be looking into it.
> 
> In addition, could you add BUG=chromium-os:10944 line in the
> description?
> 
> 
> On 2011/01/26 02:55:02, Takayoshi Kochi wrote:
> > LGTM
> > 
> > Thanks for the fix.
> > This addresses James's comment on
> > http://codereview.chromium.org/6282014/

[kochi, 2011-01-26 06:18:20]

http://codereview.chromium.org/6380015/diff/5001/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/6380015/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_accounts_user_list.js:174:
this.user.name;
How about adding 'title' attribute for labelEmail and labelName in case they are
too long to fit in the list view?

e.g.
labelEmail.textContent = labelEmail.title = this.user.email;

labelName.textContent = labelEmail.title = this.user.owner ? ...

[xiyuan, 2011-01-26 18:27:44]

Thanks Kochi for testing the patch. I have updated the CL to include your fix
for break X functionality and added a tooltip per your suggest.

http://codereview.chromium.org/6380015/diff/5001/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/6380015/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_accounts_user_list.js:174:
this.user.name;
On 2011/01/26 06:18:20, Takayoshi Kochi wrote:
> How about adding 'title' attribute for labelEmail and labelName in case they
are
> too long to fit in the list view?
> 
> e.g.
> labelEmail.textContent = labelEmail.title = this.user.email;
> 
> labelName.textContent = labelEmail.title = this.user.owner ? ...

Done. Good idea.

[kochi, 2011-01-27 01:49:13]

LGTM

Thanks and the part of adding the tooltip looks good, too.