feat: enable navigation between submenus via arrow keys

[mlaurencin]

Description of Change

Closes #52055

Navigation can now occur between menu bar headers using the left and right arrows keys. Windows conventions were followed when determining situations when such navigation should be disallowed (e.g. navigating to the left when the currently selected item is a submenu).

Checklist

• I have built and tested this change
• I have filled out the PR description
• I have reviewed and verified the changes
• npm test passes
• tests are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Added functionality to allow navigation between menu bar headers via left and right arrow keys

[jkleinsc]

API LGTM

[jkleinsc]

Overall looks good, just some minor nits.

Also, I do think that this patch probably wouldn't be accepted upstream since Chromium doesn't use top level menus on Windows.

[jkleinsc]

This is never used

[jkleinsc]

The lone caller to this method doesn't do anything with the modified ElectronMenuModel, so is this parameter really necessary?

[jkleinsc]

menu_bar_->GetSiblingMenuButtonByDirection already does this check

[jkleinsc]

using setTimeout(s) in tests is a code smell for flaky tests because depending on CI sometimes the timeout isn't enough. If possible we should use the waitUntil helper to wait until a certain condition is satisfied.

[jkleinsc]

eg

diff --git a/spec/api-menu-keyboard-spec.ts b/spec/api-menu-keyboard-spec.ts
index 7fde930474..b727f4c0e9 100644
--- a/spec/api-menu-keyboard-spec.ts
+++ b/spec/api-menu-keyboard-spec.ts
@@ -5,7 +5,7 @@ import { expect } from 'chai';
 import { setTimeout } from 'node:timers/promises';
 
 import { hasCapturableScreen } from './lib/screen-helpers';
-import { ifdescribe } from './lib/spec-helpers';
+import { ifdescribe, waitUntil } from './lib/spec-helpers';
 import { closeAllWindows } from './lib/window-helpers';
 
 // These tests are skipped when robotjs or a capturable screen is unavailable
@@ -147,7 +147,7 @@ ifdescribe(process.platform !== 'darwin')('menu bar keyboard sibling switching',
       'enter' // activate Beta's first item
     ]);
 
-    await setTimeout(KEY_DELAY);
+    await waitUntil(() => lastClicked !== null);
     expect(lastClicked).to.equal('beta-leaf');
   });
 
@@ -160,7 +160,7 @@ ifdescribe(process.platform !== 'darwin')('menu bar keyboard sibling switching',
       'enter' // activate Gamma's first item
     ]);
 
-    await setTimeout(KEY_DELAY);
+    await waitUntil(() => lastClicked !== null);
     expect(lastClicked).to.equal('gamma-item');
   });
 
@@ -173,7 +173,7 @@ ifdescribe(process.platform !== 'darwin')('menu bar keyboard sibling switching',
       'enter' // activate Alpha's first item
     ]);
 
-    await setTimeout(KEY_DELAY);
+    await waitUntil(() => lastClicked !== null);
     expect(lastClicked).to.equal('alpha-item');
   });
 
@@ -187,7 +187,7 @@ ifdescribe(process.platform !== 'darwin')('menu bar keyboard sibling switching',
       'enter' // activate Beta More Item
     ]);
 
-    await setTimeout(KEY_DELAY);
+    await waitUntil(() => lastClicked !== null);
     expect(lastClicked).to.equal('beta-more-item');
   });
 
@@ -202,7 +202,7 @@ ifdescribe(process.platform !== 'darwin')('menu bar keyboard sibling switching',
       'enter' // activate Beta Leaf
     ]);
 
-    await setTimeout(KEY_DELAY);
+    await waitUntil(() => lastClicked !== null);
     expect(lastClicked).to.equal('beta-leaf');
   });
 
@@ -217,7 +217,7 @@ ifdescribe(process.platform !== 'darwin')('menu bar keyboard sibling switching',
       'enter' // activate Beta More Item
     ]);
 
-    await setTimeout(KEY_DELAY);
+    await waitUntil(() => lastClicked !== null);
     expect(lastClicked).to.equal('beta-more-item');
   });
 });

[jkleinsc]

same comment as above about setTimeout vs waitUntil.

[jkleinsc]

same comment as above about setTimeout vs waitUntil.

[jkleinsc]

same comment as above about setTimeout vs waitUntil.

[jkleinsc]

same comment as above about setTimeout vs waitUntil.

[jkleinsc]

same comment as above about setTimeout vs waitUntil.

[erickzhao]

API LGTM

[jkleinsc]

Test should be updated to use waitUntil instead of setTimeout. My comment on this didn't render properly, so I have updated it to more clearly explain the change.

[ckerr]

API LGTM

[ckerr]

These tests are intended to cover Linux, but I think they never run on Linux because hasCapturableScreen() is hardcoded to return false on Linux CI. We don't need screen capture for this test, and xvfb should be able to handle RobotJS, so let's try enabling the tests on Linux:

Suggested change

	if (!robot || !robot.keyTap || !hasCapturableScreen()) {
	if (!robot?.keyTap) {

[mlaurencin]

My understanding though is that the virtual display we have for Linux CI is 0x0, so the focus + mouseClick combination used to ensure each test has proper initial setup would not function. I may be missing an alternative, but I think tests for this functionality cannot be run with our current CI and can only be tested locally.

[mlaurencin]

Test should be updated to use waitUntil instead of setTimeout. My comment on this didn't render properly, so I have updated it to more clearly explain the change.

I have an updated commit to address the setTimeout usage in window creation that I could push, but I think my previous Linux comment pertains to this as well since Windows virtual displays on CI are also 0x0.