Enable keyboard accelerators while a menu is open

ui/views/controls/menu/menu_message_pump_dispatcher_win.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/menu/menu_message_pump_dispatcher_win.h"
 
 #include <windowsx.h>
 
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/keyboard_code_conversion.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/views/controls/menu/menu_controller.h"
 #include "ui/views/controls/menu/menu_item_view.h"
 
 namespace views {
 namespace internal {
 
 MenuMessagePumpDispatcher::MenuMessagePumpDispatcher(MenuController* controller)
     : menu_controller_(controller) {}
 
 MenuMessagePumpDispatcher::~MenuMessagePumpDispatcher() {}
 
 uint32_t MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {

[pkotwicz, 2015/06/16 16:04:59]

It would be nice if MenuEventFilter handled as many events as possible (all of
the events except WM_CONTEXTMENU).
This would make the code more readable in my opinion because the event handling
in MenuMessagePumpDispatcher and MenuEventFilter would be mutually exclusive
(Yes, this would require #ifdefs but that's ok in my opinion)

[afakhry, 2015/06/16 23:56:39]

That would be my preference too but we can't do that unfortunately.
1- We must handle WM_KEYDOWN here explicitly so that we can call
DispatchMessage() only (Which will make the event reach the
MenuEventFilter::OnKeyEvent() without calling TranslateMessage(). Otherwise
TranslateMessage() will be called by MessagePumpForUI::ProcessMessageHelper()
here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
Which will result in the generation of WM_CHAR. WM_CHAR events take a very
different path than normally-dispatched WM_KEYDOWN events. See 
HWNDMessageHandler::OnImeMessages() at:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/win/hwnd_...

This path will never hit the MenuEventFilter::OnKeyEvent() so handling these
events there will never happen.
If we allow the genration of WM_CHAR, the above path will lead to inserting a
character in the omnibox or any enabled textfiled.

2- We must handle WM_CHAR explicitly here so that we can set
should_perform_default = false for the same above reason.

3- As for WM_CANCELMODE, It's similar to WM_CONTEXTMENU, it seems that it's not
represented in the ui::Event stack.

[pkotwicz, 2015/06/17 14:26:24]

I was unaware of the special handling for WM_CHAR. Thanks for pointing it out.

Pardon my ignorance. Why is calling TranslateMessage() as a result of WM_KEYDOWN
bad? The old code was calling TranslateMessage() as a result of WM_KEYDOWN
without any negative effects. I also applied your CL locally and made the change
and did not see negative effects. (I do understand that you need to return
POST_DISPATCH_NONE as a result of WM_CHAR events)

The ET_CANCEL_MODE event is dispatched as a result of WM_CANCELMODE

[afakhry, 2015/06/17 18:48:00]

If you want to see the negative effect I was talking about, try
TranslateMessage() before DispatchMessage() in WM_KEYDOWN. Open the wrench menu,
press the 'T' key (for new tab) see what happens. A new tab is opened, and the
letter 't' shows up in the omnibox. Clearly that's not what we want.

The old code needed to call TranslateMessage() as it was handling the selection
by mnemonics (MenuController::SelectByChar()) at WM_CHAR. In this CL however, at
WM_KEYDOWN we instead just DispatchMessage() which will lead to the event being
handled by MenuEventFilter::OnKeyEvent() which handles both
MenuController::OnKeyDown() and MenuController::SelectByChar().

[pkotwicz, 2015/06/18 19:45:57]

I tried it again. I get the same behavior that you do but only if I call
TranslateMessage() more than once as a result of WM_KEYDOWN (e.g. 2, 5, 10
times). I suspect that you were calling TranslateMessage() more than once.

I do not know why the behavior is different when TranslateMessage() is called
multiple times. It would be good to find out.

[pkotwicz, 2015/07/10 17:33:21]

Have you addressed this comment?

[afakhry, 2015/07/10 23:49:46]

Yes, I spent the whole day yesterday testing on Windows. As I described above,
in this patch, if we call TranslateMessage() before DispatchMessage() in
WM_KEYDOWN this will result in the insertion of mnemonic characters in the
omnibox if it is in focus. I involved oshima@ in the tests yesterday and we made
an experiment that led to filing this bug
https://code.google.com/p/chromium/issues/detail?id=508668

We also made another experiment (hack actually) if you insist on calling
TranslateMessage(). We will keep a MSG msg_; as a member here. Then the below
cases for WM_KEYDOWN and WM_CHAR become as the following:

case WM_KEYDOWN: {                                                      
  ui::KeyboardCode key_code = ui::KeyboardCodeFromNative(msg);
  if (key_code >= ui::VKEY_0 && key_code <= ui::VKEY_Z) {
    // Alpha-numeric key (mnemonic) should be handled at WM_CHAR.
    // Cache the msg for later and translate.
    msg_ = msg;
    TranslateMessage(&msg);
  } else {
    // Dispatch now to be handled now by MenuEventFilter.
    DispatchMessage(&msg);
  }
       
  should_perform_default = false;                                       
  break;                                                                
}                                                                       
case WM_CHAR: {                                                         
  // Dispatch the cached msg_.
  DispatchMessage(&msg_); 
                                     
  should_perform_default = false;                                       
  break;                                                                
}         

The above is a hack as you can see but it works. I'm not saying we should commit
such code, all I'm trying to do is to make you aware of the issue.

Let me know what you think. We can also request review from someone who's an
expert on Windows events handling.                                              
               

[pkotwicz, 2015/07/13 23:29:17]

Thank you for pointing me to the bug and for the explanation. I now understand
your point.

However, this CL causes a regression. Currently, on Windows and Windows only, if
I switch to using a French keyboard, bookmark www.google.com, set the bookmark's
name to "étrange", if I type "é" while the "Bookmarks" menu is open, Chrome
navigates to the bookmarked URL. This CL breaks this feature. It would be nice
to know if this feature is intentional or not. (I suspect that it is)

[afakhry, 2015/07/14 20:50:43]

I will try to investigate this later on Windows. I don't have a french keyboard
nor a Windows workstation and the only way for me to test this on Windows is on
my personal laptop, and you can imagine how frustrating and time consuming this
is. My guess is that we need to handle mnemnoics at WM_CHAR (rather than
WM_KEYDOWN) so that the character is in its final state (i.e. 'e' with an accent
rather than just 'e'). Maybe the hacky solution that I sent you above that
caches the MSG for later could fix this issue. Could you please try it if you
can? Thanks!

[pkotwicz, 2015/07/15 00:37:14]

The hacky solution does not work. The "é" key does not have a keycode between
ui::VKEY_0 and ui::VKEY_Z. This is what a French Canadian Keyboard looks like if
you are interested
http://ergocanada.com/products/keyboards/sk595_cdn_french_layout.gif

Caching the MSG does not sound like a good idea. The wParam attribute of the msg
struct is different in the WM_KEYDOWN and WM_CHAR messages.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646280(v=vs.85).aspx
vs
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646276(v=vs.85).aspx

Using your personal laptop to build Chrome sounds less than ideal. I do not have
a Windows machine either. The Waterloo office has an off-corp shared z600
Windows machine. It has served me well.

   DCHECK(menu_controller_->IsBlockingRun());
 
   bool should_quit = false;
   bool should_perform_default = true;
   if (menu_controller_->exit_type() == MenuController::EXIT_ALL ||
       menu_controller_->exit_type() == MenuController::EXIT_DESTROYED) {
     should_quit = true;
   } else {
     // NOTE: we don't get WM_ACTIVATE or anything else interesting in here.
     switch (msg.message) {
       case WM_CONTEXTMENU: {
         MenuItemView* item = menu_controller_->pending_state_.item;
         if (item && item->GetRootMenuItem() != item) {
           gfx::Point screen_loc(0, item->height());
           View::ConvertPointToScreen(item, &screen_loc);
           ui::MenuSourceType source_type = ui::MENU_SOURCE_MOUSE;
           if (GET_X_LPARAM(msg.lParam) == -1 && GET_Y_LPARAM(msg.lParam) == -1)
             source_type = ui::MENU_SOURCE_KEYBOARD;
           item->GetDelegate()->ShowContextMenu(
               item, item->GetCommand(), screen_loc, source_type);
         }
         should_quit = false;
         should_perform_default = false;
         break;
       }
 
-      // NOTE: focus wasn't changed when the menu was shown. As such, don't
-      // dispatch key events otherwise the focused window will get the events.
       case WM_KEYDOWN: {
-        bool result =
-            menu_controller_->OnKeyDown(ui::KeyboardCodeFromNative(msg));
-        TranslateMessage(&msg);
+        // We do nothing here as the key event will be processed by
+        // MenuEventFilter::OnKeyEvent() as a result of the following dispatch
+        // call. The MenuEventFilter will stop the event propagation, so the
+        // focused window will not get the event.
+        //
+        // NOTE: We don't call TranslateMessage() here as we don't want to
+        // produce a WM_CHAR which will result in inserting a character in a
+        // Textfield while the menu is still open or has just exited.
+        // MenuEventFilter::OnKeyEvent() takes care of everything.
+        DispatchMessage(&msg);
         should_perform_default = false;
-        should_quit = !result;
         break;
       }
       case WM_CHAR: {
-        should_quit = menu_controller_->SelectByChar(
-            static_cast<base::char16>(msg.wParam));
+        // We do nothing here. Selecting a menu item by Char is done at the
+        // MenuEventFilter::OnKeyEvent().
         should_perform_default = false;
         break;
       }
       case WM_KEYUP:
       case WM_SYSKEYUP:
         // We may have been shown on a system key, as such don't do anything
         // here. If another system key is pushed we'll get a WM_SYSKEYDOWN and
         // close the menu.
         should_quit = false;
         should_perform_default = false;
@@ skipping 13 matching lines @@
   }
 
   if (should_quit || menu_controller_->exit_type() != MenuController::EXIT_NONE)
     menu_controller_->TerminateNestedMessageLoop();
   return should_perform_default ? POST_DISPATCH_PERFORM_DEFAULT
                                 : POST_DISPATCH_NONE;
 }
 
 }  // namespace internal
 }  // namespace views