GTK Keyboard Support for New Autofill

chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc

Index: chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
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 bebb0b835784ac37c4eedcfc40e509fcdfd73318..5edb76cd58f53f612aa3804fe48244963d7e1657 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_);
 }
 
@@ -128,12 +127,8 @@ gboolean AutofillPopupViewGtk::HandleButtonRelease(GtkWidget* widget,
     return FALSE;
 
   size_t line = LineFromY(event->y);
-  DCHECK_LT(line, autofill_values().size());
-
-  external_delegate()->DidAcceptAutofillSuggestions(
-      autofill_values()[line],
-      autofill_unique_ids()[line],
-      line);
+  SetSelectedLine(line);

[Ilya Sherman, 2012/03/21 23:35:30]

Is it actually possible that this line was not already selected?  If so, how?

[csharp, 2012/03/23 15:10:52]

I'm not sure, I'm mostly concerned that there could be a race condition where
the selection event is received before the movement. I'll replace this with a
DCHECK.

+  AcceptSelectedLine();
 
   return TRUE;
 }
@@ -224,6 +219,40 @@ 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);

[Ilya Sherman, 2012/03/21 23:35:30]

Is this intentionally limited only to the suggestions, and not to e.g. the
"Chrome Autofill settings" entry?

[csharp, 2012/03/23 15:10:52]

The suggestion list isn't just the suggestions, it includes the other items
(warnings, options, etc), so they are included here.
(OnSuggestionsReturned is where they are added to the suggestion vector)

+      return true;
+    case GDK_Escape:
+      Hide();
+      return true;
+    case GDK_Delete:
+    case GDK_KP_Delete:
+      if (selected_line() == kNoSelection) {

[Ilya Sherman, 2012/03/21 23:35:30]

Should this be "!="?

[csharp, 2012/03/23 15:10:52]

Done.

+        RemoveLine(selected_line());

[Ilya Sherman, 2012/03/21 23:35:30]

How about instead "return RemoveSelectedLine()", and have the
RemoveSelectedLine() method figure out if the line can be removed?

[csharp, 2012/03/23 15:10:52]

Done.

+        return true;
+      }

[Ilya Sherman, 2012/03/21 23:35:30]

This currently incorrectly falls through if the if-stmt condition is not
satisfied.

[csharp, 2012/03/23 15:10:52]

Done.

+    case GDK_Return:
+    case GDK_KP_Enter:
+      if (selected_line() == kNoSelection) {
+        AcceptSelectedLine();

[Ilya Sherman, 2012/03/21 23:35:30]

Likewise, how about instead "return AcceptSelectedLine()", and have the
AcceptSelectedLine() method figure out if the line can be removed?

[csharp, 2012/03/23 15:10:52]

Done.

+        return true;
+      }
+  }
+
+  return false;
+}
+
 void AutofillPopupViewGtk::SetupLayout(const gfx::Rect& window_rect,
                                        const GdkColor& text_color) {
   int allocated_content_width = window_rect.width();
@@ -242,6 +271,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_;
 }