[Mac] Fix menu closing immediately after opening with keyboard shortcuts [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
rob...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ui/views/config.gwsq)
Related details
- Robert Liao
- Yuanjun Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Liao (Gerrit)
Robert Liao added 6 comments![]( "Open in Gerrit")
// Only trigger menu cancellation on key press, not on key release.
// This prevents menus from closing when users release modifier keys after
// opening the menu with a keyboard shortcut (e.g., Cmd+Ctrl+Comma).
if (accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
return false;
}@mmen...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
// menu. This prevents the menu from flashing open and immediately closing when
// users release keys after opening a menu with a keyboard shortcut (e.g.,
// Cmd+Ctrl+Comma).I don't believe Chromium has this behavior, so you'll want to use a different scenario to illustrate the point.
EXPECT_TRUE(showing());Change to `ASSERT_TRUE`. If this check fails, there's no need to continue the test.
// Cmd+Ctrl+Comma. This should NOT cancel the menu.Chromium doesn't use this key combo, so it's not clear what this is testing. Recommend choosing a different key combination.
// Verify that pressing a new accelerator still closes the menu.
// Press Cmd+T (New Tab) - this SHOULD close the menu.
ui::KeyEvent press_t(ui::EventType::kKeyPressed, ui::VKEY_T,
ui::EF_COMMAND_DOWN);
menu_controller()->OnWillDispatchKeyEvent(&press_t);
EXPECT_FALSE(showing());Move this into a separate test as it's testing something different.
Related details
- Yuanjun Zhu
- liang zeng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
liang zeng (Gerrit)
liang zeng added 1 comment![]( "Open in Gerrit")
// Verify that pressing a new accelerator still closes the menu.
// Press Cmd+T (New Tab) - this SHOULD close the menu.
ui::KeyEvent press_t(ui::EventType::kKeyPressed, ui::VKEY_T,
ui::EF_COMMAND_DOWN);
menu_controller()->OnWillDispatchKeyEvent(&press_t);
EXPECT_FALSE(showing());Move this into a separate test as it's testing something different.
This is necessary because we need to confirm that pressing other shortcut keys (such as "Cmd+T") will close the menu correctly.
Related details
- Robert Liao
- Yuanjun Zhu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Liao (Gerrit)
Robert Liao added 2 comments![]( "Open in Gerrit")
// Only trigger menu cancellation on key press, not on key release.
// This prevents menus from closing when users release modifier keys after
// opening the menu with a keyboard shortcut (e.g., Cmd+Ctrl+Comma).
if (accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
return false;
}@mmen...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
Fixing to @ma...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
// Verify that pressing a new accelerator still closes the menu.
// Press Cmd+T (New Tab) - this SHOULD close the menu.
ui::KeyEvent press_t(ui::EventType::kKeyPressed, ui::VKEY_T,
ui::EF_COMMAND_DOWN);
menu_controller()->OnWillDispatchKeyEvent(&press_t);
EXPECT_FALSE(showing());liang zengMove this into a separate test as it's testing something different.
This is necessary because we need to confirm that pressing other shortcut keys (such as "Cmd+T") will close the menu correctly.
That's fine. This behavior is independent of the preceding sequence, correct?
Related details
- Mark Mentovai
- Yuanjun Zhu
- liang zeng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
liang zeng (Gerrit)
liang zeng added 1 comment![]( "Open in Gerrit")
// menu. This prevents the menu from flashing open and immediately closing when
// users release keys after opening a menu with a keyboard shortcut (e.g.,
// Cmd+Ctrl+Comma).I don't believe Chromium has this behavior, so you'll want to use a different scenario to illustrate the point.
Yes, I haven't found a corresponding shortcut to bring up the menu in Chromium yet, but using it (for example, in Edge) causes a bug. What do you suggest for the design of this test case? Or should we remove these changes first?
Related details
- Mark Mentovai
- Robert Liao
- Yuanjun Zhu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Liao (Gerrit)
Robert Liao added 1 comment![]( "Open in Gerrit")
// menu. This prevents the menu from flashing open and immediately closing when
// users release keys after opening a menu with a keyboard shortcut (e.g.,
// Cmd+Ctrl+Comma).liang zengI don't believe Chromium has this behavior, so you'll want to use a different scenario to illustrate the point.
Yes, I haven't found a corresponding shortcut to bring up the menu in Chromium yet, but using it (for example, in Edge) causes a bug. What do you suggest for the design of this test case? Or should we remove these changes first?
I think in this case, you should genericize the test case to an arbitrary key shortcut. The issue still stands that if Chromium were to bring up a menu with a shortcut, it would immediately dismiss.
Related details
- Mark Mentovai
- liang zeng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
liang zeng (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Mark Mentovai
- Robert Liao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
liang zeng (Gerrit)
liang zeng added 4 comments![]( "Open in Gerrit")
// menu. This prevents the menu from flashing open and immediately closing when
// users release keys after opening a menu with a keyboard shortcut (e.g.,
// Cmd+Ctrl+Comma).liang zengI don't believe Chromium has this behavior, so you'll want to use a different scenario to illustrate the point.
Robert LiaoYes, I haven't found a corresponding shortcut to bring up the menu in Chromium yet, but using it (for example, in Edge) causes a bug. What do you suggest for the design of this test case? Or should we remove these changes first?
I think in this case, you should genericize the test case to an arbitrary key shortcut. The issue still stands that if Chromium were to bring up a menu with a shortcut, it would immediately dismiss.
Updated the test case and comments.
Change to `ASSERT_TRUE`. If this check fails, there's no need to continue the test.
Fixed, thanks.
Chromium doesn't use this key combo, so it's not clear what this is testing. Recommend choosing a different key combination.
Updated, thanks.
// Verify that pressing a new accelerator still closes the menu.
// Press Cmd+T (New Tab) - this SHOULD close the menu.
ui::KeyEvent press_t(ui::EventType::kKeyPressed, ui::VKEY_T,
ui::EF_COMMAND_DOWN);
menu_controller()->OnWillDispatchKeyEvent(&press_t);
EXPECT_FALSE(showing());liang zengMove this into a separate test as it's testing something different.
Robert LiaoThis is necessary because we need to confirm that pressing other shortcut keys (such as "Cmd+T") will close the menu correctly.
That's fine. This behavior is independent of the preceding sequence, correct?
Moved this to case `KeyPressWithModifierCancelsMenu`
Related details
- Mark Mentovai
- Robert Liao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
liang zeng (Gerrit)
liang zeng added 1 comment![]( "Open in Gerrit")
// Only trigger menu cancellation on key press, not on key release.
// This prevents menus from closing when users release modifier keys after
// opening the menu with a keyboard shortcut (e.g., Cmd+Ctrl+Comma).
if (accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
return false;
}Robert Liao@mmen...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
Fixing to @ma...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
Hi @ma...@chromium.org could you help to review this change?
liang zeng (Gerrit)
liang zeng voted Commit-Queue+1![]( "Open in Gerrit")
- Avi Drissman
- Mark Mentovai
- Robert Liao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
1 comment
// Only trigger menu cancellation on key press, not on key release.
// This prevents menus from closing when users release modifier keys after
// opening the menu with a keyboard shortcut (e.g., Cmd+Ctrl+Comma).
if (accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
return false;
}Robert Liao@mmen...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
liang zengFixing to @ma...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
Hi @ma...@chromium.org could you help to review this change?
I think that this is correct.
Avi’s listed as a reviewer too, maybe he can brainstorm something?
Related details
- Avi Drissman
- Robert Liao
- liang zeng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Avi Drissman
| Code-Review | +1 |
1 comment
// Only trigger menu cancellation on key press, not on key release.
// This prevents menus from closing when users release modifier keys after
// opening the menu with a keyboard shortcut (e.g., Cmd+Ctrl+Comma).
if (accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
return false;
}Robert Liao@mmen...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
liang zengFixing to @ma...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
Mark MentovaiHi @ma...@chromium.org could you help to review this change?
I think that this is correct.
Avi’s listed as a reviewer too, maybe he can brainstorm something?
This seems right to me. I’m actually wondering if we shouldn’t go further. Why do purely modifier-only keyup/downs cancel menus? I don’t believe they do so for native menus.
Related details
- Robert Liao
- liang zeng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
liang zeng (Gerrit)
liang zeng added 1 comment![]( "Open in Gerrit")
// Only trigger menu cancellation on key press, not on key release.
// This prevents menus from closing when users release modifier keys after
// opening the menu with a keyboard shortcut (e.g., Cmd+Ctrl+Comma).
if (accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
return false;
}Robert Liao@mmen...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
liang zengFixing to @ma...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
Mark MentovaiHi @ma...@chromium.org could you help to review this change?
Avi DrissmanI think that this is correct.
Avi’s listed as a reviewer too, maybe he can brainstorm something?
This seems right to me. I’m actually wondering if we shouldn’t go further. Why do purely modifier-only keyup/downs cancel menus? I don’t believe they do so for native menus.
Yes, it only affects view-based menus on Mac.
Related details
- Robert Liao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
// Only trigger menu cancellation on key press, not on key release.
// This prevents menus from closing when users release modifier keys after
// opening the menu with a keyboard shortcut (e.g., Cmd+Ctrl+Comma).
if (accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
return false;
}Robert Liao@mmen...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
liang zengFixing to @ma...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
Mark MentovaiHi @ma...@chromium.org could you help to review this change?
Avi DrissmanI think that this is correct.
Avi’s listed as a reviewer too, maybe he can brainstorm something?
liang zengThis seems right to me. I’m actually wondering if we shouldn’t go further. Why do purely modifier-only keyup/downs cancel menus? I don’t believe they do so for native menus.
Yes, it only affects view-based menus on Mac.
When we built MacViews, we tried to align with native Mac behavior whenever feasible. I appreciate that in this change, you’re trying to make minimal changes, but I’d say that as a future change (not necessarily in this CL) I’d be OK with completely ignoring modifier changes, both key up and key down, on the Mac, assuming it didn’t break anything.
In any case, happy to see this CL land as-is to unbreak Edge.
Related details
- Robert Liao
- liang zeng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Liao (Gerrit)
Robert Liao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Robert Liao
| Code-Review | +1 |
1 comment
Thanks for working through the review!
Related details
- liang zeng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Liao (Gerrit)
Robert Liao voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Mac] Fix menu closing immediately after opening with keyboard shortcuts
When opening menus with keyboard shortcuts containing multiple
modifiers, the menu would flash open and immediately close when users
released the keys. This made such shortcuts effectively unusable.
Root Cause: The AcceleratorShouldCancelMenu() function on macOS was
treating key release events the same as key press events. When users
released keys after opening a menu, if any modifier (Cmd/Ctrl/Alt) was
still held, the function would return true and close the menu.
Fix: Add a check at the beginning of AcceleratorShouldCancelMenu() to
only trigger menu cancellation on key press events, not key release
events. This aligns with the semantic meaning of keyboard shortcuts -
actions should be triggered on key press, not key release.
The fix is minimal and targeted:
- Only affects the menu closing behavior
- Preserves existing functionality: pressing new shortcuts while a menu
is open still correctly closes the menu
- Prevents menus from closing when users simply release keys
Testing: Added MenuControllerTest.KeyReleaseDoesNotCancelMenu to verify:
1. Key release events (with modifiers) don't close menus 2. Key press
events (with modifiers) still correctly close menus
- M ui/views/controls/menu/menu_controller.cc
- M ui/views/controls/menu/menu_controller_unittest.cc
Code-Review: +1 by Mark Mentovai, +1 by Avi Drissman, +1 by Robert Liao
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
liang zeng (Gerrit)
liang zeng added 1 comment![]( "Open in Gerrit")
// Only trigger menu cancellation on key press, not on key release.
// This prevents menus from closing when users release modifier keys after
// opening the menu with a keyboard shortcut (e.g., Cmd+Ctrl+Comma).
if (accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
return false;
}Robert Liao@mmen...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
liang zengFixing to @ma...@chromium.org - Do you know if any Mac menus will break if this is introduced? (I can't think of any on my cursory check).
Mark MentovaiHi @ma...@chromium.org could you help to review this change?
Avi DrissmanI think that this is correct.
Avi’s listed as a reviewer too, maybe he can brainstorm something?
liang zengThis seems right to me. I’m actually wondering if we shouldn’t go further. Why do purely modifier-only keyup/downs cancel menus? I don’t believe they do so for native menus.
Avi DrissmanYes, it only affects view-based menus on Mac.
When we built MacViews, we tried to align with native Mac behavior whenever feasible. I appreciate that in this change, you’re trying to make minimal changes, but I’d say that as a future change (not necessarily in this CL) I’d be OK with completely ignoring modifier changes, both key up and key down, on the Mac, assuming it didn’t break anything.
In any case, happy to see this CL land as-is to unbreak Edge.
Very good suggestion, thank you!
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |