Fix toolbar and menu bar getting stuck open in fullscreen after certain actions.

- Move menu bar locking into ScopedMenuBarLock to get a stronger
  guarantee that the menu bar is left unlocked on exiting fullscreen.

- Use a different test for whether there should be a tracking area.

- Lock and unlock the menu bar from -[FullscreenToolbarMouseTracker
  mouseEntered:] and -mouseExited: directly. Also don't remove the
  tracking area in mouseExited:, this caused a case where the tracking
  area got removed while the menu bar was still visible.

- Remove a couple of unit tests which mostly repeated existing tests but
  also tested assumptions that are no longer valid. It might be good to
  add an interactive UI test in the future, or a unit test for
  FullscreenToolbarMouseTracker.

Background
----------

The menu bar and toolbar would get stuck if you enter fullscreen and
either:

1. Move the mouse to the top of the screen, then move it down over the
   toolbar, then back up to the top of the screen, then down to the
   middle of the screen. Cause: the tracking was incorrectly removed in
   mouseExited:.

2. Move the mouse to the top of the screen, then open a menu, move the
   mouse down to the middle of the screen, and click to close the menu.
   Cause: The menu bar overlaps the window when it drops down. Moving
   the mouse over the menu bar fired mouseExited:, but the old code used
   -mouseInsideTrackingArea, which still returned YES because the mouse
   was technically inside the tracking area even though mouseExited: had
   already fired and wouldn't fire again.

Bug: 762160
Change-Id: Ia15228be8d62e16a66a712f78bd1c8fe62dbef26
Reviewed-on: https://chromium-review.googlesource.com/688414Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Sarah Chan <spqchan@chromium.org>
Commit-Queue: Sidney San Martín <sdy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505168}

chrome/browser/ui/BUILD.gn

@@ -2434,6 +2434,8 @@ split_static_library("ui") {
       "cocoa/profiles/profile_menu_controller.mm",
       "cocoa/renderer_context_menu/render_view_context_menu_mac.h",
       "cocoa/renderer_context_menu/render_view_context_menu_mac.mm",
+      "cocoa/scoped_menu_bar_lock.h",
+      "cocoa/scoped_menu_bar_lock.mm",
       "cocoa/status_icons/status_icon_mac.h",
       "cocoa/status_icons/status_icon_mac.mm",
       "cocoa/status_icons/status_tray_mac.h",

chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.h

@@ -60,11 +60,6 @@ struct FullscreenToolbarLayout {
   // Whether or not we are in fullscreen mode.
   BOOL inFullscreenMode_;
 
-  // Whether the menu bar is currently locked in the visible position (while
-  // the mouse is over the toolbar). AppKit counts lock/unlock calls, so it's
-  // important that locks/unlocks are balanced.
-  BOOL menubarLocked_;
-
   // Updates the fullscreen toolbar layout for changes in the menubar. This
   // object is only set when the browser is in fullscreen mode.
   base::scoped_nsobject<FullscreenMenubarTracker> menubarTracker_;
@@ -76,8 +71,9 @@ struct FullscreenToolbarLayout {
   // Manages the toolbar animations for the TOOLBAR_HIDDEN style.
   std::unique_ptr<FullscreenToolbarAnimationController> animationController_;
 
-  // Mouse tracker to track the user's interactions with the toolbar. This
-  // object is only set when the browser is in fullscreen mode.
+  // When the menu bar and toolbar are visible, creates a tracking area which
+  // is used to keep them visible until the mouse moves far enough away from
+  // them. Only set when the browser is in fullscreen mode.
   base::scoped_nsobject<FullscreenToolbarMouseTracker> mouseTracker_;
 
   // Controller for immersive fullscreen.

chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm

@@ -17,27 +17,12 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 
-@interface NSMenu (PrivateAPI)
-- (void)_lockMenuPosition;
-- (void)_unlockMenuPosition;
-@end
-
 namespace {
 
 // Visibility fractions for the menubar and toolbar.
 const CGFloat kHideFraction = 0.0;
 const CGFloat kShowFraction = 1.0;
 
-void LockMenu() {
-  if ([NSMenu instancesRespondToSelector:@selector(_lockMenuPosition)])
-    [[NSApp mainMenu] _lockMenuPosition];
-}
-
-void UnlockMenu() {
-  if ([NSMenu instancesRespondToSelector:@selector(_unlockMenuPosition)])
-    [[NSApp mainMenu] _unlockMenuPosition];
-}
-
 }  // namespace
 
 @implementation FullscreenToolbarController
@@ -74,8 +59,7 @@ void UnlockMenu() {
     menubarTracker_.reset([[FullscreenMenubarTracker alloc]
         initWithFullscreenToolbarController:self]);
     mouseTracker_.reset([[FullscreenToolbarMouseTracker alloc]
-        initWithFullscreenToolbarController:self
-                        animationController:animationController_.get()]);
+        initWithFullscreenToolbarController:self]);
   }
 }
 
