Remove More and Close buttons from macOS 11 notifications

macOS 11 introduced a new notification UI that has a dedicated close
button in the top left corner. Unfortunately we're no longer able to
override the caption of the overflow menu button via the
NSUserNotification API so this will also remove the "More" button. The
overflow button will now have the default label which is "Options"
instead. Once we switch to the new API we can have our own labels on
macOS 11 again.

Before: https://imgur.com/6pOt5NH
After: https://imgur.com/ImCqjQ5

Bug: 1157781
Change-Id: I3924fed16b54147f6de7fadb65ab32ac18f250a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587014Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836872}

chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm

@@ -137,8 +137,10 @@ TEST_F(NotificationPlatformBridgeMacTest, TestDisplayNoButtons) {
   EXPECT_NSEQ(@"Title", [delivered_notification title]);
   EXPECT_NSEQ(@"Context", [delivered_notification informativeText]);
   EXPECT_NSEQ(@"gmail.com", [delivered_notification subtitle]);
-  EXPECT_NSEQ(@"Close", [delivered_notification otherButtonTitle]);
   EXPECT_NSEQ(@"Settings", [delivered_notification actionButtonTitle]);
+
+  if (!base::mac::IsAtLeastOS11())
+    EXPECT_NSEQ(@"Close", [delivered_notification otherButtonTitle]);
 }
 
 TEST_F(NotificationPlatformBridgeMacTest, TestDisplayNoSettings) {
@@ -159,8 +161,10 @@ TEST_F(NotificationPlatformBridgeMacTest, TestDisplayNoSettings) {
   EXPECT_NSEQ(@"Title", [delivered_notification title]);
   EXPECT_NSEQ(@"Context", [delivered_notification informativeText]);
   EXPECT_NSEQ(@"gmail.com", [delivered_notification subtitle]);
-  EXPECT_NSEQ(@"Close", [delivered_notification otherButtonTitle]);
   EXPECT_FALSE([delivered_notification hasActionButton]);
+
+  if (!base::mac::IsAtLeastOS11())
+    EXPECT_NSEQ(@"Close", [delivered_notification otherButtonTitle]);
 }
 
 TEST_F(NotificationPlatformBridgeMacTest, TestDisplayOneButton) {
@@ -179,8 +183,11 @@ TEST_F(NotificationPlatformBridgeMacTest, TestDisplayOneButton) {
   EXPECT_NSEQ(@"Title", [delivered_notification title]);
   EXPECT_NSEQ(@"Context", [delivered_notification informativeText]);
   EXPECT_NSEQ(@"gmail.com", [delivered_notification subtitle]);
+
+  if (!base::mac::IsAtLeastOS11()) {
     EXPECT_NSEQ(@"Close", [delivered_notification otherButtonTitle]);
     EXPECT_NSEQ(@"More", [delivered_notification actionButtonTitle]);
+  }
 }
 
 TEST_F(NotificationPlatformBridgeMacTest, TestDisplayProgress) {

chrome/browser/notifications/notification_platform_bridge_mac_unnotification_unittest.mm

@@ -5,6 +5,7 @@
 #import <UserNotifications/UserNotifications.h>
 
 #include "base/bind.h"
+#include "base/mac/mac_util.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
@@ -416,8 +417,12 @@ TEST_F(UNNotificationPlatformBridgeMacTest, TestNotificationNoButtons) {
       // If this selector from the private API is available the close button
       // will be set in alernateAction, and the other buttons will be set in
       // actions. Otherwise, all buttons will be setting in actions which causes
-      // the total count to differ by one.
-      if ([category respondsToSelector:@selector(alternateAction)]) {
+      // the total count to differ by one. On macOS 11+ we don't need to add the
+      // close button as there will always be one in the top left corner.
+      if (base::mac::IsAtLeastOS11()) {
+        ASSERT_EQ(1ul, [[category actions] count]);
+        EXPECT_NSEQ(@"Settings", [[[category actions] lastObject] title]);
+      } else if ([category respondsToSelector:@selector(alternateAction)]) {
         ASSERT_EQ(1ul, [[category actions] count]);
         EXPECT_NSEQ(@"Close",
                     [[category valueForKey:@"_alternateAction"] title]);

chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm

@@ -64,12 +64,14 @@
 
     [toast setValue:@YES forKey:@"_showsButtons"];
     // A default close button label is provided by the platform but we
-    // explicitly override it in case the user decides to not
-    // use the OS language in Chrome.
+    // explicitly override it in case the user decides to not use the OS
+    // language in Chrome. macOS 11 shows a close button in the top-left corner.
+    if (!base::mac::IsAtLeastOS11()) {
       [toast
-        setOtherButtonTitle:[_notificationData
-                                objectForKey:notification_constants::
+          setOtherButtonTitle:
+              [_notificationData objectForKey:notification_constants::
                                                   kNotificationCloseButtonTag]];
+    }
 
     NSMutableArray* buttons = [NSMutableArray arrayWithCapacity:3];
     if ([_notificationData
@@ -108,10 +110,18 @@
           [toast respondsToSelector:@selector(_alwaysShowAlternateActionMenu)]);
       DCHECK(
           [toast respondsToSelector:@selector(_alternateActionButtonTitles)]);
+      // macOS 11 does not support overriding the text of the overflow button
+      // and will always show "Options" via this API. Setting actionButtonTitle
+      // just appends another button into the overflow menu. Only the new
+      // UNNotification API allows overriding this title on macOS 11.
+      if (base::mac::IsAtLeastOS11()) {
+        [toast setValue:@NO forKey:@"_hasActionButton"];
+      } else {
         [toast setActionButtonTitle:
                    [_notificationData
                        objectForKey:notification_constants::
                                         kNotificationOptionsButtonTag]];
+      }
       [toast setValue:@YES forKey:@"_alwaysShowAlternateActionMenu"];
       [toast setValue:buttons forKey:@"_alternateActionButtonTitles"];
     }

chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm

@@ -5,6 +5,7 @@
 #import <AppKit/AppKit.h>
 
 #include "base/mac/foundation_util.h"
+#include "base/mac/mac_util.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/strings/sys_string_conversions.h"
 #include "chrome/browser/notifications/notification_handler.h"
@@ -44,7 +45,11 @@ TEST(NotificationBuilderMacTest, TestNotificationNoButtons) {
   EXPECT_TRUE([notification hasActionButton]);
   EXPECT_EQ("Settings",
             base::SysNSStringToUTF8([notification actionButtonTitle]));
-  EXPECT_EQ("Close", base::SysNSStringToUTF8([notification otherButtonTitle]));
+
+  if (!base::mac::IsAtLeastOS11()) {
+    EXPECT_EQ("Close",
+              base::SysNSStringToUTF8([notification otherButtonTitle]));
+  }
 }
 
 TEST(NotificationBuilderMacTest, TestNotificationOneButton) {
@@ -75,11 +80,13 @@ TEST(NotificationBuilderMacTest, TestNotificationOneButton) {
   EXPECT_EQ("https://www.miguel.com",
             base::SysNSStringToUTF8([notification subtitle]));
 
+  if (!base::mac::IsAtLeastOS11()) {
     EXPECT_TRUE([notification hasActionButton]);
-
     EXPECT_EQ("Options",
               base::SysNSStringToUTF8([notification actionButtonTitle]));
-  EXPECT_EQ("Close", base::SysNSStringToUTF8([notification otherButtonTitle]));
+    EXPECT_EQ("Close",
+              base::SysNSStringToUTF8([notification otherButtonTitle]));
+  }
 
   NSArray* buttons = [notification valueForKey:@"_alternateActionButtonTitles"];
   ASSERT_EQ(2u, buttons.count);
@@ -115,11 +122,13 @@ TEST(NotificationBuilderMacTest, TestNotificationTwoButtons) {
   EXPECT_EQ("https://www.miguel.com",
             base::SysNSStringToUTF8([notification subtitle]));
 
+  if (!base::mac::IsAtLeastOS11()) {
     EXPECT_TRUE([notification hasActionButton]);
-
     EXPECT_EQ("Options",
               base::SysNSStringToUTF8([notification actionButtonTitle]));
-  EXPECT_EQ("Close", base::SysNSStringToUTF8([notification otherButtonTitle]));
+    EXPECT_EQ("Close",
+              base::SysNSStringToUTF8([notification otherButtonTitle]));
+  }
 
   NSArray* buttons = [notification valueForKey:@"_alternateActionButtonTitles"];
   ASSERT_EQ(3u, buttons.count);
@@ -149,7 +158,11 @@ TEST(NotificationBuilderMacTest, TestNotificationExtensionNoButtons) {
   NSUserNotification* notification = [builder buildUserNotification];
 
   EXPECT_FALSE(notification.hasActionButton);
-  EXPECT_EQ("Close", base::SysNSStringToUTF8([notification otherButtonTitle]));
+
+  if (!base::mac::IsAtLeastOS11()) {
+    EXPECT_EQ("Close",
+              base::SysNSStringToUTF8([notification otherButtonTitle]));
+  }
 }
 
 TEST(NotificationBuilderMacTest, TestNotificationExtensionOneButton) {
@@ -177,7 +190,11 @@ TEST(NotificationBuilderMacTest, TestNotificationExtensionOneButton) {
   EXPECT_TRUE([notification hasActionButton]);
   EXPECT_EQ("Button1",
             base::SysNSStringToUTF8([notification actionButtonTitle]));
-  EXPECT_EQ("Close", base::SysNSStringToUTF8([notification otherButtonTitle]));
+
+  if (!base::mac::IsAtLeastOS11()) {
+    EXPECT_EQ("Close",
+              base::SysNSStringToUTF8([notification otherButtonTitle]));
+  }
 }
 
 TEST(NotificationBuilderMacTest, TestNotificationExtensionButtons) {

chrome/browser/ui/cocoa/notifications/unnotification_builder_mac.mm

@@ -38,6 +38,8 @@
                                objectForKey:notification_constants::
                                                 kNotificationCloseButtonTag]
                    options:UNNotificationActionOptionNone];
+  // macOS 11 shows a close button in the top-left corner.
+  if (!base::mac::IsAtLeastOS11())
     [buttonsArray addObject:closeButton];
 
   if ([_notificationData
@@ -77,7 +79,7 @@
   // be set as [button, Close] so that close is on top. This is to safeguard the
   // order of the buttons in case respondsToSelector:@selector(alternateAction)
   // were to return false.
-  if ([buttonsArray count] == 2) {
+  if (!base::mac::IsAtLeastOS11() && [buttonsArray count] == 2) {
     // Remove the close button and move it to the end of the array
     [buttonsArray removeObject:closeButton];
     [buttonsArray addObject:closeButton];
@@ -98,8 +100,9 @@
   // This uses a private API to make sure the close button is always visible in
   // both alerts and banners, and modifies its content so that it is consistent
   // with the rest of the notification buttons. Otherwise, the text inside the
-  // close button will come from the Apple API
-  if ([category respondsToSelector:@selector(alternateAction)]) {
+  // close button will come from the Apple API.
+  if (!base::mac::IsAtLeastOS11() &&
+      [category respondsToSelector:@selector(alternateAction)]) {
     [buttonsArray removeObject:closeButton];
     [category setValue:buttonsArray forKey:@"actions"];
     [category setValue:closeButton forKey:@"_alternateAction"];

chrome/browser/ui/cocoa/notifications/unnotification_builder_mac_unittest.mm

@@ -5,6 +5,7 @@
 #import <UserNotifications/UserNotifications.h>
 
 #include "base/files/file_util.h"
+#include "base/mac/mac_util.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
@@ -65,7 +66,16 @@ TEST(UNNotificationBuilderMacTest, TestNotificationNoButtons) {
     UNNotificationCategory* category = [builder buildCategory];
 
     // Test contents of the category
-    if ([category respondsToSelector:@selector(alternateAction)]) {
+    if (base::mac::IsAtLeastOS11()) {
+      EXPECT_EQ("Settings", base::SysNSStringToUTF8(
+                                [[[category actions] lastObject] title]));
+      EXPECT_EQ(base::SysNSStringToUTF8(
+                    notification_constants::kNotificationSettingsButtonTag),
+                base::SysNSStringToUTF8(
+                    [[[category actions] lastObject] identifier]));
+
+      EXPECT_EQ(1ul, [[category actions] count]);
+    } else if ([category respondsToSelector:@selector(alternateAction)]) {
       EXPECT_EQ("Close", base::SysNSStringToUTF8([[category
                              valueForKey:@"_alternateAction"] title]));
       EXPECT_EQ(base::SysNSStringToUTF8(
@@ -115,7 +125,15 @@ TEST(UNNotificationBuilderMacTest, TestNotificationOneButton) {
     UNNotificationCategory* category = [builder buildCategory];
 
     // Test contents of the category
-    if ([category respondsToSelector:@selector(alternateAction)]) {
+    if (base::mac::IsAtLeastOS11()) {
+      EXPECT_EQ("Button1",
+                base::SysNSStringToUTF8([[category actions][0] title]));
+      EXPECT_EQ(base::SysNSStringToUTF8(
+                    notification_constants::kNotificationButtonOne),
+                base::SysNSStringToUTF8([[category actions][0] identifier]));
+
+      EXPECT_EQ(2ul, [[category actions] count]);
+    } else if ([category respondsToSelector:@selector(alternateAction)]) {
       EXPECT_EQ("Close", base::SysNSStringToUTF8([[category
                              valueForKey:@"_alternateAction"] title]));
       EXPECT_EQ(base::SysNSStringToUTF8(
@@ -175,7 +193,21 @@ TEST(UNNotificationBuilderMacTest, TestNotificationTwoButtons) {
     UNNotificationCategory* category = [builder buildCategory];
 
     // Test contents of the category
-    if ([category respondsToSelector:@selector(alternateAction)]) {
+    if (base::mac::IsAtLeastOS11()) {
+      EXPECT_EQ("Button1",
+                base::SysNSStringToUTF8([[category actions][0] title]));
+      EXPECT_EQ(base::SysNSStringToUTF8(
+                    notification_constants::kNotificationButtonOne),
+                base::SysNSStringToUTF8([[category actions][0] identifier]));
+
+      EXPECT_EQ("Button2",
+                base::SysNSStringToUTF8([[category actions][1] title]));
+      EXPECT_EQ(base::SysNSStringToUTF8(
+                    notification_constants::kNotificationButtonTwo),
+                base::SysNSStringToUTF8([[category actions][1] identifier]));
+
+      EXPECT_EQ(3ul, [[category actions] count]);
+    } else if ([category respondsToSelector:@selector(alternateAction)]) {
       EXPECT_EQ("Close", base::SysNSStringToUTF8([[category
                              valueForKey:@"_alternateAction"] title]));
       EXPECT_EQ(base::SysNSStringToUTF8(
@@ -246,7 +278,9 @@ TEST(UNNotificationBuilderMacTest, TestNotificationExtensionNoButtons) {
     UNNotificationCategory* category = [builder buildCategory];
 
     // Test contents of the category
-    if ([category respondsToSelector:@selector(alternateAction)]) {
+    if (base::mac::IsAtLeastOS11()) {
+      EXPECT_EQ(0ul, [[category actions] count]);
+    } else if ([category respondsToSelector:@selector(alternateAction)]) {
       EXPECT_EQ("Close", base::SysNSStringToUTF8([[category
                              valueForKey:@"_alternateAction"] title]));
       EXPECT_EQ(base::SysNSStringToUTF8(
@@ -281,7 +315,15 @@ TEST(UNNotificationBuilderMacTest, TestNotificationExtensionTwoButtons) {
     UNNotificationCategory* category = [builder buildCategory];
 
     // Test contents of the category
-    if ([category respondsToSelector:@selector(alternateAction)]) {
+    if (base::mac::IsAtLeastOS11()) {
+      EXPECT_EQ("Button1",
+                base::SysNSStringToUTF8([[category actions][0] title]));
+      EXPECT_EQ(base::SysNSStringToUTF8(
+                    notification_constants::kNotificationButtonOne),
+                base::SysNSStringToUTF8([[category actions][0] identifier]));
+
+      EXPECT_EQ(2ul, [[category actions] count]);
+    } else if ([category respondsToSelector:@selector(alternateAction)]) {
       EXPECT_EQ("Close", base::SysNSStringToUTF8([[category
                              valueForKey:@"_alternateAction"] title]));
       EXPECT_EQ(base::SysNSStringToUTF8(
@@ -526,6 +568,12 @@ TEST(UNNotificationBuilderMacTest, MAYBE_TestIconWrongPath) {
     [builder setIconPath:@"wrong-path"];
     UNMutableNotificationContent* content = [builder buildUserNotification];
 
+    if (base::mac::IsAtLeastOS11()) {
+      // TODO(knollr): Figure out why macOS 11 allows creating a
+      // UNNotificationAttachment with an invalid path.
+      EXPECT_EQ(1ul, [[content attachments] count]);
+    } else {
       EXPECT_EQ(0ul, [[content attachments] count]);
     }
+  }
 }