GTK Keyboard Support for New Autofill

Adds keyboard support to the new browser autofill.

BUG=51644
TEST=When using the new autofill, keyboard presses should work as they do in the old autofill


Review URL: http://codereview.chromium.org/9432029

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@129553 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 259 insertions, 60 deletions

diff --git a/chrome/browser/autofill/autofill_external_delegate.cc b/chrome/browser/autofill/autofill_external_delegate.cc
index 4105709..13f0b7d 100644
--- a/chrome/browser/autofill/autofill_external_delegate.cc
+++ b/chrome/browser/autofill/autofill_external_delegate.cc
@@ -140,13 +140,13 @@ void AutofillExternalDelegate::DidEndTextFieldEditing() {
   has_shown_autofill_popup_for_current_edit_ = false;
 }
 
-void AutofillExternalDelegate::DidAcceptAutofillSuggestions(
+bool AutofillExternalDelegate::DidAcceptAutofillSuggestions(
     const string16& value,
     int unique_id,
     unsigned index) {
   // If the selected element is a warning we don't want to do anything.
   if (unique_id < 0)
-    return;
+    return false;
 
   // TODO(csharp): Add the password autofill manager.
   // if (password_autofill_manager_->DidAcceptAutofillSuggestion(node, value))
@@ -174,6 +174,8 @@ void AutofillExternalDelegate::DidAcceptAutofillSuggestions(
   }
 
   HideAutofillPopup();
+
+  return true;
 }
 
 void AutofillExternalDelegate::ClearPreviewedForm() {
diff --git a/chrome/browser/autofill/autofill_external_delegate.h b/chrome/browser/autofill/autofill_external_delegate.h
index b341304..11a5341 100644
--- a/chrome/browser/autofill/autofill_external_delegate.h
+++ b/chrome/browser/autofill/autofill_external_delegate.h
@@ -61,8 +61,9 @@ class AutofillExternalDelegate {
   // used to help record the metrics of when a new popup is shown.
   void DidEndTextFieldEditing();
 
-  // Inform the delegate that an autofill suggestion have been chosen.
-  void DidAcceptAutofillSuggestions(const string16& value,
+  // Inform the delegate that an autofill suggestion have been chosen. Returns
+  // true if the suggestion was selected.
+  bool DidAcceptAutofillSuggestions(const string16& value,
                                     int unique_id,
                                     unsigned index);
 
diff --git a/chrome/browser/autofill/autofill_popup_unittest.cc b/chrome/browser/autofill/autofill_popup_unittest.cc
new file mode 100644
index 0000000..4332cd6
--- /dev/null
+++ b/chrome/browser/autofill/autofill_popup_unittest.cc
@@ -0,0 +1,109 @@
+// Copyright (c) 2012 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.
+
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/autofill/autofill_popup_view.h"
+#include "chrome/browser/autofill/test_autofill_external_delegate.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace {
+
+class MockAutofillExternalDelegate : public TestAutofillExternalDelegate {
+ public:
+  MockAutofillExternalDelegate() : TestAutofillExternalDelegate(NULL, NULL) {};
+  virtual ~MockAutofillExternalDelegate() {};
+
+  virtual void SelectAutofillSuggestionAtIndex(int unique_id, int list_index)
+      OVERRIDE {}
+};
+
+class TestAutofillPopupView : public AutofillPopupView {
+ public:
+  explicit TestAutofillPopupView(AutofillExternalDelegate* external_delegate) :
+      AutofillPopupView(NULL, external_delegate) {
+    std::vector<string16> autofill_values;
+    autofill_values.push_back(string16());
+    autofill_values.push_back(string16());
+
+    std::vector<int> autofill_ids;
+    autofill_ids.push_back(0);
+    autofill_ids.push_back(1);
+
+    Show(autofill_values, autofill_values, autofill_values, autofill_ids, 0);
+  }
+  virtual ~TestAutofillPopupView() {}
+
+  // Making protected functions public for testing
+  const std::vector<string16>& autofill_values() const {
+    return AutofillPopupView::autofill_values();
+  }
+  int selected_line() const {
+    return AutofillPopupView::selected_line();
+  }
+  void SetSelectedLine(size_t selected_line) {
+    AutofillPopupView::SetSelectedLine(selected_line);
+  }
+  void SelectNextLine() {
+    AutofillPopupView::SelectNextLine();
+  }
+  void SelectPreviousLine() {
+    AutofillPopupView::SelectPreviousLine();
+  }
+
+  MOCK_METHOD1(InvalidateRow, void(size_t));
+
+ private:
+  virtual void ShowInternal() OVERRIDE {}
+  virtual void HideInternal() OVERRIDE {}
+};
+
+}  // namespace
+
+class AutofillPopupViewUnitTest : public ::testing::Test {
+ public:
+  AutofillPopupViewUnitTest() {
+    autofill_popup_view_.reset(new TestAutofillPopupView(&external_delegate_));
+  }
+  virtual ~AutofillPopupViewUnitTest() {}
+
+  scoped_ptr<TestAutofillPopupView> autofill_popup_view_;
+
+ private:
+  MockAutofillExternalDelegate external_delegate_;
+};
+
+TEST_F(AutofillPopupViewUnitTest, ChangeSelectedLine) {
+  EXPECT_LT(autofill_popup_view_->selected_line(), 0);
+  // Check that there are at least 2 values so that the first and last selection
+  // are different.
+  EXPECT_GE(2,
+            static_cast<int>(autofill_popup_view_->autofill_values().size()));
+
+  // Test wrapping before the front.
+  autofill_popup_view_->SelectPreviousLine();
+  EXPECT_EQ(
+      static_cast<int>(autofill_popup_view_->autofill_values().size() - 1),
+      autofill_popup_view_->selected_line());
+
+  // Test wrapping after the end.
+  autofill_popup_view_->SelectNextLine();
+  EXPECT_EQ(0, autofill_popup_view_->selected_line());
+}
+
+TEST_F(AutofillPopupViewUnitTest, RedrawSelectedLine) {
+  // Make sure that when a new line is selected, it is invalidated so it can
+  // be updated to show it is selected.
+  int selected_line = 0;
+  EXPECT_CALL(*autofill_popup_view_, InvalidateRow(selected_line));
+  autofill_popup_view_->SetSelectedLine(selected_line);
+
+  // Ensure that the row isn't invalidated if it didn't change.
+  EXPECT_CALL(*autofill_popup_view_, InvalidateRow(selected_line)).Times(0);
+  autofill_popup_view_->SetSelectedLine(selected_line);
+
+  // Change back to no selection.
+  EXPECT_CALL(*autofill_popup_view_, InvalidateRow(selected_line));
+  autofill_popup_view_->SetSelectedLine(-1);
+}
diff --git a/chrome/browser/autofill/autofill_popup_view.cc b/chrome/browser/autofill/autofill_popup_view.cc
index 7e3ba5a..cfad2ac 100644
--- a/chrome/browser/autofill/autofill_popup_view.cc
+++ b/chrome/browser/autofill/autofill_popup_view.cc
@@ -12,11 +12,21 @@
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 
+namespace {
+
+// Used to indicate that no line is currently selected by the user.
+const int kNoSelection = -1;
+
+}  // end namespace
+
 AutofillPopupView::AutofillPopupView(
     content::WebContents* web_contents,
     AutofillExternalDelegate* external_delegate)
     : external_delegate_(external_delegate),
-      selected_line_(-1) {
+      selected_line_(kNoSelection) {
+  if (!web_contents)
+    return;
+
   registrar_.Add(this,
                  content::NOTIFICATION_WEB_CONTENTS_HIDDEN,
                  content::Source<content::WebContents>(web_contents));
@@ -54,21 +64,60 @@ void AutofillPopupView::SetSelectedLine(int selected_line) {
   if (selected_line_ == selected_line)
     return;
 
-  if (selected_line_ != -1)
+  if (selected_line_ != kNoSelection)
     InvalidateRow(selected_line_);
 
-  if (selected_line != -1)
+  if (selected_line != kNoSelection)
     InvalidateRow(selected_line);
 
   selected_line_ = selected_line;
 
-  if (selected_line_ != -1) {
+  if (selected_line_ != kNoSelection) {
     external_delegate_->SelectAutofillSuggestionAtIndex(
         autofill_unique_ids_[selected_line_],
         selected_line);
   }
 }
 
+void AutofillPopupView::SelectNextLine() {
+  int new_selected_line = selected_line_ + 1;
+
+  if (new_selected_line == static_cast<int>(autofill_values_.size()))
+    new_selected_line = 0;
+
+  SetSelectedLine(new_selected_line);
+}
+
+void AutofillPopupView::SelectPreviousLine() {
+  int new_selected_line = selected_line_ - 1;
+
+  if (new_selected_line <= kNoSelection)
+    new_selected_line = autofill_values_.size() - 1;
+
+  SetSelectedLine(new_selected_line);
+}
+
+bool AutofillPopupView::AcceptSelectedLine() {
+  if (selected_line_ == kNoSelection)
+    return false;
+
+  DCHECK_GE(selected_line_, 0);
+  DCHECK_LT(selected_line_, static_cast<int>(autofill_values_.size()));
+
+  return external_delegate()->DidAcceptAutofillSuggestions(
+      autofill_values_[selected_line_],
+      autofill_unique_ids_[selected_line_],
+      selected_line_);
+}
+
+bool AutofillPopupView::RemoveSelectedLine() {
+  if (selected_line_ == kNoSelection)
+    return false;
+
+  // TODO(csharp) add removal code.
+  return false;
+}
+
 void AutofillPopupView::Observe(int type,
                                 const content::NotificationSource& source,
                                 const content::NotificationDetails& details) {
diff --git a/chrome/browser/autofill/autofill_popup_view.h b/chrome/browser/autofill/autofill_popup_view.h
index 1ca8908..6bf13214 100644
--- a/chrome/browser/autofill/autofill_popup_view.h
+++ b/chrome/browser/autofill/autofill_popup_view.h
@@ -75,6 +75,18 @@ class AutofillPopupView : public content::NotificationObserver {
   // Change which line is currently selected by the user.
   void SetSelectedLine(int selected_line);
 
+  // Increase the selected line by 1, properly handling wrapping.
+  void SelectNextLine();
+
+  // Decrease the selected line by 1, properly handling wrapping.
+  void SelectPreviousLine();
+
+  // The user has choosen the selected line.
+  bool AcceptSelectedLine();
+
+  // The user has removed a suggestion.
+  bool RemoveSelectedLine();
+
  private:
   // content::NotificationObserver method override.
   virtual void Observe(int type,
@@ -100,7 +112,7 @@ class AutofillPopupView : public content::NotificationObserver {
   int separator_index_;
 
   // The line that is currently selected by the user.
-  // -1 indicates that no line is currently selected.
+  // |kNoSelection| indicates that no line is currently selected.
   int selected_line_;
 };
 
diff --git a/chrome/browser/autofill/autofill_popup_view_browsertest.cc b/chrome/browser/autofill/autofill_popup_view_browsertest.cc
index 4a9dfac..635dc35 100644
--- a/chrome/browser/autofill/autofill_popup_view_browsertest.cc
+++ b/chrome/browser/autofill/autofill_popup_view_browsertest.cc
@@ -35,7 +35,7 @@ class MockAutofillExternalDelegate : public TestAutofillExternalDelegate {
 
 class TestAutofillPopupView : public AutofillPopupView {
  public:
-  explicit TestAutofillPopupView(
+  TestAutofillPopupView(
       content::WebContents* web_contents,
       AutofillExternalDelegate* autofill_external_delegate)
       : AutofillPopupView(web_contents, autofill_external_delegate) {}
@@ -43,16 +43,12 @@ class TestAutofillPopupView : public AutofillPopupView {
 
   MOCK_METHOD0(Hide, void());
 
-  MOCK_METHOD1(InvalidateRow, void(size_t));
-
-  void SetSelectedLine(size_t selected_line) {
-    AutofillPopupView::SetSelectedLine(selected_line);
-  }
-
  protected:
   virtual void ShowInternal() OVERRIDE {}
 
   virtual void HideInternal() OVERRIDE {}
+
+  virtual void InvalidateRow(size_t row) OVERRIDE {}
 };
 
 }  // namespace
@@ -109,27 +105,3 @@ IN_PROC_BROWSER_TEST_F(AutofillPopupViewBrowserTest,
 
   // The mock verifies that the call was made.
 }
-
-IN_PROC_BROWSER_TEST_F(AutofillPopupViewBrowserTest,
-                       SetSelectedAutofillLineAndCallInvalidate) {
-  std::vector<string16> autofill_values;
-  autofill_values.push_back(string16());
-  std::vector<int> autofill_ids;
-  autofill_ids.push_back(0);
-  autofill_popup_view_->Show(
-      autofill_values, autofill_values, autofill_values, autofill_ids, 0);
-
-  // Make sure that when a new line is selected, it is invalidated so it can
-  // be updated to show it is selected.
-  int selected_line = 0;
-  EXPECT_CALL(*autofill_popup_view_, InvalidateRow(selected_line));
-  autofill_popup_view_->SetSelectedLine(selected_line);
-
-  // Ensure that the row isn't invalidated if it didn't change.
-  EXPECT_CALL(*autofill_popup_view_, InvalidateRow(selected_line)).Times(0);
-  autofill_popup_view_->SetSelectedLine(selected_line);
-
-  // Change back to no selection.
-  EXPECT_CALL(*autofill_popup_view_, InvalidateRow(selected_line));
-  autofill_popup_view_->SetSelectedLine(-1);
-}
diff --git a/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc b/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
index bebb0b8..ed1f84a 100644
--- a/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
+++ b/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
@@ -4,10 +4,14 @@
 
 #include "autofill_popup_view_gtk.h"
 
+#include <gdk/gdkkeysyms.h>
+
 #include "base/logging.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autofill/autofill_external_delegate.h"
 #include "chrome/browser/ui/gtk/gtk_util.h"
+#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/web_contents.h"
 #include "ui/base/gtk/gtk_compat.h"
 #include "ui/base/gtk/gtk_hig_constants.h"
 #include "ui/base/gtk/gtk_windowing.h"
@@ -43,7 +47,8 @@ AutofillPopupViewGtk::AutofillPopupViewGtk(
     GtkWidget* parent)
     : AutofillPopupView(web_contents, external_delegate),
       parent_(parent),
-      window_(gtk_window_new(GTK_WINDOW_POPUP)) {
+      window_(gtk_window_new(GTK_WINDOW_POPUP)),
+      render_view_host_(web_contents->GetRenderViewHost()) {
   CHECK(parent != NULL);
   gtk_window_set_resizable(GTK_WINDOW(window_), FALSE);
   gtk_widget_set_app_paintable(window_, TRUE);
@@ -74,21 +79,11 @@ AutofillPopupViewGtk::~AutofillPopupViewGtk() {
 }
 
 void AutofillPopupViewGtk::ShowInternal() {
-  // Find out the maximum bounds required by the popup.
-  // TODO(csharp): Once the icon is also displayed it will affect the required
-  // size so it will need to be included in the calculation.
-  int popup_width = element_bounds().width();
-  DCHECK_EQ(autofill_values().size(), autofill_labels().size());
-  for (size_t i = 0; i < autofill_values().size(); ++i) {
-    popup_width = std::max(popup_width,
-                           font_.GetStringWidth(autofill_values()[i]) +
-                           kMiddlePadding +
-                           font_.GetStringWidth(autofill_labels()[i]));
-  }
-
   gint origin_x, origin_y;
   gdk_window_get_origin(gtk_widget_get_window(parent_), &origin_x, &origin_y);
 
+  int popup_width = GetPopupRequiredWidth();
+
   // Move the popup to appear right below the text field it is using.
   bounds_.SetRect(
       origin_x + element_bounds().x(),
@@ -103,6 +98,8 @@ void AutofillPopupViewGtk::ShowInternal() {
       popup_width,
       row_height_ * autofill_values().size());
 
+  render_view_host_->AddKeyboardListener(this);
+
   gtk_widget_show(window_);
 
   GtkWidget* toplevel = gtk_widget_get_toplevel(parent_);
@@ -111,6 +108,8 @@ void AutofillPopupViewGtk::ShowInternal() {
 }
 
 void AutofillPopupViewGtk::HideInternal() {
+  render_view_host_->RemoveKeyboardListener(this);
+
   gtk_widget_hide(window_);
 }
 
@@ -127,13 +126,9 @@ gboolean AutofillPopupViewGtk::HandleButtonRelease(GtkWidget* widget,
   if (event->button != 1)
     return FALSE;
 
-  size_t line = LineFromY(event->y);
-  DCHECK_LT(line, autofill_values().size());
+  DCHECK_EQ(selected_line(), LineFromY(event->y));
 
-  external_delegate()->DidAcceptAutofillSuggestions(
-      autofill_values()[line],
-      autofill_unique_ids()[line],
-      line);
+  AcceptSelectedLine();
 
   return TRUE;
 }
@@ -224,6 +219,34 @@ gboolean AutofillPopupViewGtk::HandleMotion(GtkWidget* widget,
   return TRUE;
 }
 
+bool AutofillPopupViewGtk::HandleKeyPressEvent(GdkEventKey* event) {
+  switch (event->keyval) {
+    case GDK_Up:
+      SelectPreviousLine();
+      return true;
+    case GDK_Down:
+      SelectNextLine();
+      return true;
+    case GDK_Page_Up:
+      SetSelectedLine(0);
+      return true;
+    case GDK_Page_Down:
+      SetSelectedLine(autofill_values().size() - 1);
+      return true;
+    case GDK_Escape:
+      Hide();
+      return true;
+    case GDK_Delete:
+    case GDK_KP_Delete:
+      return (event->state == GDK_SHIFT_MASK) && RemoveSelectedLine();
+    case GDK_Return:
+    case GDK_KP_Enter:
+      return AcceptSelectedLine();
+  }
+
+  return false;
+}
+
 void AutofillPopupViewGtk::SetupLayout(const gfx::Rect& window_rect,
                                        const GdkColor& text_color) {
   int allocated_content_width = window_rect.width();
@@ -242,6 +265,21 @@ void AutofillPopupViewGtk::SetupLayout(const gfx::Rect& window_rect,
   pango_attr_list_unref(attrs);
 }
 
+int AutofillPopupViewGtk::GetPopupRequiredWidth() {
+  // TODO(csharp): Once the icon is also displayed it will affect the required
+  // size so it will need to be included in the calculation.
+  int popup_width = element_bounds().width();
+  DCHECK_EQ(autofill_values().size(), autofill_labels().size());
+  for (size_t i = 0; i < autofill_values().size(); ++i) {
+    popup_width = std::max(popup_width,
+                           font_.GetStringWidth(autofill_values()[i]) +
+                           kMiddlePadding +
+                           font_.GetStringWidth(autofill_labels()[i]));
+  }
+
+  return popup_width;
+}
+
 int AutofillPopupViewGtk::LineFromY(int y) {
   return y / row_height_;
 }
diff --git a/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h b/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h
index 999dd46..3fb7f53 100644
--- a/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h
+++ b/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h
@@ -9,21 +9,28 @@
 #include <pango/pango.h>
 
 #include "chrome/browser/autofill/autofill_popup_view.h"
+#include "content/public/browser/keyboard_listener.h"
 #include "ui/base/glib/glib_integers.h"
 #include "ui/base/gtk/gtk_signal.h"
 #include "ui/gfx/font.h"
 
+namespace content {
+class RenderViewHost;
+}
+
 namespace gfx {
 class Rect;
 }
 
 typedef struct _GdkEventButton GdkEventButton;
 typedef struct _GdkEventExpose GdkEventExpose;
+typedef struct _GdkEventKey GdkEventKey;
 typedef struct _GdkEventMotion GdkEventMotion;
 typedef struct _GdkColor GdkColor;
 typedef struct _GtkWidget GtkWidget;
 
-class AutofillPopupViewGtk : public AutofillPopupView {
+class AutofillPopupViewGtk : public AutofillPopupView,
+                             public KeyboardListener {
  public:
   AutofillPopupViewGtk(content::WebContents* web_contents,
                        AutofillExternalDelegate* external_delegate,
@@ -44,9 +51,15 @@ class AutofillPopupViewGtk : public AutofillPopupView {
   CHROMEGTK_CALLBACK_1(AutofillPopupViewGtk, gboolean, HandleMotion,
                        GdkEventMotion*);
 
+  //  KeyboardListener implementation.
+  virtual bool HandleKeyPressEvent(GdkEventKey* event) OVERRIDE;
+
   // Setup the pango layout to display the autofill results.
   void SetupLayout(const gfx::Rect& window_rect, const GdkColor& text_color);
 
+  // Get width of popup needed by values.
+  int GetPopupRequiredWidth();
+
   // Convert a y-coordinate to the closest line.
   int LineFromY(int y);
 
@@ -60,6 +73,8 @@ class AutofillPopupViewGtk : public AutofillPopupView {
 
   // The size of the popup.
   gfx::Rect bounds_;
+
+  content::RenderViewHost* render_view_host_;  // Weak reference.
 };
 
 #endif  // CHROME_BROWSER_UI_GTK_AUTOFILL_AUTOFILL_POPUP_VIEW_GTK_H_
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 0e5fcf9..30ceb24 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1264,6 +1264,7 @@
         'browser/autofill/autofill_merge_unittest.cc',
         'browser/autofill/autofill_metrics_unittest.cc',
         'browser/autofill/autofill_profile_unittest.cc',
+        'browser/autofill/autofill_popup_unittest.cc',
         'browser/autofill/autofill_type_unittest.cc',
         'browser/autofill/autofill_xml_parser_unittest.cc',
         'browser/autofill/contact_info_unittest.cc',