reland r57885 with a fix for linux/views.

[GTK] a couple of constrained window fixes:

1) don't grab focus when the parent tab isn't showing. Grab the focus when the tab is brought to the front.
2) handle escape via normal key handling rather than accelerator keys
3) don't allow the content view to take focus (via tab) when the constrained window is showing.

BUG=53242, 50799
TEST=see bugs. Also, tabbing between constrained window and url bar should work as expected.

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

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

9 files changed, 140 insertions, 111 deletions

diff --git a/chrome/browser/gtk/constrained_window_gtk.cc b/chrome/browser/gtk/constrained_window_gtk.cc
index 321aa16..8ec5195 100644
--- a/chrome/browser/gtk/constrained_window_gtk.cc
+++ b/chrome/browser/gtk/constrained_window_gtk.cc
@@ -15,8 +15,7 @@ ConstrainedWindowGtk::ConstrainedWindowGtk(
     TabContents* owner, ConstrainedWindowGtkDelegate* delegate)
     : owner_(owner),
       delegate_(delegate),
-      visible_(false),
-      accel_group_(gtk_accel_group_new()) {
+      visible_(false) {
   DCHECK(owner);
   DCHECK(delegate);
   GtkWidget* dialog = delegate->GetWidgetRoot();
@@ -34,20 +33,14 @@ ConstrainedWindowGtk::ConstrainedWindowGtk(
   gtk_container_add(GTK_CONTAINER(frame), alignment);
   gtk_container_add(GTK_CONTAINER(ebox), frame);
   border_.Own(ebox);
-  ConnectAccelerators();
+
+  gtk_widget_add_events(widget(), GDK_KEY_PRESS_MASK);
+  g_signal_connect(widget(), "key-press-event", G_CALLBACK(OnKeyPressThunk),
+                   this);
 }
 
 ConstrainedWindowGtk::~ConstrainedWindowGtk() {
   border_.Destroy();
-
-  gtk_accel_group_disconnect_key(accel_group_, GDK_Escape,
-                                 static_cast<GdkModifierType>(0));
-  if (ContainingView() && ContainingView()->GetTopLevelNativeWindow()) {
-    gtk_window_remove_accel_group(
-        GTK_WINDOW(ContainingView()->GetTopLevelNativeWindow()),
-        accel_group_);
-  }
-  g_object_unref(accel_group_);
 }
 
 void ConstrainedWindowGtk::ShowConstrainedWindow() {
@@ -66,36 +59,23 @@ void ConstrainedWindowGtk::CloseConstrainedWindow() {
   delegate_->DeleteDelegate();
   owner_->WillClose(this);
 
-  delete this;
+  // Let the stack unwind so any relevant event handler can release its ref
+  // on widget().
+  MessageLoop::current()->DeleteSoon(FROM_HERE, this);
 }
 
 TabContentsViewGtk* ConstrainedWindowGtk::ContainingView() {
   return static_cast<TabContentsViewGtk*>(owner_->view());
 }
 
-void ConstrainedWindowGtk::ConnectAccelerators() {
-  gtk_accel_group_connect(accel_group_,
-                          GDK_Escape, static_cast<GdkModifierType>(0),
-                          static_cast<GtkAccelFlags>(0),
-                          g_cclosure_new(G_CALLBACK(OnEscapeThunk),
-                                         this, NULL));
-  gtk_window_add_accel_group(
-      GTK_WINDOW(ContainingView()->GetTopLevelNativeWindow()),
-      accel_group_);
-}
-
-
-gboolean ConstrainedWindowGtk::OnEscape(GtkAccelGroup* group,
-                                        GObject* acceleratable,
-                                        guint keyval,
-                                        GdkModifierType modifier) {
-  // Handle this accelerator only if this is on the currently selected tab.
-  Browser* browser = BrowserList::GetLastActive();
-  if (!browser || browser->GetSelectedTabContents() != owner_)
-    return FALSE;
+gboolean ConstrainedWindowGtk::OnKeyPress(GtkWidget* sender,
+                                          GdkEventKey* key) {
+  if (key->keyval == GDK_Escape) {
+    CloseConstrainedWindow();
+    return TRUE;
+  }
 
-  CloseConstrainedWindow();
-  return TRUE;
+  return FALSE;
 }
 
 // static
@@ -104,4 +84,3 @@ ConstrainedWindow* ConstrainedWindow::CreateConstrainedDialog(
     ConstrainedWindowGtkDelegate* delegate) {
   return new ConstrainedWindowGtk(parent, delegate);
 }
-
diff --git a/chrome/browser/gtk/constrained_window_gtk.h b/chrome/browser/gtk/constrained_window_gtk.h
index 2312cb3..4c0a443 100644
--- a/chrome/browser/gtk/constrained_window_gtk.h
+++ b/chrome/browser/gtk/constrained_window_gtk.h
@@ -15,7 +15,6 @@
 
 class TabContents;
 class TabContentsViewGtk;
-typedef struct _GtkWidget GtkWidget;
 
 class ConstrainedWindowGtkDelegate {
  public:
@@ -56,12 +55,9 @@ class ConstrainedWindowGtk : public ConstrainedWindow {
   ConstrainedWindowGtk(TabContents* owner,
                        ConstrainedWindowGtkDelegate* delegate);
 
-  // Connects the ESC accelerator to the window.
-  void ConnectAccelerators();
-
-  // Handles an ESC accelerator being pressed.
-  CHROMEG_CALLBACK_3(ConstrainedWindowGtk, gboolean, OnEscape, GtkAccelGroup*,
-                     GObject*, guint, GdkModifierType);
+  // Handler for Escape.
+  CHROMEGTK_CALLBACK_1(ConstrainedWindowGtk, gboolean, OnKeyPress,
+                       GdkEventKey*);
 
   // The TabContents that owns and constrains this ConstrainedWindow.
   TabContents* owner_;
@@ -75,8 +71,6 @@ class ConstrainedWindowGtk : public ConstrainedWindow {
   // Stores if |ShowConstrainedWindow()| has been called.
   bool visible_;
 
-  GtkAccelGroup* accel_group_;
-
   DISALLOW_COPY_AND_ASSIGN(ConstrainedWindowGtk);
 };
 
diff --git a/chrome/browser/gtk/focus_store_gtk.cc b/chrome/browser/gtk/focus_store_gtk.cc
index dc099fc..50ebfa7 100644
--- a/chrome/browser/gtk/focus_store_gtk.cc
+++ b/chrome/browser/gtk/focus_store_gtk.cc
@@ -19,19 +19,22 @@ FocusStoreGtk::~FocusStoreGtk() {
 }
 
 void FocusStoreGtk::Store(GtkWidget* widget) {
-  DisconnectDestroyHandler();
-  if (!widget) {
-    widget_ = NULL;
-    return;
+  GtkWidget* focus_widget = NULL;
+  if (widget) {
+    GtkWindow* window = platform_util::GetTopLevel(widget);
+    if (window)
+      focus_widget = window->focus_widget;
   }
 
-  GtkWindow* window = platform_util::GetTopLevel(widget);
-  if (!window) {
-    widget_ = NULL;
-    return;
-  }
+  SetWidget(focus_widget);
+}
 
-  widget_ = window->focus_widget;
+void FocusStoreGtk::SetWidget(GtkWidget* widget) {
+  DisconnectDestroyHandler();
+
+  // We don't add a ref. The signal handler below effectively gives us a weak
+  // reference.
+  widget_ = widget;
   if (widget_) {
     // When invoked, |gtk_widget_destroyed| will set |widget_| to NULL.
     destroy_handler_id_ = g_signal_connect(widget_, "destroy",
@@ -41,6 +44,8 @@ void FocusStoreGtk::Store(GtkWidget* widget) {
 }
 
 void FocusStoreGtk::DisconnectDestroyHandler() {
-  if (widget_)
+  if (widget_) {
     g_signal_handler_disconnect(widget_, destroy_handler_id_);
+    widget_ = NULL;
+  }
 }
diff --git a/chrome/browser/gtk/focus_store_gtk.h b/chrome/browser/gtk/focus_store_gtk.h
index ce3b68b..2159f1e 100644
--- a/chrome/browser/gtk/focus_store_gtk.h
+++ b/chrome/browser/gtk/focus_store_gtk.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -19,9 +19,12 @@ class FocusStoreGtk {
   GtkWidget* widget() const { return widget_; }
 
   // Save the widget that is currently focused for |widget|'s toplevel (NOT
-  // |widget|). Call with NULL to clear |widget_|.
+  // |widget|).
   void Store(GtkWidget* widget);
 
+  // Save |widget| as the focus widget. Call with NULL to clear |widget_|.
+  void SetWidget(GtkWidget* widget);
+
  private:
   // Disconnect the previous destroy handler (if any).
   void DisconnectDestroyHandler();
diff --git a/chrome/browser/gtk/gtk_util.cc b/chrome/browser/gtk/gtk_util.cc
index 5a5a815..541d2da 100644
--- a/chrome/browser/gtk/gtk_util.cc
+++ b/chrome/browser/gtk/gtk_util.cc
@@ -1059,4 +1059,11 @@ string16 GetStockPreferencesMenuLabel() {
   return preferences;
 }
 
+bool IsWidgetAncestryVisible(GtkWidget* widget) {
+  GtkWidget* parent = widget;
+  while (parent && GTK_WIDGET_VISIBLE(parent))
+    parent = parent->parent;
+  return !parent;
+}
+
 }  // namespace gtk_util
diff --git a/chrome/browser/gtk/gtk_util.h b/chrome/browser/gtk/gtk_util.h
index 20d3c6a..95200d4 100644
--- a/chrome/browser/gtk/gtk_util.h
+++ b/chrome/browser/gtk/gtk_util.h
@@ -315,6 +315,10 @@ gfx::Rect GetDialogBounds(GtkWidget* dialog);
 // empty string if no stock item found.
 string16 GetStockPreferencesMenuLabel();
 
+// Checks whether a widget is actually visible, i.e. whether it and all its
+// ancestors up to its toplevel are visible.
+bool IsWidgetAncestryVisible(GtkWidget* widget);
+
 }  // namespace gtk_util
 
 #endif  // CHROME_BROWSER_GTK_GTK_UTIL_H_
diff --git a/chrome/browser/login_prompt_gtk.cc b/chrome/browser/login_prompt_gtk.cc
index c3e89d0..a9cfb4b 100644
--- a/chrome/browser/login_prompt_gtk.cc
+++ b/chrome/browser/login_prompt_gtk.cc
@@ -7,6 +7,7 @@
 #include <gtk/gtk.h>
 
 #include "app/l10n_util.h"
+#include "app/gtk_signal.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/gtk/constrained_window_gtk.h"
@@ -16,6 +17,7 @@
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
 #include "chrome/browser/tab_contents/navigation_controller.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
+#include "chrome/browser/tab_contents/tab_contents_view_gtk.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/common/notification_service.h"
 #include "grit/generated_resources.h"
@@ -89,15 +91,15 @@ class LoginHandlerGtk : public LoginHandler,
     gtk_button_set_label(
         GTK_BUTTON(ok_),
         l10n_util::GetStringUTF8(IDS_LOGIN_DIALOG_OK_BUTTON_LABEL).c_str());
-    g_signal_connect(ok_, "clicked", G_CALLBACK(OnOKClicked), this);
+    g_signal_connect(ok_, "clicked", G_CALLBACK(OnOKClickedThunk), this);
     gtk_box_pack_end(GTK_BOX(hbox), ok_, FALSE, FALSE, 0);
 
     GtkWidget* cancel = gtk_button_new_from_stock(GTK_STOCK_CANCEL);
-    g_signal_connect(cancel, "clicked", G_CALLBACK(OnCancelClicked), this);
+    g_signal_connect(cancel, "clicked", G_CALLBACK(OnCancelClickedThunk), this);
     gtk_box_pack_end(GTK_BOX(hbox), cancel, FALSE, FALSE, 0);
 
     g_signal_connect(root_.get(), "hierarchy-changed",
-                     G_CALLBACK(OnPromptShown), this);
+                     G_CALLBACK(OnPromptHierarchyChangedThunk), this);
 
     SetModel(manager);
 
@@ -129,34 +131,10 @@ class LoginHandlerGtk : public LoginHandler,
  private:
   friend class LoginPrompt;
 
-  static void OnOKClicked(GtkButton *button, LoginHandlerGtk* handler) {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-
-    handler->SetAuth(
-        UTF8ToWide(gtk_entry_get_text(GTK_ENTRY(handler->username_entry_))),
-        UTF8ToWide(gtk_entry_get_text(GTK_ENTRY(handler->password_entry_))));
-  }
-
-  static void OnCancelClicked(GtkButton *button, LoginHandlerGtk* handler) {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-
-    handler->CancelAuth();
-  }
-
-  static void OnPromptShown(GtkButton* root,
-                            GtkWidget* previous_toplevel,
-                            LoginHandlerGtk* handler) {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-
-    if (!GTK_WIDGET_TOPLEVEL(gtk_widget_get_toplevel(handler->ok_)))
-      return;
-
-    // Now that we have attached ourself to the window, we can make our OK
-    // button the default action and mess with the focus.
-    GTK_WIDGET_SET_FLAGS(handler->ok_, GTK_CAN_DEFAULT);
-    gtk_widget_grab_default(handler->ok_);
-    gtk_widget_grab_focus(handler->username_entry_);
-  }
+  CHROMEGTK_CALLBACK_0(LoginHandlerGtk, void, OnOKClicked);
+  CHROMEGTK_CALLBACK_0(LoginHandlerGtk, void, OnCancelClicked);
+  CHROMEGTK_CALLBACK_1(LoginHandlerGtk, void, OnPromptHierarchyChanged,
+                       GtkWidget*);
 
   // The GtkWidgets that form our visual hierarchy:
   // The root container we pass to our parent.
@@ -170,6 +148,42 @@ class LoginHandlerGtk : public LoginHandler,
   DISALLOW_COPY_AND_ASSIGN(LoginHandlerGtk);
 };
 
+void LoginHandlerGtk::OnOKClicked(GtkWidget* sender) {
+  SetAuth(
+      UTF8ToWide(gtk_entry_get_text(GTK_ENTRY(username_entry_))),
+      UTF8ToWide(gtk_entry_get_text(GTK_ENTRY(password_entry_))));
+}
+
+void LoginHandlerGtk::OnCancelClicked(GtkWidget* sender) {
+  CancelAuth();
+}
+
+void LoginHandlerGtk::OnPromptHierarchyChanged(GtkWidget* sender,
+                                               GtkWidget* previous_toplevel) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
+  if (!GTK_WIDGET_TOPLEVEL(gtk_widget_get_toplevel(ok_)))
+    return;
+
+  // Now that we have attached ourself to the window, we can make our OK
+  // button the default action and mess with the focus.
+  GTK_WIDGET_SET_FLAGS(ok_, GTK_CAN_DEFAULT);
+  gtk_widget_grab_default(ok_);
+
+  // The user may have focused another tab. In this case do not grab focus
+  // until this tab is refocused.
+  if (gtk_util::IsWidgetAncestryVisible(username_entry_)) {
+    gtk_widget_grab_focus(username_entry_);
+  } else {
+  // TODO(estade): this define should not need to be here because this class
+  // should not be used on linux/views.
+#if defined(TOOLKIT_GTK)
+    static_cast<TabContentsViewGtk*>(GetTabContentsForLogin()->view())->
+        SetFocusedWidget(username_entry_);
+#endif
+  }
+}
+
 // static
 LoginHandler* LoginHandler::Create(net::AuthChallengeInfo* auth_info,
                                    URLRequest* request) {
diff --git a/chrome/browser/tab_contents/tab_contents_view_gtk.cc b/chrome/browser/tab_contents/tab_contents_view_gtk.cc
index 9787050..11a0da0 100644
--- a/chrome/browser/tab_contents/tab_contents_view_gtk.cc
+++ b/chrome/browser/tab_contents/tab_contents_view_gtk.cc
@@ -41,25 +41,6 @@ using WebKit::WebDragOperationsMask;
 
 namespace {
 
-// Called when the content view gtk widget is tabbed to, or after the call to
-// gtk_widget_child_focus() in TakeFocus(). We return true
-// and grab focus if we don't have it. The call to
-// FocusThroughTabTraversal(bool) forwards the "move focus forward" effect to
-// webkit.
-gboolean OnFocus(GtkWidget* widget, GtkDirectionType focus,
-                 TabContents* tab_contents) {
-  // If we already have focus, let the next widget have a shot at it. We will
-  // reach this situation after the call to gtk_widget_child_focus() in
-  // TakeFocus().
-  if (gtk_widget_is_focus(widget))
-    return FALSE;
-
-  gtk_widget_grab_focus(widget);
-  bool reverse = focus == GTK_DIR_TAB_BACKWARD;
-  tab_contents->FocusThroughTabTraversal(reverse);
-  return TRUE;
-}
-
 // Called when the mouse leaves the widget. We notify our delegate.
 gboolean OnLeaveNotify(GtkWidget* widget, GdkEventCrossing* event,
                        TabContents* tab_contents) {
@@ -165,8 +146,7 @@ RenderWidgetHostView* TabContentsViewGtk::CreateViewForWidget(
       new RenderWidgetHostViewGtk(render_widget_host);
   view->InitAsChild();
   gfx::NativeView content_view = view->native_view();
-  g_signal_connect(content_view, "focus",
-                   G_CALLBACK(OnFocus), tab_contents());
+  g_signal_connect(content_view, "focus", G_CALLBACK(OnFocusThunk), this);
   g_signal_connect(content_view, "leave-notify-event",
                    G_CALLBACK(OnLeaveNotify), tab_contents());
   g_signal_connect(content_view, "motion-notify-event",
@@ -243,7 +223,7 @@ void TabContentsViewGtk::SizeContents(const gfx::Size& size) {
 void TabContentsViewGtk::Focus() {
   if (tab_contents()->showing_interstitial_page()) {
     tab_contents()->interstitial_page()->Focus();
-  } else {
+  } else if (!constrained_window_) {
     GtkWidget* widget = GetContentNativeView();
     if (widget)
       gtk_widget_grab_focus(widget);
@@ -268,6 +248,10 @@ void TabContentsViewGtk::RestoreFocus() {
     SetInitialFocus();
 }
 
+void TabContentsViewGtk::SetFocusedWidget(GtkWidget* widget) {
+  focus_store_.SetWidget(widget);
+}
+
 void TabContentsViewGtk::UpdateDragCursor(WebDragOperation operation) {
   drag_dest_->UpdateDragStatus(operation);
 }
@@ -332,6 +316,38 @@ void TabContentsViewGtk::InsertIntoContentArea(GtkWidget* widget) {
   gtk_container_add(GTK_CONTAINER(expanded_), widget);
 }
 
+// Called when the content view gtk widget is tabbed to, or after the call to
+// gtk_widget_child_focus() in TakeFocus(). We return true
+// and grab focus if we don't have it. The call to
+// FocusThroughTabTraversal(bool) forwards the "move focus forward" effect to
+// webkit.
+gboolean TabContentsViewGtk::OnFocus(GtkWidget* widget,
+                                     GtkDirectionType focus) {
+  // If we are showing a constrained window, don't allow the native view to take
+  // focus.
+  if (constrained_window_) {
+    // If we return false, it will revert to the default handler, which will
+    // take focus. We don't want that. But if we return true, the event will
+    // stop being propagated, leaving focus wherever it is currently. That is
+    // also bad. So we return false to let the default handler run, but take
+    // focus first so as to trick it into thinking the view was already focused
+    // and allowing the event to propagate.
+    gtk_widget_grab_focus(widget);
+    return FALSE;
+  }
+
+  // If we already have focus, let the next widget have a shot at it. We will
+  // reach this situation after the call to gtk_widget_child_focus() in
+  // TakeFocus().
+  if (gtk_widget_is_focus(widget))
+    return FALSE;
+
+  gtk_widget_grab_focus(widget);
+  bool reverse = focus == GTK_DIR_TAB_BACKWARD;
+  tab_contents()->FocusThroughTabTraversal(reverse);
+  return TRUE;
+}
+
 gboolean TabContentsViewGtk::OnMouseDown(GtkWidget* widget,
                                          GdkEventButton* event) {
   last_mouse_down_ = *event;
diff --git a/chrome/browser/tab_contents/tab_contents_view_gtk.h b/chrome/browser/tab_contents/tab_contents_view_gtk.h
index e99321d..990ee81 100644
--- a/chrome/browser/tab_contents/tab_contents_view_gtk.h
+++ b/chrome/browser/tab_contents/tab_contents_view_gtk.h
@@ -36,6 +36,10 @@ class TabContentsViewGtk : public TabContentsView,
   void AttachConstrainedWindow(ConstrainedWindowGtk* constrained_window);
   void RemoveConstrainedWindow(ConstrainedWindowGtk* constrained_window);
 
+  // Override the stored focus widget. This call only makes sense when the
+  // tab contents is not focused.
+  void SetFocusedWidget(GtkWidget* widget);
+
   // TabContentsView implementation --------------------------------------------
 
   virtual void CreateView(const gfx::Size& initial_size);
@@ -79,6 +83,9 @@ class TabContentsViewGtk : public TabContentsView,
 
   void CancelDragIfAny();
 
+  // Handle focus traversal on the render widget native view.
+  CHROMEGTK_CALLBACK_1(TabContentsViewGtk, gboolean, OnFocus, GtkDirectionType);
+
   // We keep track of the timestamp of the latest mousedown event.
   CHROMEGTK_CALLBACK_1(TabContentsViewGtk, gboolean, OnMouseDown,
                        GdkEventButton*);