fix: update label/sublabel/icon in MenuItems on open (#49972)

fix: update label/sublabel/icon in macOS item on open

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

docs/api/menu-item.md

@@ -73,13 +73,16 @@ The following properties are available on instances of `MenuItem`:
 
 #### `menuItem.id`
 
-A `string` indicating the item's unique id. This property can be
-dynamically changed.
+A `string` indicating the item's unique id.
+
+This property can be dynamically changed.
 
 #### `menuItem.label`
 
 A `string` indicating the item's visible label.
 
+This property can be dynamically changed.
+
 #### `menuItem.click`
 
 A `Function` that is fired when the MenuItem receives a click event.
@@ -118,31 +121,37 @@ An `Accelerator | null` indicating the item's [user-assigned accelerator](https:
 
 #### `menuItem.icon`
 
-A `NativeImage | string` (optional) indicating the
-item's icon, if set.
+A `NativeImage | string` (optional) indicating the item's icon, if set.
+
+This property can be dynamically changed.
 
 #### `menuItem.sublabel`
 
 A `string` indicating the item's sublabel.
 
+This property can be dynamically changed.
+
 #### `menuItem.toolTip` _macOS_
 
 A `string` indicating the item's hover text.
 
 #### `menuItem.enabled`
 
-A `boolean` indicating whether the item is enabled. This property can be
-dynamically changed.
+A `boolean` indicating whether the item is enabled.
+
+This property can be dynamically changed.
 
 #### `menuItem.visible`
 
-A `boolean` indicating whether the item is visible. This property can be
-dynamically changed.
+A `boolean` indicating whether the item is visible.
+
+This property can be dynamically changed.
 
 #### `menuItem.checked`
 
-A `boolean` indicating whether the item is checked. This property can be
-dynamically changed.
+A `boolean` indicating whether the item is checked.
+
+This property can be dynamically changed.
 
 A `checkbox` menu item will toggle the `checked` property on and off when
 selected.

lib/browser/api/menu-item.ts

@@ -26,9 +26,9 @@ const MenuItem = function (this: any, options: any) {
   this.overrideReadOnlyProperty('type', roles.getDefaultType(this.role));
   this.overrideReadOnlyProperty('role');
   this.overrideReadOnlyProperty('accelerator', roles.getDefaultAccelerator(this.role));
-  this.overrideReadOnlyProperty('icon');
   this.overrideReadOnlyProperty('submenu');
 
+  this.overrideProperty('icon');
   this.overrideProperty('label', roles.getDefaultLabel(this.role));
   this.overrideProperty('sublabel', '');
   this.overrideProperty('toolTip', '');

lib/browser/api/menu.ts

@@ -53,6 +53,18 @@ Menu.prototype._isCommandIdVisible = function (id) {
   return this.commandsMap[id]?.visible ?? false;
 };
 
+Menu.prototype._getLabelForCommandId = function (id) {
+  return this.commandsMap[id]?.label ?? '';
+};
+
+Menu.prototype._getSecondaryLabelForCommandId = function (id) {
+  return this.commandsMap[id]?.sublabel ?? '';
+};
+
+Menu.prototype._getIconForCommandId = function (id) {
+  return this.commandsMap[id]?.icon ?? null;
+};
+
 Menu.prototype._getAcceleratorForCommandId = function (id, useDefaultAccelerator) {
   const command = this.commandsMap[id];
   if (!command) return;
@@ -158,7 +170,6 @@ Menu.prototype.insert = function (pos, item) {
   insertItemByType.call(this, item, pos);
 
   // set item properties
-  if (item.sublabel) this.setSublabel(pos, item.sublabel);
   if (item.toolTip) this.setToolTip(pos, item.toolTip);
   if (item.icon) this.setIcon(pos, item.icon);
   if (item.role) this.setRole(pos, item.role);

shell/browser/api/electron_api_menu.cc

@@ -103,6 +103,40 @@ bool Menu::IsCommandIdEnabled(int command_id) const {
   return InvokeBoolMethod(this, "_isCommandIdEnabled", command_id);
 }
 
+std::u16string Menu::GetLabelForCommandId(int command_id) const {
+  v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Value> val = gin_helper::CallMethod(
+      isolate, const_cast<Menu*>(this), "_getLabelForCommandId", command_id);
+  std::u16string label;
+  if (!gin::ConvertFromV8(isolate, val, &label))
+    label.clear();
+  return label;
+}
+
+std::u16string Menu::GetSecondaryLabelForCommandId(int command_id) const {
+  v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Value> val =
+      gin_helper::CallMethod(isolate, const_cast<Menu*>(this),
+                             "_getSecondaryLabelForCommandId", command_id);
+  std::u16string label;
+  if (!gin::ConvertFromV8(isolate, val, &label))
+    label.clear();
+  return label;
+}
+
+ui::ImageModel Menu::GetIconForCommandId(int command_id) const {
+  v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Value> val = gin_helper::CallMethod(
+      isolate, const_cast<Menu*>(this), "_getIconForCommandId", command_id);
+  gfx::Image icon;
+  if (!gin::ConvertFromV8(isolate, val, &icon))
+    icon = gfx::Image();
+  return ui::ImageModel::FromImage(icon);
+}
+
 bool Menu::IsCommandIdVisible(int command_id) const {
   return InvokeBoolMethod(this, "_isCommandIdVisible", command_id);
 }
@@ -206,10 +240,6 @@ void Menu::SetIcon(int index, const gfx::Image& image) {
   model_->SetIcon(index, ui::ImageModel::FromImage(image));
 }
 
-void Menu::SetSublabel(int index, const std::u16string& sublabel) {
-  model_->SetSecondaryLabel(index, sublabel);
-}
-
 void Menu::SetToolTip(int index, const std::u16string& toolTip) {
   model_->SetToolTip(index, toolTip);
 }
@@ -291,7 +321,6 @@ void Menu::FillObjectTemplate(v8::Isolate* isolate,
       .SetMethod("insertSeparator", &Menu::InsertSeparatorAt)
       .SetMethod("insertSubMenu", &Menu::InsertSubMenuAt)
       .SetMethod("setIcon", &Menu::SetIcon)
-      .SetMethod("setSublabel", &Menu::SetSublabel)
       .SetMethod("setToolTip", &Menu::SetToolTip)
       .SetMethod("setRole", &Menu::SetRole)
       .SetMethod("setCustomType", &Menu::SetCustomType)

shell/browser/api/electron_api_menu.h

@@ -73,6 +73,9 @@ class Menu : public gin::Wrappable<Menu>,
   bool IsCommandIdChecked(int command_id) const override;
   bool IsCommandIdEnabled(int command_id) const override;
   bool IsCommandIdVisible(int command_id) const override;
+  std::u16string GetLabelForCommandId(int command_id) const override;
+  std::u16string GetSecondaryLabelForCommandId(int command_id) const override;
+  ui::ImageModel GetIconForCommandId(int command_id) const override;
   bool ShouldCommandIdWorkWhenHidden(int command_id) const override;
   bool GetAcceleratorForCommandIdWithParams(
       int command_id,

shell/browser/ui/cocoa/electron_menu_controller.mm

@@ -351,10 +351,10 @@ NSArray* ConvertSharingItemToNS(const SharingItem& item) {
   // If the menu item has an icon, set it.
   ui::ImageModel icon = model->GetIconAt(index);
   if (icon.IsImage())
-    [item setImage:icon.GetImage().ToNSImage()];
+    item.image = icon.GetImage().ToNSImage();
 
   std::u16string toolTip = model->GetToolTipAt(index);
-  [item setToolTip:base::SysUTF16ToNSString(toolTip)];
+  item.toolTip = base::SysUTF16ToNSString(toolTip);
 
   if (role == u"services") {
     std::u16string title = u"Services";
@@ -499,6 +499,25 @@ NSArray* ConvertSharingItemToNS(const SharingItem& item) {
   item.hidden = !model->IsVisibleAt(index);
   item.state = model->IsItemCheckedAt(index) ? NSControlStateValueOn
                                              : NSControlStateValueOff;
+  std::u16string label16 = model->GetLabelAt(index);
+  NSString* label = l10n_util::FixUpWindowsStyleLabel(label16);
+  item.title = label;
+
+  std::u16string rawSecondaryLabel = model->GetSecondaryLabelAt(index);
+  if (!rawSecondaryLabel.empty()) {
+    if (@available(macOS 14.4, *)) {
+      NSString* secondary_label =
+          l10n_util::FixUpWindowsStyleLabel(rawSecondaryLabel);
+      item.subtitle = secondary_label;
+    }
+  }
+
+  ui::ImageModel icon = model->GetIconAt(index);
+  if (icon.IsImage()) {
+    item.image = icon.GetImage().ToNSImage();
+  } else {
+    item.image = nil;
+  }
 }
 
 - (void)refreshMenuTree:(NSMenu*)menu {

shell/browser/ui/electron_menu_model.cc

@@ -60,16 +60,22 @@ std::u16string ElectronMenuModel::GetRoleAt(size_t index) {
   return iter == std::end(roles_) ? std::u16string() : iter->second;
 }
 
-void ElectronMenuModel::SetSecondaryLabel(size_t index,
-                                          const std::u16string& sublabel) {
-  int command_id = GetCommandIdAt(index);
-  sublabels_[command_id] = sublabel;
+std::u16string ElectronMenuModel::GetLabelAt(size_t index) const {
+  if (delegate_)
+    return delegate_->GetLabelForCommandId(GetCommandIdAt(index));
+  return std::u16string();
 }
 
 std::u16string ElectronMenuModel::GetSecondaryLabelAt(size_t index) const {
-  int command_id = GetCommandIdAt(index);
-  const auto iter = sublabels_.find(command_id);
-  return iter == std::end(sublabels_) ? std::u16string() : iter->second;
+  if (delegate_)
+    return delegate_->GetSecondaryLabelForCommandId(GetCommandIdAt(index));
+  return std::u16string();
+}
+
+ui::ImageModel ElectronMenuModel::GetIconAt(size_t index) const {
+  if (delegate_)
+    return delegate_->GetIconForCommandId(GetCommandIdAt(index));
+  return ui::ImageModel();
 }
 
 bool ElectronMenuModel::GetAcceleratorAtWithParams(

shell/browser/ui/electron_menu_model.h

@@ -88,8 +88,9 @@ class ElectronMenuModel : public ui::SimpleMenuModel {
   std::u16string GetCustomTypeAt(size_t index);
   void SetRole(size_t index, const std::u16string& role);
   std::u16string GetRoleAt(size_t index);
-  void SetSecondaryLabel(size_t index, const std::u16string& sublabel);
+  std::u16string GetLabelAt(size_t index) const override;
   std::u16string GetSecondaryLabelAt(size_t index) const override;
+  ui::ImageModel GetIconAt(size_t index) const override;
   bool GetAcceleratorAtWithParams(size_t index,
                                   bool use_default_accelerator,
                                   ui::Accelerator* accelerator) const;
@@ -124,9 +125,8 @@ class ElectronMenuModel : public ui::SimpleMenuModel {
   std::optional<SharingItem> sharing_item_;
 #endif
 
-  base::flat_map<int, std::u16string> toolTips_;   // command id -> tooltip
-  base::flat_map<int, std::u16string> roles_;      // command id -> role
-  base::flat_map<int, std::u16string> sublabels_;  // command id -> sublabel
+  base::flat_map<int, std::u16string> toolTips_;  // command id -> tooltip
+  base::flat_map<int, std::u16string> roles_;     // command id -> role
   base::flat_map<int, std::u16string>
       customTypes_;  // command id -> custom type
   base::ObserverList<Observer> observers_;

typings/internal-electron.d.ts

@@ -157,6 +157,9 @@ declare namespace Electron {
     _isCommandIdEnabled(id: string): boolean;
     _shouldCommandIdWorkWhenHidden(id: string): boolean;
     _isCommandIdVisible(id: string): boolean;
+    _getLabelForCommandId(id: string): string;
+    _getSecondaryLabelForCommandId(id: string): string;
+    _getIconForCommandId(id: string): string | Electron.NativeImage | null;
     _getAcceleratorForCommandId(id: string, useDefaultAccelerator: boolean): Accelerator | undefined;
     _shouldRegisterAcceleratorForCommandId(id: string): boolean;
     _getSharingItemForCommandId(id: string): SharingItem | null;