Config UI for Extension Commands (part 1).

Config UI for Extension Commands (part 1).

This changes the current overlay so that input is possible (selecting which keyboard shortcut to use). It also suspends current Chrome keyboard handling (Views only), while capturing the shortcut, so that you can press ie. Ctrl-F without Chrome eating your keyboard assignment and instead showing the Find box. 

NOTE: This does not persist the user's choice (future changelist).

BUG=121420
TEST=With a keybinding Extension installed, open chrome://extensions, click on Configure Commands, and notice that the shortcuts are listed in text boxes that are clickable and accept keyboard input.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=141189

[Finnur, 2012-06-04 17:34:44]

jcivelli, can you give OWNERS approval for ui/views/focus?

Ben, can you give OWNERS approval for chrome/browser/ui and review the following
closely (disabling of shortcut handling in Chrome). If you (or Jay) have ideas
on a better way to suspend shortcut handling, just let me know:

ui/views/focus/focus_manager.h
ui/views/focus/focus_manager.cc
chrome/browser/extensions/api/commands/command_service.h
chrome/browser/extensions/extension_keybinding_registry.h
chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
chrome/browser/ui/views/frame/browser_view.cc

estade, can you give OWNERS approval for the gtk file above and review:

chrome/app/generated_resources.grd
chrome/app/theme/theme_resources.grd
chrome/browser/resources/extensions/extension_command_list.js
chrome/browser/resources/extensions/extension_commands_overlay.css
chrome/browser/resources/extensions/extension_commands_overlay.html
chrome/browser/ui/webui/extensions/command_handler.h
chrome/browser/ui/webui/extensions/command_handler.cc

[Ben Goodger (Google), 2012-06-04 17:48:10]

Can you briefly describe the flow of events here? I am trying to think of how we
want to expose this at the framework level.

[Finnur, 2012-06-04 19:08:30]

On 2012/06/04 17:48:10, Ben Goodger (Google) wrote:
> Can you briefly describe the flow of events here? I am trying to think of how
we
> want to expose this at the framework level.

Not sure what the scope of your question is... but let me take a stab at it...

- User installs an extension containing an extension command with a suggested
shortcut. 
- a) if no conflict, shortcut takes effect and user is told (in the install
bubble): You can use this keystroke to activate foo.
- b) if a conflict exists, shortcut does not take effect and user is told: go
here to configure the shortcut. User presses configure:
- the config WebuUI (seen in bug) is shown listing all possible extension
commands and their current keyboard shortcut assignment.
- User sees 'Activate extension' is not set and a textbox that s/he can click on
to set a shortcut for the extension. When clicked, capture starts and keyboard
handling in Chrome is suspended.
- Key combinations (on Windows atm) containing Ctrl/Alt and optionally shift in
combination with A-Z or 0-9 are recorded.
- Once a valid combination is selected, the WebUI will call into the C++ code
asking for the key to be set and capture ends (and normal shortcut handling of
Chrome resumes). 

Does that answer your question?

[Ben Goodger (Google), 2012-06-04 20:38:36]

Sorry let me be more specific.

The codepath you're modifying with this "suppress" mode is the code that is
called from AcceleratorHandler on Windows when we process any key event.

Who processes the key event instead? How do they hear about it if the focus
manager isn't forwarding it to them? What if the keyboard event is
unhandled, does FocusManager::OnKeyEvent get called again?

-Ben

On Mon, Jun 4, 2012 at 12:08 PM, <finnur@chromium.org> wrote:

