Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Msg view: Add context menu #192

Closed
benbucksch opened this issue Sep 2, 2024 · 8 comments
Closed

Msg view: Add context menu #192

benbucksch opened this issue Sep 2, 2024 · 8 comments
Assignees
Labels

Comments

@benbucksch
Copy link
Collaborator

Reproduction:

  • Open an HTML email or even plaintext email
  • Right-click on a link
  • Right-click on an inline image
  • Select some text
  • Right-click on the selected text

Actual result:

  • No context menu

Expected result:

  • A context menu, suited for what you clicked on. Roughly similar to the web browser context-menu.
@benbucksch
Copy link
Collaborator Author

I've tried to use electron-context-menu, but it doesn't build.

@jermy-c
Copy link
Collaborator

jermy-c commented Sep 3, 2024

I tried to use electron-context-menu also for #110 and got the same error.

@jermy-c
Copy link
Collaborator

jermy-c commented Sep 4, 2024

[email protected] with [email protected] supports ESM output and removes the error but gives the error:

App threw an error during load
file:///Users/jeremyc/Documents/GitHub/mustang/e2/out/main/index.js:35
import { autoUpdater } from "electron-updater";
         ^^^^^^^^^^^
SyntaxError: Named export 'autoUpdater' not found. The requested module 'electron-updater' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'electron-updater';
const { autoUpdater } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:134:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:217:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async loadApplicationPackage (file:///Users/jeremyc/Documents/GitHub/mustang/e2/node_modules/electron/dist/Electron.app/Contents/Resources/default_app.asar/main.js:129:9)
    at async file:///Users/jeremyc/Documents/GitHub/mustang/e2/node_modules/electron/dist/Electron.app/Contents/Resources/default_app.asar/main.js:241:9

After doing that I get the error:

App threw an error during load
ReferenceError: __dirname is not defined in ES module scope
This file is being treated as an ES module because it has a '.js' file extension and '/Users/jeremyc/Documents/GitHub/mustang/e2/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.
    at file:///Users/jeremyc/Documents/GitHub/mustang/e2/out/main/index.js:126492:21
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async loadApplicationPackage (file:///Users/jeremyc/Documents/GitHub/mustang/e2/node_modules/electron/dist/Electron.app/Contents/Resources/default_app.asar/main.js:129:9)
    at async file:///Users/jeremyc/Documents/GitHub/mustang/e2/node_modules/electron/dist/Electron.app/Contents/Resources/default_app.asar/main.js:241:9

It's converting to ESM but the filepaths are being converted from ESM to commonJS e.g.const icon = join$2(__dirname, "../../resources/icon.png");.

Using [email protected] works because that's the last commonJS version. And any version after that requires Node.js 18 and Electron 30 because only version later than those support ESM.

The context menu applies to all windows, we would need to do some communications between the main and renderer processes to let it know which window to apply it.

@benbucksch
Copy link
Collaborator Author

benbucksch commented Sep 4, 2024 via email

@jermy-c
Copy link
Collaborator

jermy-c commented Sep 5, 2024

#193 Updates the packages to output ESM and fixes any errors cause by it.

@jermy-c
Copy link
Collaborator

jermy-c commented Sep 9, 2024

contextMenu() does not add context menus to <webview> tags for some reason but only the main window has the context menu.

When not specified, the context menu will be added to all existing and new windows.

@benbucksch
Copy link
Collaborator Author

Yes, I noticed the same. I tried to pass a webview DOM element, but that didn't work. I tried to re-use the electron-context-menu code, by using a lower level function, but it doesn't export any of the lower level functions.

However, I did manage to attach a webviewE.addEventListener("context-menu", ...) event listener, and the event.params contains the interesting info as documented. We can work off that. Note: This is in the frontend code, in our <WebView> svelte component.

So, I think it makes sense to build the context menu ourselves, with our own menu. The menu can then be built using different UI components. The trigger would be the context-menu event above. So, essentially, we only need the parts of the function that take context info, and determine the menu items to show.

It appears the best approach is to fork the module, make the code more flexible. Only a function that accepts the event.params data, and a config, and then returns a list of menu items and functions to call. Ideally, the functions (what happens when you click on a menu item) would be not be inline functions, but separate exported functions that I can override, call, or augment. By default, the menu items should call these functions, but I should be able to change that.

@benbucksch benbucksch assigned benbucksch and unassigned jermy-c Sep 9, 2024
@benbucksch
Copy link
Collaborator Author

DUP of #110

@benbucksch benbucksch closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2024
@github-staff github-staff deleted a comment from maher-nakesh Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants