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

[Bug]: menu-will-show, menu-will-close and the click handler are never triggered for top-level menus #31915

Closed
3 tasks done
fabiospampinato opened this issue Nov 19, 2021 · 16 comments

Comments

@fabiospampinato
Copy link
Contributor

fabiospampinato commented Nov 19, 2021

Preflight Checklist

Electron Version

13.1.7

What operating system are you using?

macOS

Operating System Version

Monterey

What arch are you using?

arm64 (including Apple Silicon)

Last Known Working Electron version

No response

Expected Behavior

  • I'd expect the menu-will-show and menu-will-close events to work for app menus too.
  • I'd expect the click function to be called when root-level menu items in app menus are clicked too.

Actual Behavior

None of that is happening.

Testcase Gist URL

No response

Additional Information

The Fiddle just updated itself and the uploader thing is spinning endlessly, so here are the files to reproduce the issue:

main.js (the only file I changed):

// Modules to control application life and create native browser window
const {app, BrowserWindow, Menu} = require('electron')
const path = require('path')

function createWindow () {
  // Create the browser window.
  const mainWindow = new BrowserWindow({
    width: 800,
    height: 600,
    webPreferences: {
      preload: path.join(__dirname, 'preload.js')
    }
  })

  // and load the index.html of the app.
  mainWindow.loadFile('index.html')

  // Open the DevTools.
  // mainWindow.webContents.openDevTools()
}

// This method will be called when Electron has finished
// initialization and is ready to create browser windows.
// Some APIs can only be used after this event occurs.
app.whenReady().then(() => {
  createWindow()
  
  app.on('activate', function () {
    // On macOS it's common to re-create a window in the app when the
    // dock icon is clicked and there are no other windows open.
    if (BrowserWindow.getAllWindows().length === 0) createWindow()
  })

  const menu = Menu.buildFromTemplate ([
    {
      label: 'Electron'
    },
    {
      label: 'Root',
      click: () => {
        console.log ( 'ROOT CLICK' );
      },
      submenu: [
        {
          label: 'Nested',
          click: () => {
            console.log ( 'NESTED CLICK' );
          },  
        }
      ]
    }
  ])

  menu.on ( 'menu-will-close', () => {
    console.log ( 'WILL close' );
  })

  menu.on ( 'menu-will-show', () => {
    console.log ( 'WILL SHOW' );
  })

  Menu.setApplicationMenu ( menu );

})

// Quit when all windows are closed, except on macOS. There, it's common
// for applications and their menu bar to stay active until the user quits
// explicitly with Cmd + Q.
app.on('window-all-closed', function () {
  if (process.platform !== 'darwin') app.quit()
})

// In this file you can include the rest of your app's specific main process
// code. You can also put them in separate files and require them here.

index.html:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <!-- https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP -->
    <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'">
    <meta http-equiv="X-Content-Security-Policy" content="default-src 'self'; script-src 'self'">
    <title>Hello World!</title>
  </head>
  <body>
    <h1>Hello World!</h1>
    We are using Node.js <span id="node-version"></span>,
    Chromium <span id="chrome-version"></span>,
    and Electron <span id="electron-version"></span>.

    <!-- You can also require other files to run in this process -->
    <script src="./renderer.js"></script>
  </body>
</html>

renderer.js

// This file is required by the index.html file and will
// be executed in the renderer process for that window.
// No Node.js APIs are available in this process because
// `nodeIntegration` is turned off. Use `preload.js` to
// selectively enable features needed in the rendering
// process.

preload.js

// All of the Node.js APIs are available in the preload process.
// It has the same sandbox as a Chrome extension.
window.addEventListener('DOMContentLoaded', () => {
  const replaceText = (selector, text) => {
    const element = document.getElementById(selector)
    if (element) element.innerText = text
  }

  for (const type of ['chrome', 'node', 'electron']) {
    replaceText(`${type}-version`, process.versions[type])
  }
})
@fabiospampinato
Copy link
Contributor Author

fabiospampinato commented Nov 19, 2021

Maybe I should add that I stumbled on these issues while trying to update my app's menu right before the user needs with, as otherwise I'm just throwing some performance out of the window (#24724).

Is there any way to do that? I'm thinking of updating the menu when the cursor moves over the global menubar using an hard-coded height value for the menubar, and I'd really rather do something less hacky. (besides that won't work well if the menu is opened with a shortcut)

@robertpatrick
Copy link

I am seeing the same behavior in Electron 16.0.5...

@robertpatrick
Copy link

robertpatrick commented Jan 16, 2022

In our case, we need to force a blur event to occur so that the data in our embedded ACE editor is synced with the underlying "model" (in a knockout MMVV context). This creates a significant usability issue when the user is editing text and goes to the menu to invoke a menu item (e.g., Save, Validate, Prepare, and a dozen other possible actions) that grabs the text out of the underlying "model".

We could theoretically add MacOS-specific code (since this is working properly on Windows and Linux) to every single action that depends on the editor contents to force a blur event but that gets really ugly, really fast. Please fix this bug.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@github-actions github-actions bot added the stale label Oct 5, 2022
@rpatrick00
Copy link

Ping

@github-actions github-actions bot removed the stale label Oct 6, 2022
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@github-actions github-actions bot added the stale label Jan 10, 2023
@robertpatrick
Copy link

Bump

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@github-actions github-actions bot added the stale label Apr 22, 2023
@fabiospampinato
Copy link
Contributor Author

Bump.

@github-actions github-actions bot removed the stale label Apr 23, 2023
@gknapp
Copy link

gknapp commented Jun 8, 2023

Not seeing these events triggered in Electron 25 either, specifically I'd like to update a BrowserView's bounds (notably, the Y offset) when the user presses Alt to toggle to application menu bar visibility (Windows).

@electron-issue-triage
Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@robertpatrick
Copy link

Still broken

@electron-issue-triage
Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@robertpatrick
Copy link

Still busted and no one cares

@electron-issue-triage
Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@electron-issue-triage
Copy link

This issue has been closed due to inactivity, and will not be monitored. If this is a bug and you can reproduce this issue on a supported version of Electron please open a new issue and include instructions for reproducing the issue.

@electron-issue-triage electron-issue-triage bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants