Commit da0f0b78 authored by Leonard Grey's avatar Leonard Grey Committed by Commit Bot

Revert "Fix cmd+w closing the browser window instead of just the dictionary popover."

This reverts commit d3d72b4f.

Reason for revert: crbug/1017263

Original change's description:
> Fix cmd+w closing the browser window instead of just the dictionary popover.
> 
> It's been said before and will be said again: keyboard event handling in
> Chrome is a monster. Several things came together to make this happen:
> 
> First, in most Mac apps, cmd+w closes the dictionary popover if it's
> visible. This specifically happens through the `performClose:` action
> sent by the main menu. But, in Chrome, cmd+w closes the tab and not the
> window, so cmd+w sends a different action. This meant that cmd+w
> originally closed the tab and left the dictionary popover floating. This
> was fixed many years ago (see r118131 and r322075) by making
> `-[AppController menuNeedsUpdate:]` check the current target of
> `performClose:` and only map cmd+w to "close tab" when the target is a
> browser window.
> 
> This would work but for another layer of keyboard shortcut handling
> trickery. For a number of reasons (see comment beginning "If this
> keyEquivalent" added in r565490), Chrome doesn't actually let
> `NSMenuItem`s send their own actions. Instead,
> ChromeCommandDispatcherDelegate determines which menu item *would* have
> been targeted by a given event, then handles the event itself.
> 
> The end result is that, when the dictionary popover is visible, the menu
> items are successfully remapped so that cmd+w means "Close Window". But,
> any actual cmd+w event is grabbed by ChromeCommandDispatcherDelegate and
> sent directly to Chrome's own keyboard shortcut handling… so the browser
> window closes.
> 
> This change adds a check that was proposed back in r322075, and makes
> ChromeCommandDispatcherDelegate only grab the event when the found menu
> item's action would be sent to its own window.
> 
> Bug: 947752
> Change-Id: Ib32988d5fb69c79f899069feb02f7529ea05225f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849431
> Commit-Queue: Sidney San Martín <sdy@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#705275}

TBR=avi@chromium.org,sdy@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 947752
Change-Id: I4d2aef624e111d4e14c4a0d36e7f6a08f2fa08cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1876690Reviewed-by: default avatarLeonard Grey <lgrey@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708671}
parent 17e4a047
...@@ -35,9 +35,6 @@ struct CommandForKeyEventResult { ...@@ -35,9 +35,6 @@ struct CommandForKeyEventResult {
// The command to execute. -1 if none was found. // The command to execute. -1 if none was found.
int chrome_command; int chrome_command;
// The action, if any, which the key event would trigger.
SEL action;
// Whether the command was from a mapping in the main menu. Only relevant if // Whether the command was from a mapping in the main menu. Only relevant if
// command != -1. // command != -1.
bool from_main_menu; bool from_main_menu;
......
...@@ -59,7 +59,7 @@ NSMenuItem* FindMenuItem(NSEvent* key, NSMenu* menu) { ...@@ -59,7 +59,7 @@ NSMenuItem* FindMenuItem(NSEvent* key, NSMenu* menu) {
return result; return result;
} }
int MenuCommandForKeyEvent(NSEvent* event, SEL* action) { int MenuCommandForKeyEvent(NSEvent* event) {
if ([event type] != NSKeyDown) if ([event type] != NSKeyDown)
return -1; return -1;
...@@ -78,10 +78,8 @@ int MenuCommandForKeyEvent(NSEvent* event, SEL* action) { ...@@ -78,10 +78,8 @@ int MenuCommandForKeyEvent(NSEvent* event, SEL* action) {
if (!item) if (!item)
return -1; return -1;
if ([item action] == @selector(commandDispatch:) && [item tag] > 0) { if ([item action] == @selector(commandDispatch:) && [item tag] > 0)
*action = [item action];
return [item tag]; return [item tag];
}
// "Close window" doesn't use the |commandDispatch:| mechanism. Menu items // "Close window" doesn't use the |commandDispatch:| mechanism. Menu items
// that do not correspond to IDC_ constants need no special treatment however, // that do not correspond to IDC_ constants need no special treatment however,
...@@ -120,15 +118,15 @@ GetDelayedShortcutsNotPresentInMainMenu() { ...@@ -120,15 +118,15 @@ GetDelayedShortcutsNotPresentInMainMenu() {
} }
CommandForKeyEventResult NoCommand() { CommandForKeyEventResult NoCommand() {
return {-1, nil, /*from_main_menu=*/false}; return {-1, /*from_main_menu=*/false};
} }
CommandForKeyEventResult MainMenuCommand(int cmd, SEL action) { CommandForKeyEventResult MainMenuCommand(int cmd) {
return {cmd, action, /*from_main_menu=*/true}; return {cmd, /*from_main_menu=*/true};
} }
CommandForKeyEventResult ShortcutCommand(int cmd) { CommandForKeyEventResult ShortcutCommand(int cmd) {
return {cmd, nil, /*from_main_menu=*/false}; return {cmd, /*from_main_menu=*/false};
} }
} // namespace } // namespace
...@@ -205,10 +203,9 @@ CommandForKeyEventResult CommandForKeyEvent(NSEvent* event) { ...@@ -205,10 +203,9 @@ CommandForKeyEventResult CommandForKeyEvent(NSEvent* event) {
if ([event type] != NSKeyDown) if ([event type] != NSKeyDown)
return NoCommand(); return NoCommand();
SEL action; int cmdNum = MenuCommandForKeyEvent(event);
int cmdNum = MenuCommandForKeyEvent(event, &action);
if (cmdNum != -1) if (cmdNum != -1)
return MainMenuCommand(cmdNum, action); return MainMenuCommand(cmdNum);
// Scan through keycodes and see if it corresponds to one of the non-menu // Scan through keycodes and see if it corresponds to one of the non-menu
// shortcuts. // shortcuts.
......
...@@ -94,24 +94,17 @@ ...@@ -94,24 +94,17 @@
// By not passing the event to AppKit, we do lose out on the brief // By not passing the event to AppKit, we do lose out on the brief
// highlighting of the NSMenu. // highlighting of the NSMenu.
CommandForKeyEventResult result = CommandForKeyEvent(event); CommandForKeyEventResult result = CommandForKeyEvent(event);
if (result.found()) {
if (!result.found()) auto* bridge =
return ui::PerformKeyEquivalentResult::kUnhandled; remote_cocoa::NativeWidgetNSWindowBridge::GetFromNativeWindow(window);
if (bridge) {
// If the menu item will dispatch to a different window (real-world example: bool was_executed = false;
// the dictionary definition popover), don't handle the event here. bridge->host()->ExecuteCommand(
if (result.action && [NSApp targetForAction:result.action] != window) result.chrome_command, WindowOpenDisposition::CURRENT_TAB,
return ui::PerformKeyEquivalentResult::kUnhandled; true /* is_before_first_responder */, &was_executed);
if (was_executed)
auto* bridge = return ui::PerformKeyEquivalentResult::kHandled;
remote_cocoa::NativeWidgetNSWindowBridge::GetFromNativeWindow(window); }
if (bridge) {
bool was_executed = false;
bridge->host()->ExecuteCommand(
result.chrome_command, WindowOpenDisposition::CURRENT_TAB,
true /* is_before_first_responder */, &was_executed);
if (was_executed)
return ui::PerformKeyEquivalentResult::kHandled;
} }
return ui::PerformKeyEquivalentResult::kUnhandled; return ui::PerformKeyEquivalentResult::kUnhandled;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment