From d8db6fa9dac40e0f7de41d8e0eba0f7f1c718180 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Tue, 12 Apr 2022 15:46:09 -0400 Subject: [PATCH] mac: cefclient: Use RootWindowManager to track key window status (fixes issue #3307) This change provides a generic solution for active (key) window tracking that works with both Views-hosted and native windows on MacOS. With this new approach we can now successfully route top menu actions to the currently active window. Prior to this change CEF's Views API was using focus notifications as a proxy for window activation notifications. That doesn't work on MacOS where NSWindow activation (key status) is independent of NSView focus (first responder) status, and changes in activation don't necessarily generate focus notifications (see NativeWidgetMac::OnWindowKeyStatusChanged). To make this work reliably on all platforms we now expose a CefWindowDelegate::OnWindowActivationChanged callback. This change also fixes an uninitialized variable (RootWindowMacImpl::with_extension_) that was causing flaky behavior in RootWindowManager::OnRootWindowActivated. To test: 1. Run `cefclient [--use-views]` 2. Select Popup Window from the Tests menu. Do not explicitly activate the popup window (e.g. don't click on it). 3. Verify that further Tests menu actions go to the popup window. 4. Change activation to a first window by clicking on it. Verify that Test menu actions go to that window. 5. Close the currently active window. Do not explicitly activate the remaining window (e.g. don't click on it). 6. Verify that Test menu actions go to the only remaining window. --- include/capi/views/cef_window_delegate_capi.h | 10 +++- include/cef_api_hash.h | 8 +-- include/views/cef_window_delegate.h | 7 +++ libcef/browser/views/window_view.cc | 7 +++ libcef/browser/views/window_view.h | 1 + .../cpptoc/views/window_delegate_cpptoc.cc | 25 +++++++++- .../ctocpp/views/window_delegate_ctocpp.cc | 24 ++++++++- .../ctocpp/views/window_delegate_ctocpp.h | 4 +- tests/cefclient/browser/root_window.h | 10 ---- tests/cefclient/browser/root_window_mac.mm | 49 ++++++------------- tests/cefclient/browser/views_window.cc | 11 +++-- tests/cefclient/browser/views_window.h | 2 + tests/cefclient/cefclient_mac.mm | 16 +++--- 13 files changed, 109 insertions(+), 65 deletions(-) diff --git a/include/capi/views/cef_window_delegate_capi.h b/include/capi/views/cef_window_delegate_capi.h index 3bcffb3e2..de4862213 100644 --- a/include/capi/views/cef_window_delegate_capi.h +++ b/include/capi/views/cef_window_delegate_capi.h @@ -33,7 +33,7 @@ // by hand. See the translator.README.txt file in the tools directory for // more information. // -// $hash=cd5d7c4e83237ceb39c5639489ca6004d2d69f0c$ +// $hash=7f88c6428929c0511ff90b6137efcc4cebcfeae4$ // #ifndef CEF_INCLUDE_CAPI_VIEWS_CEF_WINDOW_DELEGATE_CAPI_H_ @@ -73,6 +73,14 @@ typedef struct _cef_window_delegate_t { void(CEF_CALLBACK* on_window_destroyed)(struct _cef_window_delegate_t* self, struct _cef_window_t* window); + /// + // Called when |window| is activated or deactivated. + /// + void(CEF_CALLBACK* on_window_activation_changed)( + struct _cef_window_delegate_t* self, + struct _cef_window_t* window, + int active); + /// // Return the parent for |window| or NULL if the |window| does not have a // parent. Windows with parents will not get a taskbar button. Set |is_menu| diff --git a/include/cef_api_hash.h b/include/cef_api_hash.h index a56cf26c8..8dd257fc7 100644 --- a/include/cef_api_hash.h +++ b/include/cef_api_hash.h @@ -42,13 +42,13 @@ // way that may cause binary incompatibility with other builds. The universal // hash value will change if any platform is affected whereas the platform hash // values will change only if that particular platform is affected. -#define CEF_API_HASH_UNIVERSAL "34a42688778747c68f31e0194fea8323bc5388dc" +#define CEF_API_HASH_UNIVERSAL "1d7470ed03e8ddca6998284bef50694f5bad036c" #if defined(OS_WIN) -#define CEF_API_HASH_PLATFORM "c78306ad4768dc6c4ca1c376fa4c3799db8fd0eb" +#define CEF_API_HASH_PLATFORM "1c2e8c364d8dead07b5601bcac5c51392eb0821d" #elif defined(OS_MAC) -#define CEF_API_HASH_PLATFORM "c25d8bb5b7118a8f50c84e8a2746b2a14d7193ef" +#define CEF_API_HASH_PLATFORM "934c730bb8b18a6b987612e5e99b28173f189c72" #elif defined(OS_LINUX) -#define CEF_API_HASH_PLATFORM "742fc8a47e757e34a87c7c814fc884591d5e9f98" +#define CEF_API_HASH_PLATFORM "f45d62cfe6b26112a91404c0ff1332e3ff153b8b" #endif #ifdef __cplusplus diff --git a/include/views/cef_window_delegate.h b/include/views/cef_window_delegate.h index 00a604d19..1b4776f76 100644 --- a/include/views/cef_window_delegate.h +++ b/include/views/cef_window_delegate.h @@ -63,6 +63,13 @@ class CefWindowDelegate : public CefPanelDelegate { /*--cef()--*/ virtual void OnWindowDestroyed(CefRefPtr window) {} + /// + // Called when |window| is activated or deactivated. + /// + /*--cef()--*/ + virtual void OnWindowActivationChanged(CefRefPtr window, + bool active) {} + /// // Return the parent for |window| or NULL if the |window| does not have a // parent. Windows with parents will not get a taskbar button. Set |is_menu| diff --git a/libcef/browser/views/window_view.cc b/libcef/browser/views/window_view.cc index 88d775e17..3a9f255ba 100644 --- a/libcef/browser/views/window_view.cc +++ b/libcef/browser/views/window_view.cc @@ -524,6 +524,13 @@ void CefWindowView::ViewHierarchyChanged( ParentClass::ViewHierarchyChanged(details); } +void CefWindowView::OnWidgetActivationChanged(views::Widget* widget, + bool active) { + if (cef_delegate()) { + cef_delegate()->OnWindowActivationChanged(GetCefWindow(), active); + } +} + void CefWindowView::OnWidgetBoundsChanged(views::Widget* widget, const gfx::Rect& new_bounds) { MoveOverlaysIfNecessary(); diff --git a/libcef/browser/views/window_view.h b/libcef/browser/views/window_view.h index 7a26d18d5..6789636b8 100644 --- a/libcef/browser/views/window_view.h +++ b/libcef/browser/views/window_view.h @@ -79,6 +79,7 @@ class CefWindowView const views::ViewHierarchyChangedDetails& details) override; // views::WidgetObserver methods: + void OnWidgetActivationChanged(views::Widget* widget, bool active) override; void OnWidgetBoundsChanged(views::Widget* widget, const gfx::Rect& new_bounds) override; diff --git a/libcef_dll/cpptoc/views/window_delegate_cpptoc.cc b/libcef_dll/cpptoc/views/window_delegate_cpptoc.cc index 537e1f20d..d727303c1 100644 --- a/libcef_dll/cpptoc/views/window_delegate_cpptoc.cc +++ b/libcef_dll/cpptoc/views/window_delegate_cpptoc.cc @@ -9,7 +9,7 @@ // implementations. See the translator.README.txt file in the tools directory // for more information. // -// $hash=6639552a1a56f42d7f73ffec99d28fb98173dbd1$ +// $hash=60fce570867addbf622a0f74422d225f23942e4d$ // #include "libcef_dll/cpptoc/views/window_delegate_cpptoc.h" @@ -62,6 +62,27 @@ window_delegate_on_window_destroyed(struct _cef_window_delegate_t* self, CefWindowCToCpp::Wrap(window)); } +void CEF_CALLBACK window_delegate_on_window_activation_changed( + struct _cef_window_delegate_t* self, + cef_window_t* window, + int active) { + shutdown_checker::AssertNotShutdown(); + + // AUTO-GENERATED CONTENT - DELETE THIS COMMENT BEFORE MODIFYING + + DCHECK(self); + if (!self) + return; + // Verify param: window; type: refptr_diff + DCHECK(window); + if (!window) + return; + + // Execute + CefWindowDelegateCppToC::Get(self)->OnWindowActivationChanged( + CefWindowCToCpp::Wrap(window), active ? true : false); +} + cef_window_t* CEF_CALLBACK window_delegate_get_parent_window(struct _cef_window_delegate_t* self, cef_window_t* window, @@ -575,6 +596,8 @@ void CEF_CALLBACK window_delegate_on_blur(struct _cef_view_delegate_t* self, CefWindowDelegateCppToC::CefWindowDelegateCppToC() { GetStruct()->on_window_created = window_delegate_on_window_created; GetStruct()->on_window_destroyed = window_delegate_on_window_destroyed; + GetStruct()->on_window_activation_changed = + window_delegate_on_window_activation_changed; GetStruct()->get_parent_window = window_delegate_get_parent_window; GetStruct()->get_initial_bounds = window_delegate_get_initial_bounds; GetStruct()->get_initial_show_state = window_delegate_get_initial_show_state; diff --git a/libcef_dll/ctocpp/views/window_delegate_ctocpp.cc b/libcef_dll/ctocpp/views/window_delegate_ctocpp.cc index 2e71f32eb..d45ffc9ce 100644 --- a/libcef_dll/ctocpp/views/window_delegate_ctocpp.cc +++ b/libcef_dll/ctocpp/views/window_delegate_ctocpp.cc @@ -9,7 +9,7 @@ // implementations. See the translator.README.txt file in the tools directory // for more information. // -// $hash=557b305c33b5975b197bf930cc223f76b3032288$ +// $hash=423d12cda6148d99e5afb0cc4aa11818e0740e1e$ // #include "libcef_dll/ctocpp/views/window_delegate_ctocpp.h" @@ -57,6 +57,28 @@ void CefWindowDelegateCToCpp::OnWindowDestroyed(CefRefPtr window) { _struct->on_window_destroyed(_struct, CefWindowCppToC::Wrap(window)); } +NO_SANITIZE("cfi-icall") +void CefWindowDelegateCToCpp::OnWindowActivationChanged( + CefRefPtr window, + bool active) { + shutdown_checker::AssertNotShutdown(); + + cef_window_delegate_t* _struct = GetStruct(); + if (CEF_MEMBER_MISSING(_struct, on_window_activation_changed)) + return; + + // AUTO-GENERATED CONTENT - DELETE THIS COMMENT BEFORE MODIFYING + + // Verify param: window; type: refptr_diff + DCHECK(window.get()); + if (!window.get()) + return; + + // Execute + _struct->on_window_activation_changed(_struct, CefWindowCppToC::Wrap(window), + active); +} + NO_SANITIZE("cfi-icall") CefRefPtr CefWindowDelegateCToCpp::GetParentWindow( CefRefPtr window, diff --git a/libcef_dll/ctocpp/views/window_delegate_ctocpp.h b/libcef_dll/ctocpp/views/window_delegate_ctocpp.h index 5071dea81..60f6945ad 100644 --- a/libcef_dll/ctocpp/views/window_delegate_ctocpp.h +++ b/libcef_dll/ctocpp/views/window_delegate_ctocpp.h @@ -9,7 +9,7 @@ // implementations. See the translator.README.txt file in the tools directory // for more information. // -// $hash=e61d67d8295c9fcc3e801bf61f4381434924940c$ +// $hash=4041b496f1e9ed673f99211be26b8fa98967fece$ // #ifndef CEF_LIBCEF_DLL_CTOCPP_VIEWS_WINDOW_DELEGATE_CTOCPP_H_ @@ -39,6 +39,8 @@ class CefWindowDelegateCToCpp // CefWindowDelegate methods. void OnWindowCreated(CefRefPtr window) override; void OnWindowDestroyed(CefRefPtr window) override; + void OnWindowActivationChanged(CefRefPtr window, + bool active) override; CefRefPtr GetParentWindow(CefRefPtr window, bool* is_menu, bool* can_activate_menu) override; diff --git a/tests/cefclient/browser/root_window.h b/tests/cefclient/browser/root_window.h index c5d859b3e..7341355d6 100644 --- a/tests/cefclient/browser/root_window.h +++ b/tests/cefclient/browser/root_window.h @@ -18,10 +18,6 @@ #include "tests/cefclient/browser/image_cache.h" #include "tests/shared/browser/main_message_loop.h" -#if defined(OS_MAC) && __OBJC__ -@class NSWindow; -#endif // defined(OS_MAC) && __OBJC__ - namespace client { // Used to configure how a RootWindow is created. @@ -122,12 +118,6 @@ class RootWindow // called on the main thread. static scoped_refptr GetForBrowser(int browser_id); -#if defined(OS_MAC) && __OBJC__ - // Returns the RootWindow associated with the specified |window|. Must be - // called on the main thread. - static scoped_refptr GetForNSWindow(NSWindow* window); -#endif - // Initialize as a normal window. This will create and show a native window // hosting a single browser instance. This method may be called on any thread. // |delegate| must be non-nullptr and outlive this object. diff --git a/tests/cefclient/browser/root_window_mac.mm b/tests/cefclient/browser/root_window_mac.mm index 5886728c0..acea3925f 100644 --- a/tests/cefclient/browser/root_window_mac.mm +++ b/tests/cefclient/browser/root_window_mac.mm @@ -120,45 +120,33 @@ class RootWindowMacImpl // After initialization all members are only accessed on the main thread. // Members set during initialization. RootWindowMac& root_window_; - bool with_controls_; - bool with_osr_; - bool with_extension_; - bool is_popup_; + bool with_controls_ = false; + bool with_osr_ = false; + bool with_extension_ = false; + bool is_popup_ = false; CefRect start_rect_; std::unique_ptr browser_window_; - bool initialized_; + bool initialized_ = false; // Main window. - NSWindow* window_; - RootWindowDelegate* window_delegate_; + NSWindow* window_ = nil; + RootWindowDelegate* window_delegate_ = nil; // Buttons. - NSButton* back_button_; - NSButton* forward_button_; - NSButton* reload_button_; - NSButton* stop_button_; + NSButton* back_button_ = nil; + NSButton* forward_button_ = nil; + NSButton* reload_button_ = nil; + NSButton* stop_button_ = nil; // URL text field. - NSTextField* url_textfield_; + NSTextField* url_textfield_ = nil; - bool window_destroyed_; - bool browser_destroyed_; + bool window_destroyed_ = false; + bool browser_destroyed_ = false; }; RootWindowMacImpl::RootWindowMacImpl(RootWindowMac& root_window) - : root_window_(root_window), - with_controls_(false), - with_osr_(false), - is_popup_(false), - initialized_(false), - window_(nil), - back_button_(nil), - forward_button_(nil), - reload_button_(nil), - stop_button_(nil), - url_textfield_(nil), - window_destroyed_(false), - browser_destroyed_(false) {} + : root_window_(root_window) {} RootWindowMacImpl::~RootWindowMacImpl() { REQUIRE_MAIN_THREAD(); @@ -741,13 +729,6 @@ void RootWindowMac::OnNativeWindowClosed() { impl_->OnNativeWindowClosed(); } -// static -scoped_refptr RootWindow::GetForNSWindow(NSWindow* window) { - RootWindowDelegate* delegate = - static_cast([window delegate]); - return [delegate root_window]; -} - } // namespace client @implementation RootWindowDelegate diff --git a/tests/cefclient/browser/views_window.cc b/tests/cefclient/browser/views_window.cc index 2b1480912..4dd597745 100644 --- a/tests/cefclient/browser/views_window.cc +++ b/tests/cefclient/browser/views_window.cc @@ -574,6 +574,14 @@ void ViewsWindow::OnWindowDestroyed(CefRefPtr window) { window_ = nullptr; } +void ViewsWindow::OnWindowActivationChanged(CefRefPtr window, + bool active) { + if (!active) + return; + + delegate_->OnViewsWindowActivated(this); +} + bool ViewsWindow::CanClose(CefRefPtr window) { CEF_REQUIRE_UI_THREAD(); @@ -706,9 +714,6 @@ void ViewsWindow::OnFocus(CefRefPtr view) { SetMenuFocusable(false); } } - - if (view_id == ID_BROWSER_VIEW) - delegate_->OnViewsWindowActivated(this); } void ViewsWindow::OnBlur(CefRefPtr view) { diff --git a/tests/cefclient/browser/views_window.h b/tests/cefclient/browser/views_window.h index 0472cf98b..f91f29085 100644 --- a/tests/cefclient/browser/views_window.h +++ b/tests/cefclient/browser/views_window.h @@ -153,6 +153,8 @@ class ViewsWindow : public CefBrowserViewDelegate, // CefWindowDelegate methods: void OnWindowCreated(CefRefPtr window) override; void OnWindowDestroyed(CefRefPtr window) override; + void OnWindowActivationChanged(CefRefPtr window, + bool active) override; CefRefPtr GetParentWindow(CefRefPtr window, bool* is_menu, bool* can_activate_menu) override; diff --git a/tests/cefclient/cefclient_mac.mm b/tests/cefclient/cefclient_mac.mm index 59e84dd78..afeb3121b 100644 --- a/tests/cefclient/cefclient_mac.mm +++ b/tests/cefclient/cefclient_mac.mm @@ -237,13 +237,11 @@ NSMenuItem* GetMenuItemWithAction(NSMenu* menu, SEL action_selector) { - (void)testsItemSelected:(int)command_id { // Retrieve the active RootWindow. - NSWindow* key_window = [[NSApplication sharedApplication] keyWindow]; - if (!key_window) + auto root_window = + client::MainContext::Get()->GetRootWindowManager()->GetActiveRootWindow(); + if (!root_window) return; - scoped_refptr root_window = - client::RootWindow::GetForNSWindow(key_window); - CefRefPtr browser = root_window->GetBrowser(); if (browser.get()) client::test_runner::RunTest(browser, command_id); @@ -319,13 +317,11 @@ NSMenuItem* GetMenuItemWithAction(NSMenu* menu, SEL action_selector) { - (void)enableAccessibility:(bool)bEnable { // Retrieve the active RootWindow. - NSWindow* key_window = [[NSApplication sharedApplication] keyWindow]; - if (!key_window) + auto root_window = + client::MainContext::Get()->GetRootWindowManager()->GetActiveRootWindow(); + if (!root_window) return; - scoped_refptr root_window = - client::RootWindow::GetForNSWindow(key_window); - CefRefPtr browser = root_window->GetBrowser(); if (browser.get()) { browser->GetHost()->SetAccessibilityState(bEnable ? STATE_ENABLED