Index: chrome/browser/mac/patch_nsmenu_key_window_bug.mm |
diff --git a/chrome/browser/mac/patch_nsmenu_key_window_bug.mm b/chrome/browser/mac/patch_nsmenu_key_window_bug.mm |
new file mode 100644 |
index 0000000000000000000000000000000000000000..0c707f462d6e45010d773c1d4f76811019ea273e |
--- /dev/null |
+++ b/chrome/browser/mac/patch_nsmenu_key_window_bug.mm |
@@ -0,0 +1,72 @@ |
+// Copyright 2017 The Chromium Authors. All rights reserved. |
+// Use of this source code is governed by a BSD-style license that can be |
+// found in the LICENSE file. |
+ |
+#import <AppKit/AppKit.h> |
+#import <objc/runtime.h> |
+ |
+// https://crbug.com/692408 |
Avi (use Gerrit)
2017/02/28 16:24:24
Radar # too?
Sidney San Martín
2017/03/01 00:50:20
Done.
|
+// |
+// Back as far as macOS 10.9, and up to at least macOS 10.12.3, an AppKit bug |
+// can leave window A in what looks like a key state while window B is actually |
+// key. Key presses go to window B even after you click on window A. |
+// |
+// To trigger the bug, open a menu which has custom views (e.g. the wrench |
+// menu) and then, while the menu is open, click the titlebar of a different |
+// window to bring it to the front. Then, click the first window to bring it |
+// *back* to the front and try to type. Input will go to the other window. |
+// |
+// ## Cause |
+// When a menu has any custom views, AppKit uses a different window |
+// class (NSCarbonMenuWindow) to represent it than it does otherwise |
+// (NSLimitedMenuViewWindow). NSCarbonMenuWindow overrides -[NSWindow |
+// makeKeyWindow] to make itself the key window without telling the old key |
+// window that it's lost key status (so that its appearance doesn't change). It |
+// saves the old key window in an instance variable. |
+// |
+// When the menu closes, it doesn't just make the saved key window key again. |
+// Instead, it picks the frontmost window that can become key. When the |
+// frontmost window is different — and clicking a window's titlebar brings it |
+// to the front — it never sends -[NSWindow resignKeyWindow] to the old key |
+// window, so it never loses key appearance and doesn't ask NSApplication to |
+// make it key when you click on it (since it thinks it's already key). |
+// |
+// ## Solution |
+// This category replaces a method on NSCarbonMenuWindow to send |
+// -resignKeyWindow to the saved window if the AppKit implementation made a |
+// different window key. It degrades to a noop if NSCarbonMenuWindow, its |
+// _rememberedKeyWindow ivar, or its |
+// _restorePreviousKeyWindowFromSavedProperties method no longer exists. |
+ |
+WEAK_IMPORT_ATTRIBUTE |
tapted
2017/02/28 23:24:13
Is this needed? I don't see it elsewhere in Chrome
Sidney San Martín
2017/03/01 00:50:20
Added a comment — or did you just mean a comment h
|
+@interface NSCarbonMenuWindow : NSWindow |
+- (void)_restorePreviousKeyWindowFromSavedProperties; |
+@end |
+ |
+static void (*_restorePreviousKeyWindowFromSavedProperties)(NSCarbonMenuWindow*, |
+ SEL); |
+ |
+static void _restorePreviousKeyWindowFromSavedProperties_patch( |
+ NSCarbonMenuWindow* self, |
+ SEL _cmd) { |
+ NSWindow* rememberedKeyWindow = nil; |
+ object_getInstanceVariable(self, "_rememberedKeyWindow", |
tapted
2017/03/01 02:12:20
What happens if `_rememberedKeyWindow` gets rename
Sidney San Martín
2017/03/01 03:08:35
In that case, object_getInstanceVariable() returns
tapted
2017/03/01 07:56:02
The return value of object_getInstanceVariable isn
Sidney San Martín
2017/03/01 17:42:59
OK, done.
|
+ (void**)&rememberedKeyWindow); |
+ _restorePreviousKeyWindowFromSavedProperties(self, _cmd); |
+ if ([NSApp keyWindow] != rememberedKeyWindow) { |
tapted
2017/02/28 23:24:13
What happens when switching applications? I think
Sidney San Martín
2017/03/01 00:50:20
Switching applications is OK. This all happens in
|
+ [rememberedKeyWindow resignKeyWindow]; |
tapted
2017/02/28 23:24:13
Is the menu still open at this point? Can we do so
Sidney San Martín
2017/03/01 00:50:20
Every NSCarbonMenuWindow has this bug, not just th
tapted
2017/03/01 02:12:20
I was going to ask "does Chrome have any other men
Sidney San Martín
2017/03/01 03:08:35
That's awesome! I just updated the radar with repr
|
+ } |
+} |
+ |
+@implementation NSCarbonMenuWindow (KeyWindowPatch) |
+ |
++ (void)load { |
+ _restorePreviousKeyWindowFromSavedProperties = |
tapted
2017/02/28 23:24:13
This needs to safely bail out (and DCHECK) if Appl
Sidney San Martín
2017/03/01 00:50:20
It should be safe in these cases; let me know if i
tapted
2017/03/01 02:12:20
Is that how WEAK_IMPORT_ATTRIBUTE works? Wouldn't
Sidney San Martín
2017/03/01 03:08:35
I'm as sure as I can be that this attribute affect
tapted
2017/03/01 07:56:02
Maybe we can make a little test project? My readin
tapted
2017/03/01 08:20:35
Is the API documented to do nothing? Is it really
Sidney San Martín
2017/03/01 17:42:59
It's not mentioned for this function, but many run
Sidney San Martín
2017/03/01 17:42:59
You're right that it'll fail at compile time if th
tapted
2017/03/01 22:56:46
I guess this thread falls under "general advice" (
tapted
2017/03/01 22:56:46
Does + (void)load behave different? I suspect it d
|
+ (void (*)(NSCarbonMenuWindow*, SEL))method_setImplementation( |
+ class_getInstanceMethod( |
+ [self class], |
+ @selector(_restorePreviousKeyWindowFromSavedProperties)), |
+ (IMP)_restorePreviousKeyWindowFromSavedProperties_patch); |
+} |
+ |
+@end |