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

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

BUG=557558
Committed: https://crrev.com/ac440714860679857517ac7dd4c6282acbfa4033
Cr-Commit-Position: refs/heads/master@{#374787}

[meacer, 2015-12-22 23:23:53]

meacer@chromium.org changed reviewers:
+ msw@chromium.org

[meacer, 2015-12-22 23:23:53]

msw: Can you PTAL?

[msw, 2015-12-23 04:06:25]

Won't other menus (context, bookmark, etc.) require similar fixes?
lgtm as a temporary workaround, but we'll likely want a wider fix.
Can you take a quick look to see if that's achievable in this CL?

https://codereview.chromium.org/1544803004/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.h (right):

https://codereview.chromium.org/1544803004/diff/20001/ui/views/controls/butto...
ui/views/controls/button/custom_button.h:124: // button is inactive. In-menu
buttons in the hamburger menu such as zoom and
"Menu widgets are inactive, but buttons therein (eg. the app menu's zoom and
cut/copy/paste) should still handle accelerators. "

[meacer, 2015-12-24 21:40:02]

meacer@chromium.org changed reviewers:
+ sadrul@chromium.org

[meacer, 2015-12-24 21:40:02]

> Won't other menus (context, bookmark, etc.) require similar fixes?
lgtm as a temporary workaround, but we'll likely want a wider fix.
Can you take a quick look to see if that's achievable in this CL?

Just checked, context menus, other items in the app menu, bookmarks and a bunch
of other menus work fine with or without the patch.

https://codereview.chromium.org/1544803004/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.h (right):

https://codereview.chromium.org/1544803004/diff/20001/ui/views/controls/butto...
ui/views/controls/button/custom_button.h:124: // button is inactive. In-menu
buttons in the hamburger menu such as zoom and
On 2015/12/23 04:06:25, msw wrote:
> "Menu widgets are inactive, but buttons therein (eg. the app menu's zoom and
> cut/copy/paste) should still handle accelerators. "

Done.

[meacer, 2015-12-24 21:40:37]

sadrul: PTAL at ui/views/controls/button/custom_button.cc/h as owner

[sadrul, 2016-01-04 20:26:06]

On 2015/12/24 21:40:02, Mustafa Emre Acer wrote:
> > Won't other menus (context, bookmark, etc.) require similar fixes?
> lgtm as a temporary workaround, but we'll likely want a wider fix.
> Can you take a quick look to see if that's achievable in this CL?
> 
> Just checked, context menus, other items in the app menu, bookmarks and a
bunch
> of other menus work fine with or without the patch.

That's probably because those menus don't have CustomButtons in them?

I think the issue here is that MenuHost widget cannot be activated, and so
GetWidget()->IsActive() always returns false, causing the early return from
AcceleratorPressed().

A much better fix would be to do what sky@ had suggested in
https://codereview.chromium.org/1437523005#msg13, and block the accelerator
dispatch at the Widget level. If that is not possible, then I think you need to
update the check in CustomButton::AcceleratorPressed() to something like this:

  if (IsChildWidget() && !FocusInChildWidget())
    return false;
  bool in_toplevel_widget = GetWidget() && !IsChildWidget();
  if (in_toplevel_widget && GetWidget()->CanActivate() &&
!GetWidget()->IsActive())
    return false;

