From 521835d70ba3b2237f003946178ea522eb7f4eb0 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Wed, 11 Dec 2024 13:24:32 -0500 Subject: [PATCH] fix: context-menu emitted twice (#44997) * fix: context-menu emitted twice Co-authored-by: Samuel Maddock * refactor: simplify disabling draggable regions Co-authored-by: Samuel Maddock * cleanup Co-authored-by: Samuel Maddock --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Samuel Maddock --- .../browser/api/electron_api_web_contents.cc | 12 +++++ shell/browser/api/electron_api_web_contents.h | 14 +++--- .../electron_inspectable_web_contents_view.h | 2 - .../electron_inspectable_web_contents_view.mm | 23 ---------- shell/browser/ui/cocoa/electron_ns_window.mm | 45 ++++++------------- spec/api-web-contents-spec.ts | 20 +++++++++ 6 files changed, 51 insertions(+), 65 deletions(-) diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index f545325187d4..abd9ec4f1232 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -371,6 +371,9 @@ namespace electron::api { namespace { +// Global toggle for disabling draggable regions checks. +bool g_disable_draggable_regions = false; + constexpr std::string_view CursorTypeToString( ui::mojom::CursorType cursor_type) { switch (cursor_type) { @@ -2049,6 +2052,10 @@ void WebContents::DraggableRegionsChanged( draggable_region_ = DraggableRegionsToSkRegion(regions); } +SkRegion* WebContents::draggable_region() { + return g_disable_draggable_regions ? nullptr : draggable_region_.get(); +} + void WebContents::DidStartNavigation( content::NavigationHandle* navigation_handle) { EmitNavigationEvent("did-start-navigation", navigation_handle); @@ -4592,6 +4599,11 @@ std::list WebContents::GetWebContentsList() { return list; } +// static +void WebContents::SetDisableDraggableRegions(bool disable) { + g_disable_draggable_regions = disable; +} + // static gin::WrapperInfo WebContents::kWrapperInfo = {gin::kEmbedderNativeGin}; diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h index 44252d7ba56c..ac91f0db0cbf 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -151,6 +151,10 @@ class WebContents final : public ExclusiveAccessContext, static WebContents* FromID(int32_t id); static std::list GetWebContentsList(); + // Whether to disable draggable regions globally. This can be used to allow + // events to skip client region hit tests. + static void SetDisableDraggableRegions(bool disable); + // Get the V8 wrapper of the |web_contents|, or create one if not existed. // // The lifetime of |web_contents| is NOT managed by this class, and the type @@ -485,13 +489,7 @@ class WebContents final : public ExclusiveAccessContext, void PDFReadyToPrint(); - SkRegion* draggable_region() { - return force_non_draggable_ ? nullptr : draggable_region_.get(); - } - - void SetForceNonDraggable(bool force_non_draggable) { - force_non_draggable_ = force_non_draggable; - } + SkRegion* draggable_region(); // disable copy WebContents(const WebContents&) = delete; @@ -888,8 +886,6 @@ class WebContents final : public ExclusiveAccessContext, std::unique_ptr draggable_region_; - bool force_non_draggable_ = false; - base::WeakPtrFactory weak_factory_{this}; }; diff --git a/shell/browser/ui/cocoa/electron_inspectable_web_contents_view.h b/shell/browser/ui/cocoa/electron_inspectable_web_contents_view.h index 677e76db2a92..4286312573aa 100644 --- a/shell/browser/ui/cocoa/electron_inspectable_web_contents_view.h +++ b/shell/browser/ui/cocoa/electron_inspectable_web_contents_view.h @@ -49,8 +49,6 @@ using electron::InspectableWebContentsViewMac; - (void)setTitle:(NSString*)title; - (NSString*)getTitle; -- (void)redispatchContextMenuEvent:(base::apple::OwnedNSEvent)theEvent; - @end #endif // ELECTRON_SHELL_BROWSER_UI_COCOA_ELECTRON_INSPECTABLE_WEB_CONTENTS_VIEW_H_ diff --git a/shell/browser/ui/cocoa/electron_inspectable_web_contents_view.mm b/shell/browser/ui/cocoa/electron_inspectable_web_contents_view.mm index aec0537f28c5..d719fc164e47 100644 --- a/shell/browser/ui/cocoa/electron_inspectable_web_contents_view.mm +++ b/shell/browser/ui/cocoa/electron_inspectable_web_contents_view.mm @@ -298,29 +298,6 @@ [self notifyDevToolsFocused]; } -- (void)redispatchContextMenuEvent:(base::apple::OwnedNSEvent)event { - DCHECK(event.Get().type == NSEventTypeRightMouseDown || - (event.Get().type == NSEventTypeLeftMouseDown && - (event.Get().modifierFlags & NSEventModifierFlagControl))); - content::WebContents* contents = - inspectableWebContentsView_->inspectable_web_contents()->GetWebContents(); - electron::api::WebContents* api_contents = - electron::api::WebContents::From(contents); - if (api_contents) { - // Temporarily pretend that the WebContents is fully non-draggable while we - // re-send the mouse event. This allows the re-dispatched event to "land" - // on the WebContents, instead of "falling through" back to the window. - auto* rwhv = contents->GetRenderWidgetHostView(); - if (rwhv) { - api_contents->SetForceNonDraggable(true); - BaseView* contentsView = - (BaseView*)rwhv->GetNativeView().GetNativeNSView(); - [contentsView mouseEvent:event.Get()]; - api_contents->SetForceNonDraggable(false); - } - } -} - #pragma mark - NSWindowDelegate - (void)windowWillClose:(NSNotification*)notification { diff --git a/shell/browser/ui/cocoa/electron_ns_window.mm b/shell/browser/ui/cocoa/electron_ns_window.mm index 8da35c0bb75f..5c5f1512f4a3 100644 --- a/shell/browser/ui/cocoa/electron_ns_window.mm +++ b/shell/browser/ui/cocoa/electron_ns_window.mm @@ -6,6 +6,7 @@ #include "base/strings/sys_string_conversions.h" #include "electron/mas.h" +#include "shell/browser/api/electron_api_web_contents.h" #include "shell/browser/native_window_mac.h" #include "shell/browser/ui/cocoa/delayed_native_view_host.h" #include "shell/browser/ui/cocoa/electron_inspectable_web_contents_view.h" @@ -193,41 +194,23 @@ void SwizzleSwipeWithEvent(NSView* view, SEL swiz_selector) { - (void)sendEvent:(NSEvent*)event { // Draggable regions only respond to left-click dragging, but the system will - // still suppress right-clicks in a draggable region. Forwarding right-clicks - // and ctrl+left-clicks allows the underlying views to respond to right-click - // to potentially bring up a frame context menu. WebContentsView is now a - // sibling view of the NSWindow contentView, so we need to intercept the event - // here as NativeWidgetMacNSWindow won't forward it to the WebContentsView - // anymore. - if (event.type == NSEventTypeRightMouseDown || - (event.type == NSEventTypeLeftMouseDown && - ([event modifierFlags] & NSEventModifierFlagControl))) { - // We're looking for the NativeViewHost that contains the WebContentsView. - // There can be two possible NativeViewHosts - one containing the - // WebContentsView (present for BrowserWindows) and the one containing the - // VibrantView (present when vibrancy is set). We want the one containing - // the WebContentsView if it exists. - const auto& children = shell_->GetContentsView()->children(); - const auto it = std::ranges::find_if(children, [&](views::View* child) { - if (std::strcmp(child->GetClassName(), "NativeViewHost") == 0) { - auto* nvh = static_cast(child); - return nvh->native_view().GetNativeNSView() != [self vibrantView]; - } - return false; - }); + // still suppress right-clicks in a draggable region. Temporarily disabling + // draggable regions allows the underlying views to respond to right-click + // to potentially bring up a frame context menu. + BOOL shouldDisableDraggable = + (event.type == NSEventTypeRightMouseDown || + (event.type == NSEventTypeLeftMouseDown && + ([event modifierFlags] & NSEventModifierFlagControl))); - if (it != children.end()) { - auto ns_view = static_cast(*it) - ->native_view() - .GetNativeNSView(); - if (ns_view) { - [static_cast(ns_view) - redispatchContextMenuEvent:base::apple::OwnedNSEvent(event)]; - } - } + if (shouldDisableDraggable) { + electron::api::WebContents::SetDisableDraggableRegions(true); } [super sendEvent:event]; + + if (shouldDisableDraggable) { + electron::api::WebContents::SetDisableDraggableRegions(false); + } } - (void)rotateWithEvent:(NSEvent*)event { diff --git a/spec/api-web-contents-spec.ts b/spec/api-web-contents-spec.ts index cd3bea682c73..5b097b9717a4 100644 --- a/spec/api-web-contents-spec.ts +++ b/spec/api-web-contents-spec.ts @@ -2731,6 +2731,26 @@ describe('webContents module', () => { expect(params.y).to.be.a('number'); }); + // Skipping due to lack of native click support. + it.skip('emits the correct number of times when right-clicked in page', async () => { + const w = new BrowserWindow({ show: true }); + await w.loadFile(path.join(fixturesPath, 'pages', 'base-page.html')); + + let contextMenuEmitCount = 0; + + w.webContents.on('context-menu', () => { + contextMenuEmitCount++; + }); + + // TODO(samuelmaddock): Perform native right-click. We've tried then + // dropped robotjs and nutjs so for now this is a manual test. + + await once(w.webContents, 'context-menu'); + await setTimeout(100); + + expect(contextMenuEmitCount).to.equal(1); + }); + it('emits when right-clicked in page in a draggable region', async () => { const w = new BrowserWindow({ show: false }); await w.loadFile(path.join(fixturesPath, 'pages', 'draggable-page.html'));