Fix dialogs losing focus while using ⌘` to switch windows.

- If a browser window becomes key while a dialog is up and the web
  content is first responder, give focus to the dialog.

- If the web content *loses* first responder while the dialog is key,
  make the browser window key. This makes sure that clicking the omnibox
  or typing ⌘L, for instance, focuses the browser window.

The final version of this change is incomplete: The separation between
"focused" ("the web content is focused within its window") and "active" ("the
web content's window is the active window") is mostly an illusion because it
only exists on Mac and I kept running into cases where it breaks down (e.g.
setting "focused" implicitly sets "active" deep inside WebViewImpl). A bunch of
tests also check that certain functions and IPCs happen in a certain order, so
fiddling with anything just breaks a bunch of tests in ways that don't map to
real-world behavior. Now isn't the right time to mess with that, especially in
Cocoa code, so this leaves one case unfixed:

- If a dialog becomes key some other way, make sure that the web content
  is first responder in its parent window (so that focus returns to the
  web content on dismissing the dialog, and no other views have first
  responder appearance).

I filed https://crbug.com/821931 to track it.

Bug: 658405
Change-Id: I7ec11568d860e7371f552baa44a0d6b45db6dc22
Reviewed-on: https://chromium-review.googlesource.com/935424
Commit-Queue: Sidney San Martín <sdy@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543170}

chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm

@@ -6,6 +6,7 @@
 
 #include <cmath>
 
+#include "base/auto_reset.h"
 #include "base/strings/sys_string_conversions.h"
 #include "chrome/browser/devtools/devtools_window.h"
 #include "chrome/browser/profiles/profile.h"
@@ -19,6 +20,7 @@
 #include "components/spellcheck/browser/pref_names.h"
 #include "components/spellcheck/browser/spellcheck_platform.h"
 #include "components/spellcheck/common/spellcheck_panel.mojom.h"
+#include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
@@ -32,7 +34,9 @@ using content::RenderViewHost;
 @interface ChromeRenderWidgetHostViewMacDelegate () <HistorySwiperDelegate>
 @end
 
-@implementation ChromeRenderWidgetHostViewMacDelegate
+@implementation ChromeRenderWidgetHostViewMacDelegate {
+  BOOL resigningFirstResponder_;
+}
 
 - (id)initWithRenderWidgetHost:(content::RenderWidgetHost*)renderWidgetHost {
   self = [super init];
@@ -235,4 +239,66 @@ using content::RenderViewHost;
 
 // END Spellchecking methods
 
+// If a dialog is visible, make its window key. See becomeFirstResponder.
+- (void)makeAnyDialogKey {
+  if (const auto* contents = content::WebContents::FromRenderViewHost(
+          RenderViewHost::From(renderWidgetHost_))) {
+    if (const auto* manager =
+            web_modal::WebContentsModalDialogManager::FromWebContents(
+                contents)) {
+      // IsDialogActive() returns true if a dialog exists.
+      if (manager->IsDialogActive()) {
+        manager->FocusTopmostDialog();
+      }
+    }
+  }
+}
+
+// If the RenderWidgetHostView becomes first responder while it has a dialog
+// (say, if the user was interacting with the omnibox and then tabs back into
+// the web contents), then make the dialog window key.
+- (void)becomeFirstResponder {
+  [self makeAnyDialogKey];
+}
+
+// If the RenderWidgetHostView is asked to resign first responder while a child
+// window is key, then the user performed some action which targets the browser
+// window, like clicking the omnibox or typing cmd+L. In that case, the browser
+// window should become key.
+- (void)resignFirstResponder {
+  NSWindow* browserWindow =
+      renderWidgetHost_->GetView()->GetNativeView().window;
+  DCHECK(browserWindow);
+
+  // If the browser window is already key, there's nothing to do.
+  if (browserWindow.isKeyWindow)
+    return;
+
+  // Otherwise, look for it in the key window's chain of parents.
+  NSWindow* keyWindowOrParent = NSApp.keyWindow;
+  while (keyWindowOrParent && keyWindowOrParent != browserWindow)
+    keyWindowOrParent = keyWindowOrParent.parentWindow;
+
+  // If the browser window isn't among the parents, there's nothing to do.
+  if (keyWindowOrParent != browserWindow)
+    return;
+
+  // Otherwise, temporarily set an ivar so that -windowDidBecomeKey, below,
+  // doesn't immediately make the dialog key.
+  base::AutoReset<BOOL> scoped(&resigningFirstResponder_, YES);
+
+  // …then make the browser window key.
+  [browserWindow makeKeyWindow];
+}
+
+// If the browser window becomes key while the RenderWidgetHostView is first
+// responder, make the dialog key (if there is one).
+- (void)windowDidBecomeKey {
+  if (resigningFirstResponder_)
+    return;
+  NSView* view = renderWidgetHost_->GetView()->GetNativeView();
+  if (view.window.firstResponder == view)
+    [self makeAnyDialogKey];
+}
+
 @end

chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac.mm

@@ -166,7 +166,7 @@ void SingleWebContentsDialogManagerViewsMac::Close() {
 }
 
 void SingleWebContentsDialogManagerViewsMac::Focus() {
-  // Handled by ConstrainedWindowSheetController.
+  [sheet_ makeSheetKeyAndOrderFront];
 }
 void SingleWebContentsDialogManagerViewsMac::Pulse() {
   // Handled by ConstrainedWindowSheetController.

content/browser/renderer_host/render_widget_host_view_mac.mm

@@ -2818,7 +2818,9 @@ Class GetRenderWidgetHostViewCocoaClassForTesting() {
 - (void)windowDidBecomeKey:(NSNotification*)notification {
   DCHECK([self window]);
   DCHECK_EQ([self window], [notification object]);
-  if ([[self window] firstResponder] == self)
+  if ([responderDelegate_ respondsToSelector:@selector(windowDidBecomeKey)])
+    [responderDelegate_ windowDidBecomeKey];
+  if ([self window].isKeyWindow && [[self window] firstResponder] == self)
     renderWidgetHostView_->SetActive(true);
 }
 
@@ -2840,6 +2842,8 @@ Class GetRenderWidgetHostViewCocoaClassForTesting() {
 - (BOOL)becomeFirstResponder {
   if (!renderWidgetHostView_->render_widget_host_)
     return NO;
+  if ([responderDelegate_ respondsToSelector:@selector(becomeFirstResponder)])
+    [responderDelegate_ becomeFirstResponder];
 
   renderWidgetHostView_->render_widget_host_->GotFocus();
   renderWidgetHostView_->SetTextInputActive(true);
@@ -2864,6 +2868,8 @@ Class GetRenderWidgetHostViewCocoaClassForTesting() {
 }
 
 - (BOOL)resignFirstResponder {
+  if ([responderDelegate_ respondsToSelector:@selector(resignFirstResponder)])
+    [responderDelegate_ resignFirstResponder];
   renderWidgetHostView_->SetTextInputActive(false);
   if (!renderWidgetHostView_->render_widget_host_)
     return YES;

content/public/browser/render_widget_host_view_mac_delegate.h

@@ -27,22 +27,6 @@ struct DidOverscrollParams;
 
 @class NSEvent;
 @protocol RenderWidgetHostViewMacDelegate
-@optional
-// Notification that the view is gone.
-- (void)viewGone:(NSView*)view;
-
-// Handle an event. All incoming key and mouse events flow through this delegate
-// method if implemented. Return YES if the event is fully handled, or NO if
-// normal processing should take place.
-- (BOOL)handleEvent:(NSEvent*)event;
-
-// Provides validation of user interface items. If the return value is NO, then
-// the delegate is unaware of that item and |valid| is undefined.  Otherwise,
-// |valid| contains the validity of the specified item.
-- (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item
-                      isValidItem:(BOOL*)valid;
-
-@required
 // Notification of when a gesture begins/ends.
 - (void)beginGestureWithEvent:(NSEvent*)event;
 - (void)endGestureWithEvent:(NSEvent*)event;
@@ -62,6 +46,26 @@ struct DidOverscrollParams;
 - (void)rendererHandledGestureScrollEvent:(const blink::WebGestureEvent&)event
                                  consumed:(BOOL)consumed;
 - (void)rendererHandledOverscrollEvent:(const ui::DidOverscrollParams&)params;
+
+@optional
+// Notification that the view is gone.
+- (void)viewGone:(NSView*)view;
+
+// Handle an event. All incoming key and mouse events flow through this delegate
+// method if implemented. Return YES if the event is fully handled, or NO if
+// normal processing should take place.
+- (BOOL)handleEvent:(NSEvent*)event;
+
+// Provides validation of user interface items. If the return value is NO, then
+// the delegate is unaware of that item and |valid| is undefined.  Otherwise,
+// |valid| contains the validity of the specified item.
+- (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item
+                      isValidItem:(BOOL*)valid;
+
+- (void)becomeFirstResponder;
+- (void)resignFirstResponder;
+
+- (void)windowDidBecomeKey;
 @end
 
 #endif  // CONTENT_PUBLIC_BROWSER_RENDER_WIDGET_HOST_VIEW_MAC_DELEGATE_H_