Make UX-requested updates to Translate bubble.

* Rename "More Options" back to "Options" (distinguisher no longer
  necessary).
* Update in-progress title to "Translating...".
* Update done title to "Translated".
* Show "Options" menu from done panel.
* Move "Always translate" checkbox inside advanced panel.
* Remove "Language settings" link from advanced panel (not slated for
  updated UI either).
* Update "never" options to state "Don't translate X" instead of "Never
  translate X".

Also replaces the views::MenuButton with a Harmony-compliant bubble and
updates the option menu on every click so any language changes done
inside the advanced panel are reflected in the menu outside.

TBR=tapted@chromium.org

Bug: chromium:730521
Change-Id: I2e05bdacfb7c9e487e4f7f32e7934ba521ef54bd
Reviewed-on: https://chromium-review.googlesource.com/832850
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526085}

chrome/app/generated_resources.grd

@@ -8509,7 +8509,7 @@ Please help our engineers fix this problem. Tell us what happened right before y
             Options
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_ADVANCED_BUTTON" desc="Text to show for the translate bubble button to jump to the advanced panel.">
-            More options
+            Options
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_ADVANCED_MENU_BUTTON" desc="Text to show for the translate bubble menu button to jump to the advanced panel, specifically to change which languages are translated between.">
             Change languages
@@ -8533,13 +8533,13 @@ Please help our engineers fix this problem. Tell us what happened right before y
             Never translate this site
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_TRANSLATING" desc="Text to show for the translate bubble label when page is currently being translated by servers">
-            This page is being translated...
+            Translating...
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_TRANSLATED" desc="Text to show for the translate bubble label when the page has been translated from one language to another">
             This page has been translated.
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_TRANSLATED_TITLE" desc="Title text for the translate bubble label when the page has been translated from one language to another">
-            This page has been translated
+            Translated
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_REVERT" desc="Text to show for the translate bubble button to revert translation of translated page">
             Show original
@@ -8583,7 +8583,7 @@ Please help our engineers fix this problem. Tell us what happened right before y
             Options
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_ADVANCED_BUTTON" desc="In Title Case: Text to show for the translate bubble button to jump to the advanced panel.">
-            More Options
+            Options
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_ADVANCED_MENU_BUTTON" desc="In Title Case: Text to show for the translate bubble menu button to jump to the advanced panel, specifically to change which languages are translated between.">
             Change Languages
@@ -8607,13 +8607,13 @@ Please help our engineers fix this problem. Tell us what happened right before y
             Never Translate This Site
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_TRANSLATING" desc="In Title Case: Text to show for the translate bubble label when page is currently being translated by servers">
-            This Page Is Being Translated...
+            Translating...
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_TRANSLATED" desc="In Title Case: Text to show for the translate bubble label when the page has been translated from one language to another">
             This Page Has Been Translated.
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_TRANSLATED_TITLE" desc="In Title Case: Title text for the translate bubble label when the page has been translated from one language to another">
-            This Page Has Been Translated
+            Translated
           </message>
           <message name="IDS_TRANSLATE_BUBBLE_REVERT" desc="In Title Case: Text to show for the translate bubble button to revert translation of translated page">
             Show Original

chrome/browser/ui/cocoa/translate/translate_bubble_test_utils_views_cocoa.mm

@@ -11,6 +11,8 @@
 #include "chrome/browser/ui/cocoa/translate/translate_bubble_controller.h"
 #include "chrome/browser/ui/translate/translate_bubble_model.h"
 #include "chrome/browser/ui/views/translate/translate_bubble_view.h"
+#include "ui/events/keycodes/dom/dom_code.h"
+#include "ui/views/controls/button/label_button.h"
 
 // TODO(groby): Share with translate_bubble_controller_unittest.mm
 @implementation BrowserWindowController (ForTesting)
@@ -44,7 +46,13 @@ void PressTranslate(Browser* browser) {
   if (chrome::ShowAllDialogsWithViewsToolkit()) {
     TranslateBubbleView* bubble = TranslateBubbleView::GetCurrentBubble();
     DCHECK(bubble);
-    bubble->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_TRANSLATE);
+
+    views::LabelButton button(nullptr, base::string16());
+    button.set_id(TranslateBubbleView::BUTTON_ID_TRANSLATE);
+
+    bubble->ButtonPressed(&button,
+                          ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_RETURN,
+                                       ui::DomCode::ENTER, ui::EF_NONE));
     return;
   }
 

chrome/browser/ui/views/translate/translate_bubble_test_utils_views.cc

@@ -7,6 +7,8 @@
 #include "base/logging.h"
 #include "chrome/browser/ui/translate/translate_bubble_model.h"
 #include "chrome/browser/ui/views/translate/translate_bubble_view.h"
+#include "ui/events/keycodes/dom/dom_code.h"
+#include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/combobox/combobox.h"
 
 namespace translate {
@@ -23,14 +25,26 @@ void PressTranslate(Browser* browser) {
   DCHECK(browser);
   TranslateBubbleView* bubble = TranslateBubbleView::GetCurrentBubble();
   DCHECK(bubble);
-  bubble->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_TRANSLATE);
+
+  views::LabelButton button(nullptr, base::string16());
+  button.set_id(TranslateBubbleView::BUTTON_ID_TRANSLATE);
+
+  bubble->ButtonPressed(&button,
+                        ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_RETURN,
+                                     ui::DomCode::ENTER, ui::EF_NONE));
 }
 
 void PressRevert(Browser* browser) {
   DCHECK(browser);
   TranslateBubbleView* bubble = TranslateBubbleView::GetCurrentBubble();
   DCHECK(bubble);
-  bubble->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_SHOW_ORIGINAL);
+
+  views::LabelButton button(nullptr, base::string16());
+  button.set_id(TranslateBubbleView::BUTTON_ID_SHOW_ORIGINAL);
+
+  bubble->ButtonPressed(&button,
+                        ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_RETURN,
+                                     ui::DomCode::ENTER, ui::EF_NONE));
 }
 
 void SelectTargetLanguageByDisplayName(Browser* browser,

chrome/browser/ui/views/translate/translate_bubble_view.h

@@ -40,13 +40,12 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
                             public views::ButtonListener,
                             public views::ComboboxListener,
                             public views::LinkListener,
-                            public views::MenuButtonListener,
                             public ui::SimpleMenuModel::Delegate,
                             public views::StyledLabelListener,
                             public content::WebContentsObserver {
  public:
-  // Item IDs for the denial button's menu.
-  enum DenialMenuItem {
+  // Item IDs for the option button's menu.
+  enum OptionsMenuItem {
     ALWAYS_TRANSLATE_LANGUAGE,
     NEVER_TRANSLATE_LANGUAGE,
     NEVER_TRANSLATE_SITE,
@@ -97,11 +96,6 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
   // views::LinkListener method.
   void LinkClicked(views::Link* source, int event_flags) override;
 
-  // views::MenuButtonListener method.
-  void OnMenuButtonClicked(views::MenuButton* source,
-                           const gfx::Point& point,
-                           const ui::Event* event) override;
-
   // ui::SimpleMenuModel::Delegate methods.
   bool IsCommandIdChecked(int command_id) const override;
   bool IsCommandIdEnabled(int command_id) const override;
@@ -128,7 +122,6 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
  private:
   enum LinkID {
     LINK_ID_ADVANCED,
-    LINK_ID_LANGUAGE_SETTINGS,
   };
 
   enum ButtonID {
@@ -139,6 +132,7 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
     BUTTON_ID_TRY_AGAIN,
     BUTTON_ID_ALWAYS_TRANSLATE,
     BUTTON_ID_ADVANCED,
+    BUTTON_ID_OPTIONS_MENU,
   };
 
   enum ComboboxID {
@@ -173,11 +167,11 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
                            CancelButtonReturningAfterTranslate);
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest, CancelButtonReturningError);
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
-                           DenialMenuNeverTranslateLanguage);
+                           OptionsMenuNeverTranslateLanguage);
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
-                           DenialMenuRespectsBlacklistSite);
+                           OptionsMenuRespectsBlacklistSite);
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
-                           DenialMenuNeverTranslateSite);
+                           OptionsMenuNeverTranslateSite);
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
                            AlwaysTranslateLanguageMenuItem);
   FRIEND_TEST_ALL_PREFIXES(TranslateLanguageBrowserTest, TranslateAndRevert);
@@ -193,8 +187,8 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
   // Returns the current child view.
   views::View* GetCurrentView() const;
 
-  // Handles the event when the user presses a button.
-  void HandleButtonPressed(ButtonID sender_id);
+  // Triggers options menu.
+  void ShowOptionsMenu(views::Button* source);
 
   // Handles the event when the user clicks a link.
   void HandleLinkClicked(LinkID sender_id);
@@ -235,6 +229,11 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
   // Updates the advanced view.
   void UpdateAdvancedView();
 
+  // Actions for button presses shared with accelerators.
+  void Translate();
+  void ShowOriginal();
+  void ConfirmAdvancedOptions();
+
   static TranslateBubbleView* translate_bubble_view_;
 
   views::View* before_translate_view_;
@@ -255,7 +254,8 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
   views::LabelButton* advanced_cancel_button_;
   views::LabelButton* advanced_done_button_;
 
-  views::MenuButton* options_menu_button_;
+  // Used to trigger the options menu in tests.
+  views::Button* before_translate_options_button_;
   std::unique_ptr<ui::SimpleMenuModel> options_menu_model_;
   std::unique_ptr<views::MenuRunner> options_menu_runner_;
 

chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc

@@ -183,6 +183,15 @@ class TranslateBubbleViewTest : public views::ViewsTestBase {
     views::BubbleDialogDelegateView::CreateBubble(bubble_)->Show();
   }
 
+  void PressButton(TranslateBubbleView::ButtonID id) {
+    views::LabelButton button(nullptr, base::ASCIIToUTF16("hello"));
+    button.set_id(id);
+
+    bubble_->ButtonPressed(&button,
+                           ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_RETURN,
+                                        ui::DomCode::ENTER, ui::EF_NONE));
+  }
+
   void TearDown() override {
     bubble_->GetWidget()->CloseNow();
     anchor_widget_.reset();
@@ -192,8 +201,9 @@ class TranslateBubbleViewTest : public views::ViewsTestBase {
 
   bool denial_button_clicked() { return mock_model_->translation_declined_; }
   void TriggerOptionsMenu() {
-    bubble_->options_menu_button_->OnKeyPressed(ui::KeyEvent(
-        ui::ET_KEY_PRESSED, ui::VKEY_RETURN, ui::DomCode::ENTER, ui::EF_NONE));
+    bubble_->ButtonPressed(bubble_->before_translate_options_button_,
+                           ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_RETURN,
+                                        ui::DomCode::ENTER, ui::EF_NONE));
   }
 
   ui::SimpleMenuModel* options_menu_model() {
@@ -211,7 +221,7 @@ TEST_F(TranslateBubbleViewTest, TranslateButton) {
   EXPECT_FALSE(mock_model_->translate_called_);
 
   // Press the "Translate" button.
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_TRANSLATE);
+  PressButton(TranslateBubbleView::BUTTON_ID_TRANSLATE);
   EXPECT_TRUE(mock_model_->translate_called_);
 }
 
@@ -221,7 +231,7 @@ TEST_F(TranslateBubbleViewTest, TranslateButtonIn2016Q2UI) {
   EXPECT_FALSE(mock_model_->translate_called_);
 
   // Press the "Translate" button.
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_TRANSLATE);
+  PressButton(TranslateBubbleView::BUTTON_ID_TRANSLATE);
   EXPECT_TRUE(mock_model_->translate_called_);
 }
 
@@ -259,7 +269,7 @@ TEST_F(TranslateBubbleViewTest, CloseButton) {
   EXPECT_TRUE(mock_model_->translation_declined_);
 }
 
