Mac: Ignore sheets when deciding whether to dismiss a browser action popup.

Since Chrome 8, invoking <input type="file"> in a browser action popup
on OSX would show, then immediately close the sheet (and the popup).

This CL fixes it by ignoring loss of key status on the bubble window
when it has an attached sheet. Loss of key status (or clicks) on windows
that are sheets themselves are also ignored, otherwise dismissing the
sheet would close the bubble as well, since it's already lost its "sheet
attached" status by that stage.

BUG=61632

Review URL: https://codereview.chromium.org/576643002

Cr-Commit-Position: refs/heads/master@{#295198}

chrome/browser/ui/cocoa/base_bubble_controller.mm

@@ -241,19 +241,26 @@
 - (void)windowDidResignKey:(NSNotification*)notification {
   NSWindow* window = [self window];
   DCHECK_EQ([notification object], window);
-  if ([window isVisible] && [self shouldCloseOnResignKey]) {
-    // If the window isn't visible, it is already closed, and this notification
-    // has been sent as part of the closing operation, so no need to close.
+
+  // If the window isn't visible, it is already closed, and this notification
+  // has been sent as part of the closing operation, so no need to close.
+  if (![window isVisible])
+    return;
+
+  // Don't close when explicily disabled, or if there's an attached sheet (e.g.
+  // Open File dialog).
+  if ([self shouldCloseOnResignKey] && ![window attachedSheet]) {
     [self close];
-  } else if ([window isVisible]) {
-    // The bubble should not receive key events when it is no longer key window,
-    // so disable sharing parent key state. Share parent key state is only used
-    // to enable the close/minimize/maximize buttons of the parent window when
-    // the bubble has key state, so disabling it here is safe.
-    InfoBubbleWindow* bubbleWindow =
-        base::mac::ObjCCastStrict<InfoBubbleWindow>([self window]);
-    [bubbleWindow setAllowShareParentKeyState:NO];
+    return;
   }
+
+  // The bubble should not receive key events when it is no longer key window,
+  // so disable sharing parent key state. Share parent key state is only used
+  // to enable the close/minimize/maximize buttons of the parent window when
+  // the bubble has key state, so disabling it here is safe.
+  InfoBubbleWindow* bubbleWindow =
+      base::mac::ObjCCastStrict<InfoBubbleWindow>([self window]);
+  [bubbleWindow setAllowShareParentKeyState:NO];
 }
 
 - (void)windowDidBecomeKey:(NSNotification*)notification {
@@ -264,9 +271,13 @@
   [bubbleWindow setAllowShareParentKeyState:YES];
 }
 
-// Since the bubble shares first responder with its parent window, set
-// event handlers to dismiss the bubble when it would normally lose key
-// state.
+// Since the bubble shares first responder with its parent window, set event
+// handlers to dismiss the bubble when it would normally lose key state.
+// Events on sheets are ignored: this assumes the sheet belongs to the bubble
+// since, to affect a sheet on a different window, the bubble would also lose
+// key status in -[NSWindowDelegate windowDidResignKey:]. This keeps the logic
+// simple, since -[NSWindow attachedSheet] returns nil while the sheet is still
+// closing.
 - (void)registerKeyStateEventTap {
   // Parent key state sharing is only avaiable on 10.7+.
   if (!base::mac::IsOSLionOrLater())
@@ -283,7 +294,7 @@
       addLocalMonitorForEventsMatchingMask:NSLeftMouseDownMask |
                                            NSRightMouseDownMask
       handler:^NSEvent* (NSEvent* event) {
-          if (event.window != window) {
+          if ([event window] != window && ![[event window] isSheet]) {
             // Do it right now, because if this event is right mouse event,
             // it may pop up a menu. windowDidResignKey: will not run until
             // the menu is closed.
@@ -302,7 +313,8 @@
                           object:nil
                            queue:[NSOperationQueue mainQueue]
                       usingBlock:^(NSNotification* notif) {
-                          [self windowDidResignKey:note];
+                          if (![[notif object] isSheet])
+                            [self windowDidResignKey:note];
                       }];
 }
 

chrome/browser/ui/cocoa/base_bubble_controller_unittest.mm

@@ -86,33 +86,68 @@ const CGFloat kAnchorPointY = 300;
 
 class BaseBubbleControllerTest : public CocoaTest {
  public:
+  BaseBubbleControllerTest() : controller_(nil) {}
+
   virtual void SetUp() OVERRIDE {
-    bubbleWindow_.reset([[InfoBubbleWindow alloc]
+    bubble_window_.reset([[InfoBubbleWindow alloc]
         initWithContentRect:NSMakeRect(0, 0, kBubbleWindowWidth,
                                        kBubbleWindowHeight)
                   styleMask:NSBorderlessWindowMask
                     backing:NSBackingStoreBuffered
                       defer:YES]);
-    [bubbleWindow_ setAllowedAnimations:0];
+    [bubble_window_ setAllowedAnimations:0];
 
     // The bubble controller will release itself when the window closes.
     controller_ = [[BaseBubbleController alloc]
-        initWithWindow:bubbleWindow_.get()
+        initWithWindow:bubble_window_
           parentWindow:test_window()
             anchoredAt:NSMakePoint(kAnchorPointX, kAnchorPointY)];
     EXPECT_TRUE([controller_ bubble]);
+    EXPECT_EQ(bubble_window_.get(), [controller_ window]);
   }
 
   virtual void TearDown() OVERRIDE {
     // Close our windows.
     [controller_ close];
-    bubbleWindow_.reset(NULL);
+    bubble_window_.reset();
     CocoaTest::TearDown();
   }
 
- public:
-  base::scoped_nsobject<InfoBubbleWindow> bubbleWindow_;
+  // Closing the bubble will autorelease the controller. Give callers a keep-
+  // alive to run checks after closing.
+  base::scoped_nsobject<BaseBubbleController> ShowBubble() WARN_UNUSED_RESULT {
+    base::scoped_nsobject<BaseBubbleController> keep_alive(
+        [controller_ retain]);
+    EXPECT_FALSE([bubble_window_ isVisible]);
+    [controller_ showWindow:nil];
+    EXPECT_TRUE([bubble_window_ isVisible]);
+    return keep_alive;
+  }
+
+  // Fake the key state notification. Because unit_tests is a "daemon" process
+  // type, its windows can never become key (nor can the app become active).
+  // Instead of the hacks below, one could make a browser_test or transform the
+  // process type, but this seems easiest and is best suited to a unit test.
+  //
+  // On Lion and above, which have the event taps, simply post a notification
+  // that will cause the controller to call |-windowDidResignKey:|. Earlier
+  // OSes can call through directly.
+  void SimulateKeyStatusChange() {
+    NSNotification* notif =
+        [NSNotification notificationWithName:NSWindowDidResignKeyNotification
+                                      object:[controller_ window]];
+    if (base::mac::IsOSLionOrLater())
+      [[NSNotificationCenter defaultCenter] postNotification:notif];
+    else
+      [controller_ windowDidResignKey:notif];
+  }
+
+ protected:
+  base::scoped_nsobject<InfoBubbleWindow> bubble_window_;
   BaseBubbleController* controller_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BaseBubbleControllerTest);
 };
 
 // Test that kAlignEdgeToAnchorEdge and a left bubble arrow correctly aligns the
@@ -200,42 +235,19 @@ TEST_F(BaseBubbleControllerTest, AnchorAlignCenterArrow) {
 // Tests that when a new window gets key state (and the bubble resigns) that
 // the key window changes.
 TEST_F(BaseBubbleControllerTest, ResignKeyCloses) {
-  // Closing the bubble will autorelease the controller.
-  base::scoped_nsobject<BaseBubbleController> keep_alive([controller_ retain]);
-
-  NSWindow* bubble_window = [controller_ window];
-  EXPECT_FALSE([bubble_window isVisible]);
-
   base::scoped_nsobject<NSWindow> other_window(
       [[NSWindow alloc] initWithContentRect:NSMakeRect(500, 500, 500, 500)
                                   styleMask:NSTitledWindowMask
                                     backing:NSBackingStoreBuffered
                                       defer:YES]);
-  EXPECT_FALSE([other_window isVisible]);
 
-  [controller_ showWindow:nil];
-  EXPECT_TRUE([bubble_window isVisible]);
+  base::scoped_nsobject<BaseBubbleController> keep_alive = ShowBubble();
   EXPECT_FALSE([other_window isVisible]);
 
   [other_window makeKeyAndOrderFront:nil];
-  // Fake the key state notification. Because unit_tests is a "daemon" process
-  // type, its windows can never become key (nor can the app become active).
-  // Instead of the hacks below, one could make a browser_test or transform the
-  // process type, but this seems easiest and is best suited to a unit test.
-  //
-  // On Lion and above, which have the event taps, simply post a notification
-  // that will cause the controller to call |-windowDidResignKey:|. Earlier
-  // OSes can call through directly.
-  NSNotification* notif =
-      [NSNotification notificationWithName:NSWindowDidResignKeyNotification
-                                    object:bubble_window];
-  if (base::mac::IsOSLionOrLater())
-    [[NSNotificationCenter defaultCenter] postNotification:notif];
-  else
-    [controller_ windowDidResignKey:notif];
+  SimulateKeyStatusChange();
 
-
-  EXPECT_FALSE([bubble_window isVisible]);
+  EXPECT_FALSE([bubble_window_ isVisible]);
   EXPECT_TRUE([other_window isVisible]);
 }
 
@@ -246,17 +258,10 @@ TEST_F(BaseBubbleControllerTest, LionClickOutsideClosesWithoutContextMenu) {
   if (!base::mac::IsOSLionOrLater())
     return;
 
-  // Closing the bubble will autorelease the controller.
-  base::scoped_nsobject<BaseBubbleController> keep_alive([controller_ retain]);
+  base::scoped_nsobject<BaseBubbleController> keep_alive = ShowBubble();
   NSWindow* window = [controller_ window];
 
   EXPECT_TRUE([controller_ shouldCloseOnResignKey]);  // Verify default value.
-  EXPECT_FALSE([window isVisible]);
-
-  [controller_ showWindow:nil];
-
-  EXPECT_TRUE([window isVisible]);
-
   [controller_ setShouldCloseOnResignKey:NO];
   NSEvent* event = cocoa_test_event_utils::LeftMouseDownAtPointInWindow(
       NSMakePoint(10, 10), test_window());
@@ -295,17 +300,9 @@ TEST_F(BaseBubbleControllerTest, LionRightClickOutsideClosesWithContextMenu) {
   if (!base::mac::IsOSLionOrLater())
     return;
 
-  // Closing the bubble will autorelease the controller.
-  base::scoped_nsobject<BaseBubbleController> keep_alive([controller_ retain]);
+  base::scoped_nsobject<BaseBubbleController> keep_alive = ShowBubble();
   NSWindow* window = [controller_ window];
 
-  EXPECT_TRUE([controller_ shouldCloseOnResignKey]);  // Verify default value.
-  EXPECT_FALSE([window isVisible]);
-
-  [controller_ showWindow:nil];
-
-  EXPECT_TRUE([window isVisible]);
-
   base::scoped_nsobject<NSMenu> context_menu(
       [[NSMenu alloc] initWithTitle:@""]);
   [context_menu addItemWithTitle:@"ContextMenuTest"
@@ -341,3 +338,46 @@ TEST_F(BaseBubbleControllerTest, LionRightClickOutsideClosesWithContextMenu) {
   EXPECT_TRUE([menu_controller didOpen]);
 }
 
+// Test that the bubble is not dismissed when it has an attached sheet, or when
+// a sheet loses key status (since the sheet is not attached when that happens).
+TEST_F(BaseBubbleControllerTest, BubbleStaysOpenWithSheet) {
+  base::scoped_nsobject<BaseBubbleController> keep_alive = ShowBubble();
+
+  // Make a dummy NSPanel for the sheet. Don't use [NSOpenPanel openPanel],
+  // otherwise a stray FI_TFloatingInputWindow is created which the unit test
+  // harness doesn't like.
+  base::scoped_nsobject<NSPanel> panel(
+      [[NSPanel alloc] initWithContentRect:NSMakeRect(0, 0, 100, 50)
+                                 styleMask:NSTitledWindowMask
+                                   backing:NSBackingStoreBuffered
+                                     defer:YES]);
+  EXPECT_FALSE([panel isReleasedWhenClosed]);  // scoped_nsobject releases it.
+
+  // With a NSOpenPanel, we would call -[NSSavePanel beginSheetModalForWindow]
+  // here. In 10.9, we would call [NSWindow beginSheet:]. For 10.6, this:
+  [[NSApplication sharedApplication] beginSheet:panel
+                                 modalForWindow:bubble_window_
+                                  modalDelegate:nil
+                                 didEndSelector:NULL
+                                    contextInfo:NULL];
+
+  EXPECT_TRUE([bubble_window_ isVisible]);
+  EXPECT_TRUE([panel isVisible]);
+  // Losing key status while there is an attached window should not close the
+  // bubble.
+  SimulateKeyStatusChange();
+  EXPECT_TRUE([bubble_window_ isVisible]);
+  EXPECT_TRUE([panel isVisible]);
+
+  // Closing the attached sheet should not close the bubble.
+  [[NSApplication sharedApplication] endSheet:panel];
+  [panel close];
+
+  EXPECT_FALSE([bubble_window_ attachedSheet]);
+  EXPECT_TRUE([bubble_window_ isVisible]);
+  EXPECT_FALSE([panel isVisible]);
+
+  // Now that the sheet is gone, a key status change should close the bubble.
+  SimulateKeyStatusChange();
+  EXPECT_FALSE([bubble_window_ isVisible]);
+}