Clicking the titlebar of any other Chrome window while the app/hotdog menu is open "locks" key focus to that window |
||||||||
Issue descriptionChrome Version : 58.0.3004.3 OS Version: OS X 10.12.3 What steps will reproduce the problem? 1. Open Chrome, press Cmd+n (have two windows) 2. Open hotdog menu (in either) 3. Click the *titlebar* of the other window (the one not showing the menu) (here, note that both windows' "traffic lights" are lit, even though only one window has a darker titlebar) 4. Try to switch back to the other window (the one that was showing the menu) by any means 5. Type (e.g. in the omnibox or a password field) What is the expected result? keystrokes appear in the omnibox with the focus ring around it What happens instead of that? - keystrokes go to the other window - There's no way to activate the browser window that was showing the NSMenu until a new NSWindow of some kind is created, or another app is activated. This also happens with any popup window, any packaged app window, Hangouts, etc.. This is particularly bad considering the flow of: - Hangouts extension installed -> hangouts Browser Action has overflowed into the menu - User opens app menu, clicks the Hangouts icon, hangouts raises itself (menu stays open) - User clicks titlebar of hangouts extension window to dismiss menu - User tries to go back to browser - all their keystrokes get sent over a hangouts chat (see Issue 686595) I don't know if we can fix this ourselves -- it feels very much like an AppKit bug. Context menus and the MainMenu do not trigger it -- just the Chrome menu. Maybe it's something to do with putting NSViews in there? I can spend some time investigating, but +cc a bunch of people in case they have any advice. UserAgentString: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3004.3 Safari/537.36
,
Feb 17 2017
> Maybe it's something to do with putting NSViews in there?
Seems like this is the case.
I can repro in Chromium. But with this change:
--- a/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm
+++ b/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm
@@ -201,8 +201,8 @@ class ToolbarActionsBarObserverHelper : public ToolbarActionsBarObserver {
// Non-button item types should be built as normal items, with the exception
// of the extensions overflow menu.
int command_id = model->GetCommandIdAt(index);
- if (model->GetTypeAt(index) != ui::MenuModel::TYPE_BUTTON_ITEM &&
- command_id != IDC_EXTENSIONS_OVERFLOW_MENU) {
+ if (true || (model->GetTypeAt(index) != ui::MenuModel::TYPE_BUTTON_ITEM &&
+ command_id != IDC_EXTENSIONS_OVERFLOW_MENU)) {
[super addItemToMenu:menu
atIndex:index
fromModel:model];
The repro stops.
It's not (just) the extension overflow items. Repro comes back if I only add other buttons (or only add the extension overflow items). That is, only with all the non-standard menu items removed does the repro stop.
,
Feb 17 2017
Made a repro in XCode. (did it without a single line of Objective C \o/ #achievement)
,
Feb 17 2017
filed rdar://30572648 If we want to fix this sooner for Chrome, we'll need to remove all the custom NSViews from the menu (or use views::MenuItemView).
,
Feb 21 2017
Dug into this last week. It looks like AppKit takes a different, ancient-looking code path when you have custom views in a menu, and uses a window class called NSCarbonMenuWindow instead of NSLimitedMenuViewWindow. NSCarbonMenuWindow does nastier things with the app's main/key window state, and has this bug. I wrote a runtime patch, attached. Senior AppKit hackers — thoughts?
,
Feb 21 2017
Attached a slightly-updated patch that doesn't use [NSApp mainWindow] as a proxy for "what was the old key window?", since that could be wrong. It accesses a private ivar instead. Quick explanation of the whole thing: NSCarbonMenuWindow overrides -makeKeyWindow: to skip telling the old key window that it's no longer key, and just makes itself the key window directly (-[NSApplication _setKeyWindow:]). There's an assumption that, when dismissed, it will give key status back to the same window. In practice, it makes the *frontmost* window key. Since clicking a window's titlebar brings it to the front, the clicked window becomes key and the old key window never gets told that it's no longer key. It keeps key appearance and, crucially, doesn't try to become key when you click on it. This patch tests for the case where the new window isn't the old one, and calls -resignKeyWindow on the old key window so that it no longer believes itself to be key.
,
Feb 21 2017
Can you make a cl for this? That'll make it much easier to review and comment on.
,
Feb 21 2017
Yes, in progress.
,
Feb 21 2017
Could we instead detect when the menu has stopped tracking via NSMenuDelegate and fixup the key window after that?
,
Feb 21 2017
,
Feb 21 2017
Re. #9: That should be possible, but the fix would either need to get at the saved key window at the right time or also save it in advance. It also sounds like that would be on a case-by-case basis, whereas this is a global fix. Since this really does seem to be an AppKit bug, I like the global fix, but I guess I could be convinced that a case-by-case fix is better.
,
Aug 16 2017
This no longer repros in macOS 10.13 High Sierra (tested beta 5). It still repros in 10.12.6 (Fixed? Or Fixed once we drop 10.12 support ¯\_(ツ)_/¯ )
,
Aug 16 2017
We *could* land a patch for macOS < 10.13 if this is something that bugs people? My vote would be WontFix… the fact that it works in 10.13 reaffirms its status as an OS bug.
,
Aug 21
Archiving old bugs that haven't been actively assigned in over a year. If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!
,
Aug 21
Archiving old bugs that haven't been actively assigned in over a year. If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!
,
Aug 21
Archiving old bugs that haven't been actively assigned in over a year. If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks! |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by tapted@chromium.org
, Feb 15 2017