From 42ce23b4a4e188b1d8712cbbd39ed381a4ee80d9 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 23 Aug 2023 15:55:31 +0200 Subject: [PATCH] fix: ensure `BrowserView` bounds are always relative to window (#39605) fix: ensure BrowserView bounds are always relative to window --- shell/browser/api/electron_api_base_window.cc | 10 +++++ shell/browser/api/electron_api_browser_view.h | 5 ++- shell/browser/native_browser_view_mac.mm | 22 ++++----- spec/api-browser-view-spec.ts | 45 +++++++++++++++++++ 4 files changed, 70 insertions(+), 12 deletions(-) diff --git a/shell/browser/api/electron_api_base_window.cc b/shell/browser/api/electron_api_base_window.cc index 22dbf3825ac39..8bccf457a06e2 100644 --- a/shell/browser/api/electron_api_base_window.cc +++ b/shell/browser/api/electron_api_base_window.cc @@ -768,10 +768,20 @@ void BaseWindow::AddBrowserView(gin::Handle browser_view) { browser_view->SetOwnerWindow(nullptr); } + // If the user set the BrowserView's bounds before adding it to the window, + // we need to get those initial bounds *before* adding it to the window + // so bounds isn't returned relative despite not being correctly positioned + // relative to the window. + auto bounds = browser_view->GetBounds(); + window_->AddBrowserView(browser_view->view()); window_->AddDraggableRegionProvider(browser_view.get()); browser_view->SetOwnerWindow(this); browser_views_.emplace_back().Reset(isolate(), browser_view.ToV8()); + + // Recalibrate bounds relative to the containing window. + if (!bounds.IsEmpty()) + browser_view->SetBounds(bounds); } } diff --git a/shell/browser/api/electron_api_browser_view.h b/shell/browser/api/electron_api_browser_view.h index 6cf9ee20fdc24..3025022830950 100644 --- a/shell/browser/api/electron_api_browser_view.h +++ b/shell/browser/api/electron_api_browser_view.h @@ -66,6 +66,9 @@ class BrowserView : public gin::Wrappable, BrowserView(const BrowserView&) = delete; BrowserView& operator=(const BrowserView&) = delete; + gfx::Rect GetBounds(); + void SetBounds(const gfx::Rect& bounds); + protected: BrowserView(gin::Arguments* args, const gin_helper::Dictionary& options); ~BrowserView() override; @@ -78,8 +81,6 @@ class BrowserView : public gin::Wrappable, private: void SetAutoResize(AutoResizeFlags flags); - void SetBounds(const gfx::Rect& bounds); - gfx::Rect GetBounds(); void SetBackgroundColor(const std::string& color_name); v8::Local GetWebContents(v8::Isolate*); diff --git a/shell/browser/native_browser_view_mac.mm b/shell/browser/native_browser_view_mac.mm index 14cc582d90db9..cf63032585fa7 100644 --- a/shell/browser/native_browser_view_mac.mm +++ b/shell/browser/native_browser_view_mac.mm @@ -55,25 +55,27 @@ auto* iwc_view = GetInspectableWebContentsView(); if (!iwc_view) return; + auto* view = iwc_view->GetNativeView().GetNativeNSView(); - auto* superview = view.superview; - const auto superview_height = superview ? superview.frame.size.height : 0; - view.frame = - NSMakeRect(bounds.x(), superview_height - bounds.y() - bounds.height(), - bounds.width(), bounds.height()); + const auto superview_height = + view.superview ? view.superview.frame.size.height : 0; + int y_coord = superview_height - bounds.y() - bounds.height(); + + view.frame = NSMakeRect(bounds.x(), y_coord, bounds.width(), bounds.height()); } gfx::Rect NativeBrowserViewMac::GetBounds() { auto* iwc_view = GetInspectableWebContentsView(); if (!iwc_view) return gfx::Rect(); + NSView* view = iwc_view->GetNativeView().GetNativeNSView(); const int superview_height = - (view.superview) ? view.superview.frame.size.height : 0; - return gfx::Rect( - view.frame.origin.x, - superview_height - view.frame.origin.y - view.frame.size.height, - view.frame.size.width, view.frame.size.height); + view.superview ? view.superview.frame.size.height : 0; + int y_coord = superview_height - view.frame.origin.y - view.frame.size.height; + + return gfx::Rect(view.frame.origin.x, y_coord, view.frame.size.width, + view.frame.size.height); } void NativeBrowserViewMac::SetBackgroundColor(SkColor color) { diff --git a/spec/api-browser-view-spec.ts b/spec/api-browser-view-spec.ts index a45790644b53f..ec4d5a38aa44f 100644 --- a/spec/api-browser-view-spec.ts +++ b/spec/api-browser-view-spec.ts @@ -147,6 +147,41 @@ describe('BrowserView module', () => { view.setBounds({} as any); }).to.throw(/conversion failure/); }); + + it('can set bounds after view is added to window', () => { + view = new BrowserView(); + + const bounds = { x: 0, y: 0, width: 50, height: 50 }; + + w.addBrowserView(view); + view.setBounds(bounds); + + expect(view.getBounds()).to.deep.equal(bounds); + }); + + it('can set bounds before view is added to window', () => { + view = new BrowserView(); + + const bounds = { x: 0, y: 0, width: 50, height: 50 }; + + view.setBounds(bounds); + w.addBrowserView(view); + + expect(view.getBounds()).to.deep.equal(bounds); + }); + + it('can update bounds', () => { + view = new BrowserView(); + w.addBrowserView(view); + + const bounds1 = { x: 0, y: 0, width: 50, height: 50 }; + view.setBounds(bounds1); + expect(view.getBounds()).to.deep.equal(bounds1); + + const bounds2 = { x: 0, y: 150, width: 50, height: 50 }; + view.setBounds(bounds2); + expect(view.getBounds()).to.deep.equal(bounds2); + }); }); describe('BrowserView.getBounds()', () => { @@ -156,6 +191,16 @@ describe('BrowserView module', () => { view.setBounds(bounds); expect(view.getBounds()).to.deep.equal(bounds); }); + + it('does not changer after being added to a window', () => { + view = new BrowserView(); + const bounds = { x: 10, y: 20, width: 30, height: 40 }; + view.setBounds(bounds); + expect(view.getBounds()).to.deep.equal(bounds); + + w.addBrowserView(view); + expect(view.getBounds()).to.deep.equal(bounds); + }); }); describe('BrowserWindow.setBrowserView()', () => {