Thanks for filing this!

These shortcuts tie in with some recent proposals around modality changes in the browser too.

For Cmd+w I'm inclined to say that it should go to the bubble (and close the bubble), rather than close the tab. This is how NSPopover behaves in Safari.

Cmd+t in Safari, however, does still open a new tab. This does feel correct, for the reasons you cite above.

But we need to nail down more precisely what the behaviour should be, so that we can finally close out bugs like  Issue 603881  and  Issue 605374 

Some things should perhaps be blocked, or NSBeep. E.g. Cmd+f currently shows the 'find in page' dropdown, but it's on the tab _behind_ the popup, but the popup stays up, so keystrokes don't go to the newly shown textfield in the find popup.

Maybe _only_ cmd+t / cmd+n should be allowed - we might need to make a full list.

> These shortcuts tie in with some recent proposals around modality changes in the browser too.

Are you referring to https://bugs.chromium.org/p/chromium/issues/detail?id=677012#c5 ?

Also, what about platforms other than Mac? They exhibit the same problem. For example, comparing it to Edge on Win10: Ctrl-T, Ctrl-W and Ctrl-Tab work while popups are open (tried on page info and bookmarks; permission requests are displayed in something similar to an infobar).

Hi!

Thanks for reporting and discussing this issue. 

re: cmd+t, I'm looking into a possibility of closing the owning tab, and potential issues by doing so. 

Addressing this issue is likely to be part of a dialog behavior polish proposal, which intends to include what shortcut behaviors should be allowed/blocked, what menus/features should be allowed/blocked, what type of dialog/modality to use when, how to close them, etc.

> Are you referring to https://bugs.chromium.org/p/chromium/issues/detail?id=677012#c5 ?

That's part of it. There's a doc also (internal - sorry).

https://codereview.chromium.org/2604793002/#ps1 is too complex, since we haven't mapped out exactly how these should behave yet. However, I suggested some alternatives in my comment that could lead to a less complex solution, which still has the flexibility to easily explore the approaches discussed here.

(in fact, the comment that went
"""
modify the implementation of BrowserWindowCommandHandler so that the lines like

Browser* browser = chrome::FindBrowserWithWindow(window);

are able to support walking up the hierarchy of parent windows until am actual
browser window is found.
"""

could also allow  Issue 603881  to be fixed for the Cocoa browser. If the fix is simple enough, we might just land that and worry about filtering of specific commands in a follow-up.

*correction on c3: I meant cmd+w to close tab (not t, note: we may eventually want to include cmd-t too although it might be through a separate follow-up).

> If the fix is simple enough, we might just land that and worry about filtering of specific commands in a follow-up.

That would enable passing of all accelerators from all of the bubbles. For example, website settings popup does not expect that active tab might change while it's still open; also when pressing Cmd-F while bookmarks bubble is open will make them intersect in a weird way.

Also, I'm worried about other platforms, as I do feel that solution should be somewhat cross-platform. And I like the idea of attaching an accelerator handler (that might also be attached to other windows, including a browser window) to bubbles instead of passing accelerators from bubbles to their parent windows. Maybe we can work out a way to do something similar on other toolkit-views platforms?

> re: cmd+t, I'm looking into a possibility of closing the owning tab, and potential issues by doing so.

Thanks!

> Maybe we can work out a way to do something similar on other toolkit-views platforms?

Yeah - I haven't looked too much into what happens in Aura. On ChromeOS/Ash some things bubble up to the root window and do meaningful things there. It does feel like we need something similar.

On Ash, it's done by

bool AshFocusManagerFactory::Delegate::ProcessAccelerator(
    const ui::Accelerator& accelerator) {
  AcceleratorController* controller = WmShell::Get()->accelerator_controller();
  if (controller)
    return controller->Process(accelerator);
  return false;
}

Perhaps we can have BrowserView implement FocusManagerDelegate as well. Add a FocusManager::SetDelegate(..) method. Then hook them up in an appropriate place.


E.g. ChromeViewsDelegate::OnBeforeWidgetInit(..) could use FindBrowserWithWindow(..) on the Widget's parent and, if it's a Browser* with a FocusManagerDelegate (and the child widget FocusManagerDelegate isn't set), set it to the BrowserView.

Then BrowserView's FocusManagerDelegate::ProcessAccelerator does something like check in |accelerator_table_| for a new field like |process_accelerator_from_child_| so that it processes unprocessed accelerators from child widgets in the Browser*.

That should work on both Aura and MacViewsBrowser.  For MacViews, we could make a shim FocusManagerDelegate that lives in ChromeCommandDispatcherDelegate or similar.

After reading some more, it now seems that the only cross-platform thing here is a list of allowed "shared" hotkeys. The reason for this: MacViews deals with accelerators via command_ids, Aura via ui::Accelerator. So, here's what I've sketched so far:

c/b/u/v/accelerator_table.h:
// Get a list of accelerators that can be shared between browser window and its popup children.
std::vector<AcceleratorMapping> GetSharedAcceleratorList();

ViewsDelegate:
#if defined(OS_MACOSX)
// Create a command handler that forwards command requests to a shared handler associated with |parent|.
UserInterfaceItemCommandHandler* CreateSharedCommandHandler(gfx::NativeView parent);
#endif
// Create a FocusManagerDelegate that forwards accelerator press requests to a shared handler associated with |parent|.
std::unique_ptr<views::FocusManagerDelegate> CreateSharedAcceleratorHandler(gfx::NativeView parent);

ChromeViewsDelegate takes GetSharedAcceleratorList() and
* in CreateSharedCommandHandler creates a UserInterfaceItemCommandHandler that forwards commands to BrowserWindowCommandHandler found by walking up window hierarchy from |parent|.
* in CreateSharedAcceleratorHandler create FocusManagerDelegate that forwards accelerators to BrowserView found from TopLevelWidget of |parent|.

Widget can use this FocusManagerDelegate while creating a FocusManager.
NativeWidgetMac can use UserInterfaceItemCommandHandler when creating NativeWidgetMacNSWindow.

To limit which Widgets have this accelerator sharing behavior, we can introduce Widget::InitParam::use_shared_accelerator_handler.

Does this sound sensible?

On the other hand, looking up the Browser by NativeWindow seems roundabout here: when you're creating a popup for a browser window, you're likely to already have a Browser object somewhere near. Maybe this "shared accelerator handler" binding can be done from outside:

c/b/u/v/accelerator_table.h:
// Get a list of accelerators that can be shared between browser window and its popup children.
std::vector<AcceleratorMapping> GetSharedAcceleratorList();
// Forward (a subset of) accelerators from |widget| to |browser|.
void AttachSharedAcceleratorHandler(Browser* browser, Widget* widget);

c/b/u/v/accelerator_table_aura.cc:
void AttachSharedAcceleratorHandler(Browser* browser, Widget* widget) {
  if (!widget->HasFocusManager())
    return;
  auto* focus_manager = widget->GetFocusManager();
  // ForwardingAcceleratorsFocusManager takes a Browser* to forward to (via BrowserView*), std::vector<AcceleratorMapping> of allowed accelerators and std::unique_ptr<FocusManagerDelegate> as a fallback delegate (or does it make sense to first try this delegate and fallback to Browser later).
  focus_manager->SetDelegate(new ForwardingAcceleratorsFocusManager(browser, GetSharedAcceleratorList(), focus_manager->TakeDelegate()));
}

c/b/u/v/accelerator_table_mac.mm:
void AttachSharedAcceleratorHandler(Browser* browser, Widget* widget) {
  NativeWidgetMacNSWindow* window = nullptr;
  if ([widget->GetNativeWindow() isKindOfClass:[NativeWidgetMacNSWindow class])
    window = static_cast<NativeWidgetMacNSWindow*>(widget->GetNativeWindow());
  if (!window)
    return;
  // Like Aura implementation from above but instead of forwarding to BrowserView it forwards to BrowserWindowCommandHandler.
  [window setCommandHandler:[[ForwardingCommandHandler alloc] initWithBrowser:browser accelerators:GetSharedAcceleratorList() fallbackCommandHandler:[window commandHandler]]];
}

Bubbles that want to opt-in to accelerator sharing just call AttachSharedAcceleratorHandler after creating bubble's Widget. What do you think?

> The reason for this: MacViews deals with accelerators via command_ids, Aura via ui::Accelerator. So, here's what I've sketched so far:

BrowserView on MacViews still uses ui::Accelerator for some things. We could actually increase the overlap - ui/base/cocoa/command_dispatcher.h doesn't currently use ui::Accelerator, but it could. Instead it's using these `KeyToCommandMapper` function pointers. We didn't want to refactor the Cocoa browser too much at the same time as introducing the CommandDispatcher stuff, but maybe now is the time for that.

ui::Accelerator is used elsewhere in the Cocoa browser too -- e.g. extension keymappings I think.

I suspect the difference for the browser window is that we need to tie in to the mainMenu. Because:
 1. Activating the main menubar needs to query the accelerator table to see what items should be enabled,
 2. Users can reconfigure the keyboard shortcut for items that appear in the main menubar on Mac, and
 3. Activating a shortcut that appears in the main menubar must "flash" the menuitem in which it appears at the top of the screen.


If replacing global_keyboard_shortcuts_mac.hs's KeyboardShortcutData with a pair<ui::Accelerator, int /* chrome command */> will make things simpler, then that may be a good first step. There is some subtle stuff in global_keyboard_shortcuts_mac.mm -- these concepts will need to be added to ui::Accelerator (or ui::PlatformAccelerator? - it's confusing having both of those - maybe they can be merged).


> Does this sound sensible?

Mostly yes, I think ideally we would want to use FocusManagerDelegate on all platforms. But make it "aware" of those additional Mac requirements above for handling shortcuts (i.e. ui::Accelerator) in the mainMenu. Wrapping the existing UserInterfaceItemCommandHandler interfaces may be the way to do this on Mac.

We may be able to simplify things by omitting Widget::InitParam::use_shared_accelerator_handler -- we don't want every Widget initialization to have to make a decision about shortcuts. I think we can assume that if there's a parent window/Widget, and the child Widget didn't handle an accelerator, then it's automatically sent to parent->GetWidget()->GetFocusManager()->ProcessAccelerator(..). But we may want to add a field to ui::Accelerator about whether it bubbles up across windows, or pass a "from child" argument.


Sorry none of this is particularly concrete. The shortcut handling has become complex, and we need to look for ways to reduce that complexity without introducing regressions. This is one of the great challenges in programming :). So whatever solution we come up with, it will depend on how the final code looks.

The end goal we strive for in Chrome is typically "The simplest thing that works", (e.g. since simplicity is one of the core principles - https://www.chromium.org/developers/core-principles ).

Note if we are modifying CommandDispatcher, there may be changes required for the "Browser Shortcuts in Fullscreen" proposal -- https://groups.google.com/a/chromium.org/d/msg/chromium-dev/q0ICx0y5540/CuufU0TNAgAJ

That's not necessarily a blocking requirement for this stuff around bubbles. Just giving a heads up in case of merge conflicts.

Assigning a blocker to track, but we can explore this in parallel to earlier phases.

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

commit e91c25b113011f055428c46609e19ce6550b41ea
Author: tapted <tapted@chromium.org>
Date: Thu Feb 02 22:43:45 2017

Allow permission bubbles to participate in key event dispatch as if they were a Browser.

Accomplish this by allowing the `CommandDispatcher` we use for
ChomeEventProcessingWindows to bubble up key handling, and command
validation/dispatch. Dispatch goes up to a parent window's
CommandDispatcher when certain conditions are met.

Currently all ChomeEventProcessingWindows and Views'
NativeWidgetMacNSWindows have a CommandDispatcher, but only browser
windows provide a CommandHandler. By asking the CommandHandler in the
parent window, we can validate commands in the mainMenu, then forward
the -commandDispatch: action from the menu item when NSMenu calls it on
the key window (which might not be a browser).

Adds a rather fun interactive UI test for this that runs with Views and
Cocoa permissions bubbles, and tests commands dispatched via the
mainMenu (Cmd+w, Cmd+Alt+Left) and performKeyEquivalent (Cmd+Shift+'{')

BUG= 603881 , 679339

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

[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/permissions/permission_request_manager.h
[add] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/permissions/permission_request_manager_test_api.cc
[add] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/permissions/permission_request_manager_test_api.h
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/ui/cocoa/chrome_event_processing_window.mm
[add] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_interactive_uitest.mm
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/test/BUILD.gn
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/ui/base/cocoa/command_dispatcher.h
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/ui/base/cocoa/command_dispatcher.mm
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/ui/views/cocoa/native_widget_mac_nswindow.mm

tapted@ - what's left on this bug? It looks like you landed some code to fix it.

Hm.. Cocoa webmodals still have this bug. E.g. print preview: Cmd+w/t don't work, with or without --secondary-ui-md (but they do work on ChromeOS). Mac+http-auth: Cmd+w/t work with --secondary-ui-md, but not without. (print preview with --secondary-ui-md still uses a Cocoa container).

I've left this open because of #c1:

"..we need to nail down more precisely what the behaviour should be, so that we can finally close out bugs like   Issue 603881   and  Issue 605374 

Some things should perhaps be blocked, or NSBeep. E.g. Cmd+f currently shows the 'find in page' dropdown, but it's on the tab _behind_ the popup, but the popup stays up, so keystrokes don't go to the newly shown textfield in the find popup.

Maybe _only_ cmd+t / cmd+n should be allowed - we might need to make a full list."


But that `full list` can be punted to HarmonyPhase 2. There was more about shortcut behavior in one of hwi@'s docs/sheets. But I couldn't find what I was thinking of. Doesn't seem to be go/zmdssb go/zmdss or go/keyld

I'm calling this M-X. tapted@, can you own following up with UI on this during Harmony phase 2? it would be good to have a consistent idea of which hotkeys are available in what contexts.

re: c16 for HarmonyPhase2, yes, I'll document & discuss a proposal as part of the preparation for HarmonyPhase2. 

Also related: crbug.com/775717

Issue 819232 has been merged into this issue.

MacViews triage: this is going to require working with UI to decide what precisely the behavior should be, which is unlikely to happen before M-69. I'm going to call this M-X and leave it available.

This is not a Target-69 bug.

 Issue 628944  has been merged into this issue.

Note Issue 775717 is Windows. We're now better positioned to make hotkeys / mainMenu work in a cross-platform manner when {tab-modals, app modals, popups, window-modals} are open.

(But I think we also want to kill-off some of those dialog types).