Fix conflicts between accelerator keys and HTML DOM accesskeys....

Fix conflicts between accelerator keys and HTML DOM accesskeys.

This CL fixes conflicts between accelerator keys and HTML DOM accesskeys by suppressing Char events if corresponding RawKeyDown event was handled by the browser after returning from the renderer unhandled.

This CL not only fixes this conflict issue, but also makes the behavior of handling accelerator keys similar than IE, which also suppresses a key press event if the key down event was handled as an accelerator key.

BUG=21624
accesskey attributes conflict with browser shortcuts (like tab-switching)
TEST=Open http://djmitche.github.com/buildbot/docs/0.7.11/ in one of opened multiple tabs, switch to another tab by pressing an alt-# key binding, then switch back to the original page to see if it's just as you left it before switching tabs.

[James Su, 2009-09-30 15:46:20]

Hi all,
  I think this CL is in good shape for review. Though it lacks a test, I'll add
it as soon as possible.
  I tested it locally on Linux Windows and Mac, no issue was found so far.
  As this CL changes the keyboard event flow, which may have potential problem,
I'd like to let you review it and give me your opinion as early as possible.

Regards
James Su

[jam, 2009-10-06 21:51:48]

Not sure what's the status of this patch/bug?  Just wanted to point out that
after http://src.chromium.org/viewvc/chrome?view=rev&revision=27814, the fix for
this bug is simple:

Browser::IsReservedAccelerator just needs:

#if defined(OS_LINUX)
  // On Linux we want to reserve alt+0...9.
  if (command_id >= IDC_SELECT_TAB_0 &&
      command_id <= IDC_SELECT_LAST_TAB &&
      event.modifiers == NativeWebKeyboardEvent::AltKey) {
    return true;
  }
#endif

[James Su, 2009-10-07 04:17:31]

I'm still waiting for your feedback of this CL. These days (Oct. 1-8) are our
national holiday. I'll test your CL for r27814 and implement test code for this
CL after I go back to work.
This issue is not simple indeed. Besides alt+0...9, you need handle alt+a...z as
well. And you even don't know if any web app requires the keypress events of
ctrl-a...z. This CL provides a generic solution for this kind of issues, not
only for alt+0...9.

Regards
James Su

On 2009/10/06 21:51:48, John Abd-El-Malek wrote:
> Not sure what's the status of this patch/bug?  Just wanted to point out that
> after http://src.chromium.org/viewvc/chrome?view=rev&revision=27814, the fix
for
> this bug is simple:
> 
> Browser::IsReservedAccelerator just needs:
> 
> #if defined(OS_LINUX)
>   // On Linux we want to reserve alt+0...9.
>   if (command_id >= IDC_SELECT_TAB_0 &&
>       command_id <= IDC_SELECT_LAST_TAB &&
>       event.modifiers == NativeWebKeyboardEvent::AltKey) {
>     return true;
>   }
> #endif

[Evan Stade, 2009-10-09 22:59:41]

ping Darin (?)

[Evan Martin, 2009-10-16 00:58:26]

I think John is the right reviewer for this, since he did work on browser accels
recently.  John, poor James has been waiting for weeks on his reviews lately --
can you quickly respond with "yes I will review this" or "no", even if you can't
get to this review right now?

[jam, 2009-10-16 06:49:28]

Sorry for the delay, I thought more qualified reviewers would respond first.
 I have done some changes there recently, but I'm not that familiar with how
input works to give useful feedback.  I'd check with Ben to see who's most
familiar with this code, if you can't figure it out by the svn log or
#chormium.

On Thu, Oct 15, 2009 at 5:58 PM, <evan@chromium.org> wrote:

> I think John is the right reviewer for this, since he did work on browser
> accels
> recently.  John, poor James has been waiting for weeks on his reviews
> lately --
> can you quickly respond with "yes I will review this" or "no", even if you
> can't
> get to this review right now?
>
>
> http://codereview.chromium.org/235039
>

[James Su, 2009-10-16 09:53:06]

I added hbono and avi to reviewers, hope they can help.

Regards
James Su

On 2009/10/16 06:49:28, John Abd-El-Malek wrote:
> Sorry for the delay, I thought more qualified reviewers would respond first.
>  I have done some changes there recently, but I'm not that familiar with how
> input works to give useful feedback.  I'd check with Ben to see who's most
> familiar with this code, if you can't figure it out by the svn log or
> #chormium.
> 
> On Thu, Oct 15, 2009 at 5:58 PM, <mailto:evan@chromium.org> wrote:
> 
> > I think John is the right reviewer for this, since he did work on browser
> > accels
> > recently.  John, poor James has been waiting for weeks on his reviews
> > lately --
> > can you quickly respond with "yes I will review this" or "no", even if you
> > can't
> > get to this review right now?
> >
> >
> > http://codereview.chromium.org/235039
> >
>