@@ -157,7 +141,6 @@ void UnlockMenu() {
 
   FullscreenMenubarState menubarState = [menubarTracker_ state];
   return menubarState == FullscreenMenubarState::SHOWN ||
-         [mouseTracker_ mouseInsideTrackingArea] ||
          [visibilityLockController_ isToolbarVisibilityLocked];
 }
 
@@ -184,16 +167,6 @@ void UnlockMenu() {
 }
 
 - (void)updateToolbarLayout {
-  if ([mouseTracker_ mouseInsideTrackingArea]) {
-    if (!menubarLocked_)
-      LockMenu();
-    menubarLocked_ = YES;
-  } else {
-    if (menubarLocked_)
-      UnlockMenu();
-    menubarLocked_ = NO;
-  }
-
   [browserController_ layoutSubviews];
   animationController_->ToolbarDidUpdate();
   [mouseTracker_ updateTrackingArea];

chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm

@@ -18,34 +18,6 @@
 #import "third_party/ocmock/OCMock/OCMock.h"
 #include "ui/base/cocoa/appkit_utils.h"
 
-//////////////////////////////////////////////////////////////////
-// MockFullscreenToolbarMouseTracker
-// Mocks the mouse interactions with the toolbar.
-
-@interface MockFullscreenToolbarMouseTracker : FullscreenToolbarMouseTracker
-
-@property(nonatomic, assign) BOOL mouseInside;
-
-// Overridden to prevent a tracking area from being created.
-- (void)updateTrackingArea;
-
-- (BOOL)mouseInsideTrackingArea;
-
-@end
-
-@implementation MockFullscreenToolbarMouseTracker
-
-@synthesize mouseInside = mouseInside_;
-
-- (void)updateTrackingArea {
-}
-
-- (BOOL)mouseInsideTrackingArea {
-  return mouseInside_;
-}
-
-@end
-
 //////////////////////////////////////////////////////////////////
 // MockFullscreenMenubarTracker
 // Mocks the state of the menubar.
@@ -85,11 +57,6 @@
 
 @end
 
-@interface NSMenu (PrivateAPI)
-- (void)_lockMenuPosition;
-- (void)_unlockMenuPosition;
-@end
-
 #define CHECK_LAYOUT(TOOLBAR_FRACTION, MENUBAR_FRACTION)                       \
   {                                                                            \
     FullscreenToolbarLayout layout = [controller_ computeLayout];              \
@@ -124,7 +91,9 @@ class FullscreenToolbarControllerTest : public testing::Test {
     [menubar_tracker_ setMenubarProgress:0.0];
     [controller_ setMenubarTracker:menubar_tracker_];
 
-    mouse_tracker_.reset([[MockFullscreenToolbarMouseTracker alloc] init]);
+    mouse_tracker_ =
+        [OCMockObject mockForClass:[FullscreenToolbarMouseTracker class]];
+    [[mouse_tracker_ stub] updateTrackingArea];
     [controller_ setMouseTracker:mouse_tracker_];
 
     [controller_ animationController]->SetAnimationDuration(0.0);
@@ -142,15 +111,15 @@ class FullscreenToolbarControllerTest : public testing::Test {
   // A mock BrowserWindowController object.
   id bwc_;
 
+  // A mock FullscreenToolbarMouseTracker object.
+  id mouse_tracker_;
+
   // The FullscreenToolbarController object being tested.
   base::scoped_nsobject<FullscreenToolbarController> controller_;
 
   // Mocks the state of the menubar.
   base::scoped_nsobject<MockFullscreenMenubarTracker> menubar_tracker_;
 
-  // Mocks the mouse interactions with the toolbar.
-  base::scoped_nsobject<MockFullscreenToolbarMouseTracker> mouse_tracker_;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(FullscreenToolbarControllerTest);
 };
@@ -238,77 +207,4 @@ TEST_F(FullscreenToolbarControllerTest, TestHiddenToolbarWithVisibilityLocks) {
   CHECK_LAYOUT(0, 0);
 }
 
-// Basic test that checks the toolbar fraction for different mouse tracking
-// values.
-TEST_F(FullscreenToolbarControllerTest, TestHiddenToolbarWithMouseTracking) {
-  CHECK_LAYOUT(0, 0);
-
-  [mouse_tracker_ setMouseInside:YES];
-  CHECK_LAYOUT(1, 0);
-
-  [menubar_tracker_ setMenubarProgress:1];
-  CHECK_LAYOUT(1, 1);
-
-  [menubar_tracker_ setMenubarProgress:0];
-  CHECK_LAYOUT(1, 0);
-
-  [mouse_tracker_ setMouseInside:NO];
-  CHECK_LAYOUT(0, 0);
-}
-
-// Test that checks the toolbar fraction with mouse tracking, menubar fraction,
-// and visibility locks.
-TEST_F(FullscreenToolbarControllerTest, TestHiddenToolbarWithMultipleFactors) {
-  FullscreenToolbarVisibilityLockController* locks =
-      [controller_ visibilityLockController];
-  base::scoped_nsobject<NSObject> owner([[NSObject alloc] init]);
-  CHECK_LAYOUT(0, 0);
-
-  // Toolbar should be shown with the menubar.
-  [menubar_tracker_ setMenubarProgress:1];
-  CHECK_LAYOUT(1, 1);
-
-  // Move the mouse to the toolbar and start hiding the menubar. Toolbar
-  // should be fully visible.
-  [mouse_tracker_ setMouseInside:YES];
-  CHECK_LAYOUT(1, 1);
-  [menubar_tracker_ setMenubarProgress:0.5];
-  CHECK_LAYOUT(1, 0.5);
-
-  // Lock the toolbar's visibility.
-  [locks lockToolbarVisibilityForOwner:owner.get() withAnimation:NO];
-  CHECK_LAYOUT(1, 0.5);
-
-  // Hide the menubar. Toolbar should be fully visible.
-  [menubar_tracker_ setMenubarProgress:0];
-  CHECK_LAYOUT(1, 0);
-
-  // Lock the toolbar's visibility. Toolbar should be fully visible.
-  [locks releaseToolbarVisibilityForOwner:owner.get() withAnimation:NO];
-  CHECK_LAYOUT(1, 0);
-
-  // Move the mouse away from the toolbar. Toolbar should hide.
-  [mouse_tracker_ setMouseInside:NO];
-  CHECK_LAYOUT(0, 0);
-
-  // Lock the toolbar and move the mouse to it.
-  [locks lockToolbarVisibilityForOwner:owner.get() withAnimation:NO];
-  [mouse_tracker_ setMouseInside:YES];
-  CHECK_LAYOUT(1, 0);
-
-  // Move the mouse away from the toolbar. Toolbar should be fully visible.
-  [mouse_tracker_ setMouseInside:NO];
-  CHECK_LAYOUT(1, 0);
-
-  // Release the toolbar. Toolbar should be hidden.
-  [locks releaseToolbarVisibilityForOwner:owner.get() withAnimation:NO];
-}
-
-// Verify that private methods used by FullscreenToolbarController still exist.
-TEST_F(FullscreenToolbarControllerTest, PrivateAPIs) {
-  EXPECT_TRUE([NSMenu instancesRespondToSelector:@selector(_lockMenuPosition)]);
-  EXPECT_TRUE(
-      [NSMenu instancesRespondToSelector:@selector(_unlockMenuPosition)]);
-}
-
 }  // namespace

chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_mouse_tracker.h

@@ -7,17 +7,14 @@
 
 #import <Cocoa/Cocoa.h>
 
-class FullscreenToolbarAnimationController;
 @class FullscreenToolbarController;
 
 // Class that tracks mouse interactions with the fullscreen toolbar.
 @interface FullscreenToolbarMouseTracker : NSObject
 
 // Designated initializer.
-- (instancetype)
-initWithFullscreenToolbarController:(FullscreenToolbarController*)owner
-                animationController:
-                    (FullscreenToolbarAnimationController*)animationController;
+- (instancetype)initWithFullscreenToolbarController:
+    (FullscreenToolbarController*)owner;
 
 // Updates the tracking area's frame to the given toolbar frame.
 - (void)updateToolbarFrame:(NSRect)frame;
@@ -28,9 +25,6 @@ initWithFullscreenToolbarController:(FullscreenToolbarController*)owner
 // Removes the tracking area.
 - (void)removeTrackingArea;
 
-// Returns YES if the mouse is inside the tracking area.
-- (BOOL)mouseInsideTrackingArea;
-
 @end
 
-#endif  // CHROME_BROWSER_UI_COCOA_FULLSCREEN_FULLSCREEN_TOOLBAR_MOUSE_TRACKER_H_
\ No newline at end of file
+#endif  // CHROME_BROWSER_UI_COCOA_FULLSCREEN_FULLSCREEN_TOOLBAR_MOUSE_TRACKER_H_

chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_mouse_tracker.mm

@@ -5,9 +5,8 @@
 #import "chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_mouse_tracker.h"
 
 #include "chrome/browser/ui/cocoa/browser_window_controller.h"
-#include "chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h"
 #import "chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.h"
-#include "ui/base/cocoa/appkit_utils.h"
+#include "chrome/browser/ui/cocoa/scoped_menu_bar_lock.h"
 #import "ui/base/cocoa/tracking_area.h"
 
 namespace {
@@ -29,27 +28,24 @@ const CGFloat kTrackingAreaAdditionalThreshold = 50;
   // is still on the toolbar.
   base::scoped_nsobject<CrTrackingArea> trackingArea_;
 
+  // Keeps the menu bar from hiding until the mouse exits the tracking area.
+  std::unique_ptr<ScopedMenuBarLock> menuBarLock_;
+
   // The content view for the window.
   NSView* contentView_;  // weak
 
   // The owner of this class.
   FullscreenToolbarController* owner_;  // weak
-
-  // The object managing the fullscreen toolbar's animations.
-  FullscreenToolbarAnimationController* animationController_;  // weak
 }
 
 @end
 
 @implementation FullscreenToolbarMouseTracker
 
-- (instancetype)
-initWithFullscreenToolbarController:(FullscreenToolbarController*)owner
-                animationController:
-                    (FullscreenToolbarAnimationController*)animationController {
+- (instancetype)initWithFullscreenToolbarController:
+    (FullscreenToolbarController*)owner {
   if ((self = [super init])) {
     owner_ = owner;
-    animationController_ = animationController;
   }
 
   return self;
@@ -61,9 +57,10 @@ initWithFullscreenToolbarController:(FullscreenToolbarController*)owner
 }
 
 - (void)updateTrackingArea {
-  // Remove the tracking area if the toolbar isn't fully shown.
-  if (!ui::IsCGFloatEqual([owner_ toolbarFraction], 1.0)) {
+  // Remove the tracking area if the toolbar and menu bar aren't both visible.
+  if ([owner_ toolbarFraction] == 0 || ![NSMenu menuBarVisible]) {
     [self removeTrackingArea];
+    menuBarLock_.reset();
     return;
   }
 
@@ -108,21 +105,12 @@ initWithFullscreenToolbarController:(FullscreenToolbarController*)owner
   contentView_ = nil;
 }
 
-- (BOOL)mouseInsideTrackingArea {
-  return [trackingArea_ mouseInsideTrackingAreaForView:contentView_];
-}
-
 - (void)mouseEntered:(NSEvent*)event {
-  // Empty implementation. Required for CrTrackingArea.
+  menuBarLock_.reset(new ScopedMenuBarLock());
 }
 
 - (void)mouseExited:(NSEvent*)event {
-  DCHECK_EQ([event trackingArea], trackingArea_.get());
-
-  animationController_->AnimateToolbarOutIfPossible();
-
-  [owner_ updateToolbarLayout];
-  [self removeTrackingArea];
+  menuBarLock_.reset();
 }
 
 @end

chrome/browser/ui/cocoa/scoped_menu_bar_lock.h

+// Copyright 2017 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.
+
+#ifndef CHROME_BROWSER_UI_COCOA_SCOPED_MENU_BAR_LOCK_H_
+#define CHROME_BROWSER_UI_COCOA_SCOPED_MENU_BAR_LOCK_H_
+
+#include "base/macros.h"
+
+// ScopedMenuBarLock uses an AppKit API to lock the menu bar in its current
+// state (visible/hidden). Useful to temporarily keep the menu bar visible
+// after appearing when the user moved the mouse to the top of the screen, or
+// to temporarily prevent the menu bar from showing automatically.
+// ScopedMenuBarLock helps enforce that lock/unlock calls are balanced.
+class ScopedMenuBarLock {
+ public:
+  ScopedMenuBarLock();
+  ~ScopedMenuBarLock();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ScopedMenuBarLock);
+};
+
+#endif  // CHROME_BROWSER_UI_COCOA_SCOPED_MENU_BAR_LOCK_H_

chrome/browser/ui/cocoa/scoped_menu_bar_lock.mm

+// Copyright 2017 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.
+
+#import "chrome/browser/ui/cocoa/scoped_menu_bar_lock.h"
+
+#import <AppKit/AppKit.h>
+
+@interface NSMenu (PrivateAPI)
+- (void)_lockMenuPosition;
+- (void)_unlockMenuPosition;
+@end
+
+ScopedMenuBarLock::ScopedMenuBarLock() {
+  if ([NSMenu instancesRespondToSelector:@selector(_lockMenuPosition)])
+    [[NSApp mainMenu] _lockMenuPosition];
+}
+
+ScopedMenuBarLock::~ScopedMenuBarLock() {
+  if ([NSMenu instancesRespondToSelector:@selector(_unlockMenuPosition)])
+    [[NSApp mainMenu] _unlockMenuPosition];
+}

chrome/browser/ui/cocoa/scoped_menu_bar_lock_unittest.mm

+// Copyright 2017 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.
+
+#import "chrome/browser/ui/cocoa/scoped_menu_bar_lock.h"
+
+#import <AppKit/AppKit.h>
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+// Verify that creating and tearing down a ScopedMenuBarLock doesn't crash.
+TEST(ScopedMenuBarLockTest, CreateAndDestroy) {
+  ScopedMenuBarLock menuBarLock;
+}
+
+// Verify that the API still exists.
+TEST(ScopedMenuBarLockTest, PrivateAPIs) {
+  EXPECT_TRUE([NSMenu instancesRespondToSelector:@selector(_lockMenuPosition)]);
+  EXPECT_TRUE(
+      [NSMenu instancesRespondToSelector:@selector(_unlockMenuPosition)]);
+}

chrome/test/BUILD.gn

@@ -4761,6 +4761,7 @@ test("unit_tests") {
         "../browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm",
         "../browser/ui/cocoa/profiles/profile_menu_controller_unittest.mm",
         "../browser/ui/cocoa/profiles/user_manager_mac_unittest.mm",
+        "../browser/ui/cocoa/scoped_menu_bar_lock_unittest.mm",
         "../browser/ui/cocoa/screen_capture_notification_ui_cocoa_unittest.mm",
         "../browser/ui/cocoa/spinner_view_unittest.mm",
         "../browser/ui/cocoa/sprite_view_unittest.mm",