New issue
Advanced search Search tips

Issue 665823 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Feb 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug

Blocking:
issue 671916



Sign in to add a comment

MacViewsBrowser: accelerator_table.cc's table is not used when focus is in Omnibox

Reported by mbl...@yandex-team.ru, Nov 16 2016

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 YaBrowser/16.11.0.1821 (beta) Yowser/2.5 Safari/537.36

Steps to reproduce the problem:
There's an additional table in global_keyboard_shortcuts_mac.mm, and it's used by ChromeCommandDispatcherDelegate. If GetWindowKeyboardShortcutTable() is replaced by an empty table, the following shortcuts will stop to work even if accelerator_table.cc provides a suitable alternative:

{Cmd+},         IDC_SELECT_NEXT_TAB}
{Cmd+{,         IDC_SELECT_PREVIOUS_TAB}
{Ctrl+PageDown, IDC_SELECT_NEXT_TAB}
{Ctrl+Tab,      IDC_SELECT_NEXT_TAB}
{Ctrl+PageUp,   IDC_SELECT_PREVIOUS_TAB}
{Shift+Ctrl+Tab,IDC_SELECT_PREVIOUS_TAB}
{Cmd+1,         IDC_SELECT_TAB_0}
{Cmd+2,         IDC_SELECT_TAB_1}
{Cmd+3,         IDC_SELECT_TAB_2}
{Cmd+4,         IDC_SELECT_TAB_3}
{Cmd+5,         IDC_SELECT_TAB_4}
{Cmd+6,         IDC_SELECT_TAB_5}
{Cmd+7,         IDC_SELECT_TAB_6}
{Cmd+8,         IDC_SELECT_TAB_7}
{Cmd+9,         IDC_SELECT_LAST_TAB}
{Cmd+Shift+M,   IDC_SHOW_AVATAR_MENU}
{Cmd+Opt+L,     IDC_SHOW_DOWNLOADS}

Steps to reproduce:
1. Ensure that GetWindowKeyboardShortcutTable() is empty and accelerator_table.cc provides a suitable alternative (currently requires this CL: https://codereview.chromium.org/2074643003/);
2. Open several tabs;
3. Click on Omnibox;
4. Try pressing Cmd+Numbers, it should select different tabs but it won't work.

What is the expected behavior?

What went wrong?
Currently when Omnibox is in focus, the Browser relies on ChromeCommandDispatcherDelegate in order for these accelerators to work.

Instead the events should be dispatched to the Views' Widget in a fashion similar to -[BridgedContentView handleKeyEvent:].

Did this work before? N/A 

Chrome version: 54.0.2840.71  Channel: n/a
OS Version: OS X 10.12.1
Flash Version: 

Related issue: https://bugs.chromium.org/p/chromium/issues/detail?id=620315 (MacViewsBrowser: Views accelerators table should match the Cocoa one)
 
Labels: M-54
Cc: tapted@chromium.org
Status: Untriaged (was: Unconfirmed)

Comment 4 by tapted@chromium.org, Nov 18 2016

Status: Started (was: Untriaged)
https://codereview.chromium.org/2505943002/
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e405f4e5df76138d888527165ea13446a1e08296

commit e405f4e5df76138d888527165ea13446a1e08296
Author: mblsha <mblsha@yandex-team.ru>
Date: Fri Nov 25 05:32:31 2016

MacViews: Fix accelerator handling while Omnibox is in focus.

Currently in both Cocoa and MacViewsBrowser when Omnibox is in focus the
accelerators are only handled in -[NativeWidgetMacNSWindow
performKeyEquivalent:], which internally uses ChromeCommandDispatcherDelegate,
which consults global_keyboard_shortcuts_cocoa_mac.mm's tables.

In  crbug.com/620315  we're trying to migrate the
global_keyboard_shortcuts_cocoa_mac.mm's tables over to accelerator_table.cc.

BUG= 665823 

Review-Url: https://codereview.chromium.org/2505943002
Cr-Commit-Position: refs/heads/master@{#434436}

[modify] https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296/ui/events/base_event_utils.cc
[modify] https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296/ui/events/test/event_generator.cc
[modify] https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296/ui/events/test/event_generator.h
[modify] https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296/ui/views/controls/textfield/textfield_unittest.cc
[modify] https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296/ui/views/event_monitor_unittest.cc
[modify] https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296/ui/views/test/event_generator_delegate_mac.mm
[modify] https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296/ui/views/view_unittest.cc

GlobalKeyboardShortcutsTest.SwitchTabsMac fails on MacViews after r434436:
../../chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:34: Failure
Value of: tab_strip->IsTabSelected(0)
  Actual: false
Expected: true

Proposed fix: https://codereview.chromium.org/2531033003/
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2394f02438a9b0c5c8aa20a099ed02d7a5867eba

commit 2394f02438a9b0c5c8aa20a099ed02d7a5867eba
Author: mblsha <mblsha@yandex-team.ru>
Date: Fri Dec 02 12:03:03 2016

MacViews: Fix GlobalKeyboardShortcutsTest.SwitchTabsMac.

The old test always called -[NSWindow performKeyEquivalent:] but as of r434436
we need to ensure that both performKeyEquivalent: and keyDown: are called in
order for window accelerators to work.

BUG= 665823 

Review-Url: https://codereview.chromium.org/2531033003
Cr-Commit-Position: refs/heads/master@{#435918}

[modify] https://crrev.com/2394f02438a9b0c5c8aa20a099ed02d7a5867eba/chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm
[modify] https://crrev.com/2394f02438a9b0c5c8aa20a099ed02d7a5867eba/ui/views/test/event_generator_delegate_mac.mm

Comment 8 by tapted@chromium.org, Feb 24 2017

Blocking: 671916
Labels: phase4
Status: Fixed (was: Started)
I think this is fixed.

Sign in to add a comment