New issue
Advanced search Search tips

Issue 845465 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug

Blocked on:
issue 846893



Sign in to add a comment

[macviews] cmd-1/2/3 to switch tabs doesn't work when omnibox has focus

Project Member Reported by thakis@chromium.org, May 22 2018

Issue description

What steps will reproduce the problem?
1. Open two tabs
2. Click first tab, focus the web contents
3. Click second tab, focus omnibox
4. Hit cmd-1 to switch back to first tab
5. Hit cmd-2 to go back to 2nd tab
6. Go to 4

What is the expected result?

Tabs switch back and forth reliably.


What happens instead of that?

Very often get stuck on tab 2 and cmd-1 has no effect. (cmd-[ still works in that situation)


Please provide any additional information below. Attach a screenshot if
possible.

UserAgentString: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.106 Safari/537.36



 

Comment 1 by meh...@chromium.org, May 22 2018

Labels: -Pri-3 Proj-MacViews Pri-2
Owner: erikc...@chromium.org
Status: Assigned (was: Untriaged)
Triage: Routing to erikchen@ for the hotkey usage.
Blockedon: 846893
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 8 2018

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

commit 8f2f9568808ee117698e0a9008a0b52902d2363e
Author: Erik Chen <erikchen@chromium.org>
Date: Fri Jun 08 00:44:54 2018

Unification of macOS keyEquivalent (hotkey) handling.

This CL makes the CommandDispatcher class responsible for all NSMenu-related
keyEquivalent handling. The logic is now shared by both Views and Cocoa. This
fixes many Views bugs, and some Cocoa bugs.

Previously, there several different pieces of logic for handling NSMenu-related
keyEquivalents.

* If a RenderWidgetHostViewCocoa was the firstResponder, then all the logic in
  CommandDispatcher was skipped.
  * In both Cocoa and Views, there was logic in content/ via RenderWidget*
    classes that would short-circuit but also fail to handle the keyEquivalent.
    [e.g. if renderer process was killed].
    * In Cocoa, content/ would call into
      BrowserWindowCocoa::PreHandleKeyboardEvent(). This had mostly the right
      behavior, except it missed out on edge-case logic in CommandDispatcher.
    * In Views, content/ would call into BrowserView::PreHandleKeyboardEvent.
      This relied on AcceleratorTable to lookup the command for a keyEquivalent.
      This entire class is unusable for keyEquivalents because it holds a static
      mapping, whereas on macOS, the mapping is user configurable and dynamic.
  * If the hotkey was not immediately consumed, it would be passed to the
    renderer process. If the renderer process chose not to consume the hotkey,
    the browser process would get it again.
    * In Cocoa, the event would be redispatched to CommandDispatcher. This had
      the right behavior.
    * In Views, the event would be sent to AcceleratorTable again, once again
      having the wrong behavior.
* If any other class was firstResponder, then the logic in CommandDispatcher
  would trigger.

This CL makes CommandDispatcher the only place to handle NSMenu-related
keyEquivalents. This allows us to centralize the logic, including workarounds.

New logic:

* The CommandDispatcherDelegate is given an opportunity to process the
  keyEquivalent via prePerformKeyEquivalent:. If the keyEquivalent is reserved
  by the browser, it is immediately handled.
* Next, the firstResponder is given a chance to process the keyEquivalent. If
  the firstResponder is a RenderWidgetHostViewCocoa, this goes through the same
  code paths as above, except BrowserWindowCocoa::PreHandleKeyboardEvent() is
  mostly a no-op.
* Then, the CommandDispatcherDelegate gets another chance to process the
  keyEquivalent via postPerformKeyEquivalent:.

If the firstResponder was a RenderWidgetHostViewCocoa which asynchronously chose
to not process the event, then both Cocoa and Views will take the returning
event and call redispatchEvent:, which will also call into
-[CommandDispatcherDelegate postPerformKeyEquivalent:].

Change-Id: If724e125c6acf64ddd9f1d9cc296e761ffb2a886
Bug:  846893 ,  836947 ,  845465 ,  47134 
Reviewed-on: https://chromium-review.googlesource.com/1082818
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565490}
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac_unittest.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/accelerators_cocoa.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/accelerators_cocoa.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_interactive_uitest.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/browser_window_cocoa.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/browser_window_utils.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/accelerator_table.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/accelerator_table.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/accelerator_table_unittest_mac.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_ash.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_ash.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mac.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mac.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mash.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mash.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_view_unittest.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/desktop_browser_frame_aura.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/desktop_browser_frame_aura.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/native_browser_frame.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/media_router/presentation_receiver_window_view.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/test/BUILD.gn
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/content/browser/renderer_host/render_widget_host_view_cocoa.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/base/cocoa/command_dispatcher.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/base/cocoa/command_dispatcher.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/events/keycodes/keyboard_code_conversion_mac.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/events/keycodes/keyboard_code_conversion_mac.mm

Status: Fixed (was: Assigned)

Sign in to add a comment