[Translate] Don't go back to before translating when confirming advanced options

In the Tab UI, we shouldn't go back to the pre-translating state on
confirming advanced option when the languages stayed the same. We should
stay in the after translate state.

In addition, this cl fixes a bug for the button to confirm advanced
options. In the Tab UI, both the source and target language views own
their own done buttons. They both need to be stored and updated with
'Translate' and 'Done' in UpdateAdvancedView. Previously just the most
recently created button that is stored in advanced_done_button_ was
getting updated, since advanced_done_button_ was overridden on the
second call to CreateViewAdvancedTabUi.

Bug: 976352
Change-Id: Ie16534717f5f8fdf0a608bd654b9e26db82d3a51
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1935725
Commit-Queue: Megan Jablonski <megjablon@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721092}

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

@@ -697,7 +697,7 @@ void TranslateBubbleView::ConfirmAdvancedOptions() {
   model_->SetAlwaysTranslate(should_always_translate_);
   if (bubble_ui_model_ == language::TranslateUIBubbleModel::TAB) {
     if (model_->IsPageTranslatedInCurrentLanguages()) {
-      SwitchView(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE);
+      SwitchView(TranslateBubbleModel::VIEW_STATE_AFTER_TRANSLATE);
       SizeToContents();
     } else {
       base::string16 original_language_name;
@@ -1250,7 +1250,7 @@ std::unique_ptr<views::View> TranslateBubbleView::CreateViewAdvanced() {
       this, l10n_util::GetStringUTF16(IDS_CANCEL));
   advanced_cancel_button->SetID(BUTTON_ID_CANCEL);
   advanced_done_button_ = layout->AddView(std::move(advanced_done_button));
-  advanced_cancel_button_ = layout->AddView(std::move(advanced_cancel_button));
+  layout->AddView(std::move(advanced_cancel_button));
 
   UpdateAdvancedView();
 
@@ -1280,8 +1280,16 @@ TranslateBubbleView::TabUiCreateViewAdvanedSource() {
   source_language_combobox->SetID(COMBOBOX_ID_SOURCE_LANGUAGE);
   source_language_combobox->set_listener(this);
   source_language_combobox_ = source_language_combobox.get();
+
+  auto advanced_done_button = views::MdTextButton::CreateSecondaryUiButton(
+      this, l10n_util::GetStringUTF16(IDS_DONE));
+  advanced_done_button->SetID(BUTTON_ID_DONE);
+  advanced_done_button->SetIsDefault(true);
+  advanced_done_button_source_ = advanced_done_button.get();
+
   return CreateViewAdvancedTabUi(std::move(source_language_combobox),
-                                 std::move(source_language_title_label));
+                                 std::move(source_language_title_label),
+                                 std::move(advanced_done_button));
 }
 
 std::unique_ptr<views::View>
@@ -1304,13 +1312,22 @@ TranslateBubbleView::TabUiCreateViewAdvanedTarget() {
   target_language_combobox->SetID(COMBOBOX_ID_TARGET_LANGUAGE);
   target_language_combobox->set_listener(this);
   target_language_combobox_ = target_language_combobox.get();
+
+  auto advanced_done_button = views::MdTextButton::CreateSecondaryUiButton(
+      this, l10n_util::GetStringUTF16(IDS_DONE));
+  advanced_done_button->SetID(BUTTON_ID_DONE);
+  advanced_done_button->SetIsDefault(true);
+  advanced_done_button_target_ = advanced_done_button.get();
+
   return CreateViewAdvancedTabUi(std::move(target_language_combobox),
-                                 std::move(target_language_title_label));
+                                 std::move(target_language_title_label),
+                                 std::move(advanced_done_button));
 }
 
 std::unique_ptr<views::View> TranslateBubbleView::CreateViewAdvancedTabUi(
     std::unique_ptr<views::Combobox> combobox,
-    std::unique_ptr<views::Label> language_title_label) {
+    std::unique_ptr<views::Label> language_title_label,
+    std::unique_ptr<views::Button> advanced_done_button) {
   const int language_icon_id = IDR_TRANSLATE_BUBBLE_ICON;
   std::unique_ptr<views::ImageView> language_icon =
       std::make_unique<views::ImageView>();
@@ -1408,12 +1425,8 @@ std::unique_ptr<views::View> TranslateBubbleView::CreateViewAdvancedTabUi(
   auto advanced_reset_button = views::MdTextButton::CreateSecondaryUiButton(
       this, l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_RESET));
   advanced_reset_button->SetID(BUTTON_ID_RESET);
-  auto advanced_done_button = views::MdTextButton::CreateSecondaryUiButton(
-      this, l10n_util::GetStringUTF16(IDS_DONE));
-  advanced_done_button->SetID(BUTTON_ID_DONE);
-  advanced_done_button->SetIsDefault(true);
-  advanced_cancel_button_ = layout->AddView(std::move(advanced_reset_button));
-  advanced_done_button_ = layout->AddView(std::move(advanced_done_button));
+  layout->AddView(std::move(advanced_reset_button));
+  layout->AddView(std::move(advanced_done_button));
 
   UpdateAdvancedView();
 
@@ -1486,11 +1499,25 @@ void TranslateBubbleView::SwitchToErrorView(
 }
 
 void TranslateBubbleView::UpdateAdvancedView() {
-  DCHECK(advanced_done_button_);
-  advanced_done_button_->SetText(
-      l10n_util::GetStringUTF16(model_->IsPageTranslatedInCurrentLanguages()
-                                    ? IDS_DONE
-                                    : IDS_TRANSLATE_BUBBLE_ACCEPT));
+  if (advanced_done_button_) {
+    advanced_done_button_->SetText(
+        l10n_util::GetStringUTF16(model_->IsPageTranslatedInCurrentLanguages()
+                                      ? IDS_DONE
+                                      : IDS_TRANSLATE_BUBBLE_ACCEPT));
+  }
+  if (advanced_done_button_source_) {
+    advanced_done_button_source_->SetText(
+        l10n_util::GetStringUTF16(model_->IsPageTranslatedInCurrentLanguages()
+                                      ? IDS_DONE
+                                      : IDS_TRANSLATE_BUBBLE_ACCEPT));
+  }
+  if (advanced_done_button_target_) {
+    advanced_done_button_target_->SetText(
+        l10n_util::GetStringUTF16(model_->IsPageTranslatedInCurrentLanguages()
+                                      ? IDS_DONE
+                                      : IDS_TRANSLATE_BUBBLE_ACCEPT));
+  }
+  Layout();
 }
 
 void TranslateBubbleView::UpdateLanguageNames(

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

@@ -168,11 +168,7 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
                            DoneButtonWithoutTranslating);
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
-                           TabUiSourceDoneButtonWithoutTranslating);
-  FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
-                           TabUiTargetDoneButtonWithoutTranslating);
-  FRIEND_TEST_ALL_PREFIXES(TabUiSourceTranslateBubbleViewTest,
-                           DoneButtonWithoutTranslating);
+                           TabUiDoneButtonWithoutTranslating);
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
                            CancelButtonReturningBeforeTranslate);
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
@@ -274,7 +270,8 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
   // TAB UI. Caller takes ownership of the returned view.
   std::unique_ptr<views::View> CreateViewAdvancedTabUi(
       std::unique_ptr<views::Combobox> combobox,
-      std::unique_ptr<views::Label> language_title_label);
+      std::unique_ptr<views::Label> language_title_label,
+      std::unique_ptr<views::Button> advance_done_button);
 
   std::unique_ptr<views::Button> CreateCloseButton();
 
@@ -329,8 +326,9 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
   views::Checkbox* advanced_always_translate_checkbox_ = nullptr;
   views::TabbedPane* tabbed_pane_ = nullptr;
 
-  views::LabelButton* advanced_cancel_button_ = nullptr;
   views::LabelButton* advanced_done_button_ = nullptr;
+  views::LabelButton* advanced_done_button_source_ = nullptr;
+  views::LabelButton* advanced_done_button_target_ = nullptr;
 
   // Default source/target language without user interaction.
   int previous_source_language_index_;

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

@@ -442,6 +442,9 @@ TEST_F(TranslateBubbleViewTest, TabUiSourceDoneButton) {
   // Expected value is (set id - 1) because user selected id is actual id + 1
   EXPECT_EQ(9, mock_model_->original_language_index_);
   EXPECT_EQ(20, mock_model_->target_language_index_);
+
+  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_AFTER_TRANSLATE,
+            bubble_->GetViewState());
 }
 
 TEST_F(TranslateBubbleViewTest, TabUiTargetDoneButton) {
@@ -466,42 +469,12 @@ TEST_F(TranslateBubbleViewTest, TabUiTargetDoneButton) {
   EXPECT_TRUE(mock_model_->translate_called_);
   EXPECT_EQ(9, mock_model_->original_language_index_);
   EXPECT_EQ(20, mock_model_->target_language_index_);
-}
-
-TEST_F(TranslateBubbleViewTest, DoneButtonWithoutTranslating) {
-  CreateAndShowBubble();
-  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
-            bubble_->GetViewState());
-
-  // Translate the page once.
-  mock_model_->Translate();
-  EXPECT_TRUE(mock_model_->translate_called_);
 
-  // Go back to the initial view.
-  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
-            bubble_->GetViewState());
-  mock_model_->translate_called_ = false;
-
-  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
-            bubble_->GetViewState());
-  bubble_->SwitchView(TranslateBubbleModel::VIEW_STATE_ADVANCED);
-
-  // 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.
-  PressButton(TranslateBubbleView::BUTTON_ID_DONE);
-  EXPECT_FALSE(mock_model_->translate_called_);
-
-  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
+  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_AFTER_TRANSLATE,
             bubble_->GetViewState());
 }
 