[James Su, 2009-10-16 09:54:04]

And the test code for this CL: http://codereview.chromium.org/268035/show

[James Su, 2009-10-20 10:16:59]

Can anybody help review this CL? It's already pending for quite a long time.

Thanks
James Su

[Evan Martin, 2009-10-20 17:12:32]

Another try on finding a reviewer: erg.  Though this looks like it touches a ton
of files, most of them are just changes to function prototypes; the meat is in
the renderer->browser key queue handling that erg wrote.

[Elliot Glaysher, 2009-10-20 18:37:38]

I want to NACK this patch. It's really complicated, and it's not obvious that it
works. OTOH, I generally understand the logic behind it and the sort of
synchronization problems you're trying to address.

For my sanity checking: if we're not sending certain events to webkit now,
what's going on with |suppress_next_keypress_event_| in webview_impl.cc? Can
that be simplified now that it looks like we're just not sending certain char
events?

The road to getting a LGTM:

- You have TODOs in TabContentsViewWin and TabContentsViewMac regarding making
them work on those platforms. Make them not TODOs.
- Add unit tests. You've written IN_PROC_BROWSER_TESTS in #258035 and that's
great, and I still want those to be committed, but this is a really subtle patch
and I'd like to see tests of the individual pieces of logic in
RenderWidgetHost::ForwardKeyboardEvent() and
RenderWidgetHost::OnMsgInputEventAck() tested instead of firing up an entire
kitchen sink. You've modified render_widget_host_unittest.cc so you've seen that
it exists. Add a bunch of tests for the different synchronization cases there,
mocking communication with the render_widget. See
RenderWidgetHostTest.{HandleKeyEventsWeSent,IgnoreKeyEventsWeDidntSend} as
examples. (These were the tests I added when I first added the keyboard queue).

[James Su, 2009-10-21 02:12:41]

Thanks for your valuable feedback.

On 2009/10/20 18:37:38, Elliot Glaysher wrote:
> I want to NACK this patch. It's really complicated, and it's not obvious that
it
> works. OTOH, I generally understand the logic behind it and the sort of
> synchronization problems you're trying to address.
> 
> For my sanity checking: if we're not sending certain events to webkit now,
> what's going on with |suppress_next_keypress_event_| in webview_impl.cc? Can
> that be simplified now that it looks like we're just not sending certain char
> events?
|suppress_next_keypress_event_| in webview_impl.cc is still useful, because if a
key down event was processed inside webkit, the following key press event will
still be sent to webkit, and whether or not to suppress the key press event is
decided in webkit according to the value of |suppress_next_keypress_event_|.

> 
> The road to getting a LGTM:
> 
> - You have TODOs in TabContentsViewWin and TabContentsViewMac regarding making
> them work on those platforms. Make them not TODOs.
I'm not familiar with Windows and Mac platform, it'll be great if somebody can
help me on them.

> - Add unit tests. You've written IN_PROC_BROWSER_TESTS in #258035 and that's
> great, and I still want those to be committed, but this is a really subtle
patch
> and I'd like to see tests of the individual pieces of logic in
> RenderWidgetHost::ForwardKeyboardEvent() and
> RenderWidgetHost::OnMsgInputEventAck() tested instead of firing up an entire
> kitchen sink. You've modified render_widget_host_unittest.cc so you've seen
that
> it exists. Add a bunch of tests for the different synchronization cases there,
> mocking communication with the render_widget. See
> RenderWidgetHostTest.{HandleKeyEventsWeSent,IgnoreKeyEventsWeDidntSend} as
> examples. (These were the tests I added when I first added the keyboard
queue).
I'll look into this piece of code and try to add the tests.

Regards
James Su

[James Su, 2009-10-21 14:41:24]

Hi,
  Unit tests have been added, please help review.

Regards
James Su

[jam, 2009-10-21 15:36:46]

What are the effects of this TODO in the Windows code?  i.e. will it result
in a key being used by both browser and renderer?

 408   // TODO(suzhe): How to know if DefWindowProc() actually processed the
event?
 409   // Seems that it always returns 0.
 410   return false;

On Wed, Oct 21, 2009 at 7:41 AM, <suzhe@chromium.org> wrote:

> Hi,
>  Unit tests have been added, please help review.
>
>
> Regards
> James Su
>
> http://codereview.chromium.org/235039
>

[Elliot Glaysher, 2009-10-21 17:37:46]

