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..172245924fa7234d746a2f49d206f567acb2ac9b 100644
--- a/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
+++ b/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
@@ -4,15 +4,19 @@
 
 #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 "ui/base/gtk/gtk_compat.h"
 #include "ui/base/gtk/gtk_hig_constants.h"
 #include "ui/base/gtk/gtk_windowing.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/gfx/rect.h"
+#include "content/public/browser/web_contents.h"

[Ilya Sherman, 2012/03/15 20:10:56]

nit: Alpha-order

[csharp, 2012/03/21 15:32:28]

Done.

 
 namespace {
 const GdkColor kBorderColor = GDK_COLOR_RGB(0xc7, 0xca, 0xce);
@@ -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);
@@ -73,22 +78,49 @@ AutofillPopupViewGtk::~AutofillPopupViewGtk() {
   gtk_widget_destroy(window_);
 }
 
-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]));
+bool AutofillPopupViewGtk::HandleKeyPressEvent(GdkEventKey* event) {
+  switch (event->keyval) {
+    case GDK_Up: {
+      DecreaseSelectedLine();
+      return true;
+    }

[Ilya Sherman, 2012/03/15 20:10:56]

nit: No need for curly braces for these case stmts, since you are not declaring
any local variables.

[csharp, 2012/03/21 15:32:28]

Done.

+    case GDK_Down: {
+      IncreaseSelectedLine();
+      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: {
+      RemoveLine(selected_line());

[Ilya Sherman, 2012/03/15 20:10:56]

We should only remove the line if the user types shift-delete.  As with Enter
below, the Autofill popup should only consume the event if there is a line
selected.

[csharp, 2012/03/21 15:32:28]

Ok, in my playing around I see that Autofill profiles need shift-delete to be
removed, but regular Autocomplete entries just need a delete. Is this the
behaviour we want or should both actions require a shift-delete?

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

I wasn't aware that Autocomplete entries behaved differently.  I'm pretty sure
we want shift-delete to consistently be the way to remove items from our popups
(e.g. including the Omnibox popup).

+      return true;
+    }
+    case GDK_Return:
+    case GDK_KP_Enter: {
+      ChooseLine(selected_line());
+      return true;

[Ilya Sherman, 2012/03/15 20:10:56]

This should only consume the event if the popup has a line selected.  Otherwise,
this should bubble normally, which will typically result in the form getting
submitted.

[csharp, 2012/03/21 15:32:28]

Done.

+    }
   }
 
+  return false;
+}
+
+void AutofillPopupViewGtk::ShowInternal() {
   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 +135,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 +145,8 @@ void AutofillPopupViewGtk::ShowInternal() {
 }
 
 void AutofillPopupViewGtk::HideInternal() {
+  render_view_host_->RemoveKeyboardListener(this);
+
   gtk_widget_hide(window_);
 }
 
@@ -121,6 +157,13 @@ void AutofillPopupViewGtk::InvalidateRow(size_t row) {
   gdk_window_invalidate_rect(gdk_window, &row_rect, FALSE);
 }
 
+void AutofillPopupViewGtk::UpdatePopupSize() {
+  gtk_widget_set_size_request(
+      window_,
+      GetPopupRequiredWidth(),
+      row_height_ * autofill_values().size());
+}
+
 gboolean AutofillPopupViewGtk::HandleButtonRelease(GtkWidget* widget,
                                                    GdkEventButton* event) {
   // We only care about the left click.
@@ -128,12 +171,7 @@ 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);
+  ChooseLine(line);
 
   return TRUE;
 }
@@ -242,6 +280,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_;
 }