Remove overflow menu for single notification action on macOS

If a notification does not need the Settings button and only has one
action button we don't need the overflow menu as it would only have a
single entry. Instead just show the action as the second button in the
notification.

Before: https://imgur.com/SUvMXlW
After: https://imgur.com/QRijUhf

Bug: 1139892
Change-Id: Ia8b95aeffcea95345a9d483d065f818d03584939
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485153
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818563}

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

@@ -83,24 +83,39 @@
                                 objectForKey:notification_constants::
                                                  kNotificationCloseButtonTag]];
 
-    // Display the Settings button as the action button if there are either no
-    // developer-provided action buttons, or the alternate action menu is not
-    // available on this Mac version. This avoids needlessly showing the menu.
-    if (![_notificationData
-            objectForKey:notification_constants::kNotificationButtonOne] ||
-        ![toast respondsToSelector:@selector(_alwaysShowAlternateActionMenu)]) {
-      if (settingsButton) {
-        [toast setActionButtonTitle:
-                   [_notificationData
-                       objectForKey:notification_constants::
-                                        kNotificationSettingsButtonTag]];
-      } else {
-        [toast setHasActionButton:NO];
+    NSMutableArray* buttons = [NSMutableArray arrayWithCapacity:3];
+    if ([_notificationData
+            objectForKey:notification_constants::kNotificationButtonOne]) {
+      [buttons addObject:[_notificationData
+                             objectForKey:notification_constants::
+                                              kNotificationButtonOne]];
+    }
+    if ([_notificationData
+            objectForKey:notification_constants::kNotificationButtonTwo]) {
+      [buttons addObject:[_notificationData
+                             objectForKey:notification_constants::
+                                              kNotificationButtonTwo]];
+    }
+    if (settingsButton) {
+      // If we can't show an action menu but need a settings button, only show
+      // the settings button and don't show developer provided actions.
+      if (![toast
+              respondsToSelector:@selector(_alwaysShowAlternateActionMenu)]) {
+        [buttons removeAllObjects];
       }
+      [buttons addObject:[_notificationData
+                             objectForKey:notification_constants::
+                                              kNotificationSettingsButtonTag]];
+    }
 
+    if ([buttons count] == 0) {
+      // Don't show action button if no actions needed.
+      [toast setHasActionButton:NO];
+    } else if ([buttons count] == 1) {
+      // Only one action so we don't need a menu. Just set the button title.
+      [toast setActionButtonTitle:[buttons firstObject]];
     } else {
-      // Otherwise show the alternate menu, then show the developer actions and
-      // finally the settings one if needed.
+      // Show the alternate menu with developer actions and settings if needed.
       DCHECK(
           [toast respondsToSelector:@selector(_alwaysShowAlternateActionMenu)]);
       DCHECK(
@@ -110,24 +125,6 @@
                      objectForKey:notification_constants::
                                       kNotificationOptionsButtonTag]];
       [toast setValue:@YES forKey:@"_alwaysShowAlternateActionMenu"];
-
-      NSMutableArray* buttons = [NSMutableArray arrayWithCapacity:3];
-      [buttons addObject:[_notificationData
-                             objectForKey:notification_constants::
-                                              kNotificationButtonOne]];
-      if ([_notificationData
-              objectForKey:notification_constants::kNotificationButtonTwo]) {
-        [buttons addObject:[_notificationData
-                               objectForKey:notification_constants::
-                                                kNotificationButtonTwo]];
-      }
-      if (settingsButton) {
-        [buttons
-            addObject:[_notificationData
-                          objectForKey:notification_constants::
-                                           kNotificationSettingsButtonTag]];
-      }
-
       [toast setValue:buttons forKey:@"_alternateActionButtonTitles"];
     }
   }

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

@@ -152,6 +152,34 @@ TEST(NotificationBuilderMacTest, TestNotificationExtensionNoButtons) {
   EXPECT_EQ("Close", base::SysNSStringToUTF8([notification otherButtonTitle]));
 }
 