-TEST_F(TranslateBubbleViewTest, TabUiSourceDoneButtonWithoutTranslating) {
-  scoped_feature_list_.InitAndEnableFeatureWithParameters(
-      language::kUseButtonTranslateBubbleUi,
-      {{language::kTranslateUIBubbleKey,
-        language::kTranslateUIBubbleTabValue}});
-
+TEST_F(TranslateBubbleViewTest, DoneButtonWithoutTranslating) {
   CreateAndShowBubble();
   EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
             bubble_->GetViewState());
@@ -517,7 +490,7 @@ TEST_F(TranslateBubbleViewTest, TabUiSourceDoneButtonWithoutTranslating) {
 
   EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
             bubble_->GetViewState());
-  bubble_->SwitchView(TranslateBubbleModel::VIEW_STATE_SOURCE_LANGUAGE);
+  bubble_->SwitchView(TranslateBubbleModel::VIEW_STATE_ADVANCED);
 
   // Click the "Done" button with the current language pair. This time,
   // translation is not performed and the view state will be back to the
@@ -529,36 +502,32 @@ TEST_F(TranslateBubbleViewTest, TabUiSourceDoneButtonWithoutTranslating) {
             bubble_->GetViewState());
 }
 
-TEST_F(TranslateBubbleViewTest, TabUiTargetDoneButtonWithoutTranslating) {
+TEST_F(TranslateBubbleViewTest, TabUiDoneButtonWithoutTranslating) {
   scoped_feature_list_.InitAndEnableFeatureWithParameters(
       language::kUseButtonTranslateBubbleUi,
       {{language::kTranslateUIBubbleKey,
         language::kTranslateUIBubbleTabValue}});
 
   CreateAndShowBubble();
-  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
-            bubble_->GetViewState());
 
   // Translate the page once.
   mock_model_->Translate();
   EXPECT_TRUE(mock_model_->translate_called_);
 
-  // Go back to the initial view.
-  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
-            bubble_->GetViewState());
+  // Set translation called to false so it can be verified that translation is
+  // not called again.
   mock_model_->translate_called_ = false;
 
-  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
-            bubble_->GetViewState());
-  bubble_->SwitchView(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE);
-
   // 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.
+  // translation is not performed and the view state will stay at the translated
+  // view.
   PressButton(TranslateBubbleView::BUTTON_ID_DONE);
   EXPECT_FALSE(mock_model_->translate_called_);
 
-  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
+  // The page is already in the translated languages if Done doesn't trigger a
+  // translation. Clicking done ensures that the UI is in the after translation
+  // state.
+  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_AFTER_TRANSLATE,
             bubble_->GetViewState());
 }