Thanks for the unit tests. They're really comprehensive!

On 2009/10/21 15:36:46, John Abd-El-Malek wrote:
> What are the effects of this TODO in the Windows code?  i.e. will it result
> in a key being used by both browser and renderer?
> 
>  408   // TODO(suzhe): How to know if DefWindowProc() actually processed the
> event?
>  409   // Seems that it always returns 0.
>  410   return false;

This is part of why I am blocking this patch on the windows and mac
implementations. I am not sure if there's API on other platforms to check if an
event has been handled, and if there's not, then I doubt this approach isn't
going to work.

As for help on this, I don't really know that much about Windows or Mac event
handling myself, outside of the basics. At least in the Mac world, I don't think
this will work, because -[NSWindow keyDown:] and keyUp: return void, and the
pieces of sample code I've seen that override keyDown: just perform actions;
there's no bit set that represents I-Have-Handled-This-Event.

http://developer.apple.com/mac/library/documentation/Cocoa/Conceptual/EventOv...

[James Su, 2009-10-22 10:38:19]

I just updated this CL to implement the Mac part.

For Windows, according to:
http://msdn.microsoft.com/en-us/library/ms646267(VS.85).aspx
http://msdn.microsoft.com/en-us/library/ms646280(VS.85).aspx
http://msdn.microsoft.com/en-us/library/ms646286(VS.85).aspx
http://msdn.microsoft.com/en-us/library/ms633572(VS.85).aspx

As I understand, DefWindowProc() will always return 0, which means the event was
processed. See the description of this function:

The DefWindowProc function calls the default window procedure to provide default
processing for any window messages that an application does not process. This
function ensures that every message is processed.

And for key event, return 0 means the event was processed.
However, according to the first document, this function actually only handles a
few system keys, such as F10, Alt-Esc, Alt-Tab, etc. And according to my
experiments, only F10 will be handled by this function. All other system hot
keys will be handled before sending to the application. So returning false here
is just ok.

Regards
James Su

On 2009/10/21 17:37:46, Elliot Glaysher wrote:
> Thanks for the unit tests. They're really comprehensive!
> 
> On 2009/10/21 15:36:46, John Abd-El-Malek wrote:
> > What are the effects of this TODO in the Windows code?  i.e. will it result
> > in a key being used by both browser and renderer?
> > 
> >  408   // TODO(suzhe): How to know if DefWindowProc() actually processed the
> > event?
> >  409   // Seems that it always returns 0.
> >  410   return false;
> 
> This is part of why I am blocking this patch on the windows and mac
> implementations. I am not sure if there's API on other platforms to check if
an
> event has been handled, and if there's not, then I doubt this approach isn't
> going to work.
> 
> As for help on this, I don't really know that much about Windows or Mac event
> handling myself, outside of the basics. At least in the Mac world, I don't
think
> this will work, because -[NSWindow keyDown:] and keyUp: return void, and the
> pieces of sample code I've seen that override keyDown: just perform actions;
> there's no bit set that represents I-Have-Handled-This-Event.
> 
>
http://developer.apple.com/mac/library/documentation/Cocoa/Conceptual/EventOv...

[Elliot Glaysher, 2009-10-23 00:53:14]

I still don't like how complex this patch is, but I can't think of anything
simpler and you do have some comprehensive unit tests.

LGTM.

[Nico, 2009-10-24 02:31:10]

James, thanks so much for taking this on! Writing this was on my list, so I'm
very happy that I don't have to do this.

As far as I understand, this patch makes every keypress effectively a
synchronous message, right? The browser sends a rawkey event, and only sends the
char event when the renderer has ACKd the keypress. But since it's not a "real"
sync ipc call, this shouldn't lead to typing jank, so it should be ok.

