[extensions] Fix context menu radio button update

Currently, if there are multiple radio buttons in a context menu and an
unchecked button is updated to checked, nothing happens.

Instead what should happen is that the updated radio button should be checked,
and all other radio buttons should be unchecked. This CL implements this fix.

Bug: 129421
Change-Id: Ic2301a7191efef6a4a712ab400339e897cb6a5cb
Reviewed-on: https://chromium-review.googlesource.com/576271
Commit-Queue: catmullings <catmullings@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490662}

chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h

@@ -206,7 +206,11 @@ bool UpdateMenuItem(const PropertyWithEnumT& update_properties,
       *error = kCheckedError;
       return false;
     }
-    if (checked != item->checked()) {
+    // If the item was not checked and it is updated to be checked, set it to be
+    // checked. If the radio item was unchecked, nothing should happen. The
+    // radio item should remain checked because there should always be one item
+    // checked in the radio list.
+    if (checked && !item->checked()) {
       if (!item->SetChecked(checked)) {
         *error = kCheckedError;
         return false;

chrome/browser/extensions/extension_context_menu_browsertest.cc

@@ -224,6 +224,30 @@ class ExtensionContextMenuBrowserTest : public ExtensionBrowserTest {
     }
     return false;
   }
+
+  // Given a menu item id, executes the item's command.
+  void ExecuteCommand(TestRenderViewContextMenu* menu,
+                      const extensions::ExtensionId& extension_id,
+                      const std::string& item_uid) {
+    MenuItem::Id id(false, MenuItem::ExtensionKey(extension_id));
+    id.string_uid = item_uid;
+    int command_id = -1;
+    ASSERT_TRUE(FindCommandId(menu, id, &command_id));
+    menu->ExecuteCommand(command_id, 0);
+  }
+
+  // Verifies a radio item's selection state (checked or unchecked).
+  void VerifyRadioItemSelectionState(
+      TestRenderViewContextMenu* menu,
+      const extensions::ExtensionId& extension_id,
+      const std::string& radio_uid,
+      bool should_be_checked) {
+    MenuItem::Id id(false, MenuItem::ExtensionKey(extension_id));
+    id.string_uid = radio_uid;
+    int command_id = -1;
+    ASSERT_TRUE(FindCommandId(menu, id, &command_id));
+    EXPECT_EQ(should_be_checked, menu->IsCommandIdChecked(command_id));
+  }
 };
 
 // Tests adding a simple context menu item.
@@ -300,6 +324,116 @@ IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest, UpdateOnclick) {
   ASSERT_FALSE(listener_error2.was_satisfied());
 }
 
