2040851 - macOS: Don't mirror XUL command disabled state to NSMenuItem for standard Edit menu items routed via the responder chain

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stephen A Pohl [:spohl]]

Since bug 2036608, the standard Edit menu items in widget/cocoa/nsMenuItemX.mm have been wired to NSResponder-default selectors (cut:, copy:, paste:, undo:, redo:, delete:, selectAll:) with target=nil so AppKit propagates them via the macOS responder chain. This enables native text fields inside NSSavePanel / NSOpenPanel / NSAlert sheets to handle Cmd+C/V/X natively, and on macOS 26+ also gets us SF Symbol auto-injection on those items for free.

However, the NSMenuItem's enabled state is still mirrored from the XUL <command> element's disabled attribute by nsMenuItemX::SetEnabled. That attribute is updated by editMenuOverlay.js's commandupdater listening for focus and select events inside Gecko. When focus is in a native sheet, Gecko sees neither, so the commands stay disabled, the NSMenuItems stay disabled, and:

1. The menu items appear greyed out in the Edit menu, with the user unable to click them.
2. AppKit's performKeyEquivalent: still finds the key equivalent on the disabled menu item and consumes the keystroke without firing the action. So Cmd+Shift+Z (redo) inside a native sheet is silently swallowed -- it never falls through to the sheet's NSText where redo would otherwise work. This makes the bug functional, not just cosmetic.

Bug 1154697 exposes this because it adds an Edit menu to the Profile Manager / Create Profile wizard windows, where the only useful focus context is a native NSSavePanel sub-sheet. But the same limitation has been present in every Firefox window's Edit menu since bug 2036608 landed -- it was just invisible because regular browser windows rarely keep focus in a sheet long enough for users to look at the Edit menu.

Fix: in nsMenuItemX::SetEnabled, for menu items with one of the seven standard Edit IDs (menu_undo, menu_redo, menu_cut, menu_copy, menu_paste, menu_delete, menu_selectAll), always set NSMenuItem.enabled = YES. AppKit's automatic validateUserInterfaceItem: walk on the responder chain then owns the actual enable/disable decision:

• Sheet's NSText (first responder when a sheet is up) answers YES/NO based on its selection / clipboard state. Correct.
• ChildView (first responder when Gecko has focus) doesn't implement the selectors; AppKit walks past it up to MacApplicationDelegate, whose forwarders default to returning YES. Items appear enabled and clicks no-op when Gecko has nothing to act on. Slight UX downgrade tracked as a follow-up (link will be added once filed).