> On 2012/06/04 17:48:10, Ben Goodger (Google) wrote:
>
>> Can you briefly describe the flow of events here? I am trying to think of
>> how
>>
> we
>
>> want to expose this at the framework level.
>>
>
> Not sure what the scope of your question is... but let me take a stab at
> it...
>
> - User installs an extension containing an extension command with a
> suggested
> shortcut.
> - a) if no conflict, shortcut takes effect and user is told (in the install
> bubble): You can use this keystroke to activate foo.
> - b) if a conflict exists, shortcut does not take effect and user is told:
> go
> here to configure the shortcut. User presses configure:
> - the config WebuUI (seen in bug) is shown listing all possible extension
> commands and their current keyboard shortcut assignment.
> - User sees 'Activate extension' is not set and a textbox that s/he can
> click on
> to set a shortcut for the extension. When clicked, capture starts and
> keyboard
> handling in Chrome is suspended.
> - Key combinations (on Windows atm) containing Ctrl/Alt and optionally
> shift in
> combination with A-Z or 0-9 are recorded.
> - Once a valid combination is selected, the WebUI will call into the C++
> code
> asking for the key to be set and capture ends (and normal shortcut
> handling of
> Chrome resumes).
>
> Does that answer your question?
>
>
http://codereview.chromium.**org/10514003/<http://codereview.chromium.org/105...
>

[Finnur, 2012-06-04 22:49:41]

Oh, I see. We need keyboard events to make it all the way to the renderer while
we are capturing extension command shortcut assignment in the config UI.
Basically, we want to temporarily prevent the browser from eating our keyboard
events when it has handlers for those events. There's only a brief window where
we suspend keyboard handling and it is during the capture phase, which starts
when you mouseclick on the text field and ends when the text field looses focus.

So, the event comes in through FocusManager::OnKeyEvent.

OnKeyEvent will normally pass the event to the AcceleratorManager for handling
if there is a registered handler for it. Various views (and other Extension
Commands) register as handlers for keyboard events (Find bar is one such view)
and the keyboard event might therefore get eaten before the web page sees it
(think Find bar opening instead of Ctrl-F being registered during capture). The
focused View might request to short-circuit this process and get first dibs, but
it's not guaranteed.

By checking the suspend flag early in OnKeyEvent we ensure that it does not
process the event, instead the event gets dispatched and we end up in
WebContentsImpl. Before WebContentsImpl sends it to the page, however, it gives
BrowserView a chance to handle it through PreHandleKeyboardEvent. Hence the
"suspended?" check there. If we let PreHandleKeyboardEvent handle the event,
we'd again end up with the Find box opening (if the user presses Ctrl-F) instead
of the Text field seeing Ctrl-F being pressed.

By exiting early from PreHandleKeyboardEvent we ensure that the event is
forwarded to the page, where we handle it and eat it (event.preventDefault()).
We need to eat it so that the browser does not handle it on the rebound (We
don't want the user pressing Ctrl-F during capture phase and have it register
Ctrl-F on the page *and* the Find box to open as well).

That's basically the thinking behind the suspend mechanism.

[Ben Goodger (Google), 2012-06-04 23:16:19]

On 2012/06/04 22:49:41, Finnur wrote:
> Oh, I see. We need keyboard events to make it all the way to the renderer
while
> we are capturing extension command shortcut assignment in the config UI.

Can you show me a screenshot of this UI?

-Ben

[Finnur, 2012-06-04 23:36:16]

Sure! It's in the bug comments:
http://code.google.com/p/chromium/issues/detail?id=121420
(see comment 4, picture 1 but not 2 since that's been replaced with what you see
in comment 7)

On 2012/06/04 23:16:19, Ben Goodger (Google) wrote:
> On 2012/06/04 22:49:41, Finnur wrote:
> > Oh, I see. We need keyboard events to make it all the way to the renderer
> while
> > we are capturing extension command shortcut assignment in the config UI.
> 
> Can you show me a screenshot of this UI?
> 
> -Ben

[Ben Goodger (Google), 2012-06-04 23:43:54]

Thanks for the explanation! LGTM.

[Evan Stade, 2012-06-05 00:20:36]

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_command_list.js (right):

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:110: output +=
event.shiftKey ? 'Shift + ' : '';
imo, it's easier to read

if (event.fooKey)
  output += 'Foo + ';

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_commands_overlay.css (right):

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_commands_overlay.css:11: color:
Black;
I think we don't capitalize colors

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_commands_overlay.css:14:
min-height: 18px;
should probably be expressed in em. Also what is the point of setting both
height and min-height?

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_commands_overlay.css:32: color:
Black;
please re-use the colors we are already using for normal text (not exactly
black) and disabled stuff (also not 'gray'). Why change the normal color for
<input> text?

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_commands_overlay.html
(right):

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_commands_overlay.html:41: <table>
don't use tables for layout.

[Evan Stade, 2012-06-05 01:20:31]

also please disable the UI for platforms that don't yet support it.

[Finnur, 2012-06-05 15:04:46]

PTAL.

> also please disable the UI for platforms that don't yet support it.

It is disabled by default. To turn it on you need to turn on the
experimental-extensions flag and install extensions that define extension
commands.

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_commands_overlay.css (right):

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_commands_overlay.css:14:
min-height: 18px;
I don't always set font heights, but when I do I use the latest in font sizing
technology: both belts and suspenders at the same time.

(min-height removed)

On 2012/06/05 00:20:37, Evan Stade wrote:
> should probably be expressed in em. Also what is the point of setting both
> height and min-height?

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_commands_overlay.css:32: color:
Black;
> Why change the normal color for
> <input> text?

Well, firstly, this isn't actually an input text, it is a focusable span with a
border that looks like one. When you press Ctrl in a normal text box nothing
happens but in this case you are shown 'Ctrl + ' and you can continue pressing
for more to appear.

This was designed by alcor and is similar to the way Mac and Windows implement a
config UI to capture shortcut assignments (not sure how it works in the Linux
world).