-TEST_F(TranslateBubbleViewTest, DenialMenuNeverTranslateLanguage) {
+TEST_F(TranslateBubbleViewTest, OptionsMenuNeverTranslateLanguage) {
   CreateAndShowBubble();
 
   EXPECT_FALSE(bubble_->GetWidget()->IsClosed());
@@ -276,7 +286,7 @@ TEST_F(TranslateBubbleViewTest, DenialMenuNeverTranslateLanguage) {
   EXPECT_TRUE(bubble_->GetWidget()->IsClosed());
 }
 
-TEST_F(TranslateBubbleViewTest, DenialMenuNeverTranslateSite) {
+TEST_F(TranslateBubbleViewTest, OptionsMenuNeverTranslateSite) {
   // NEVER_TRANSLATE_SITE should only show up for sites that can be blacklisted.
   mock_model_->SetCanBlacklistSite(true);
   CreateAndShowBubble();
@@ -303,7 +313,7 @@ TEST_F(TranslateBubbleViewTest, MenuButtonNeverTranslateLanguage) {
   EXPECT_FALSE(denial_button_clicked());
 
   bubble_->ExecuteCommand(
-      TranslateBubbleView::DenialMenuItem::NEVER_TRANSLATE_LANGUAGE, 0);
+      TranslateBubbleView::OptionsMenuItem::NEVER_TRANSLATE_LANGUAGE, 0);
 
   EXPECT_TRUE(denial_button_clicked());
   EXPECT_TRUE(mock_model_->never_translate_language_);
@@ -318,7 +328,7 @@ TEST_F(TranslateBubbleViewTest, MenuButtonNeverTranslateSite) {
   EXPECT_FALSE(bubble_->GetWidget()->IsClosed());
 
   bubble_->ExecuteCommand(
-      TranslateBubbleView::DenialMenuItem::NEVER_TRANSLATE_SITE, 0);
+      TranslateBubbleView::OptionsMenuItem::NEVER_TRANSLATE_SITE, 0);
 
   EXPECT_TRUE(denial_button_clicked());
   EXPECT_TRUE(mock_model_->never_translate_site_);
@@ -353,7 +363,7 @@ TEST_F(TranslateBubbleViewTest, ShowOriginalButton) {
 
   // Click the "Show original" button to revert translation.
   EXPECT_FALSE(mock_model_->revert_translation_called_);
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_SHOW_ORIGINAL);
+  PressButton(TranslateBubbleView::BUTTON_ID_SHOW_ORIGINAL);
   EXPECT_TRUE(mock_model_->revert_translation_called_);
 }
 
@@ -365,7 +375,7 @@ TEST_F(TranslateBubbleViewTest, TryAgainButton) {
 
   // Click the "Try again" button to translate.
   EXPECT_FALSE(mock_model_->translate_called_);
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_TRY_AGAIN);
+  PressButton(TranslateBubbleView::BUTTON_ID_TRY_AGAIN);
   EXPECT_TRUE(mock_model_->translate_called_);
 }
 
@@ -383,12 +393,12 @@ TEST_F(TranslateBubbleViewTest, AlwaysTranslateCheckboxAndCancelButton) {
 
   // Click the checkbox. The state is not saved yet.
   bubble_->advanced_always_translate_checkbox_->SetChecked(true);
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_ALWAYS_TRANSLATE);
+  PressButton(TranslateBubbleView::BUTTON_ID_ALWAYS_TRANSLATE);
   EXPECT_FALSE(mock_model_->should_always_translate_);
   EXPECT_EQ(0, mock_model_->set_always_translate_called_count_);
 
   // Click the cancel button. The state is not saved.
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_CANCEL);
+  PressButton(TranslateBubbleView::BUTTON_ID_CANCEL);
   EXPECT_FALSE(mock_model_->should_always_translate_);
   EXPECT_EQ(0, mock_model_->set_always_translate_called_count_);
 }