One issue that I was wondering about is how this would interact with emacs-like
keyboard shortcuts like ctrl-a. Arv wants to make ctrl-letter events fire
keypress() events (see http://crbug.com/13891 ). I guess the right sequence of
actions for ctrl-a would be:

1. keydown()
2. if that is not consumed by js, keypress()
3. if that is not consumed by js, execute the emacs action

In the light of this patch, it looks like the renderer needs to keep a queue of
keydown events, and only when the char event arrives that wasn't handled by js
should the edit action of the keydown be fired. Is this something you thought
about?

http://codereview.chromium.org/235039/diff/26023/26039
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/235039/diff/26023/26039#newcode608
Line 608: return true;
This would be easier to understand if it went like

  if (HandleC…)
    return true;
  return gtk_wind…

[James Su, 2009-10-24 10:53:47]

Hi,
  For the emacs like keyboard sequences, if you want to input a ctrl character,
eg. ^j, I think the correct sequence would be ctrl-q ctrl-j, rather than just
ctrl-j. If you just press a ctrl-something, it would execute a command, while
ctrl-q corresponds to "quote" command, which just inserts the following key
stroke into the buffer.
  And I think it's nothing to do with render_host and renderer, this part of
logic is actually related to following files:

chrome/browser/renderer_host/render_widget_host_view_mac.mm (line 580-588).
chrome/browser/cocoa/rwhvm_editcommand_helper.mm
webkit/glue/editor_client_impl.cc (line 633-636).

You may notice that the edit commands corresponding to a key event will be sent
to renderer just before sending the key event and then webkit glue's
EditorClientImpl will execute these edit commands if the key event was not
handled or cancelled (by calling
webview_->client()->handleCurrentKeyboardEvent()).

So if you want to bind the edit commands to the key press event rather than the
key down event, you need modify RenderWidgetHostViewMac class to send the edit
commands after sending the key down event and just before sending the key press
event.

But as I said at the beginning, it may not be a correct approach, as the
standard Emacs way should be using ctrl-q. 

Regards
James Su

http://codereview.chromium.org/235039/diff/26023/26039
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/235039/diff/26023/26039#newcode608
Line 608: return true;
Good point. I'll change it in an upcoming CL.

On 2009/10/24 02:31:10, Nico wrote:
> This would be easier to understand if it went like
> 
>   if (HandleC…)
>     return true;
>   return gtk_wind…

[darin (slow to review), 2009-11-24 07:47:35]

Is there no other way to solve this bug without queuing key events in the
RenderWidgetHost?

http://codereview.chromium.org/235039/diff/26023/26044
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/235039/diff/26023/26044#newcode89
chrome/browser/renderer_host/render_widget_host.cc:89: if (death_flag_)
this seems like a very ugly hack for the fact that we don't have the lifetime
management worked out properly.

http://codereview.chromium.org/235039/diff/26023/26044#newcode410
chrome/browser/renderer_host/render_widget_host.cc:410: // There are to
different situations for us to handle key events:
nit: "There are two..."

[darin (slow to review), 2009-11-24 07:51:45]

http://codereview.chromium.org/235039/diff/26023/26044
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/235039/diff/26023/26044#newcode451
chrome/browser/renderer_host/render_widget_host.cc:451: if
(suppress_next_char_events_) {
couldn't we implement this discarding of events in the renderer?  if the
renderer decides not to handle a RawKeyDown, then it could queue incoming input
events until it receives acknowledgement from the browser that the RawKeyDown
was consumed as an accelerator.

that way we only queue events if the renderer decides not to handle the event.

in other words, the case of typing into an edit field remains unchanged.  the
renderer would continue consuming input events as fast as it can.

[James Su, 2009-11-24 09:10:55]

Thanks for your feedback. Please see my comments below.

- James Su

http://codereview.chromium.org/235039/diff/26023/26044
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/235039/diff/26023/26044#newcode89
chrome/browser/renderer_host/render_widget_host.cc:89: if (death_flag_)
On 2009/11/24 07:47:36, darin wrote:
> this seems like a very ugly hack for the fact that we don't have the lifetime
> management worked out properly.

I don't like it either, but I don't know how to fix the lifetime management.
IMHO, this approach is simple enough to live with for now.

http://codereview.chromium.org/235039/diff/26023/26044#newcode410
chrome/browser/renderer_host/render_widget_host.cc:410: // There are to
different situations for us to handle key events:
On 2009/11/24 07:47:36, darin wrote:
> nit: "There are two..."

This is fixed in CL 400012.

http://codereview.chromium.org/235039/diff/26023/26044#newcode451
chrome/browser/renderer_host/render_widget_host.cc:451: if
(suppress_next_char_events_) {
On 2009/11/24 07:51:45, darin wrote:
> couldn't we implement this discarding of events in the renderer?  if the
> renderer decides not to handle a RawKeyDown, then it could queue incoming
input
> events until it receives acknowledgement from the browser that the RawKeyDown
> was consumed as an accelerator.
> 
> that way we only queue events if the renderer decides not to handle the event.
> 
> in other words, the case of typing into an edit field remains unchanged.  the
> renderer would continue consuming input events as fast as it can.

Good suggestion. I'll look into this approach to see if it's feasible. The two
issues I can see for now are:
1. We need send at least one extra message for each RawKeyDown event.
2. We still can't solve http://crbug.com/27932 without adding things similar
than CL 435002. Because no matter where we queue the events, the renderer can't
process the events in batch without waiting for the ack messages.

So I think no matter we implement this logic in the host or the renderer, the
complexity should have no big difference.