New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 895169 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug

Blocking:
issue 859152



Sign in to add a comment

RemoteMacViews: Hotkeys don't work

Project Member Reported by ccameron@chromium.org, Oct 14

Issue description

Almost 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
 
In addition to the above, there exists the following issue. Many hotkeys end up being processed via the stack
  -[CommandDispatcher redispatchKeyEvent:]
  BrowserFrameMac::HandleKeyboardEvent(content::NativeWebKeyboardEvent const&)
  BrowserView::HandleKeyboardEvent(content::NativeWebKeyboardEvent const&)
  content::RenderWidgetHostImpl::OnKeyboardEventAck

This ends up effectively forwarding un-handled events from the app shim back to the browser. We need to update BrowserFrameMac::HandleKeyboardEvent to redispatch back into the app shim process.
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?
> 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.
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Labels: proj-MacPwa
Components: UI>Browser>WebAppInstalls
Labels: -proj-MacPwa
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Labels: -Pri-3 M-73 Pri-1
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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

Project Member

Comment 11 by bugdroid1@chromium.org, 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

Project Member

Comment 12 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
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