fix: context-menu emitted twice (#44997)

* fix: context-menu emitted twice

Co-authored-by: Samuel Maddock <smaddock@slack-corp.com>

* refactor: simplify disabling draggable regions

Co-authored-by: Samuel Maddock <smaddock@slack-corp.com>

* cleanup

Co-authored-by: Samuel Maddock <smaddock@slack-corp.com>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Samuel Maddock <smaddock@slack-corp.com>

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*> WebContents::GetWebContentsList() {
   return list;
 }
 
+// static
+void WebContents::SetDisableDraggableRegions(bool disable) {
+  g_disable_draggable_regions = disable;
+}
+
 // static
 gin::WrapperInfo WebContents::kWrapperInfo = {gin::kEmbedderNativeGin};
 

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<WebContents*> 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() {
+  SkRegion* draggable_region();
-    return force_non_draggable_ ? nullptr : draggable_region_.get();
-  }
-
-  void SetForceNonDraggable(bool force_non_draggable) {
-    force_non_draggable_ = force_non_draggable;
-  }
 
   // disable copy
   WebContents(const WebContents&) = delete;
@@ -888,8 +886,6 @@ class WebContents final : public ExclusiveAccessContext,
 
   std::unique_ptr<SkRegion> draggable_region_;
 
-  bool force_non_draggable_ = false;
-
   base::WeakPtrFactory<WebContents> weak_factory_{this};
 };
 

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_

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 {

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
+  // still suppress right-clicks in a draggable region. Temporarily disabling
-  // and ctrl+left-clicks allows the underlying views to respond to right-click
+  // draggable regions allows the underlying views to respond to right-click
-  // to potentially bring up a frame context menu. WebContentsView is now a
+  // to potentially bring up a frame context menu.
-  // sibling view of the NSWindow contentView, so we need to intercept the event
+  BOOL shouldDisableDraggable =
-  // here as NativeWidgetMacNSWindow won't forward it to the WebContentsView
+      (event.type == NSEventTypeRightMouseDown ||
-  // anymore.
+       (event.type == NSEventTypeLeftMouseDown &&
-  if (event.type == NSEventTypeRightMouseDown ||
+        ([event modifierFlags] & NSEventModifierFlagControl)));
-      (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<views::NativeViewHost*>(child);
-        return nvh->native_view().GetNativeNSView() != [self vibrantView];
-      }
-      return false;
-    });
 
-    if (it != children.end()) {
+  if (shouldDisableDraggable) {
-      auto ns_view = static_cast<electron::DelayedNativeViewHost*>(*it)
+    electron::api::WebContents::SetDisableDraggableRegions(true);
-                         ->native_view()
-                         .GetNativeNSView();
-      if (ns_view) {
-        [static_cast<ElectronInspectableWebContentsView*>(ns_view)
-            redispatchContextMenuEvent:base::apple::OwnedNSEvent(event)];
-      }
-    }
   }
 
   [super sendEvent:event];
+
+  if (shouldDisableDraggable) {
+    electron::api::WebContents::SetDisableDraggableRegions(false);
+  }
 }
 
 - (void)rotateWithEvent:(NSEvent*)event {

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'));