Fix accelerator handling for in-menu buttons in the app menu.

ui/views/view.cc

 // Copyright (c) 2012 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.
 
 #define _USE_MATH_DEFINES // For VC++ to get M_PI. This has to be first.
 
 #include "ui/views/view.h"
 
 #include <algorithm>
 #include <cmath>
@@ skipping 1128 matching lines @@
 void View::ResetAccelerators() {
   if (accelerators_.get())
     UnregisterAccelerators(false);
 }
 
 bool View::AcceleratorPressed(const ui::Accelerator& accelerator) {
   return false;
 }
 
 bool View::CanHandleAccelerators() const {
-  return enabled() && IsDrawn() && GetWidget() && GetWidget()->IsVisible();
+  const Widget* widget = GetWidget();
+  if (!enabled() || !IsDrawn() || !widget || !widget->IsVisible()) {
+    return false;
+  }
+  #if !defined(OS_CHROMEOS)
+  // Should only handle accelerators when active. However, only top level
+  // widgets can be active, so for child widgets check if they are focused
+  // instead. This check is disabled on ChromeOS as ChromeOS already handles
+  // this logic properly.
+  if ((IsChildWidget() && !FocusInChildWidget()) ||

[sky, 2016/01/28 16:23:45]

I wouldn't bother with the new functions, instead:
const bool is_in_child_widget = widget->GetTopLevelWidget() != widget;
same thing for IsFocusInChildWidget.

[meacer, 2016/01/28 18:39:26]

This was so that I could override them in unit tests. I'm now looking at
building a proper view hierarchy now.

[meacer, 2016/01/29 00:37:36]

Done now.

+      (!IsChildWidget() && !widget->IsActive())) {
+    return false;
+  }
+  #endif

[sadrul, 2016/01/28 08:11:00]

I don't think this behaviour should be OS specific.

[meacer, 2016/01/28 18:39:26]

Sure, but ChromeOS seems to handle accelerators fine without this logic. I'm not
sure what would be the alternative here other than doing this.

[sky, 2016/01/28 21:18:38]

What I don't understand is how this works in chromeos, but not windows. What
path works on chromeos that doesn't work on windows?

[meacer, 2016/01/29 00:14:22]

For the original bug (https://crbug.com/541415): the omnibox correctly handles
the accelerator instead of the auth dialog on ChromeOS. There, the code never
hits View::CanHandleAccelerators as opposed to other platforms, not sure why.
Since View::CanHandleAccelerators is never hit on ChromeOS for that bug, the
extra logic added here doesn't help, and in fact breaks a test case on ChromeOS
too (BrowserActionInteractiveTest.TabSwitchClosesPopup).

I was hoping you (UI folks) could shed some light on this behavior, as I'm not
familiar with UI code to fully understand what's going on here, particularly why
View::CanHandleAccelerators is not hit when the user hits enter in the omnibox
while a modal dialog is open. What other code paths could this be going through?

+  return true;
 }
 
 // Focus -----------------------------------------------------------------------
 
 bool View::HasFocus() const {
   const FocusManager* focus_manager = GetFocusManager();
   return focus_manager && (focus_manager->GetFocusedView() == this);
 }
 
 View* View::GetNextFocusableView() {
@@ skipping 1243 matching lines @@
 
   // Message the RootView to do the drag and drop. That way if we're removed
   // the RootView can detect it and avoid calling us back.
   gfx::Point widget_location(event.location());
   ConvertPointToWidget(this, &widget_location);
   widget->RunShellDrag(this, data, widget_location, drag_operations, source);
   // WARNING: we may have been deleted.
   return true;
 }
 
+bool View::IsChildWidget() const {
+  return GetWidget() && GetWidget()->GetTopLevelWidget() != GetWidget();
+}
+
+bool View::FocusInChildWidget() const {
+  return GetWidget() &&
+         GetWidget()->GetRootView()->Contains(
+             GetFocusManager()->GetFocusedView());
+}
+
 }  // namespace views