@@ -407,12 +417,12 @@ TEST_F(TranslateBubbleViewTest, AlwaysTranslateCheckboxAndDoneButton) {
 
   // Click the checkbox. The state is not saved yet.
   bubble_->advanced_always_translate_checkbox_->SetChecked(true);
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_ALWAYS_TRANSLATE);
+  PressButton(TranslateBubbleView::BUTTON_ID_ALWAYS_TRANSLATE);
   EXPECT_FALSE(mock_model_->should_always_translate_);
   EXPECT_EQ(0, mock_model_->set_always_translate_called_count_);
 
   // Click the done button. The state is saved.
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_DONE);
+  PressButton(TranslateBubbleView::BUTTON_ID_DONE);
   EXPECT_TRUE(mock_model_->should_always_translate_);
   EXPECT_EQ(1, mock_model_->set_always_translate_called_count_);
 }
@@ -430,7 +440,7 @@ TEST_F(TranslateBubbleViewTest, DoneButton) {
   bubble_->target_language_combobox_->SetSelectedIndex(20);
   bubble_->HandleComboboxPerformAction(
       TranslateBubbleView::COMBOBOX_ID_TARGET_LANGUAGE);
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_DONE);
+  PressButton(TranslateBubbleView::BUTTON_ID_DONE);
   EXPECT_TRUE(mock_model_->translate_called_);
   EXPECT_EQ(10, mock_model_->original_language_index_);
   EXPECT_EQ(20, mock_model_->target_language_index_);
@@ -457,7 +467,7 @@ TEST_F(TranslateBubbleViewTest, DoneButtonWithoutTranslating) {
   // Click the "Done" button with the current language pair. This time,
   // translation is not performed and the view state will be back to the
   // previous view.
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_DONE);
+  PressButton(TranslateBubbleView::BUTTON_ID_DONE);
   EXPECT_FALSE(mock_model_->translate_called_);
 
   EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
@@ -471,7 +481,7 @@ TEST_F(TranslateBubbleViewTest, CancelButtonReturningBeforeTranslate) {
 
   // Click the "Cancel" button to go back.
   EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_ADVANCED, bubble_->GetViewState());
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_CANCEL);
+  PressButton(TranslateBubbleView::BUTTON_ID_CANCEL);
   EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
             bubble_->GetViewState());
 }
@@ -483,7 +493,7 @@ TEST_F(TranslateBubbleViewTest, CancelButtonReturningAfterTranslate) {
 
   // Click the "Cancel" button to go back.
   EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_ADVANCED, bubble_->GetViewState());
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_CANCEL);
+  PressButton(TranslateBubbleView::BUTTON_ID_CANCEL);
   EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_AFTER_TRANSLATE,
             bubble_->GetViewState());
 }
@@ -495,11 +505,11 @@ TEST_F(TranslateBubbleViewTest, CancelButtonReturningError) {
 
   // Click the "Cancel" button to go back.
   EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_ADVANCED, bubble_->GetViewState());
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_CANCEL);
+  PressButton(TranslateBubbleView::BUTTON_ID_CANCEL);
   EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_ERROR, bubble_->GetViewState());
 }
 
-TEST_F(TranslateBubbleViewTest, DenialMenuRespectsBlacklistSite) {
+TEST_F(TranslateBubbleViewTest, OptionsMenuRespectsBlacklistSite) {
   mock_model_->SetCanBlacklistSite(false);
   CreateAndShowBubble();
 
@@ -530,7 +540,7 @@ TEST_F(TranslateBubbleViewTest, AlwaysTranslateLanguageMenuItem) {
   // Go back to untranslated page, since the *language* should still always
   // be translated (and this "untranslate" is temporary) the option should now
   // be checked and it should be possible to disable it from the menu.
-  bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_SHOW_ORIGINAL);
+  PressButton(TranslateBubbleView::BUTTON_ID_SHOW_ORIGINAL);
   EXPECT_TRUE(mock_model_->revert_translation_called_);
 
   TriggerOptionsMenu();