fix: macOS getColor inconsistencies (#20611)

docs/api/system-preferences.md

@@ -291,7 +291,7 @@ This API is only available on macOS 10.14 Mojave or newer.
     * `window-frame` - Window frame.
     * `window-text` - Text in windows.
   * On **macOS**
-    * `alternate-selected-control-text` - The text on a selected surface in a list or table.
+    * `alternate-selected-control-text` - The text on a selected surface in a list or table. _deprecated_
     * `control-background` - The background of a large interface element, such as a browser or table.
     * `control` - The surface of a control.
     * `control-text` -The text of a control that isn’t disabled.
@@ -310,7 +310,7 @@ This API is only available on macOS 10.14 Mojave or newer.
     * `selected-content-background` - The background for selected content in a key window or view.
     * `selected-control` - The surface of a selected control.
     * `selected-control-text` - The text of a selected control.
-    * `selected-menu-item` - The text of a selected menu.
+    * `selected-menu-item-text` - The text of a selected menu.
     * `selected-text-background` - The background of selected text.
     * `selected-text` - Selected text.
     * `separator` - A separator between different sections of content.
@@ -328,6 +328,8 @@ This API is only available on macOS 10.14 Mojave or newer.
 Returns `String` - The system color setting in RGB hexadecimal form (`#ABCDEF`).
 See the [Windows docs][windows-colors] and the [MacOS docs][macos-colors] for more details.
 
+The following colors are only available on macOS 10.14: `find-highlight`, `selected-content-background`, `separator`, `unemphasized-selected-content-background`, `unemphasized-selected-text-background`, and `unemphasized-selected-text`.
+
 [windows-colors]:https://msdn.microsoft.com/en-us/library/windows/desktop/ms724371(v=vs.85).aspx
 [macos-colors]:https://developer.apple.com/design/human-interface-guidelines/macos/visual-design/color#dynamic-system-colors
 

shell/browser/api/atom_api_system_preferences.h

@@ -13,6 +13,7 @@
 #include "native_mate/handle.h"
 #include "shell/browser/api/event_emitter_deprecated.h"
 #include "shell/common/gin_helper/error_thrower.h"
+#include "shell/common/node_includes.h"
 #include "shell/common/promise_util.h"
 
 #if defined(OS_WIN)
@@ -52,7 +53,8 @@ class SystemPreferences : public mate::EventEmitter<SystemPreferences>
 
 #if defined(OS_WIN) || defined(OS_MACOSX)
   std::string GetAccentColor();
-  std::string GetColor(const std::string& color, mate::Arguments* args);
+  std::string GetColor(gin_helper::ErrorThrower thrower,
+                       const std::string& color);
 #endif
 #if defined(OS_WIN)
   bool IsAeroGlassEnabled();

shell/browser/api/atom_api_system_preferences_mac.mm

@@ -26,6 +26,7 @@
 #include "shell/browser/mac/atom_application.h"
 #include "shell/browser/mac/dict_util.h"
 #include "shell/browser/ui/cocoa/NSColor+Hex.h"
+#include "shell/common/deprecate_util.h"
 #include "shell/common/native_mate_converters/gurl_converter.h"
 #include "shell/common/native_mate_converters/value_converter.h"
 #include "ui/native_theme/native_theme.h"
@@ -503,18 +504,23 @@ bool SystemPreferences::IsTrustedAccessibilityClient(bool prompt) {
   return AXIsProcessTrustedWithOptions((CFDictionaryRef)options);
 }
 
-std::string SystemPreferences::GetColor(const std::string& color,
-                                        mate::Arguments* args) {
+std::string SystemPreferences::GetColor(gin_helper::ErrorThrower thrower,
+                                        const std::string& color) {
   NSColor* sysColor = nil;
   if (color == "alternate-selected-control-text") {
     sysColor = [NSColor alternateSelectedControlTextColor];
+    EmitDeprecationWarning(
+        node::Environment::GetCurrent(thrower.isolate()),
+        "'alternate-selected-control-text' is deprecated as an input to "
+        "getColor.  Use 'selected-content-background' instead.",
+        "electron");
   } else if (color == "control-background") {
     sysColor = [NSColor controlBackgroundColor];
   } else if (color == "control") {
     sysColor = [NSColor controlColor];
   } else if (color == "control-text") {
     sysColor = [NSColor controlTextColor];
-  } else if (color == "disabled-control") {
+  } else if (color == "disabled-control-text") {
     sysColor = [NSColor disabledControlTextColor];
   } else if (color == "find-highlight") {
     if (@available(macOS 10.14, *))
@@ -580,11 +586,12 @@ std::string SystemPreferences::GetColor(const std::string& color,
   } else if (color == "window-frame-text") {
     sysColor = [NSColor windowFrameTextColor];
   } else {
-    args->ThrowError("Unknown color: " + color);
-    return "";
+    thrower.ThrowError("Unknown color: " + color);
   }
 
+  if (sysColor)
     return base::SysNSStringToUTF8([sysColor hexadecimalValue]);
+  return "";
 }
 
 std::string SystemPreferences::GetMediaAccessStatus(

shell/browser/api/atom_api_system_preferences_win.cc

@@ -46,8 +46,8 @@ std::string SystemPreferences::GetAccentColor() {
   return hexColorDWORDToRGBA(color);
 }
 
-std::string SystemPreferences::GetColor(const std::string& color,
-                                        mate::Arguments* args) {
+std::string SystemPreferences::GetColor(gin_helper::ErrorThrower thrower,
+                                        const std::string& color) {
   int id;
   if (color == "3d-dark-shadow") {
     id = COLOR_3DDKSHADOW;
@@ -110,7 +110,7 @@ std::string SystemPreferences::GetColor(const std::string& color,
   } else if (color == "window-text") {
     id = COLOR_WINDOWTEXT;
   } else {
-    args->ThrowError("Unknown color: " + color);
+    thrower.ThrowError("Unknown color: " + color);
     return "";
   }
 

spec-main/api-system-preferences-spec.ts

@@ -134,6 +134,58 @@ describe('systemPreferences module', () => {
     })
   })
 
+  ifdescribe(process.platform === 'darwin')('systemPreferences.getColor(color)', () => {
+    it('throws on invalid colors', () => {
+      const color = 'bad-color'
+      expect(() => {
+        systemPreferences.getColor(color as any)
+      }).to.throw(`Unknown color: ${color}`)
+    })
+  
+    it('returns a valid color', () => {
+      const colors = [
+        'alternate-selected-control-text',
+        'control-background',
+        'control',
+        'control-text',
+        'disabled-control-text',
+        'find-highlight',
+        'grid',
+        'header-text',
+        'highlight',
+        'keyboard-focus-indicator',
+        'label',
+        'link',
+        'placeholder-text',
+        'quaternary-label',
+        'scrubber-textured-background',
+        'secondary-label',
+        'selected-content-background',
+        'selected-control',
+        'selected-control-text',
+        'selected-menu-item-text',
+        'selected-text-background',
+        'selected-text',
+        'separator',
+        'shadow',
+        'tertiary-label',
+        'text-background',
+        'text',
+        'under-page-background',
+        'unemphasized-selected-content-background',
+        'unemphasized-selected-text-background',
+        'unemphasized-selected-text',
+        'window-background',
+        'window-frame-text'
+      ]
+      
+      colors.forEach(color => {
+        const sysColor = systemPreferences.getColor(color as any)
+        expect(sysColor).to.be.a('string')
+      })
+    })
+  })
+
   ifdescribe(process.platform === 'darwin')('systemPreferences.appLevelAppearance', () => {
     it('has an appLevelAppearance property', () => {
       expect(systemPreferences).to.have.property('appLevelAppearance')