RemoteMacViews: Hotkeys don't work |
|||||
Issue descriptionAlmost all hotkeys don't work. This is with the WIP fix for issue 895139 landed (https://chromium-review.googlesource.com/c/chromium/src/+/1278916). Some hotkeys (the ones that are captures by WebContentsViewCocoa) do work (e.g, command-C for copy), but most don't. IIUC this is because we do not set the the command handler for app shim windows (yet). For browser windows, we do this in BrowserFrameMac::CreateNSWindow at [0]. Two issues here. - We don't go through that window creation path in app mode, and so we don't hit that code - We can't use that implementation because the created BrowserWindowCommandHandler calls directly into browser-process functions - E.g, for updating item state in UpdateToggleStateWithTag - E.g, for calling ExecuteCommandWithDisposition in commandDispatchUsingKeyModifiers The fix for this is to do the usual surgery on BrowserWindowCommandHandler. In particular - isolate the parts of it that need to call into the browser - create a mojo interface (mostly using sync calls, in this case) to encapsulate these calls - call that mojo interface from BrowserWindowCommandHandler - wire up a way to create a BrowserWindowCommandHandler in the app shim process - we currently have a very small mojom::CreateWindowParams structure that we populate at [1] - we could expand that to include instructions to create and set a BrowserWindowCommandHandler [0] https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_frame_mac.mm?rcl=e4afc431673c1fca470be320ecbdb0f68925c5f6&l=150 [1] https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_mac.mm?rcl=e4afc431673c1fca470be320ecbdb0f68925c5f6&l=122
,
Oct 20
Hmm, redispatching back into the app shim process just causes an infinite loop of events. What causes the -[NSApp sendEvent:] call made by -[CommandDispatcher redispatchKeyEvent:] to be handled differently from the original event?
,
Oct 22
> What causes the -[NSApp sendEvent:] call made by -[CommandDispatcher redispatchKeyEvent:] to be handled differently from the original event? https://cs.chromium.org/chromium/src/ui/base/cocoa/command_dispatcher.mm?type=cs&q=iseventbeingredispatched&sq=package:chromium&g=0&l=227 By returning "YES", the cmomand dispatcher claims to have handled the event [without doing anything]. This short-circuits the rest of the logic.
,
Oct 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8657d868e6ce3e114b843844ecbed67133068f1c commit 8657d868e6ce3e114b843844ecbed67133068f1c Author: Christopher Cameron <ccameron@chromium.org> Date: Mon Oct 22 22:05:33 2018 RemoteMacViews: Don't redispatch events to the browser Keyboard events that are targeted first at the web contents area, but are not handled by the web contents, are bounced back to the browser via the in content::RenderWidgetHostImpl::OnKeyboardEventAck, where they are forwarded to BrowserFrameMac::HandleKeyboardEvent to be redispatched in the browser process. This redispatch happens in the browser process even if the keyboard event originated in the app shim process. The result is strange behavior such as command-N re-focusing Chrome and creating a new browser window. Ensure that the redispatch happens in the originating process, by having BrowserFrameMac::HandleKeyboardEvent delegate the redispatch to its NativeWidgetMac base class, which will then forward the event to the app shim process. Two small wrinkles about this: - The type used by BrowserFrameMac::HandleKeyboardEvent is inconveniently content::NativeWebKeyboardEvent, which cannot be accessed in views/ - the only thing used in redispatch is |os_event|, which was reconstructed from a blink::WebKeyboardEvent - instead of passing the event over mojo, just pass the parts that are extracted by blink::WebKeyboardEvent - ideally we should switch to using a ui::KeyEvent, and just pass that directly (and add a function to reconstitute a NSEvent from a ui::KeyEvent). - When redispatched in-process, we do want to accurately know the return value from the redispatch. - To get this accurately, we take a different path in-process versus out-of-process (because the mojo call has no return value). - Always report out-of-process events as handled, to arrest further processing in the browser. Bug: 895169 Change-Id: I5fe1011374a4ca5050c7af70d6118286f7b46b34 Reviewed-on: https://chromium-review.googlesource.com/c/1292446 Reviewed-by: Robert Sesek <rsesek@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#601743} [modify] https://crrev.com/8657d868e6ce3e114b843844ecbed67133068f1c/chrome/browser/ui/views/frame/browser_frame_mac.mm [modify] https://crrev.com/8657d868e6ce3e114b843844ecbed67133068f1c/ui/views/widget/native_widget_mac.h [modify] https://crrev.com/8657d868e6ce3e114b843844ecbed67133068f1c/ui/views/widget/native_widget_mac.mm [modify] https://crrev.com/8657d868e6ce3e114b843844ecbed67133068f1c/ui/views_bridge_mac/bridged_native_widget_impl.h [modify] https://crrev.com/8657d868e6ce3e114b843844ecbed67133068f1c/ui/views_bridge_mac/bridged_native_widget_impl.mm [modify] https://crrev.com/8657d868e6ce3e114b843844ecbed67133068f1c/ui/views_bridge_mac/mojo/bridged_native_widget.mojom
,
Nov 6
,
Nov 7
,
Dec 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8cd79b95de322c7174868f46a937b12f284207c commit d8cd79b95de322c7174868f46a937b12f284207c Author: Christopher Cameron <ccameron@chromium.org> Date: Mon Dec 03 21:31:22 2018 RemoteMacViews: Break validateUserInterfaceItem across mojo This is a step toward supporting shortcut keys and menu interface items in PWAs. This breaks the function -[BrowserWindowCommandHandler validateUserInterfaceItem] into two parts - The part to look up the browser command and to update the interface item still remains in BrowserWindowCommandHandler (which will be in the in the PWA process). - The part to access the Browser*, which will be in the browser process, which is - routed through mojo to the BridgedNativeWidgetHostImpl - to the NativeWidgetMac - which is subclassed as BrowserFrameMac - which calls BrowserWindowCommandHandlerValidateUserInterfaceItem to implemented the parts that used to be in BrowserWindowCommandHandler Also add required interface to look up a BridgedNativeWidgetImpl from an NSWindow. Bug: 895169 Change-Id: Ie81a1e5a44537b07183d4c29b89d2da1951137c0 Reviewed-on: https://chromium-review.googlesource.com/c/1357829 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/heads/master@{#613255} [modify] https://crrev.com/d8cd79b95de322c7174868f46a937b12f284207c/chrome/browser/ui/cocoa/browser_window_command_handler.mm [modify] https://crrev.com/d8cd79b95de322c7174868f46a937b12f284207c/chrome/browser/ui/views/frame/browser_frame_mac.h [modify] https://crrev.com/d8cd79b95de322c7174868f46a937b12f284207c/chrome/browser/ui/views/frame/browser_frame_mac.mm [modify] https://crrev.com/d8cd79b95de322c7174868f46a937b12f284207c/ui/views/cocoa/bridged_native_widget_host_impl.h [modify] https://crrev.com/d8cd79b95de322c7174868f46a937b12f284207c/ui/views/cocoa/bridged_native_widget_host_impl.mm [modify] https://crrev.com/d8cd79b95de322c7174868f46a937b12f284207c/ui/views/widget/native_widget_mac.h [modify] https://crrev.com/d8cd79b95de322c7174868f46a937b12f284207c/ui/views_bridge_mac/bridged_native_widget_impl.h [modify] https://crrev.com/d8cd79b95de322c7174868f46a937b12f284207c/ui/views_bridge_mac/bridged_native_widget_impl.mm [modify] https://crrev.com/d8cd79b95de322c7174868f46a937b12f284207c/ui/views_bridge_mac/mojo/bridged_native_widget_host.mojom
,
Dec 5
,
Dec 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b16b13725d360cc744495efd0e7e332b47a68c2 commit 9b16b13725d360cc744495efd0e7e332b47a68c2 Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Dec 05 02:37:50 2018 RemoteMacViews: Break performKeyEquivalent and commandDispatch over mojo This is another step towards supporting shortcut keys and menu interface items in PWAs. This breaks the following functions into a Cocoa component (for the PWA process) and a Chrome component (for the browser process): -[BrowserWindowCommandHandler commandDispatch] -[BrowserWindowCommandHandler commandDispatchUsingKeyModifiers] -[ChromeCommandDispatcherDelegate prePerformKeyEquivalent] -[ChromeCommandDispatcherDelegate postPerformKeyEquivalent] The common feature of all of these functions is that they end up calling chrome::ExecuteCommand. Add a mojo method to call chrome::ExecuteCommand and implement it for BrowserFrameMac. Bug: 895169 Change-Id: Iada42fc24544ca12b8867e54eb1549da9679f6a7 Reviewed-on: https://chromium-review.googlesource.com/c/1359137 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Cr-Commit-Position: refs/heads/master@{#613826} [modify] https://crrev.com/9b16b13725d360cc744495efd0e7e332b47a68c2/chrome/browser/ui/cocoa/browser_window_command_handler.mm [modify] https://crrev.com/9b16b13725d360cc744495efd0e7e332b47a68c2/chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm [modify] https://crrev.com/9b16b13725d360cc744495efd0e7e332b47a68c2/chrome/browser/ui/views/frame/browser_frame_mac.h [modify] https://crrev.com/9b16b13725d360cc744495efd0e7e332b47a68c2/chrome/browser/ui/views/frame/browser_frame_mac.mm [modify] https://crrev.com/9b16b13725d360cc744495efd0e7e332b47a68c2/ui/views/cocoa/bridged_native_widget_host_impl.h [modify] https://crrev.com/9b16b13725d360cc744495efd0e7e332b47a68c2/ui/views/cocoa/bridged_native_widget_host_impl.mm [modify] https://crrev.com/9b16b13725d360cc744495efd0e7e332b47a68c2/ui/views/widget/native_widget_mac.h [modify] https://crrev.com/9b16b13725d360cc744495efd0e7e332b47a68c2/ui/views/widget/native_widget_mac.mm [modify] https://crrev.com/9b16b13725d360cc744495efd0e7e332b47a68c2/ui/views_bridge_mac/mojo/bridged_native_widget_host.mojom
,
Dec 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/642609c77909916568c41482e1ae56e1ca8db2ec commit 642609c77909916568c41482e1ae56e1ca8db2ec Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Dec 05 19:56:15 2018 RemoteMacViews: Break focus manager query across mojo Break the function -[ChromeCommandDispatcherDelegate eventHandledByExtensionCommand] into the parts that are to run in the PWA process and the parts that are to run in the browser process. Rename the function to eventHandledByViewsFocusManager, as is requested by the TODOs. Fix-up the mojo support for ui::Accelerator, since it is used by this method (support was mostly written, but was not actually included in the build). Bug: 895169 Change-Id: Ic9180ac55fcd7d0bce8e23e6d0c7a5e4be4cb8b0 Reviewed-on: https://chromium-review.googlesource.com/c/1358641 Reviewed-by: Ken Rockot <rockot@google.com> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#614068} [modify] https://crrev.com/642609c77909916568c41482e1ae56e1ca8db2ec/chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm [modify] https://crrev.com/642609c77909916568c41482e1ae56e1ca8db2ec/mojo/public/tools/bindings/chromium_bindings_configuration.gni [modify] https://crrev.com/642609c77909916568c41482e1ae56e1ca8db2ec/ui/base/accelerators/mojo/BUILD.gn [modify] https://crrev.com/642609c77909916568c41482e1ae56e1ca8db2ec/ui/base/accelerators/mojo/accelerator_struct_traits.h [add] https://crrev.com/642609c77909916568c41482e1ae56e1ca8db2ec/ui/base/accelerators/mojo/typemaps.gni [modify] https://crrev.com/642609c77909916568c41482e1ae56e1ca8db2ec/ui/views/cocoa/bridged_native_widget_host_impl.h [modify] https://crrev.com/642609c77909916568c41482e1ae56e1ca8db2ec/ui/views/cocoa/bridged_native_widget_host_impl.mm [modify] https://crrev.com/642609c77909916568c41482e1ae56e1ca8db2ec/ui/views_bridge_mac/BUILD.gn [modify] https://crrev.com/642609c77909916568c41482e1ae56e1ca8db2ec/ui/views_bridge_mac/mojo/bridged_native_widget_host.mojom
,
Dec 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/00e1708c3c2857798c392ac8fcb339e6a7704f51 commit 00e1708c3c2857798c392ac8fcb339e6a7704f51 Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Dec 05 20:29:56 2018 RemoteMacViews: Move command dispatcher ownership The class NativeWidgetMacNSWindow implements the CommandDispatchingWindow protocol. Move the functions to set (and own) the CommandDispatcherDelegate and CommandHandler from BrowserFrameMac to BridgedNativeWidgetImpl (the class that owns the NativeWidgetMacNSWindow) rather than BrowserFrameMac. This is necessary for PWAs because CommandDispatcherDelegate and CommandHandler will be allocated in the app process (and their old home, BrowserFrameMac, is in the browser process). This was not possible historically because BridgedNativeWidgetImpl did not have a NativeWidgetMacNSWindow (rather, it just had an NSWindow). Bug: 895169 Change-Id: I843c257345381df3132d7c57e180519d7a6e730e Reviewed-on: https://chromium-review.googlesource.com/c/1359814 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/heads/master@{#614080} [modify] https://crrev.com/00e1708c3c2857798c392ac8fcb339e6a7704f51/chrome/browser/ui/views/frame/browser_frame_mac.h [modify] https://crrev.com/00e1708c3c2857798c392ac8fcb339e6a7704f51/chrome/browser/ui/views/frame/browser_frame_mac.mm [modify] https://crrev.com/00e1708c3c2857798c392ac8fcb339e6a7704f51/ui/views/widget/native_widget_mac.h [modify] https://crrev.com/00e1708c3c2857798c392ac8fcb339e6a7704f51/ui/views/widget/native_widget_mac.mm [modify] https://crrev.com/00e1708c3c2857798c392ac8fcb339e6a7704f51/ui/views_bridge_mac/bridged_native_widget_impl.h [modify] https://crrev.com/00e1708c3c2857798c392ac8fcb339e6a7704f51/ui/views_bridge_mac/bridged_native_widget_impl.mm
,
Dec 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1c1992d02fa89792498f5e3533772bd65d93973d commit 1c1992d02fa89792498f5e3533772bd65d93973d Author: Christopher Cameron <ccameron@chromium.org> Date: Fri Dec 07 07:00:51 2018 RemoteMacViews: Wire up menu items and shortcut keys For menu items and shortcut keys to work, PWA windows need to have the ChromeCommandDispatcherDelegate and BrowserWindowCommandHandler structures set on them. This cannot be done by a method on views_bridge_mac::mojom:: BridgedNativeWidget, because the structures to be created exist in Chrome, which would be a layering violation. Introduce a method to chrome::mojom::AppShim which takes a widget id, creates the ChromeCommandDispatcherDelegate and BrowserWindowCommandHandler, and assigns them to the corresponding BridgedNativeWidgetImpl. Bug: 895169 Change-Id: Ie1d333bec235d7b6101d5b19aa89a74136d54a7a Reviewed-on: https://chromium-review.googlesource.com/c/1364210 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Cr-Commit-Position: refs/heads/master@{#614631} [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/chrome/app_shim/app_shim_controller.h [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/chrome/app_shim/app_shim_controller.mm [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/chrome/browser/apps/app_shim/app_shim_host_mac.cc [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/chrome/browser/apps/app_shim/app_shim_host_mac.h [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/chrome/browser/ui/views/frame/browser_frame_mac.mm [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/chrome/common/mac/app_shim.mojom [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/ui/views/cocoa/bridged_native_widget_unittest.mm [modify] https://crrev.com/1c1992d02fa89792498f5e3533772bd65d93973d/ui/views/widget/native_widget_mac.h
,
Dec 7
This works now ... but it raises some question about what the menu items should be doing (e.g, "New window" definitely feels wrong in its behavior). I'll file separate bugs on those. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by ccameron@chromium.org
, Oct 20