Fixed gear menu navigation using keyboard.

Fixed gear menu navigation using keyboard.

Removed CSS codes that hides gear menu items but is not handled cr.ui.Menu class.
New codes hides gear menu items by using the 'hidden' attribute.

BUG=175662
TEST=Followed the steps written in bug report that reproduce the problem

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193363

[hirono, 2013-04-08 09:48:43]

I fixed a bug of gear menu navigation with keyboard.
Could you check this CL?

Thank you very much.

[mtomasz, 2013-04-08 09:59:39]

https://codereview.chromium.org/13779006/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/13779006/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_manager.js:2039:
FileManager.prototype.updateGearMenu_ = function() {
This seems to be correct, but I think we can make it much simpler. We have
already a functionality to hide commands, which is used in
file_manager_commands.js in canExecute: event.command.setHidden(...);

We have a method: canExecuteChange: function(opt_node) in
src/ui/webui/resources/js/cr/ui/command.js. Maybe we could call it when we
detect pressing or releasing the ctrl key, which would hide/show the commands?

What do you think?

[hirono, 2013-04-09 04:05:48]

I fixed the code so that the menu items are hidden by the Command class.

Thank you.

https://codereview.chromium.org/13779006/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/13779006/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_manager.js:2039:
FileManager.prototype.updateGearMenu_ = function() {
I fixed the code so that the menu items are hidden by the command framework.

On 2013/04/08 09:59:39, mtomasz wrote:
> This seems to be correct, but I think we can make it much simpler. We have
> already a functionality to hide commands, which is used in
> file_manager_commands.js in canExecute: event.command.setHidden(...);
> 
> We have a method: canExecuteChange: function(opt_node) in
> src/ui/webui/resources/js/cr/ui/command.js. Maybe we could call it when we
> detect pressing or releasing the ctrl key, which would hide/show the commands?
> 
> What do you think?

[mtomasz, 2013-04-09 05:44:58]

I like it! Just two small comments :).

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_manager.js:2043: /**
This seems to be unused.

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_manager.js:3432:
this.ctrlKeyPressed_ = flag;
Can we use querySelector instead for consistency?

[hirono, 2013-04-09 06:01:04]

I replied the comments. Thank you.

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_manager.js:2043: /**
'updateGearMenu_' is used to update remaining space data and to hide menu items
that are not assigned commands. (e.g. 'syncButton', 'hostedButton')

On 2013/04/09 05:44:58, mtomasz wrote:
> This seems to be unused.

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_manager.js:3432:
this.ctrlKeyPressed_ = flag;
I think getElementById is a little faster than querySelector.
Is this reasonable excuse to use getElementById?
(or is consistency more important here?)

On 2013/04/09 05:44:58, mtomasz wrote:
> Can we use querySelector instead for consistency?

[mtomasz, 2013-04-09 06:11:39]

lgtm with one comment

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_manager.js:2043: /**
On 2013/04/09 06:01:04, hirono wrote:
> 'updateGearMenu_' is used to update remaining space data and to hide menu
items
> that are not assigned commands. (e.g. 'syncButton', 'hostedButton')
> 
> On 2013/04/09 05:44:58, mtomasz wrote:
> > This seems to be unused.
> 

You're right!

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_manager.js:3432:
this.ctrlKeyPressed_ = flag;
On 2013/04/09 06:01:04, hirono wrote:
> I think getElementById is a little faster than querySelector.
> Is this reasonable excuse to use getElementById?
> (or is consistency more important here?)
> 
> On 2013/04/09 05:44:58, mtomasz wrote:
> > Can we use querySelector instead for consistency?
> 

It is faster (http://jsperf.com/queryselector-vs-getdocumentbyid), but it is
still microseconds so doesn't matter here. I would go for consistency.

[hirono, 2013-04-09 06:43:13]

I fixed it.
Thank you very much!

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/13779006/diff/5001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_manager.js:3432:
this.ctrlKeyPressed_ = flag;
On 2013/04/09 06:11:39, mtomasz wrote:
> On 2013/04/09 06:01:04, hirono wrote:
> > I think getElementById is a little faster than querySelector.
> > Is this reasonable excuse to use getElementById?
> > (or is consistency more important here?)
> > 
> > On 2013/04/09 05:44:58, mtomasz wrote:
> > > Can we use querySelector instead for consistency?
> > 
> 
> It is faster (http://jsperf.com/queryselector-vs-getdocumentbyid), but it is
> still microseconds so doesn't matter here. I would go for consistency.

Done.

[commit-bot: I haz the power, 2013-04-09 08:32:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hirono@chromium.org/13779006/17001

[yoshiki, 2013-04-09 08:41:36]

Sorry for interruption and late comments.

https://codereview.chromium.org/13779006/diff/17001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/13779006/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:552: 'menuitem, hr';
Is this change necessary?

https://codereview.chromium.org/13779006/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:2051:
!this.isOnDrive();
nit: 4 space indent.

[yoshiki, 2013-04-09 08:42:47]

And could you modify chrome/browser/resources/file_manager/main_new_ui.html as
well? (this file is added recently)

[hirono, 2013-04-09 22:55:47]

Yoshiki-san,

I replied your comments.
Thank you very much!

https://codereview.chromium.org/13779006/diff/17001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/13779006/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:552: 'menuitem, hr';
These lines prevent inner elements of 'volume-space-info' to be collected as
menu items.

The default selector is '*', which selects the inner elements of
'volume-space-info' such as 'volume-space-info-label'.
 
On 2013/04/09 08:41:36, yoshiki wrote:
> Is this change necessary?

https://codereview.chromium.org/13779006/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:2051:
!this.isOnDrive();
On 2013/04/09 08:41:36, yoshiki wrote:
> nit: 4 space indent.

Done.

[hirono, 2013-04-10 03:38:20]

I applied the changes to the new interface HTML.
Could you also check it?

Thank you.

[yoshiki, 2013-04-10 03:51:29]

LGTM

https://codereview.chromium.org/13779006/diff/17001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/13779006/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:552: 'menuitem, hr';
Got it! thanks!

On 2013/04/09 22:55:48, hirono wrote:
> These lines prevent inner elements of 'volume-space-info' to be collected as
> menu items.
> 
> The default selector is '*', which selects the inner elements of
> 'volume-space-info' such as 'volume-space-info-label'.
>  
> On 2013/04/09 08:41:36, yoshiki wrote:
> > Is this change necessary?
>

[commit-bot: I haz the power, 2013-04-10 08:35:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hirono@chromium.org/13779006/45001

[commit-bot: I haz the power, 2013-04-10 10:49:24]

Change committed as 193363