There are three states, some seen in the images (http://crbug.com/121420).

1. No binding is set
  - We show "Not set" hint in gray on white background.
2. Active binding, not being changed
  - The keybinding (such as "Ctrl-Shift-F") is shown in black on white.
3. Incomplete input
  - While capturing we show a purple background and a hint in gray ("Type a
shortcut") that disappears once you start typing. Once we have a valid binding
we save it and go to state 2 but the user can continue typing and then we
restart capture and go to state 3 again.

So, to answer your question: Gray is for hints and black is to indicate that
you've made a complete and valid keybinding assignment (an active keybinding, if
you will).

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_commands_overlay.html
(right):

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_commands_overlay.html:41: <table>
On 2012/06/05 00:20:37, Evan Stade wrote:
> don't use tables for layout.

Done.

[Finnur, 2012-06-05 15:11:32]

PS. I also updated the screenshot in the bug.

On 2012/06/05 15:04:46, Finnur wrote:
> PTAL.
> 
> > also please disable the UI for platforms that don't yet support it.
> 
> It is disabled by default. To turn it on you need to turn on the
> experimental-extensions flag and install extensions that define extension
> commands.
> 
>
http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
> File chrome/browser/resources/extensions/extension_commands_overlay.css
(right):
> 
>
http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
> chrome/browser/resources/extensions/extension_commands_overlay.css:14:
> min-height: 18px;
> I don't always set font heights, but when I do I use the latest in font sizing
> technology: both belts and suspenders at the same time.
> 
> (min-height removed)
> 
> On 2012/06/05 00:20:37, Evan Stade wrote:
> > should probably be expressed in em. Also what is the point of setting both
> > height and min-height?
> 
>
http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
> chrome/browser/resources/extensions/extension_commands_overlay.css:32: color:
> Black;
> > Why change the normal color for
> > <input> text?
> 
> Well, firstly, this isn't actually an input text, it is a focusable span with
a
> border that looks like one. When you press Ctrl in a normal text box nothing
> happens but in this case you are shown 'Ctrl + ' and you can continue pressing
> for more to appear.
> 
> This was designed by alcor and is similar to the way Mac and Windows implement
a
> config UI to capture shortcut assignments (not sure how it works in the Linux
> world).
> 
> There are three states, some seen in the images (http://crbug.com/121420).
> 
> 1. No binding is set
>   - We show "Not set" hint in gray on white background.
> 2. Active binding, not being changed
>   - The keybinding (such as "Ctrl-Shift-F") is shown in black on white.
> 3. Incomplete input
>   - While capturing we show a purple background and a hint in gray ("Type a
> shortcut") that disappears once you start typing. Once we have a valid binding
> we save it and go to state 2 but the user can continue typing and then we
> restart capture and go to state 3 again.
> 
> So, to answer your question: Gray is for hints and black is to indicate that
> you've made a complete and valid keybinding assignment (an active keybinding,
if
> you will).
> 
>
http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
> File chrome/browser/resources/extensions/extension_commands_overlay.html
> (right):
> 
>
http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
> chrome/browser/resources/extensions/extension_commands_overlay.html:41:
<table>
> On 2012/06/05 00:20:37, Evan Stade wrote:
> > don't use tables for layout.
> 
> Done.

[Evan Stade, 2012-06-05 18:28:01]

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_commands_overlay.css (right):

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_commands_overlay.css:32: color:
Black;
On 2012/06/05 15:04:46, Finnur wrote:
> > Why change the normal color for
> > <input> text?
> 
> Well, firstly, this isn't actually an input text, it is a focusable span with
a
> border that looks like one. When you press Ctrl in a normal text box nothing
> happens but in this case you are shown 'Ctrl + ' and you can continue pressing
> for more to appear.
> 
> This was designed by alcor and is similar to the way Mac and Windows implement
a
> config UI to capture shortcut assignments (not sure how it works in the Linux
> world).

I don't have a problem with the UI, but I kind of doubt alcor consciously chose
just plain black while all other text on the page, input or not, isn't black.

> 
> There are three states, some seen in the images (http://crbug.com/121420).

If I am to comment on these screenshots, I would say that the dialog should look
like other dialogs. That is, the title must be bolded and there needs to be an
(x) in the top right. The CSS is already there, you just need to fiddle with the
markup.

the button text should be [ OK ], not [ Close ], the input-looking things should
have the same border color, roundedness, and height as normal text inputs.

> 
> 1. No binding is set
>   - We show "Not set" hint in gray on white background.
> 2. Active binding, not being changed
>   - The keybinding (such as "Ctrl-Shift-F") is shown in black on white.
> 3. Incomplete input
>   - While capturing we show a purple background and a hint in gray ("Type a
> shortcut") that disappears once you start typing. Once we have a valid binding
> we save it and go to state 2 but the user can continue typing and then we
> restart capture and go to state 3 again.
> 
> So, to answer your question: Gray is for hints and black is to indicate that
> you've made a complete and valid keybinding assignment (an active keybinding,
if
> you will).

that's fine, as long as you change the definitions in that sentence. "Gray" is
#999 and "black" is rgb(48, 57, 66)

[Finnur, 2012-06-05 21:12:13]

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_commands_overlay.css (right):

http://codereview.chromium.org/10514003/diff/12004/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_commands_overlay.css:32: color:
Black;
> I don't have a problem with the UI, but I kind of doubt alcor consciously
chose
> just plain black while all other text on the page, input or not, isn't black.

Hmm, I actually wasn't referring to the colors scheme for the fonts when I
referred to his design. I'm not sure he meant his mocks to go to that level of
detail (besides, it would be weird to change the font color just for this
dialog) and I didn't realize we used something slightly off-black and off-gray.
Silly really, as I could have anticipated us using a different color but didn't.

> the button text should be [ OK ], not [ Close ], the input-looking things
should
> have the same border color, roundedness, and height as normal text inputs.

Not a problem, I'll work on that tomorrow. Was the rest of the changelist ok?

> that's fine, as long as you change the definitions in that sentence. "Gray" is
> #999 and "black" is rgb(48, 57, 66)

But... that's exactly what the latest CL I asked you to review contains... does
it not?

[Evan Stade, 2012-06-05 21:25:51]

http://codereview.chromium.org/10514003/diff/17001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_command_list.js (right):

http://codereview.chromium.org/10514003/diff/17001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:111: if
(event.altKey)
if (!event.ctrlKey && event.altKey)

http://codereview.chromium.org/10514003/diff/17001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:140: if
(capturingElement !== null)
just if (capturingElement)

http://codereview.chromium.org/10514003/diff/17001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:160: if
(capturingElement === null)
ditto

http://codereview.chromium.org/10514003/diff/17001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:167: if
(currentKeyEvent == null ||
no need to wrap

http://codereview.chromium.org/10514003/diff/17001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:187: if
(capturingElement === null)
ditto

http://codereview.chromium.org/10514003/diff/17001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:224: */
document return type and meaning

http://codereview.chromium.org/10514003/diff/17001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_commands_overlay.css (right):

http://codereview.chromium.org/10514003/diff/17001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_commands_overlay.css:27: float:
right;
make the parent container a flex box.

[Finnur, 2012-06-06 15:39:50]

Addressed, PTAL. I'll update the bug with new screenshots.

https://chromiumcodereview.appspot.com/10514003/diff/17001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://chromiumcodereview.appspot.com/10514003/diff/17001/chrome/browser/res...
chrome/browser/resources/extensions/extension_command_list.js:224: */
This shouldn't be returning anything. Thanks for raising the flag.

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_command_list.js:240:
currentKeyEvent = event;
This needs to happen after endCapture because endCapture clears currentKeyEvent
(for the no-op case).

[Evan Stade, 2012-06-06 23:04:45]

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_command_list.js:19: * keyboard
event has been generated.
@type doc for these

they should all be member vars as well.

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_command_list.js:106:
keystrokeToString_: function(event) {
doesn't need to be a member function

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_command_list.js:111: output +=
'Alt + ';
indent wrong here and L109

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_command_list.js:126: validChar_:
function(keyCode) {
doesn't need to be a member function

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_command_list.js:197: // releasing
Shift, but we also don't want it to be easy to loose for
lose

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_command_list.js:225:
event.preventDefault();
I think what you want is stopPropagation (or perhaps both). <span>s don't have a
default key handler AFAIK.

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_commands_overlay.css (right):

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_commands_overlay.css:21:
-webkit-box-flex: 0.55;
why is this not 1

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_commands_overlay.css:39:
min-width: 12.5em;
I assume it grows when it needs to?

[Finnur, 2012-06-07 12:40:34]

Updated, PTAL.

> wait, this isn't going to work on linux

Not sure what you mean by 'this'. If you are talking about Linux not suspending
keyboard handling while capturing then yes, I wouldn't be surprised. It is next
on my list to fix after I check in. It still works, as long as you avoid already
registered shortcuts (Ctrl+F for Find, for example).

[Finnur, 2012-06-07 12:41:01]

And... forgot to publish my comments...

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_command_list.js:225:
event.preventDefault();
I've added stopPropagation. I found that stopPropagation on its own is not
enough.

On 2012/06/06 23:04:45, Evan Stade wrote:
> I think what you want is stopPropagation (or perhaps both). <span>s don't have
a
> default key handler AFAIK.

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
File chrome/browser/resources/extensions/extension_commands_overlay.css (right):

https://chromiumcodereview.appspot.com/10514003/diff/29001/chrome/browser/res...
chrome/browser/resources/extensions/extension_commands_overlay.css:39:
min-width: 12.5em;
Yup, it does.

On 2012/06/06 23:04:45, Evan Stade wrote:
> I assume it grows when it needs to?

[Evan Stade, 2012-06-07 19:23:03]

I just meant that I couldn't test it on my machine.

lgtm