But I can't tell if that would still address the original bug that required the
change (can I be cc'ed to crbug.com/541415?)

[Mustafa Acer, 2016-01-20 23:19:59]

On 2016/01/04 20:26:06, sadrul wrote:
> On 2015/12/24 21:40:02, Mustafa Emre Acer wrote:
> > > Won't other menus (context, bookmark, etc.) require similar fixes?
> > lgtm as a temporary workaround, but we'll likely want a wider fix.
> > Can you take a quick look to see if that's achievable in this CL?
> > 
> > Just checked, context menus, other items in the app menu, bookmarks and a
> bunch
> > of other menus work fine with or without the patch.
> 
> That's probably because those menus don't have CustomButtons in them?
> 
> I think the issue here is that MenuHost widget cannot be activated, and so
> GetWidget()->IsActive() always returns false, causing the early return from
> AcceleratorPressed().
> 
> A much better fix would be to do what sky@ had suggested in
> https://codereview.chromium.org/1437523005#msg13, and block the accelerator
> dispatch at the Widget level.

Hmm, not sure how to do this. The only accelerator related method in Widget is
GetAccelerator, should I try to check if the widget is inactive and return false
from there if that's the case?

> If that is not possible, then I think you need to
> update the check in CustomButton::AcceleratorPressed() to something like this:
> 
>   if (IsChildWidget() && !FocusInChildWidget())
>     return false;
>   bool in_toplevel_widget = GetWidget() && !IsChildWidget();
>   if (in_toplevel_widget && GetWidget()->CanActivate() &&
> !GetWidget()->IsActive())
>     return false;
> 
> But I can't tell if that would still address the original bug that required
the
> change (can I be cc'ed to crbug.com/541415?)

[meacer, 2016-01-22 00:25:32]

On 2016/01/20 23:19:59, Mustafa Acer wrote:
> On 2016/01/04 20:26:06, sadrul wrote:
> > On 2015/12/24 21:40:02, Mustafa Emre Acer wrote:
> > > > Won't other menus (context, bookmark, etc.) require similar fixes?
> > > lgtm as a temporary workaround, but we'll likely want a wider fix.
> > > Can you take a quick look to see if that's achievable in this CL?
> > > 
> > > Just checked, context menus, other items in the app menu, bookmarks and a
> > bunch
> > > of other menus work fine with or without the patch.
> > 
> > That's probably because those menus don't have CustomButtons in them?
> > 
> > I think the issue here is that MenuHost widget cannot be activated, and so
> > GetWidget()->IsActive() always returns false, causing the early return from
> > AcceleratorPressed().
> > 
> > A much better fix would be to do what sky@ had suggested in
> > https://codereview.chromium.org/1437523005#msg13, and block the accelerator
> > dispatch at the Widget level.
> 
> Hmm, not sure how to do this. The only accelerator related method in Widget is
> GetAccelerator, should I try to check if the widget is inactive and return
false
> from there if that's the case?
> 
> > If that is not possible, then I think you need to
> > update the check in CustomButton::AcceleratorPressed() to something like
this:
> > 
> >   if (IsChildWidget() && !FocusInChildWidget())
> >     return false;
> >   bool in_toplevel_widget = GetWidget() && !IsChildWidget();
> >   if (in_toplevel_widget && GetWidget()->CanActivate() &&
> > !GetWidget()->IsActive())
> >     return false;
> > 
> > But I can't tell if that would still address the original bug that required
> the
> > change (can I be cc'ed to crbug.com/541415?)

sadrul: Ping for my question above ^

[sadrul, 2016-01-22 05:05:37]

On 2016/01/20 23:19:59, Mustafa Acer wrote:
> On 2016/01/04 20:26:06, sadrul wrote:
> > On 2015/12/24 21:40:02, Mustafa Emre Acer wrote:
> > > > Won't other menus (context, bookmark, etc.) require similar fixes?
> > > lgtm as a temporary workaround, but we'll likely want a wider fix.
> > > Can you take a quick look to see if that's achievable in this CL?
> > > 
> > > Just checked, context menus, other items in the app menu, bookmarks and a
> > bunch
> > > of other menus work fine with or without the patch.
> > 
> > That's probably because those menus don't have CustomButtons in them?
> > 
> > I think the issue here is that MenuHost widget cannot be activated, and so
> > GetWidget()->IsActive() always returns false, causing the early return from
> > AcceleratorPressed().
> > 
> > A much better fix would be to do what sky@ had suggested in
> > https://codereview.chromium.org/1437523005#msg13, and block the accelerator
> > dispatch at the Widget level.
> 
> Hmm, not sure how to do this. The only accelerator related method in Widget is
> GetAccelerator, should I try to check if the widget is inactive and return
false
> from there if that's the case?

I think View::CanHandleAccelerators() override is the place to make the changes.
So the IsChildWidget()/FocusInChildWidget() checks would go out of
CustomButton::AcceleratorPressed(), into View::CanHandleAccelerators():

bool View::CanHandleAccelerators() const {
  const Widget* widget = GetWidget();
  if (!enabled() || !IsDrawn() || !widget || !widget->IsVisible())
    return false;

  bool toplevel = widget->GetTopLevelWidget() == widget;
  if (toplevel && widget->CanActivate() && !widget->IsActive())
    return false;
  bool child_widget = !toplevel;
  if (child_widget &&
!child_widget->GetRootView()->Contains(GetFocusManager()->GetFocusedView()))
    return false;
  return true;
}

Would that work?

[meacer, 2016-01-22 22:12:00]

On 2016/01/22 05:05:37, sadrul wrote:
> On 2016/01/20 23:19:59, Mustafa Acer wrote:
> > On 2016/01/04 20:26:06, sadrul wrote:
> > > On 2015/12/24 21:40:02, Mustafa Emre Acer wrote:
> > > > > Won't other menus (context, bookmark, etc.) require similar fixes?
> > > > lgtm as a temporary workaround, but we'll likely want a wider fix.
> > > > Can you take a quick look to see if that's achievable in this CL?
> > > > 
> > > > Just checked, context menus, other items in the app menu, bookmarks and
a
> > > bunch
> > > > of other menus work fine with or without the patch.
> > > 
> > > That's probably because those menus don't have CustomButtons in them?
> > > 
> > > I think the issue here is that MenuHost widget cannot be activated, and so
> > > GetWidget()->IsActive() always returns false, causing the early return
from
> > > AcceleratorPressed().
> > > 
> > > A much better fix would be to do what sky@ had suggested in
> > > https://codereview.chromium.org/1437523005#msg13, and block the
accelerator
> > > dispatch at the Widget level.
> > 
> > Hmm, not sure how to do this. The only accelerator related method in Widget
is
> > GetAccelerator, should I try to check if the widget is inactive and return
> false
> > from there if that's the case?
> 
> I think View::CanHandleAccelerators() override is the place to make the
changes.
> So the IsChildWidget()/FocusInChildWidget() checks would go out of
> CustomButton::AcceleratorPressed(), into View::CanHandleAccelerators():
> 
> bool View::CanHandleAccelerators() const {
>   const Widget* widget = GetWidget();
>   if (!enabled() || !IsDrawn() || !widget || !widget->IsVisible())
>     return false;
> 
>   bool toplevel = widget->GetTopLevelWidget() == widget;
>   if (toplevel && widget->CanActivate() && !widget->IsActive())
>     return false;
>   bool child_widget = !toplevel;
>   if (child_widget &&
> !child_widget->GetRootView()->Contains(GetFocusManager()->GetFocusedView()))
>     return false;
>   return true;
> }
> 
> Would that work?

Thanks for the pointer. Yes, it does seem to work (patchset #4) with manual
testing. I'm getting test failures in views_unittests but they happen with or
without the patch anyways.
view_unittest.cc has some accelerator handling code, but it's ifdefed out with a
TODO so I deleted the unit test code altogether. Can you PTAL?

[sadrul, 2016-01-26 17:21:49]

Looks like we are losing test-coverage? Can we have an equivalent test for the
new code?

Can you check if the test failures on the chromeos trybots relevant to this
change?

[meacer, 2016-01-26 17:59:08]

On 2016/01/26 17:21:49, sadrul wrote:
> Looks like we are losing test-coverage? Can we have an equivalent test for the
> new code?

There is accelerator tests in views code, but it's ifdefed out (and doesn't
build when put back in). What would be the best place to test this code?

> 
> Can you check if the test failures on the chromeos trybots relevant to this
> change?

Looking now.

[meacer, 2016-01-28 00:37:38]

sadrul: Can you PTAL? I've added the tests back.

The test failure on ChromeOS was caused by this patch: An extension popup was
not properly handling the accelerator that switches tabs (ctrl + tab) on
ChromeOS. However, ChromeOS seems to already handle the accelerators fine
without the extra logic added here, so I ifdefed out the logic on ChromeOS. I've
verified that this works well with the previous security bug.

[sadrul, 2016-01-28 08:10:59]

sadrul@chromium.org changed reviewers:
+ sky@chromium.org

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

+sky@ as well

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
ui/views/view.cc:1162: #endif
I don't think this behaviour should be OS specific.

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.h#newcode...
ui/views/view.h:1437: virtual bool FocusInChildWidget() const;
These should not be virtual methods. It should be easy to set-up the hierarchy
in tests.

[sky, 2016-01-28 16:23:41]

On 2016/01/28 00:37:38, Mustafa Emre Acer wrote:
> sadrul: Can you PTAL? I've added the tests back.
> 
> The test failure on ChromeOS was caused by this patch: An extension popup was
> not properly handling the accelerator that switches tabs (ctrl + tab) on
> ChromeOS. However, ChromeOS seems to already handle the accelerators fine
> without the extra logic added here, so I ifdefed out the logic on ChromeOS.
I've
> verified that this works well with the previous security bug.

Why is this specific to !chromeos?

[sky, 2016-01-28 16:23:46]

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
ui/views/view.cc:1158: if ((IsChildWidget() && !FocusInChildWidget()) ||
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]

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
ui/views/view.cc:1158: if ((IsChildWidget() && !FocusInChildWidget()) ||
On 2016/01/28 16:23:45, sky wrote:
> I wouldn't bother with the new functions, instead:
> const bool is_in_child_widget = widget->GetTopLevelWidget() != widget;
> same thing for IsFocusInChildWidget.

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

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
ui/views/view.cc:1162: #endif
On 2016/01/28 08:11:00, sadrul wrote:
> I don't think this behaviour should be OS specific.

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]

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
ui/views/view.cc:1162: #endif
On 2016/01/28 18:39:26, Mustafa Emre Acer wrote:
> On 2016/01/28 08:11:00, sadrul wrote:
> > I don't think this behaviour should be OS specific.
> 
> 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.

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]

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
ui/views/view.cc:1162: #endif
On 2016/01/28 21:18:38, sky wrote:
> https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
> File ui/views/view.cc (right):
> 
>
https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
> ui/views/view.cc:1162: #endif
> On 2016/01/28 18:39:26, Mustafa Emre Acer wrote:
> > On 2016/01/28 08:11:00, sadrul wrote:
> > > I don't think this behaviour should be OS specific.
> > 
> > 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.
> 
> 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?

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?

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

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
ui/views/view.cc:1158: if ((IsChildWidget() && !FocusInChildWidget()) ||
On 2016/01/28 18:39:26, Mustafa Emre Acer wrote:
> On 2016/01/28 16:23:45, sky wrote:
> > I wouldn't bother with the new functions, instead:
> > const bool is_in_child_widget = widget->GetTopLevelWidget() != widget;
> > same thing for IsFocusInChildWidget.
> 
> This was so that I could override them in unit tests. I'm now looking at
> building a proper view hierarchy now.

Done now.

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.h#newcode...
ui/views/view.h:1437: virtual bool FocusInChildWidget() const;
On 2016/01/28 08:11:00, sadrul wrote:
> These should not be virtual methods. It should be easy to set-up the hierarchy
> in tests.

Done.

[sky, 2016-01-29 17:29:39]

On Thu, Jan 28, 2016 at 4:14 PM,  <meacer@chromium.org> wrote:
>
>
> https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
> File ui/views/view.cc (right):
>
>
https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
> ui/views/view.cc:1162: #endif
> On 2016/01/28 21:18:38, sky wrote:
>> https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
>> File ui/views/view.cc (right):
>>
>>
>
https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
>> ui/views/view.cc:1162: #endif
>> On 2016/01/28 18:39:26, Mustafa Emre Acer wrote:
>> > On 2016/01/28 08:11:00, sadrul wrote:
>> > > I don't think this behaviour should be OS specific.
>> >
>> > 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.
>>
>> 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?
>
> 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?

I think you're asking me to debug it. Do you have a windows and linux
box that you can track down why the behavior differs?

  -Scott

>
> https://codereview.chromium.org/1544803004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[meacer, 2016-02-03 02:04:46]

On 2016/01/29 17:29:39, sky wrote:
> On Thu, Jan 28, 2016 at 4:14 PM,  <mailto:meacer@chromium.org> wrote:
> >
> >
> > https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
> > File ui/views/view.cc (right):
> >
> >
>
https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
> > ui/views/view.cc:1162: #endif
> > On 2016/01/28 21:18:38, sky wrote:
> >> https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
> >> File ui/views/view.cc (right):
> >>
> >>
> >
>
https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
> >> ui/views/view.cc:1162: #endif
> >> On 2016/01/28 18:39:26, Mustafa Emre Acer wrote:
> >> > On 2016/01/28 08:11:00, sadrul wrote:
> >> > > I don't think this behaviour should be OS specific.
> >> >
> >> > 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.
> >>
> >> 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?
> >
> > 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?
> 
> I think you're asking me to debug it. Do you have a windows and linux
> box that you can track down why the behavior differs?
> 
>   -Scott
> 
> >
> > https://codereview.chromium.org/1544803004/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

Ok, tracked this down. To clarify, the behavior was different between ChromeOS
and Windows/Linux and not between Windows and Linux. The difference in aura is
most likely caused by this CL which adds a PreTarget event handler for focused
windows: https://crrev.com/62713004

As a result of that CL, during event processing aura windows have
FocusManagerEventHandler as one of the PreTarget event handlers, which causes
the modal dialog to consume the accelerator before it reaches the omnibox.
ChromeOS or Mac don't have this, so omnibox gets to handle the accelerator.

Removing FocusManagerEventHandler altogether seems to work fine: Accelerator
handling becomes identical on all platforms, and I can't repro the bugs fixed by
crrev.com/62713004 anymore. I don't know the full ramifications of doing this
though, hopefully it won't break yet another stuff.

So there are two options here:

1) Remove FocusManagerEventHandler in desktop_native_widget_aura.cc (done in
https://crrev.com/1660173003/)
2) Keep the checks and the ifdefs in this CL, but check for aura instead of
ChromeOS.

(1) seems preferable to me. Thoughts?

[sky, 2016-02-03 16:46:38]

On Tue, Feb 2, 2016 at 6:04 PM,  <meacer@chromium.org> wrote:
> On 2016/01/29 17:29:39, sky wrote:
>> On Thu, Jan 28, 2016 at 4:14 PM,  <mailto:meacer@chromium.org> wrote:
>> >
>> >
>> > https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
>> > File ui/views/view.cc (right):
>> >
>> >
>>
>
https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
>> > ui/views/view.cc:1162: #endif
>> > On 2016/01/28 21:18:38, sky wrote:
>> >> https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
>> >> File ui/views/view.cc (right):
>> >>
>> >>
>> >
>>
>
https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
>> >> ui/views/view.cc:1162: #endif
>> >> On 2016/01/28 18:39:26, Mustafa Emre Acer wrote:
>> >> > On 2016/01/28 08:11:00, sadrul wrote:
>> >> > > I don't think this behaviour should be OS specific.
>> >> >
>> >> > 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.
>> >>
>> >> 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?
>> >
>> > 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?
>>
>> I think you're asking me to debug it. Do you have a windows and linux
>> box that you can track down why the behavior differs?
>>
>>   -Scott
>>
>> >
>> > https://codereview.chromium.org/1544803004/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> Ok, tracked this down. To clarify, the behavior was different between
> ChromeOS
> and Windows/Linux and not between Windows and Linux. The difference in aura
> is
> most likely caused by this CL which adds a PreTarget event handler for
> focused
> windows: https://crrev.com/62713004

That patch landed 2 years ago, yet the regression is recent. Have you
tried bisecting when this regression happened?

  -Scott
>
> As a result of that CL, during event processing aura windows have
> FocusManagerEventHandler as one of the PreTarget event handlers, which
> causes
> the modal dialog to consume the accelerator before it reaches the omnibox.
> ChromeOS or Mac don't have this, so omnibox gets to handle the accelerator.
>
> Removing FocusManagerEventHandler altogether seems to work fine: Accelerator
> handling becomes identical on all platforms, and I can't repro the bugs
> fixed by
> crrev.com/62713004 anymore. I don't know the full ramifications of doing
> this
> though, hopefully it won't break yet another stuff.
>
> So there are two options here:
>
> 1) Remove FocusManagerEventHandler in desktop_native_widget_aura.cc (done in
> https://crrev.com/1660173003/)
> 2) Keep the checks and the ifdefs in this CL, but check for aura instead of
> ChromeOS.
>
> (1) seems preferable to me. Thoughts?
>
>
>
>
> https://codereview.chromium.org/1544803004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[meacer, 2016-02-03 20:04:57]

On 2016/02/03 16:46:38, sky wrote:
> On Tue, Feb 2, 2016 at 6:04 PM,  <mailto:meacer@chromium.org> wrote:
> > On 2016/01/29 17:29:39, sky wrote:
> >> On Thu, Jan 28, 2016 at 4:14 PM,  <mailto:meacer@chromium.org> wrote:
> >> >
> >> >
> >> > https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
> >> > File ui/views/view.cc (right):
> >> >
> >> >
> >>
> >
>
https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
> >> > ui/views/view.cc:1162: #endif
> >> > On 2016/01/28 21:18:38, sky wrote:
> >> >> https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc
> >> >> File ui/views/view.cc (right):
> >> >>
> >> >>
> >> >
> >>
> >
>
https://codereview.chromium.org/1544803004/diff/80001/ui/views/view.cc#newcod...
> >> >> ui/views/view.cc:1162: #endif
> >> >> On 2016/01/28 18:39:26, Mustafa Emre Acer wrote:
> >> >> > On 2016/01/28 08:11:00, sadrul wrote:
> >> >> > > I don't think this behaviour should be OS specific.
> >> >> >
> >> >> > 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.
> >> >>
> >> >> 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?
> >> >
> >> > 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?
> >>
> >> I think you're asking me to debug it. Do you have a windows and linux
> >> box that you can track down why the behavior differs?
> >>
> >>   -Scott
> >>
> >> >
> >> > https://codereview.chromium.org/1544803004/
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups
> >> "Chromium-reviews" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an
> > email
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> > Ok, tracked this down. To clarify, the behavior was different between
> > ChromeOS
> > and Windows/Linux and not between Windows and Linux. The difference in aura
> > is
> > most likely caused by this CL which adds a PreTarget event handler for
> > focused
> > windows: https://crrev.com/62713004
> 
> That patch landed 2 years ago, yet the regression is recent. Have you
> tried bisecting when this regression happened?
> 

The regression this CL is trying to fix is caused by
https://codereview.chromium.org/1437523005, which was landed in November 2015,
fixing https://crbug.com/541415.

At the time I didn't realize that bug was a regression, and the bisect points
that turning on aura caused that regression
(https://chromium.googlesource.com/chromium/src/+/aa5219d8ce13c523e8be330405b7...)

So while the CL I pointed to in my previous comment isn't accurate, it's not far
off: That CL is moving around things, and the original regression has been
around for 2+ years.

>   -Scott
> >
> > As a result of that CL, during event processing aura windows have
> > FocusManagerEventHandler as one of the PreTarget event handlers, which
> > causes
> > the modal dialog to consume the accelerator before it reaches the omnibox.
> > ChromeOS or Mac don't have this, so omnibox gets to handle the accelerator.
> >
> > Removing FocusManagerEventHandler altogether seems to work fine: Accelerator
> > handling becomes identical on all platforms, and I can't repro the bugs
> > fixed by
> > crrev.com/62713004 anymore. I don't know the full ramifications of doing
> > this
> > though, hopefully it won't break yet another stuff.
> >
> > So there are two options here:
> >
> > 1) Remove FocusManagerEventHandler in desktop_native_widget_aura.cc (done in
> > https://crrev.com/1660173003/)
> > 2) Keep the checks and the ifdefs in this CL, but check for aura instead of
> > ChromeOS.
> >
> > (1) seems preferable to me. Thoughts?
> >
> >
> >
> >
> > https://codereview.chromium.org/1544803004/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2016-02-03 22:07:55]

I think this is working around what I suggested in the earlier patch. In
particular I suggested having event dispatching not dispatch to invalid widgets.
I was a bit worried that would cause regressions, but since we're now dealing
with regressions I think we should revisit what I suggested. Fix this in the
accelerator dispatching code and not here. In other words, view/button shouldn't
have to do anything special. Accelerators are only notified it active/focused.

[meacer, 2016-02-03 22:46:54]

On 2016/02/03 22:07:55, sky wrote:
> I think this is working around what I suggested in the earlier patch. In
> particular I suggested having event dispatching not dispatch to invalid
widgets.
> I was a bit worried that would cause regressions, but since we're now dealing
> with regressions I think we should revisit what I suggested. Fix this in the
> accelerator dispatching code and not here. In other words, view/button
shouldn't
> have to do anything special. Accelerators are only notified it active/focused.

What's the right place to do this? AcceleratorManager doesn't know about
active/focused status, and FocusManager doesn't know the list of accelerator
targets.

[sky, 2016-02-04 03:45:03]

Ok, what you have is the right approach. My only remaining concern is the ifdef.
You say it isn't strictly necessary on chromeos, but is there a reason to ifdef
it out? It should be a no-op there, right?

[meacer, 2016-02-04 20:46:04]

It's actually the other way: the extra logic and the ifdef is necessary only on
aura, since that's where we have FocusManagerEventHandler adding the focused
view as a pretarget. I've removed the ifdef from the test. PTAL?

(Removing FocusManagerEventHandler breaks find-in-page and ctrl+enter sequence,
so I've closed that alternate CL.)

[sky, 2016-02-04 22:40:25]

LGTM

[msw, 2016-02-04 22:45:36]

lgtm with nits and a q.

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view.cc#newco...
ui/views/view.cc:1150: if (!enabled() || !IsDrawn() || !widget ||
!widget->IsVisible()) {
nit: curlies not needed

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view.cc#newco...
ui/views/view.cc:1159: bool child = GetWidget() &&
GetWidget()->GetTopLevelWidget() != GetWidget();
nit: re-use |widget| here and below.

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view.cc#newco...
ui/views/view.cc:1163: if ((child && !focus_in_child) || (!child &&
!widget->IsActive())) {
nit: curlies not needed

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view_unittest...
ui/views/view_unittest.cc:2128: EXPECT_FALSE(widget->IsActive());
q: Odd, doesn't Show also activate the widget?

[meacer, 2016-02-06 01:39:39]

Quick update: The one test broken on ChromeOS before is still broken. The test
sends Ctrl+Tab to an extension popup and expects a tab switch.

On ChromeOS, view->IsActive()==false for the widget that receives the
accelerator (BrowserFrameAsh).
On Linux, the same check returns true (DesktopWindowTreeHostX11->IsActive()).

I've checked window activation sequences during the test and they seem to be
identical in both ChromeOS and Linux. Investigating why IsActive check is
returning different values for BrowserFrameAsh vs DesktopWindowTreeHostX11.

[meacer, 2016-02-09 20:33:32]

Added back the ChromeOS check, because widget->IsActive is different on ChromeOS
vs Linux when a browser popup is open and an accelerator arrives (which breaks
BrowserActionInteractiveTest.TabSwitchClosesPopup).

Please take another look. I'll submit this later today if there are no
objections.

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view.cc#newco...
ui/views/view.cc:1150: if (!enabled() || !IsDrawn() || !widget ||
!widget->IsVisible()) {
On 2016/02/04 22:45:36, msw wrote:
> nit: curlies not needed

Done.

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view.cc#newco...
ui/views/view.cc:1159: bool child = GetWidget() &&
GetWidget()->GetTopLevelWidget() != GetWidget();
On 2016/02/04 22:45:36, msw wrote:
> nit: re-use |widget| here and below.

Done.

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view.cc#newco...
ui/views/view.cc:1163: if ((child && !focus_in_child) || (!child &&
!widget->IsActive())) {
On 2016/02/04 22:45:36, msw wrote:
> nit: curlies not needed

Done.

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/1544803004/diff/120001/ui/views/view_unittest...
ui/views/view_unittest.cc:2128: EXPECT_FALSE(widget->IsActive());
On 2016/02/04 22:45:36, msw wrote:
> q: Odd, doesn't Show also activate the widget?

It doesn't, need to explicitly call Activate.

[sky, 2016-02-09 21:20:25]

SLGTM

[meacer, 2016-02-10 18:21:18]

The CQ bit was checked by meacer@chromium.org

[meacer, 2016-02-10 18:21:18]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1544803004/#ps140001
(title: "Disable the check on ChromeOS (again). Accelerator handling in
extension popups is different betwee…")

[commit-bot: I haz the power, 2016-02-10 18:23:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1544803004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1544803004/140001

[commit-bot: I haz the power, 2016-02-10 19:08:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-10 19:08:12]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

Transient error: No JSON object could be decoded

[commit-bot: I haz the power, 2016-02-10 19:11:19]

The CQ bit was unchecked by commit-bot@chromium.org

[meacer, 2016-02-10 23:29:50]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2016-02-10 23:33:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1544803004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1544803004/140001

[commit-bot: I haz the power, 2016-02-10 23:43:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-16 22:32:44]

Description was changed from

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

BUG=557558
==========

to

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

BUG=557558

Committed: https://crrev.com/ac440714860679857517ac7dd4c6282acbfa4033
Cr-Commit-Position: refs/heads/master@{#374787}
==========

[commit-bot: I haz the power, 2016-02-16 22:32:45]

Patchset 8 (id:??) landed as
https://crrev.com/ac440714860679857517ac7dd4c6282acbfa4033
Cr-Commit-Position: refs/heads/master@{#374787}