From 3ea26fb9d0a77d57ee88b084212a0bfc13fb2e47 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Wed, 10 Apr 2024 13:20:44 +0200 Subject: [PATCH] fix: move BrowserWindow's WebContentsView to be a child of rootview (#41802) fix: move BrowserWindow's WebContentsView to be a child of rootview (#41256) Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Jeremy Rose --- .../api/electron_api_browser_window.cc | 10 +++++- .../browser/api/electron_api_browser_window.h | 1 + shell/browser/api/frame_subscriber.cc | 1 + shell/browser/native_window_views.cc | 10 +++--- shell/browser/ui/views/root_view.cc | 35 +++++-------------- shell/browser/ui/views/root_view.h | 6 +++- spec/api-browser-window-spec.ts | 2 +- 7 files changed, 31 insertions(+), 34 deletions(-) diff --git a/shell/browser/api/electron_api_browser_window.cc b/shell/browser/api/electron_api_browser_window.cc index 106af4a6acff7..acbbd88cb4684 100644 --- a/shell/browser/api/electron_api_browser_window.cc +++ b/shell/browser/api/electron_api_browser_window.cc @@ -77,6 +77,7 @@ BrowserWindow::BrowserWindow(gin::Arguments* args, WebContentsView::Create(isolate, web_preferences); DCHECK(web_contents_view.get()); window_->AddDraggableRegionProvider(web_contents_view.get()); + web_contents_view_.Reset(isolate, web_contents_view.ToV8()); // Save a reference of the WebContents. gin::Handle web_contents = @@ -92,7 +93,14 @@ BrowserWindow::BrowserWindow(gin::Arguments* args, InitWithArgs(args); // Install the content view after BaseWindow's JS code is initialized. - SetContentView(gin::CreateHandle(isolate, web_contents_view.get())); + // The WebContentsView is added a sibling of BaseWindow's contentView (before + // it in the paint order) so that any views added to BrowserWindow's + // contentView will be painted on top of the BrowserWindow's WebContentsView. + // See https://github.com/electron/electron/pull/41256. + // Note that |GetContentsView|, confusingly, does not refer to the same thing + // as |BaseWindow::GetContentView|. + window()->GetContentsView()->AddChildViewAt(web_contents_view->view(), 0); + window()->GetContentsView()->DeprecatedLayoutImmediately(); // Init window after everything has been setup. window()->InitFromOptions(options); diff --git a/shell/browser/api/electron_api_browser_window.h b/shell/browser/api/electron_api_browser_window.h index e5d580138f56f..df9f9754ce0fd 100644 --- a/shell/browser/api/electron_api_browser_window.h +++ b/shell/browser/api/electron_api_browser_window.h @@ -95,6 +95,7 @@ class BrowserWindow : public BaseWindow, base::CancelableRepeatingClosure window_unresponsive_closure_; v8::Global web_contents_; + v8::Global web_contents_view_; base::WeakPtr api_web_contents_; base::WeakPtrFactory weak_factory_{this}; diff --git a/shell/browser/api/frame_subscriber.cc b/shell/browser/api/frame_subscriber.cc index 358ce6e116466..cdb65c76677f4 100644 --- a/shell/browser/api/frame_subscriber.cc +++ b/shell/browser/api/frame_subscriber.cc @@ -42,6 +42,7 @@ void FrameSubscriber::AttachToHost(content::RenderWidgetHost* host) { // Create and configure the video capturer. gfx::Size size = GetRenderViewSize(); + DCHECK(!size.IsEmpty()); video_capturer_ = host_->GetView()->CreateVideoCapturer(); video_capturer_->SetResolutionConstraints(size, size, true); video_capturer_->SetAutoThrottlingEnabled(false); diff --git a/shell/browser/native_window_views.cc b/shell/browser/native_window_views.cc index cd90e0ab997e6..de3c0f2bb08a8 100644 --- a/shell/browser/native_window_views.cc +++ b/shell/browser/native_window_views.cc @@ -469,12 +469,12 @@ void NativeWindowViews::SetGTKDarkThemeEnabled(bool use_dark_theme) { void NativeWindowViews::SetContentView(views::View* view) { if (content_view()) { - root_view_.RemoveChildView(content_view()); + root_view_.GetMainView()->RemoveChildView(content_view()); } set_content_view(view); focused_view_ = view; - root_view_.AddChildView(content_view()); - root_view_.DeprecatedLayoutImmediately(); + root_view_.GetMainView()->AddChildView(content_view()); + root_view_.GetMainView()->DeprecatedLayoutImmediately(); } void NativeWindowViews::Close() { @@ -1664,7 +1664,7 @@ std::u16string NativeWindowViews::GetWindowTitle() const { } views::View* NativeWindowViews::GetContentsView() { - return &root_view_; + return root_view_.GetMainView(); } bool NativeWindowViews::ShouldDescendIntoChildForEventHandling( @@ -1674,7 +1674,7 @@ bool NativeWindowViews::ShouldDescendIntoChildForEventHandling( } views::ClientView* NativeWindowViews::CreateClientView(views::Widget* widget) { - return new NativeWindowClientView{widget, GetContentsView(), this}; + return new NativeWindowClientView{widget, &root_view_, this}; } std::unique_ptr diff --git a/shell/browser/ui/views/root_view.cc b/shell/browser/ui/views/root_view.cc index 273f96f48ba44..b0f59e994b990 100644 --- a/shell/browser/ui/views/root_view.cc +++ b/shell/browser/ui/views/root_view.cc @@ -9,18 +9,12 @@ #include "content/public/common/input/native_web_keyboard_event.h" #include "shell/browser/native_window.h" #include "shell/browser/ui/views/menu_bar.h" +#include "ui/views/layout/box_layout.h" namespace electron { namespace { -// The menu bar height in pixels. -#if BUILDFLAG(IS_WIN) -const int kMenuBarHeight = 20; -#else -const int kMenuBarHeight = 25; -#endif - bool IsAltKey(const content::NativeWebKeyboardEvent& event) { return event.windows_key_code == ui::VKEY_MENU; } @@ -41,6 +35,12 @@ RootView::RootView(NativeWindow* window) : window_(window), last_focused_view_tracker_(std::make_unique()) { set_owned_by_client(); + views::BoxLayout* layout = + SetLayoutManager(std::make_unique( + views::BoxLayout::Orientation::kVertical)); + main_view_ = AddChildView(std::make_unique()); + main_view_->SetUseDefaultFillLayout(true); + layout->SetFlexForView(main_view_, 1); } RootView::~RootView() = default; @@ -77,7 +77,7 @@ bool RootView::HasMenu() const { } int RootView::GetMenuBarHeight() const { - return kMenuBarHeight; + return menu_bar_ ? menu_bar_->GetPreferredSize().height() : 0; } void RootView::SetAutoHideMenuBar(bool auto_hide) { @@ -90,10 +90,8 @@ void RootView::SetMenuBarVisibility(bool visible) { menu_bar_visible_ = visible; if (visible) { - DCHECK_EQ(children().size(), 1ul); - AddChildView(menu_bar_.get()); + AddChildViewAt(menu_bar_.get(), 0); } else { - DCHECK_EQ(children().size(), 2ul); RemoveChildView(menu_bar_.get()); } @@ -166,21 +164,6 @@ void RootView::ResetAltState() { menu_bar_alt_pressed_ = false; } -void RootView::Layout(PassKey) { - if (!window_->content_view()) // Not ready yet. - return; - - const auto menu_bar_bounds = - menu_bar_visible_ ? gfx::Rect(0, 0, size().width(), kMenuBarHeight) - : gfx::Rect(); - if (menu_bar_) - menu_bar_->SetBoundsRect(menu_bar_bounds); - - window_->content_view()->SetBoundsRect( - gfx::Rect(0, menu_bar_visible_ ? menu_bar_bounds.bottom() : 0, - size().width(), size().height() - menu_bar_bounds.height())); -} - gfx::Size RootView::GetMinimumSize() const { return window_->GetMinimumSize(); } diff --git a/shell/browser/ui/views/root_view.h b/shell/browser/ui/views/root_view.h index 7aa52a22c1a81..5f27e23c60723 100644 --- a/shell/browser/ui/views/root_view.h +++ b/shell/browser/ui/views/root_view.h @@ -46,8 +46,9 @@ class RootView : public views::View { void RegisterAcceleratorsWithFocusManager(ElectronMenuModel* menu_model); void UnregisterAcceleratorsWithFocusManager(); + views::View* GetMainView() { return main_view_; } + // views::View: - void Layout(PassKey) override; gfx::Size GetMinimumSize() const override; gfx::Size GetMaximumSize() const override; bool AcceleratorPressed(const ui::Accelerator& accelerator) override; @@ -62,6 +63,9 @@ class RootView : public views::View { bool menu_bar_visible_ = false; bool menu_bar_alt_pressed_ = false; + // Main view area. + raw_ptr main_view_; + // Map from accelerator to menu item's command id. accelerator_util::AcceleratorTable accelerator_table_; diff --git a/spec/api-browser-window-spec.ts b/spec/api-browser-window-spec.ts index 1ed5d2f4807ae..64c794163f94d 100644 --- a/spec/api-browser-window-spec.ts +++ b/spec/api-browser-window-spec.ts @@ -31,7 +31,7 @@ const isScaleFactorRounding = () => { const expectBoundsEqual = (actual: any, expected: any) => { if (!isScaleFactorRounding()) { - expect(expected).to.deep.equal(actual); + expect(actual).to.deep.equal(expected); } else if (Array.isArray(actual)) { expect(actual[0]).to.be.closeTo(expected[0], 1); expect(actual[1]).to.be.closeTo(expected[1], 1);