+TEST(NotificationBuilderMacTest, TestNotificationExtensionOneButton) {
+  base::scoped_nsobject<NotificationBuilder> builder(
+      [[NotificationBuilder alloc] initWithCloseLabel:@"Close"
+                                         optionsLabel:@"Options"
+                                        settingsLabel:@"Settings"]);
+  [builder setTitle:@"Title"];
+  [builder setSubTitle:@"https://www.miguel.com"];
+  [builder setContextMessage:@"SubTitle"];
+  [builder setButtons:@"Button1" secondaryButton:@""];
+  [builder setNotificationId:@"notificationId"];
+  [builder setProfileId:@"profileId"];
+  [builder setIncognito:false];
+  [builder setCreatorPid:@1];
+  [builder setNotificationType:[NSNumber
+                                   numberWithInteger:static_cast<int>(
+                                                         NotificationHandler::
+                                                             Type::EXTENSION)]];
+  [builder setShowSettingsButton:false];
+
+  NSUserNotification* notification = [builder buildUserNotification];
+
+  // No settings button but one action button without overflow menu.
+  EXPECT_TRUE([notification hasActionButton]);
+  EXPECT_EQ("Button1",
+            base::SysNSStringToUTF8([notification actionButtonTitle]));
+  EXPECT_EQ("Close", base::SysNSStringToUTF8([notification otherButtonTitle]));
+}
+
 TEST(NotificationBuilderMacTest, TestNotificationExtensionButtons) {
   base::scoped_nsobject<NotificationBuilder> builder(
       [[NotificationBuilder alloc] initWithCloseLabel:@"Close"

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

@@ -53,37 +53,30 @@
   // Determine whether the user clicked on a button, and if they did, whether it
   // was a developer-provided button or the  Settings button.
   if (activationType == NSUserNotificationActivationTypeActionButtonClicked) {
-    NSArray* alternateButtons = @[];
+    int buttonCount = 1;
     if ([notification
             respondsToSelector:@selector(_alternateActionButtonTitles)]) {
-      alternateButtons =
-          [notification valueForKey:@"_alternateActionButtonTitles"];
+      buttonCount =
+          [[notification valueForKey:@"_alternateActionButtonTitles"] count];
+    }
+
+    if (buttonCount > 1) {
+      // There are multiple buttons in the overflow menu. Get the clicked index.
+      buttonIndex =
+          [[notification valueForKey:@"_alternateActionIndex"] intValue];
+    } else {
+      // There was only one button so we know it was clicked.
+      buttonIndex = 0;
+      buttonCount = 1;
     }
 
     BOOL settingsButtonRequired = [hasSettingsButton boolValue];
-    BOOL multipleButtons = (alternateButtons.count > 0);
+    BOOL clickedLastButton = buttonIndex == buttonCount - 1;
 
-    // No developer actions, just the settings button.
-    if (!multipleButtons) {
-      DCHECK(settingsButtonRequired);
+    // The settings button is always the last button if present.
+    if (clickedLastButton && settingsButtonRequired) {
       operation = NotificationOperation::NOTIFICATION_SETTINGS;
       buttonIndex = notification_constants::kNotificationInvalidButtonIndex;
-    } else {
-      // 0 based array containing.
-      // Button 1
-      // Button 2 (optional)
-      // Settings (if required)
-      NSNumber* actionIndex =
-          [notification valueForKey:@"_alternateActionIndex"];
-      operation = settingsButtonRequired && (actionIndex.unsignedLongValue ==
-                                             alternateButtons.count - 1)
-                      ? NotificationOperation::NOTIFICATION_SETTINGS
-                      : NotificationOperation::NOTIFICATION_CLICK;
-      buttonIndex =
-          settingsButtonRequired &&
-                  (actionIndex.unsignedLongValue == alternateButtons.count - 1)
-              ? notification_constants::kNotificationInvalidButtonIndex
-              : actionIndex.intValue;
     }
   }
 

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

@@ -142,6 +142,30 @@ TEST_F(NotificationResponseBuilderMacTest, TestNotificationOneActionClick) {
   EXPECT_EQ(0, buttonIndex.intValue);
 }
 
+TEST_F(NotificationResponseBuilderMacTest,
+       TestNotificationOneActionNoSettingsClick) {
+  base::scoped_nsobject<NotificationBuilder> builder =
+      NewTestBuilder(NotificationHandler::Type::EXTENSION);
+  [builder setButtons:@"Button1" secondaryButton:@""];
+
+  NSUserNotification* notification = [builder buildUserNotification];
+
+  // This will be set by the notification center to indicate the only available
+  // button was clicked.
+  [notification setValue:@(NSUserNotificationActivationTypeActionButtonClicked)
+                  forKey:@"_activationType"];
+  NSDictionary* response =
+      [NotificationResponseBuilder buildActivatedDictionary:notification];
+
+  NSNumber* operation =
+      [response objectForKey:notification_constants::kNotificationOperation];
+  NSNumber* buttonIndex =
+      [response objectForKey:notification_constants::kNotificationButtonIndex];
+  EXPECT_EQ(static_cast<int>(NotificationOperation::NOTIFICATION_CLICK),
+            operation.intValue);
+  EXPECT_EQ(0, buttonIndex.intValue);
+}
+
 TEST_F(NotificationResponseBuilderMacTest, TestNotificationTwoActionClick) {
   base::scoped_nsobject<NotificationBuilder> builder =
       NewTestBuilder(NotificationHandler::Type::WEB_PERSISTENT);