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

ui/views/view.cc

Index: ui/views/view.cc
diff --git a/ui/views/view.cc b/ui/views/view.cc
index 45467ab23dbfc0fdbf68fdd9ad1ef9532bd20e64..9232987ea608bec2b6b593c9d2da9977f4d11dbb 100644
--- a/ui/views/view.cc
+++ b/ui/views/view.cc
@@ -1146,7 +1146,21 @@ bool View::AcceleratorPressed(const ui::Accelerator& accelerator) {
 }
 
 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 -----------------------------------------------------------------------
@@ -2410,4 +2424,14 @@ bool View::DoDrag(const ui::LocatedEvent& event,
   return true;
 }
 
+bool View::IsChildWidget() const {
+  return GetWidget() && GetWidget()->GetTopLevelWidget() != GetWidget();
+}
+
+bool View::FocusInChildWidget() const {
+  return GetWidget() &&
+         GetWidget()->GetRootView()->Contains(
+             GetFocusManager()->GetFocusedView());
+}
+
 }  // namespace views