+// Tests that updating the first radio item in a radio list from checked to
+// unchecked should not work. The radio button should remain checked because
+// context menu radio lists should always have one item selected.
+IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest,
+                       UpdateCheckedStateOfFirstRadioItem) {
+  ExtensionTestMessageListener listener_created_radio1("created radio1 item",
+                                                       false);
+  ExtensionTestMessageListener listener_created_radio2("created radio2 item",
+                                                       false);
+  ExtensionTestMessageListener listener_created_item1("created normal item",
+                                                      false);
+  ExtensionTestMessageListener listener_created_item2(
+      "created second normal item", false);
+
+  ExtensionTestMessageListener listener_radio2_clicked("onclick radio2", false);
+  ExtensionTestMessageListener listener_item1_clicked("onclick normal item",
+                                                      false);
+  ExtensionTestMessageListener listener_radio1_updated("radio1 updated", false);
+
+  const extensions::Extension* extension =
+      LoadContextMenuExtension("radio_check");
+  ASSERT_TRUE(extension);
+
+  // Check that all menu items are created.
+  ASSERT_TRUE(listener_created_radio1.WaitUntilSatisfied());
+  ASSERT_TRUE(listener_created_radio2.WaitUntilSatisfied());
+  ASSERT_TRUE(listener_created_item1.WaitUntilSatisfied());
+  ASSERT_TRUE(listener_created_item2.WaitUntilSatisfied());
+
+  GURL page_url("http://www.google.com");
+
+  // Create and build our test context menu.
+  std::unique_ptr<TestRenderViewContextMenu> menu(
+      TestRenderViewContextMenu::Create(GetWebContents(), page_url, GURL(),
+                                        GURL()));
+
+  VerifyRadioItemSelectionState(menu.get(), extension->id(), "radio1", true);
+  VerifyRadioItemSelectionState(menu.get(), extension->id(), "radio2", false);
+
+  // Clicking the regular item calls chrome.contextMenus.update to uncheck the
+  // first radio item.
+  ExecuteCommand(menu.get(), extension->id(), "item1");
+  ASSERT_TRUE(listener_item1_clicked.WaitUntilSatisfied());
+
+  // Unchecking the second item should not deselect it. Recall that context menu
+  // radio lists should always have one item selected.
+  ASSERT_TRUE(listener_radio1_updated.WaitUntilSatisfied());
+  VerifyRadioItemSelectionState(menu.get(), extension->id(), "radio1", true);
+  VerifyRadioItemSelectionState(menu.get(), extension->id(), "radio2", false);
+}
+
+// Tests that updating a checked radio button (that is not the first item) to be
+// unchecked should not work. The radio button should remain checked because
+// context menu radio lists should always have one item selected.
+IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest,
+                       UpdateCheckedStateOfNonfirstRadioItem) {
+  ExtensionTestMessageListener listener_created_radio1("created radio1 item",
+                                                       false);
+  ExtensionTestMessageListener listener_created_radio2("created radio2 item",
+                                                       false);
+  ExtensionTestMessageListener listener_created_item1("created normal item",
+                                                      false);
+  ExtensionTestMessageListener listener_created_item2(
+      "created second normal item", false);
+
+  ExtensionTestMessageListener listener_radio2_clicked("onclick radio2", false);
+  ExtensionTestMessageListener listener_item2_clicked(
+      "onclick second normal item", false);
+  ExtensionTestMessageListener listener_radio2_updated("radio2 updated", false);
+
+  const extensions::Extension* extension =
+      LoadContextMenuExtension("radio_check");
+  ASSERT_TRUE(extension);
+
+  // Check that all menu items are created.
+  ASSERT_TRUE(listener_created_radio1.WaitUntilSatisfied());
+  ASSERT_TRUE(listener_created_radio2.WaitUntilSatisfied());
+  ASSERT_TRUE(listener_created_item1.WaitUntilSatisfied());
+
+  ASSERT_TRUE(listener_created_item2.WaitUntilSatisfied());
+
+  GURL page_url("http://www.google.com");
+
+  // Create and build our test context menu.
+  std::unique_ptr<TestRenderViewContextMenu> menu(
+      TestRenderViewContextMenu::Create(GetWebContents(), page_url, GURL(),
+                                        GURL()));
+
+  VerifyRadioItemSelectionState(menu.get(), extension->id(), "radio1", true);
+  VerifyRadioItemSelectionState(menu.get(), extension->id(), "radio2", false);
+
+  // Click on the second radio button. This should uncheck the first radio item
+  // and check the second item.
+  ExecuteCommand(menu.get(), extension->id(), "radio2");
+  ASSERT_TRUE(listener_radio2_clicked.WaitUntilSatisfied());
+  VerifyRadioItemSelectionState(menu.get(), extension->id(), "radio1", false);
+  VerifyRadioItemSelectionState(menu.get(), extension->id(), "radio2", true);
+
+  // Clicking the regular item calls chrome.contextMenus.update to uncheck the
+  // second radio item.
+  ExecuteCommand(menu.get(), extension->id(), "item2");
+  ASSERT_TRUE(listener_item2_clicked.WaitUntilSatisfied());
+
+  // Unchecking the second item should not deselect it. Recall that context menu
+  // radio lists should always have one item selected.
+  ASSERT_TRUE(listener_radio2_updated.WaitUntilSatisfied());
+  VerifyRadioItemSelectionState(menu.get(), extension->id(), "radio1", false);
+  VerifyRadioItemSelectionState(menu.get(), extension->id(), "radio2", true);
+}
+
 // Tests that setting "documentUrlPatterns" for an item properly restricts
 // those items to matching pages.
 IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest, Patterns) {

chrome/browser/extensions/menu_manager.cc

@@ -743,7 +743,6 @@ void MenuManager::SanitizeRadioList(const MenuItem::OwnedList& item_list) {
   auto i = item_list.begin();
   while (i != item_list.end()) {
     if ((*i)->type() != MenuItem::RADIO) {
-      ++i;
       break;
     }
 
@@ -780,17 +779,22 @@ bool MenuManager::ItemUpdated(const MenuItem::Id& id) {
   MenuItem* menu_item = GetItemById(id);
   DCHECK(menu_item);
 
+  const extensions::MenuItem::OwnedList* list;
   if (menu_item->parent_id()) {
-    SanitizeRadioList(GetItemById(*menu_item->parent_id())->children());
+    list = &(GetItemById(*menu_item->parent_id())->children());
   } else {
     auto i = context_items_.find(menu_item->id().extension_key);
     if (i == context_items_.end()) {
       NOTREACHED();
       return false;
     }
-    SanitizeRadioList(i->second);
+    list = &(i->second);
   }
 
+  // If we selected a radio item, unselect all other items in its group.
+  if (menu_item->type() == MenuItem::RADIO && menu_item->checked())
+    RadioItemSelected(menu_item);
+
   return true;
 }
 

chrome/browser/extensions/menu_manager_unittest.cc

@@ -663,14 +663,24 @@ TEST_F(MenuManagerTest, SanitizeRadioButtons) {
   ASSERT_TRUE(item1_ptr->checked());
   ASSERT_FALSE(item2_ptr->checked());
 
-  // If multiple items are checked, only the last item should get checked.
+  // If multiple items are checked and one of the items is updated to be
+  // checked, then all other items should be unchecked.
+  //
+  // Note, this case of multiple checked items (i.e. SetChecked() called more
+  // than once) followed by a call to ItemUpdated() would never happen in
+  // practice. In this hypothetical scenario, the item that was updated the
+  // latest via ItemUpdated() should remain checked.
+  //
+  // Begin with two items checked.
   item1_ptr->SetChecked(true);
   item2_ptr->SetChecked(true);
   ASSERT_TRUE(item1_ptr->checked());
   ASSERT_TRUE(item2_ptr->checked());
+  // Updating item1 to be checked should result in item2 being unchecked.
   manager_.ItemUpdated(item1_ptr->id());
-  ASSERT_FALSE(item1_ptr->checked());
-  ASSERT_TRUE(item2_ptr->checked());
+  // Item 1 should be selected as it was updated the latest.
+  ASSERT_TRUE(item1_ptr->checked());
+  ASSERT_FALSE(item2_ptr->checked());
 
   // If the checked item is removed, the new first item should get checked.
   item1_ptr->SetChecked(false);

chrome/test/data/extensions/context_menus/radio_check/manifest.json

+{
+  "name" : "Context Menus Test Extension",
+  "version" : "0.1",
+  "manifest_version": 2,
+  "permissions": [ "contextMenus", "tabs" ],
+  "background": {
+    "scripts": ["test.js"]
+  }
+}

chrome/test/data/extensions/context_menus/radio_check/test.js

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+window.onload = function() {
+  chrome.contextMenus.create({
+    id: 'radio1',
+    type: 'radio',
+    title: 'Radio 1',
+    onclick: function() {
+      chrome.test.sendMessage('onclick radio1');
+    }
+  }, function() {
+    chrome.test.sendMessage('created radio1 item', function() {
+      createSecondRadioButton();
+    });
+  });
+};
+
+function createSecondRadioButton() {
+  chrome.contextMenus.create({
+    id: 'radio2',
+    type: 'radio',
+    title: 'Radio 2',
+    onclick: function() {
+      chrome.test.sendMessage('onclick radio2');
+    }
+  }, function() {
+    chrome.test.sendMessage('created radio2 item', function() {
+      createNormalMenuItem();
+    });
+  });
+}
+
+function createNormalMenuItem() {
+  chrome.contextMenus.create({
+    id: 'item1',
+    title: 'Item 1',
+    onclick: function() {
+      chrome.test.sendMessage('onclick normal item');
+      chrome.contextMenus.update('radio1', {checked: false}, function() {
+        chrome.test.sendMessage('radio1 updated');
+      });
+    }
+  }, function() {
+    chrome.test.sendMessage('created normal item', function() {
+      createSecondNormalMenuItem();
+    });
+  });
+}
+
+function createSecondNormalMenuItem() {
+  chrome.contextMenus.create({
+    id: 'item2',
+    title: 'Item 2',
+    onclick: function() {
+      chrome.test.sendMessage('onclick second normal item');
+      chrome.contextMenus.update('radio2', {checked: false}, function() {
+        chrome.test.sendMessage('radio2 updated');
+      });
+    }
+  }, function() {
+    chrome.test.sendMessage('created second normal item');
+  });
+}