fix shortcuts unexpectedly working in modal dialog

fix shortcuts unexpectedly working in modal dialog

Added call to focus manager to suspend shortcut handling when in the Join WIFI modal dialog.

BUG=134093
TEST=Open some windows; From system tray -> Ethernet -> Join other...; When modal dialog is up, press various shortcuts, observe their action does NOT take place and focus remains on dialog. Exit dialog and observe that shortcuts work again.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159189

[Daniel Erat, 2012-09-27 22:24:30]

https://chromiumcodereview.appspot.com/10993067/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

https://chromiumcodereview.appspot.com/10993067/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/options/wifi_config_view.cc:381:
views::FocusManager::set_shortcut_handling_suspended(false);
the fact that FocusManager doesn't keep a count of how many times it's been
asked to suspend/unsuspend shortcut handling makes me a bit nervous.  is it
guaranteed that no one else can make additional calls to suspend/unsuspend while
this view is shown?

[sschmitz, 2012-09-27 23:19:23]

Hmm... good point. I see suspending shortcut handling only being used
in ./chrome/browser/ui/webui/extensions/command_handler.cc
  web_ui()->RegisterMessageCallback("setShortcutHandlingSuspended" ... it
eventually comes down to the FocusManager.

I added counting (of suspend/un-suspend calls to FocusManager) on my
desktop just to try it out. It works fine in this case. But now I'm worried
that someone relies on being able to suspend several times (for ease of
logic) but only to unsuspend once. That would now be broken.
What do you think?

I wonder whether the risk is low to leave it. We are in a modal view, so
only non-user initiated stuff could be happening. And if we un-suspend
(when the dialog goes away) when we should not, the consequences are not
dire.

Or do you suggest to add counting of suspend/unsuspend calls to
FocusManager?


On Thu, Sep 27, 2012 at 3:24 PM, <derat@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10993067/diff/1/**
>
chrome/browser/chromeos/**options/wifi_config_view.cc<https://chromiumcodereview.appspot.com/10993067/diff/1/chrome/browser/chromeos/options/wifi_config_view.cc>
> File chrome/browser/chromeos/**options/wifi_config_view.cc (right):
>
> https://chromiumcodereview.**appspot.com/10993067/diff/1/**
>
chrome/browser/chromeos/**options/wifi_config_view.cc#**newcode381<https://chromiumcodereview.appspot.com/10993067/diff/1/chrome/browser/chromeos/options/wifi_config_view.cc#newcode381>
> chrome/browser/chromeos/**options/wifi_config_view.cc:**381:
> views::FocusManager::set_**shortcut_handling_suspended(**false);
> the fact that FocusManager doesn't keep a count of how many times it's
> been asked to suspend/unsuspend shortcut handling makes me a bit
> nervous.  is it guaranteed that no one else can make additional calls to
> suspend/unsuspend while this view is shown?
>
>
https://chromiumcodereview.**appspot.com/10993067/<https://chromiumcodereview...
>

[Daniel Erat, 2012-09-27 23:32:21]

LGTM

I think I'd recommend just calling it as-is now, for the sake of keeping this
change simple and not possibly breaking other stuff.  Cc-ing Finnur, who added
this in the first place, in case he has thoughts about keeping a count instead.

[commit-bot: I haz the power, 2012-09-28 00:13:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/10993067/1

[commit-bot: I haz the power, 2012-09-28 03:11:56]

Change committed as 159189

[Finnur, 2012-09-28 12:08:33]

Wow... I don't think this is the right fix. :/

The purpose of the suspend function is to temporarily disable Extension Command
shortcuts while people are configuring new ones in the shortcut config UI. So
that, if you want to set Ctrl+F as your shortcut, the request isn't treated as
'open the find box' (and subsequently eaten).

But the thing is that F5 cannot be an Extension Command (we require modifiers
like Ctrl or Alt) so I don't see why that is enabled when the dialog is modal
and I think using the suspend function is papering over the problem because I
suspect these two are not related.

Now, I don't have a Chrome OS dev environment, but if you can reproduce this
somehow on Windows I'd be happy to take a look and debug it in Visual Studio.
Alternatively, I think you should dig deeper and see why the shortcuts are
suddenly active while the dialog is up.

[sschmitz, 2012-09-28 16:06:13]

Thanks for the feedback. I will investigate more. Too bad, I thought I
found a proper and minimally invasive way to fix it. I found out a few
things:

1. Shortcuts handled by chrome are indeed disabled (in this situation as
expected). These shortcuts are defined
in ./chrome/browser/ui/views/accelerator_table.cc:

2. Shortcuts handled by ash seem to be active in this situation
(unexpectedly). These shortcuts are defined
in ./ash/accelerators/accelerator_table.cc

These two types of shortcuts are handled in different call-trees. Below are
elided call-trees for handling these shortcuts (one for chrome and for ash;
when they are both enabled):

Elided Backtrace for a Chrome shortcut:
(gdb) bt
#0  chrome::CloseTab (browser=0x7fffd6a06b80) at
../../chrome/browser/ui/browser_commands.cc:426
#1  chrome::BrowserCommandController::ExecuteCommandWithDisposition
(this=0x7fffd2bdb420, id=34015, disposition=CURRENT_TAB) at
../../chrome/browser/ui/browser_command_controller.cc:360
#2  CommandUpdater::ExecuteCommandWithDisposition (this=0x7fffd2bdb450,
id=34015, disposition=CURRENT_TAB) at
../../chrome/browser/command_updater.cc:52
#3  CommandUpdater::ExecuteCommand (this=0x7fffd2bdb450, id=34015) at
../../chrome/browser/command_updater.cc:45
#4  chrome::ExecuteCommand (browser=0x7fffd6a06b80, command=34015) at
../../chrome/browser/ui/browser_commands.cc:190
#5  BrowserView::PreHandleKeyboardEvent (this=0x7fffd24b6c00, event=...,
is_keyboard_shortcut=0x7fffffffb20d) at
../../chrome/browser/ui/views/frame/browser_view.cc:1302
#6  Browser::PreHandleKeyboardEvent (this=0x7fffd6a06b80,
source=0x7fffd2b77200, event=..., is_keyboard_shortcut=0x7fffffffb20d) at
../../chrome/browser/ui/browser.cc:1218
#7  WebContentsImpl::PreHandleKeyboardEvent (this=0x7fffd2b77200,
event=..., is_keyboard_shortcut=0x7fffffffb20d) at
../../content/browser/web_contents/web_contents_impl.cc:1224
#8  content::RenderWidgetHostImpl::ForwardKeyboardEvent
(this=0x7fffd2405a08, key_event=...) at
../../content/browser/renderer_host/render_widget_host_impl.cc:985
#9  content::RenderViewHostImpl::ForwardKeyboardEvent (this=0x7fffd2405a00,
key_event=...) at
../../content/browser/renderer_host/render_view_host_impl.cc:1634
#10 content::RenderWidgetHostViewAura::OnKeyEvent (this=0x7fffd670ad00,
event=0x7fffffffbc70) at
../../content/browser/renderer_host/render_widget_host_view_aura.cc:1458
#11 ui::EventTarget::OnKeyEvent (this=0x7fffd2b70020, event=0x7fffffffbc70)
at ../../ui/base/events/event_target.cc:90

Elided Backtrace for an Ash shortcut:
(gdb) bt
#0  ash::(anonymous namespace)::HandleNewWindow (is_incognito=false) at
../../ash/accelerators/accelerator_controller.cc:148
#1  ash::AcceleratorController::PerformAction (this=0x7fffe5ac5100,
action=22, accelerator=...) at
../../ash/accelerators/accelerator_controller.cc:486
#2  ash::AcceleratorController::AcceleratorPressed (this=0x7fffe5ac5100,
accelerator=...) at ../../ash/accelerators/accelerator_controller.cc:764
#3  ui::AcceleratorManager::Process (this=0x7fffdbb59240, accelerator=...)
at ../../ui/base/accelerators/accelerator_manager.cc:88
#4  ash::AcceleratorController::Process (this=0x7fffe5ac5100,
accelerator=...) at ../../ash/accelerators/accelerator_controller.cc:381
#5  ash::AshFocusManagerFactory::Delegate::ProcessAccelerator
(this=0x7fffd7f36d10, accelerator=...) at
../../ash/accelerators/focus_manager_factory.cc:26
#6  views::FocusManager::ProcessAccelerator (this=0x7fffd1198930,
accelerator=...) at ../../ui/views/focus/focus_manager.cc:443
#7  views::FocusManager::OnKeyEvent (this=0x7fffd1198930, event=...) at
../../ui/views/focus/focus_manager.cc:149
#8  views::NativeWidgetAura::DispatchKeyEventPostIME (this=0x7fffd672fa50,
key=...) at ../../ui/views/widget/native_widget_aura.cc:706
#9  views::InputMethodBase::DispatchKeyEventPostIME (this=0x7fffd1b27e60,
key=...) at ../../ui/views/ime/input_method_base.cc:103
#10 views::InputMethodBridge::DispatchKeyEvent (this=0x7fffd1b27e60,
key=...) at ../../ui/views/ime/input_method_bridge.cc:59
#11 views::NativeWidgetAura::OnKeyEvent (this=0x7fffd672fa50,
event=0x7fffffffb760) at ../../ui/views/widget/native_widget_aura.cc:856
#12 ui::EventTarget::OnKeyEvent (this=0x7fffd131fe20, event=0x7fffffffb760)
at ../../ui/base/events/event_target.cc:90

As you can see the ash shortcut goes through the FocusManager. So when I
found FocusManager::set_shortcut_handling_suspended, I thought "Bingo".
I will look some more. Please also tell me what you think?

On Fri, Sep 28, 2012 at 5:08 AM, <finnur@chromium.org> wrote:

> Wow... I don't think this is the right fix. :/
>
> The purpose of the suspend function is to temporarily disable Extension
> Command
> shortcuts while people are configuring new ones in the shortcut config UI.
> So
> that, if you want to set Ctrl+F as your shortcut, the request isn't
> treated as
> 'open the find box' (and subsequently eaten).
>
> But the thing is that F5 cannot be an Extension Command (we require
> modifiers
> like Ctrl or Alt) so I don't see why that is enabled when the dialog is
> modal
> and I think using the suspend function is papering over the problem
> because I
> suspect these two are not related.
>
> Now, I don't have a Chrome OS dev environment, but if you can reproduce
> this
> somehow on Windows I'd be happy to take a look and debug it in Visual
> Studio.
> Alternatively, I think you should dig deeper and see why the shortcuts are
> suddenly active while the dialog is up.
>
>
https://chromiumcodereview.**appspot.com/10993067/<https://chromiumcodereview...
>

[Daniel Erat, 2012-09-28 16:10:22]

+yusukes

Perhaps we want to disable some Ash shortcuts while modal dialogs are
shown while keeping others active.

On Fri, Sep 28, 2012 at 9:06 AM, Stefan Schmitz <sschmitz@chromium.org> wrote:
> Thanks for the feedback. I will investigate more. Too bad, I thought I found
> a proper and minimally invasive way to fix it. I found out a few things:
>
> 1. Shortcuts handled by chrome are indeed disabled (in this situation as
> expected). These shortcuts are defined in
> ./chrome/browser/ui/views/accelerator_table.cc:
>
> 2. Shortcuts handled by ash seem to be active in this situation
> (unexpectedly). These shortcuts are defined in
> ./ash/accelerators/accelerator_table.cc
>
> These two types of shortcuts are handled in different call-trees. Below are
> elided call-trees for handling these shortcuts (one for chrome and for ash;
> when they are both enabled):
>
> Elided Backtrace for a Chrome shortcut:
> (gdb) bt
> #0  chrome::CloseTab (browser=0x7fffd6a06b80) at
> ../../chrome/browser/ui/browser_commands.cc:426
> #1  chrome::BrowserCommandController::ExecuteCommandWithDisposition
> (this=0x7fffd2bdb420, id=34015, disposition=CURRENT_TAB) at
> ../../chrome/browser/ui/browser_command_controller.cc:360
> #2  CommandUpdater::ExecuteCommandWithDisposition (this=0x7fffd2bdb450,
> id=34015, disposition=CURRENT_TAB) at
> ../../chrome/browser/command_updater.cc:52
> #3  CommandUpdater::ExecuteCommand (this=0x7fffd2bdb450, id=34015) at
> ../../chrome/browser/command_updater.cc:45
> #4  chrome::ExecuteCommand (browser=0x7fffd6a06b80, command=34015) at
> ../../chrome/browser/ui/browser_commands.cc:190
> #5  BrowserView::PreHandleKeyboardEvent (this=0x7fffd24b6c00, event=...,
> is_keyboard_shortcut=0x7fffffffb20d) at
> ../../chrome/browser/ui/views/frame/browser_view.cc:1302
> #6  Browser::PreHandleKeyboardEvent (this=0x7fffd6a06b80,
> source=0x7fffd2b77200, event=..., is_keyboard_shortcut=0x7fffffffb20d) at
> ../../chrome/browser/ui/browser.cc:1218
> #7  WebContentsImpl::PreHandleKeyboardEvent (this=0x7fffd2b77200, event=...,
> is_keyboard_shortcut=0x7fffffffb20d) at
> ../../content/browser/web_contents/web_contents_impl.cc:1224
> #8  content::RenderWidgetHostImpl::ForwardKeyboardEvent
> (this=0x7fffd2405a08, key_event=...) at
> ../../content/browser/renderer_host/render_widget_host_impl.cc:985
> #9  content::RenderViewHostImpl::ForwardKeyboardEvent (this=0x7fffd2405a00,
> key_event=...) at
> ../../content/browser/renderer_host/render_view_host_impl.cc:1634
> #10 content::RenderWidgetHostViewAura::OnKeyEvent (this=0x7fffd670ad00,
> event=0x7fffffffbc70) at
> ../../content/browser/renderer_host/render_widget_host_view_aura.cc:1458
> #11 ui::EventTarget::OnKeyEvent (this=0x7fffd2b70020, event=0x7fffffffbc70)
> at ../../ui/base/events/event_target.cc:90
>
> Elided Backtrace for an Ash shortcut:
> (gdb) bt
> #0  ash::(anonymous namespace)::HandleNewWindow (is_incognito=false) at
> ../../ash/accelerators/accelerator_controller.cc:148
> #1  ash::AcceleratorController::PerformAction (this=0x7fffe5ac5100,
> action=22, accelerator=...) at
> ../../ash/accelerators/accelerator_controller.cc:486
> #2  ash::AcceleratorController::AcceleratorPressed (this=0x7fffe5ac5100,
> accelerator=...) at ../../ash/accelerators/accelerator_controller.cc:764
> #3  ui::AcceleratorManager::Process (this=0x7fffdbb59240, accelerator=...)
> at ../../ui/base/accelerators/accelerator_manager.cc:88
> #4  ash::AcceleratorController::Process (this=0x7fffe5ac5100,
> accelerator=...) at ../../ash/accelerators/accelerator_controller.cc:381
> #5  ash::AshFocusManagerFactory::Delegate::ProcessAccelerator
> (this=0x7fffd7f36d10, accelerator=...) at
> ../../ash/accelerators/focus_manager_factory.cc:26
> #6  views::FocusManager::ProcessAccelerator (this=0x7fffd1198930,
> accelerator=...) at ../../ui/views/focus/focus_manager.cc:443
> #7  views::FocusManager::OnKeyEvent (this=0x7fffd1198930, event=...) at
> ../../ui/views/focus/focus_manager.cc:149
> #8  views::NativeWidgetAura::DispatchKeyEventPostIME (this=0x7fffd672fa50,
> key=...) at ../../ui/views/widget/native_widget_aura.cc:706
> #9  views::InputMethodBase::DispatchKeyEventPostIME (this=0x7fffd1b27e60,
> key=...) at ../../ui/views/ime/input_method_base.cc:103
> #10 views::InputMethodBridge::DispatchKeyEvent (this=0x7fffd1b27e60,
> key=...) at ../../ui/views/ime/input_method_bridge.cc:59
> #11 views::NativeWidgetAura::OnKeyEvent (this=0x7fffd672fa50,
> event=0x7fffffffb760) at ../../ui/views/widget/native_widget_aura.cc:856
> #12 ui::EventTarget::OnKeyEvent (this=0x7fffd131fe20, event=0x7fffffffb760)
> at ../../ui/base/events/event_target.cc:90
>
> As you can see the ash shortcut goes through the FocusManager. So when I
> found FocusManager::set_shortcut_handling_suspended, I thought "Bingo".
> I will look some more. Please also tell me what you think?
>
> On Fri, Sep 28, 2012 at 5:08 AM, <finnur@chromium.org> wrote:
>>
>> Wow... I don't think this is the right fix. :/
>>
>> The purpose of the suspend function is to temporarily disable Extension
>> Command
>> shortcuts while people are configuring new ones in the shortcut config UI.
>> So
>> that, if you want to set Ctrl+F as your shortcut, the request isn't
>> treated as
>> 'open the find box' (and subsequently eaten).
>>
>> But the thing is that F5 cannot be an Extension Command (we require
>> modifiers
>> like Ctrl or Alt) so I don't see why that is enabled when the dialog is
>> modal
>> and I think using the suspend function is papering over the problem
>> because I
>> suspect these two are not related.
>>
>> Now, I don't have a Chrome OS dev environment, but if you can reproduce
>> this
>> somehow on Windows I'd be happy to take a look and debug it in Visual
>> Studio.
>> Alternatively, I think you should dig deeper and see why the shortcuts are
>> suddenly active while the dialog is up.
>>
>> https://chromiumcodereview.appspot.com/10993067/
>
>

[Yusuke Sato, 2012-09-28 17:37:28]

I haven't looked into this in detail yet, but according to the original bug
report, Alt+Tab which is also an Ash accelerator is correctly disabled while a
modal dialog is shown. Can't we just reuse the logic for Alt+Tab in the handler
for F5?

On 2012/09/28 16:10:22, Daniel Erat wrote:
> +yusukes
> 
> Perhaps we want to disable some Ash shortcuts while modal dialogs are
> shown while keeping others active.
> 
> On Fri, Sep 28, 2012 at 9:06 AM, Stefan Schmitz <mailto:sschmitz@chromium.org>
wrote:
> > Thanks for the feedback. I will investigate more. Too bad, I thought I found
> > a proper and minimally invasive way to fix it. I found out a few things:
> >
> > 1. Shortcuts handled by chrome are indeed disabled (in this situation as
> > expected). These shortcuts are defined in
> > ./chrome/browser/ui/views/accelerator_table.cc:
> >
> > 2. Shortcuts handled by ash seem to be active in this situation
> > (unexpectedly). These shortcuts are defined in
> > ./ash/accelerators/accelerator_table.cc
> >
> > These two types of shortcuts are handled in different call-trees. Below are
> > elided call-trees for handling these shortcuts (one for chrome and for ash;
> > when they are both enabled):
> >
> > Elided Backtrace for a Chrome shortcut:
> > (gdb) bt
> > #0  chrome::CloseTab (browser=0x7fffd6a06b80) at
> > ../../chrome/browser/ui/browser_commands.cc:426
> > #1  chrome::BrowserCommandController::ExecuteCommandWithDisposition
> > (this=0x7fffd2bdb420, id=34015, disposition=CURRENT_TAB) at
> > ../../chrome/browser/ui/browser_command_controller.cc:360
> > #2  CommandUpdater::ExecuteCommandWithDisposition (this=0x7fffd2bdb450,
> > id=34015, disposition=CURRENT_TAB) at
> > ../../chrome/browser/command_updater.cc:52
> > #3  CommandUpdater::ExecuteCommand (this=0x7fffd2bdb450, id=34015) at
> > ../../chrome/browser/command_updater.cc:45
> > #4  chrome::ExecuteCommand (browser=0x7fffd6a06b80, command=34015) at
> > ../../chrome/browser/ui/browser_commands.cc:190
> > #5  BrowserView::PreHandleKeyboardEvent (this=0x7fffd24b6c00, event=...,
> > is_keyboard_shortcut=0x7fffffffb20d) at
> > ../../chrome/browser/ui/views/frame/browser_view.cc:1302
> > #6  Browser::PreHandleKeyboardEvent (this=0x7fffd6a06b80,
> > source=0x7fffd2b77200, event=..., is_keyboard_shortcut=0x7fffffffb20d) at
> > ../../chrome/browser/ui/browser.cc:1218
> > #7  WebContentsImpl::PreHandleKeyboardEvent (this=0x7fffd2b77200, event=...,
> > is_keyboard_shortcut=0x7fffffffb20d) at
> > ../../content/browser/web_contents/web_contents_impl.cc:1224
> > #8  content::RenderWidgetHostImpl::ForwardKeyboardEvent
> > (this=0x7fffd2405a08, key_event=...) at
> > ../../content/browser/renderer_host/render_widget_host_impl.cc:985
> > #9  content::RenderViewHostImpl::ForwardKeyboardEvent (this=0x7fffd2405a00,
> > key_event=...) at
> > ../../content/browser/renderer_host/render_view_host_impl.cc:1634
> > #10 content::RenderWidgetHostViewAura::OnKeyEvent (this=0x7fffd670ad00,
> > event=0x7fffffffbc70) at
> > ../../content/browser/renderer_host/render_widget_host_view_aura.cc:1458
> > #11 ui::EventTarget::OnKeyEvent (this=0x7fffd2b70020, event=0x7fffffffbc70)
> > at ../../ui/base/events/event_target.cc:90
> >
> > Elided Backtrace for an Ash shortcut:
> > (gdb) bt
> > #0  ash::(anonymous namespace)::HandleNewWindow (is_incognito=false) at
> > ../../ash/accelerators/accelerator_controller.cc:148
> > #1  ash::AcceleratorController::PerformAction (this=0x7fffe5ac5100,
> > action=22, accelerator=...) at
> > ../../ash/accelerators/accelerator_controller.cc:486
> > #2  ash::AcceleratorController::AcceleratorPressed (this=0x7fffe5ac5100,
> > accelerator=...) at ../../ash/accelerators/accelerator_controller.cc:764
> > #3  ui::AcceleratorManager::Process (this=0x7fffdbb59240, accelerator=...)
> > at ../../ui/base/accelerators/accelerator_manager.cc:88
> > #4  ash::AcceleratorController::Process (this=0x7fffe5ac5100,
> > accelerator=...) at ../../ash/accelerators/accelerator_controller.cc:381
> > #5  ash::AshFocusManagerFactory::Delegate::ProcessAccelerator
> > (this=0x7fffd7f36d10, accelerator=...) at
> > ../../ash/accelerators/focus_manager_factory.cc:26
> > #6  views::FocusManager::ProcessAccelerator (this=0x7fffd1198930,
> > accelerator=...) at ../../ui/views/focus/focus_manager.cc:443
> > #7  views::FocusManager::OnKeyEvent (this=0x7fffd1198930, event=...) at
> > ../../ui/views/focus/focus_manager.cc:149
> > #8  views::NativeWidgetAura::DispatchKeyEventPostIME (this=0x7fffd672fa50,
> > key=...) at ../../ui/views/widget/native_widget_aura.cc:706
> > #9  views::InputMethodBase::DispatchKeyEventPostIME (this=0x7fffd1b27e60,
> > key=...) at ../../ui/views/ime/input_method_base.cc:103
> > #10 views::InputMethodBridge::DispatchKeyEvent (this=0x7fffd1b27e60,
> > key=...) at ../../ui/views/ime/input_method_bridge.cc:59
> > #11 views::NativeWidgetAura::OnKeyEvent (this=0x7fffd672fa50,
> > event=0x7fffffffb760) at ../../ui/views/widget/native_widget_aura.cc:856
> > #12 ui::EventTarget::OnKeyEvent (this=0x7fffd131fe20, event=0x7fffffffb760)
> > at ../../ui/base/events/event_target.cc:90
> >
> > As you can see the ash shortcut goes through the FocusManager. So when I
> > found FocusManager::set_shortcut_handling_suspended, I thought "Bingo".
> > I will look some more. Please also tell me what you think?
> >
> > On Fri, Sep 28, 2012 at 5:08 AM, <mailto:finnur@chromium.org> wrote:
> >>
> >> Wow... I don't think this is the right fix. :/
> >>
> >> The purpose of the suspend function is to temporarily disable Extension
> >> Command
> >> shortcuts while people are configuring new ones in the shortcut config UI.
> >> So
> >> that, if you want to set Ctrl+F as your shortcut, the request isn't
> >> treated as
> >> 'open the find box' (and subsequently eaten).
> >>
> >> But the thing is that F5 cannot be an Extension Command (we require
> >> modifiers
> >> like Ctrl or Alt) so I don't see why that is enabled when the dialog is
> >> modal
> >> and I think using the suspend function is papering over the problem
> >> because I
> >> suspect these two are not related.
> >>
> >> Now, I don't have a Chrome OS dev environment, but if you can reproduce
> >> this
> >> somehow on Windows I'd be happy to take a look and debug it in Visual
> >> Studio.
> >> Alternatively, I think you should dig deeper and see why the shortcuts are
> >> suddenly active while the dialog is up.
> >>
> >> https://chromiumcodereview.appspot.com/10993067/
> >
> >

[sschmitz, 2012-09-28 18:05:29]

I will look into Alt-Tab. thanks.

On Fri, Sep 28, 2012 at 10:37 AM, <yusukes@chromium.org> wrote:

> I haven't looked into this in detail yet, but according to the original bug
> report, Alt+Tab which is also an Ash accelerator is correctly disabled
> while a
> modal dialog is shown. Can't we just reuse the logic for Alt+Tab in the
> handler
> for F5?
>
>
> On 2012/09/28 16:10:22, Daniel Erat wrote:
>
>> +yusukes
>>
>
>  Perhaps we want to disable some Ash shortcuts while modal dialogs are
>> shown while keeping others active.
>>
>
>  On Fri, Sep 28, 2012 at 9:06 AM, Stefan Schmitz <mailto:
>> sschmitz@chromium.org>
>>
> wrote:
>
>> > Thanks for the feedback. I will investigate more. Too bad, I thought I
>> found
>> > a proper and minimally invasive way to fix it. I found out a few things:
>> >
>> > 1. Shortcuts handled by chrome are indeed disabled (in this situation as
>> > expected). These shortcuts are defined in
>> > ./chrome/browser/ui/views/**accelerator_table.cc:
>> >
>> > 2. Shortcuts handled by ash seem to be active in this situation
>> > (unexpectedly). These shortcuts are defined in
>> > ./ash/accelerators/**accelerator_table.cc
>> >
>> > These two types of shortcuts are handled in different call-trees. Below
>> are
>> > elided call-trees for handling these shortcuts (one for chrome and for
>> ash;
>> > when they are both enabled):
>> >
>> > Elided Backtrace for a Chrome shortcut:
>> > (gdb) bt
>> > #0  chrome::CloseTab (browser=0x7fffd6a06b80) at
>> > ../../chrome/browser/ui/**browser_commands.cc:426
>> > #1  chrome::**BrowserCommandController::**ExecuteCommandWithDisposition
>> > (this=0x7fffd2bdb420, id=34015, disposition=CURRENT_TAB) at
>> > ../../chrome/browser/ui/**browser_command_controller.cc:**360
>> > #2  CommandUpdater::**ExecuteCommandWithDisposition
>> (this=0x7fffd2bdb450,
>> > id=34015, disposition=CURRENT_TAB) at
>> > ../../chrome/browser/command_**updater.cc:52
>> > #3  CommandUpdater::ExecuteCommand (this=0x7fffd2bdb450, id=34015) at
>> > ../../chrome/browser/command_**updater.cc:45
>> > #4  chrome::ExecuteCommand (browser=0x7fffd6a06b80, command=34015) at
>> > ../../chrome/browser/ui/**browser_commands.cc:190
>> > #5  BrowserView::**PreHandleKeyboardEvent (this=0x7fffd24b6c00,
>> event=...,
>> > is_keyboard_shortcut=**0x7fffffffb20d) at
>> > ../../chrome/browser/ui/views/**frame/browser_view.cc:1302
>> > #6  Browser::**PreHandleKeyboardEvent (this=0x7fffd6a06b80,
>> > source=0x7fffd2b77200, event=..., is_keyboard_shortcut=**0x7fffffffb20d)
>> at
>> > ../../chrome/browser/ui/**browser.cc:1218
>> > #7  WebContentsImpl::**PreHandleKeyboardEvent (this=0x7fffd2b77200,
>> event=...,
>> > is_keyboard_shortcut=**0x7fffffffb20d) at
>> > ../../content/browser/web_**contents/web_contents_impl.cc:**1224
>> > #8  content::RenderWidgetHostImpl:**:ForwardKeyboardEvent
>> > (this=0x7fffd2405a08, key_event=...) at
>> > ../../content/browser/**renderer_host/render_widget_**host_impl.cc:985
>> > #9  content::RenderViewHostImpl::**ForwardKeyboardEvent
>> (this=0x7fffd2405a00,
>> > key_event=...) at
>> > ../../content/browser/**renderer_host/render_view_**host_impl.cc:1634
>> > #10 content::**RenderWidgetHostViewAura::**OnKeyEvent
>> (this=0x7fffd670ad00,
>> > event=0x7fffffffbc70) at
>> > ../../content/browser/**renderer_host/render_widget_**
>> host_view_aura.cc:1458
>> > #11 ui::EventTarget::OnKeyEvent (this=0x7fffd2b70020,
>> event=0x7fffffffbc70)
>> > at ../../ui/base/events/event_**target.cc:90
>> >
>> > Elided Backtrace for an Ash shortcut:
>> > (gdb) bt
>> > #0  ash::(anonymous namespace)::HandleNewWindow (is_incognito=false) at
>> > ../../ash/accelerators/**accelerator_controller.cc:148
>> > #1  ash::AcceleratorController::**PerformAction (this=0x7fffe5ac5100,
>> > action=22, accelerator=...) at
>> > ../../ash/accelerators/**accelerator_controller.cc:486
>> > #2  ash::AcceleratorController::**AcceleratorPressed
>> (this=0x7fffe5ac5100,
>> > accelerator=...) at ../../ash/accelerators/**
>> accelerator_controller.cc:764
>> > #3  ui::AcceleratorManager::**Process (this=0x7fffdbb59240,
>> accelerator=...)
>> > at ../../ui/base/accelerators/**accelerator_manager.cc:88
>> > #4  ash::AcceleratorController::**Process (this=0x7fffe5ac5100,
>> > accelerator=...) at ../../ash/accelerators/**
>> accelerator_controller.cc:381
>> > #5  ash::AshFocusManagerFactory::**Delegate::ProcessAccelerator
>> > (this=0x7fffd7f36d10, accelerator=...) at
>> > ../../ash/accelerators/focus_**manager_factory.cc:26
>> > #6  views::FocusManager::**ProcessAccelerator (this=0x7fffd1198930,
>> > accelerator=...) at ../../ui/views/focus/focus_**manager.cc:443
>> > #7  views::FocusManager::**OnKeyEvent (this=0x7fffd1198930, event=...)
>> at
>> > ../../ui/views/focus/focus_**manager.cc:149
>> > #8  views::NativeWidgetAura::**DispatchKeyEventPostIME
>> (this=0x7fffd672fa50,
>> > key=...) at ../../ui/views/widget/native_**widget_aura.cc:706
>> > #9  views::InputMethodBase::**DispatchKeyEventPostIME
>> (this=0x7fffd1b27e60,
>> > key=...) at ../../ui/views/ime/input_**method_base.cc:103
>> > #10 views::InputMethodBridge::**DispatchKeyEvent (this=0x7fffd1b27e60,
>> > key=...) at ../../ui/views/ime/input_**method_bridge.cc:59
>> > #11 views::NativeWidgetAura::**OnKeyEvent (this=0x7fffd672fa50,
>> > event=0x7fffffffb760) at ../../ui/views/widget/native_**
>> widget_aura.cc:856
>> > #12 ui::EventTarget::OnKeyEvent (this=0x7fffd131fe20,
>> event=0x7fffffffb760)
>> > at ../../ui/base/events/event_**target.cc:90
>> >
>> > As you can see the ash shortcut goes through the FocusManager. So when I
>> > found FocusManager::set_shortcut_**handling_suspended, I thought
>> "Bingo".
>> > I will look some more. Please also tell me what you think?
>> >
>> > On Fri, Sep 28, 2012 at 5:08 AM, <mailto:finnur@chromium.org> wrote:
>> >>
>> >> Wow... I don't think this is the right fix. :/
>> >>
>> >> The purpose of the suspend function is to temporarily disable Extension
>> >> Command
>> >> shortcuts while people are configuring new ones in the shortcut config
>> UI.
>> >> So
>> >> that, if you want to set Ctrl+F as your shortcut, the request isn't
>> >> treated as
>> >> 'open the find box' (and subsequently eaten).
>> >>
>> >> But the thing is that F5 cannot be an Extension Command (we require
>> >> modifiers
>> >> like Ctrl or Alt) so I don't see why that is enabled when the dialog is
>> >> modal
>> >> and I think using the suspend function is papering over the problem
>> >> because I
>> >> suspect these two are not related.
>> >>
>> >> Now, I don't have a Chrome OS dev environment, but if you can reproduce
>> >> this
>> >> somehow on Windows I'd be happy to take a look and debug it in Visual
>> >> Studio.
>> >> Alternatively, I think you should dig deeper and see why the shortcuts
>> are
>> >> suddenly active while the dialog is up.
>> >>
>> >>
https://chromiumcodereview.**appspot.com/10993067/<https://chromiumcodereview...
>> >
>> >
>>
>
>
>
>
https://chromiumcodereview.**appspot.com/10993067/<https://chromiumcodereview...
>