Test plan: in any sheet with a text field (e.g. Profile Manager's Create Profile wizard -> Choose Folder -> New Folder, or File -> Save Page As -> New Folder), type some text, select it, and verify:

• Cut / Copy / Delete light up when text is selected and work correctly via both menu click and Cmd+X / Cmd+C / Cmd+Delete shortcuts.
• After cut + paste, then Cmd+Z, Redo lights up and Cmd+Shift+Z works.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Bug 2040851: Don't mirror XUL command disabled state to NSMenuItem for standard Edit menu items on macOS. r?#mac-reviewers!

Since bug 2036608, the standard Edit menu items in widget/cocoa/
nsMenuItemX.mm have been wired to NSResponder-default selectors
(cut:, copy:, paste:, undo:, redo:, delete:, selectAll:) with
target=nil so AppKit propagates them via the macOS responder chain.
This enables native text fields inside NSSavePanel / NSOpenPanel /
NSAlert sheets to handle Cmd+C/V/X natively, and on macOS 26+ gets
us SF Symbol auto-injection on those items for free.

However, the NSMenuItem's enabled state was still mirrored from the
XUL <command> element's disabled attribute by SetEnabled(). That
attribute is updated by editMenuOverlay.js's commandupdater listening
for focus and select events inside Gecko. When focus is in a native
sheet, Gecko sees neither, so the commands stay disabled, the
NSMenuItems stay disabled, and:

1. The menu items appear greyed out in the Edit menu, with the user
   unable to click them.
2. AppKit's performKeyEquivalent: still finds the key equivalent on
   the disabled menu item and CONSUMES the keystroke without firing
   the action. So shortcuts like Cmd+Shift+Z (redo) inside a native
   sheet are silently swallowed -- they never fall through to the
   sheet's NSText where redo would otherwise work. This makes the
   bug functional, not just cosmetic.

Fix: in nsMenuItemX::SetEnabled, for menu items with one of the seven
standard Edit IDs (menu_undo, menu_redo, menu_cut, menu_copy,
menu_paste, menu_delete, menu_selectAll), always set
NSMenuItem.enabled = YES. AppKit's automatic validateUserInterfaceItem:
walk on the responder chain then owns the actual enable/disable
decision: the sheet's NSText answers YES/NO based on its selection /
clipboard state when a sheet is up, and the menu item gets shown
enabled when Gecko has focus (with command click no-ops as a small
UX downgrade tracked separately in bug 2040852).

Comment 2

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/8a7248901e14

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Is this something you wanted to nominate for Release?

Comment 5

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined/Reason for urgency: This would be a good ride-along patch to uplift if there is another 152 release. This addresses broken copy/paste and similar keyboard commands into native sheets on macOS (see for example bug 2048819). This originally broke in bug 2036608. The followup fixes in bug 2040851 and bug 2040844 made it into 153 and have been successfully baking ever since.
• Code covered by automated testing?: no
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This is a very narrow change to restore previous functionality and has been baking in 153 without any reported issues.
• String changes made/needed?: None
• Is Android affected?: no

Comment 6

[Stephen A Pohl [:spohl]]

Attachment: Bug 2040851: Don't mirror XUL command disabled state to NSMenuItem for standard Edit menu items on macOS.

Since bug 2036608, the standard Edit menu items in widget/cocoa/
nsMenuItemX.mm have been wired to NSResponder-default selectors
(cut:, copy:, paste:, undo:, redo:, delete:, selectAll:) with
target=nil so AppKit propagates them via the macOS responder chain.
This enables native text fields inside NSSavePanel / NSOpenPanel /
NSAlert sheets to handle Cmd+C/V/X natively, and on macOS 26+ gets
us SF Symbol auto-injection on those items for free.

However, the NSMenuItem's enabled state was still mirrored from the
XUL <command> element's disabled attribute by SetEnabled(). That
attribute is updated by editMenuOverlay.js's commandupdater listening
for focus and select events inside Gecko. When focus is in a native
sheet, Gecko sees neither, so the commands stay disabled, the
NSMenuItems stay disabled, and:

1. The menu items appear greyed out in the Edit menu, with the user
   unable to click them.
2. AppKit's performKeyEquivalent: still finds the key equivalent on
   the disabled menu item and CONSUMES the keystroke without firing
   the action. So shortcuts like Cmd+Shift+Z (redo) inside a native
   sheet are silently swallowed -- they never fall through to the
   sheet's NSText where redo would otherwise work. This makes the
   bug functional, not just cosmetic.

Fix: in nsMenuItemX::SetEnabled, for menu items with one of the seven
standard Edit IDs (menu_undo, menu_redo, menu_cut, menu_copy,
menu_paste, menu_delete, menu_selectAll), always set
NSMenuItem.enabled = YES. AppKit's automatic validateUserInterfaceItem:
walk on the responder chain then owns the actual enable/disable
decision: the sheet's NSText answers YES/NO based on its selection /
clipboard state when a sheet is up, and the menu item gets shown
enabled when Gecko has focus (with command click no-ops as a small
UX downgrade tracked separately in bug 2040852).

Original Revision: https://phabricator.services.mozilla.com/D301465

Comment 7

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/cf011a7ab693
https://hg.mozilla.org/releases/mozilla-release/rev/c4f7e585ba7f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Added to the 152.0.4 relnotes.