[Mac] Fullscreen Toolbar Edge Cases

chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm

Index: chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm
diff --git a/chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm b/chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm
index e34cb4cb1cd6855e6bfc1b57bef253d304ea6508..e84f9ea822627483496c985b40cf39bfc6c69bfc 100644
--- a/chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm
+++ b/chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm
@@ -18,7 +18,7 @@
 namespace {
 
 // The duration of the toolbar show/hide animation.
-const NSTimeInterval kDropdownAnimationDuration = 0.20;
+const NSTimeInterval kDropdownAnimationDuration = 0.17;

[erikchen, 2016/08/30 23:46:48]

Should this vary by OS version?

 
 // If the fullscreen toolbar is hidden, it is difficult for the user to see
 // changes in the tabstrip. As a result, if a tab is inserted or the current
@@ -155,6 +155,10 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
 // current visibility of the menu bar.
 - (BOOL)doesScreenHaveMenuBar;
 
+// Returns YES if the toolbar's visibility is locked or if the cursor is
+// interacting with it.
+- (BOOL)isToolbarFocused;
+
 // Returns YES if the window is on the primary screen.
 - (BOOL)isWindowOnPrimaryScreen;
 
@@ -191,6 +195,7 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
     browserController_ = controller;
     systemFullscreenMode_ = base::mac::kFullScreenModeNormal;
     slidingStyle_ = style;
+    hiddenToolbarState_ = fullscreen_mac::TOOLBAR_HIDDEN;
   }
 
   // Install the Carbon event handler for the menubar show, hide and
@@ -267,7 +272,7 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
   return kFloatingBarVerticalOffset;
 }
 
-- (void)ensureOverlayShownWithAnimation:(BOOL)animate {
+- (void)showToolbarIfPossibleWithAnimation:(BOOL)animate {
   if (!inFullscreenMode_)
     return;
 
@@ -281,15 +286,19 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
   [self animateToolbarVisibility:YES];
 }
 
-- (void)ensureOverlayHiddenWithAnimation:(BOOL)animate {
+- (void)hideToolbarIfPossibleWithAnimation:(BOOL)animate {
   if (!inFullscreenMode_)
     return;
 
   if (self.slidingStyle != fullscreen_mac::OMNIBOX_TABS_HIDDEN)
     return;
 
+  if ([self isToolbarFocused])
+    return;
+
   [self cancelHideTimer];
   [self animateToolbarVisibility:NO];
+  shouldShowToolbarWithoutMenu_ = NO;
 }
 
 - (void)cancelAnimationAndTimer {
@@ -304,10 +313,13 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
     return;
   }
 
+  if (self.slidingStyle != fullscreen_mac::OMNIBOX_TABS_HIDDEN)
+    return;
+
   // Reveal the toolbar for tabstrip changes if the toolbar is hidden.
-  if (IsCGFloatEqual([self toolbarFraction], kHideFraction)) {
+  if (hiddenToolbarState_ == fullscreen_mac::TOOLBAR_HIDDEN) {
     isRevealingToolbarForTabStripChanges_ = YES;
-    [self ensureOverlayShownWithAnimation:YES];
+    [self showToolbarIfPossibleWithAnimation:YES];
   }
 }
 
@@ -333,9 +345,11 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
 
   // If the menubar is gone, animate the toolbar out.
   if (IsCGFloatEqual(menubarFraction_, kHideFraction))
-    [self ensureOverlayHiddenWithAnimation:YES];
+    [self hideToolbarIfPossibleWithAnimation:YES];
 
-  [self removeTrackingAreaIfNecessary];
+  // We no longer need to track it if it's not focused.
+  if (![self isToolbarFocused])
+    [self removeTrackingAreaIfNecessary];
 }
 
 - (void)updateToolbar {
@@ -368,9 +382,6 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
 }
 
 - (CGFloat)toolbarFraction {
-  if ([browserController_ isBarVisibilityLockedForOwner:nil])
-    return kShowFraction;
-
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode))
     return kHideFraction;
 
@@ -380,13 +391,16 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
     case fullscreen_mac::OMNIBOX_TABS_NONE:
       return kHideFraction;
     case fullscreen_mac::OMNIBOX_TABS_HIDDEN:
+      if (hiddenToolbarState_ == fullscreen_mac::TOOLBAR_SHOWN)
+        return kShowFraction;
+
+      if (hiddenToolbarState_ == fullscreen_mac::TOOLBAR_HIDDEN)
+        return kHideFraction;
+
       if (currentAnimation_.get())
         return [currentAnimation_ toolbarFraction];
 
-      if (hideTimer_.get() || shouldAnimateToolbarOut_)
-        return kShowFraction;
-
-      return toolbarFractionFromMenuProgress_;
+      return shouldShowToolbarWithoutMenu_ ? kShowFraction : menubarFraction_;
   }
 }
 
@@ -405,15 +419,26 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
 
 - (void)setTrackingAreaFromOverlayFrame:(NSRect)frame {
   NSRect contentBounds = [contentView_ bounds];
+  CGFloat yOffset =
+      kFloatingBarVerticalOffset - kTrackingAreaAdditionalThreshold;
+
   trackingAreaFrame_ = frame;
-  trackingAreaFrame_.origin.y -= kTrackingAreaAdditionalThreshold;
+  trackingAreaFrame_.origin.y += yOffset;
   trackingAreaFrame_.size.height =
       NSMaxY(contentBounds) - trackingAreaFrame_.origin.y;
 }
 
 - (void)animationDidStop:(NSAnimation*)animation {
+  CGFloat toolbarFraction = [self toolbarFraction];
+  if (IsCGFloatEqual(toolbarFraction, 1.0))
+    hiddenToolbarState_ = fullscreen_mac::TOOLBAR_SHOWN;
+  else if (IsCGFloatEqual(toolbarFraction, 0.0))
+    hiddenToolbarState_ = fullscreen_mac::TOOLBAR_HIDDEN;
+  else
+    hiddenToolbarState_ = fullscreen_mac::TOOLBAR_TRANSITIONING;
+
   if (isRevealingToolbarForTabStripChanges_) {
-    if ([self toolbarFraction] > 0.0) {
+    if (hiddenToolbarState_ != fullscreen_mac::TOOLBAR_HIDDEN) {
       // Set the timer to hide the toolbar.
       [hideTimer_ invalidate];
       hideTimer_.reset(
@@ -442,20 +467,26 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
   if (![self isMouseOnScreen] && progress > menubarFraction_)
     return;
 
+  BOOL isMenuBarDisappearing = progress < menubarFraction_;
   menubarFraction_ = progress;
 
   if (self.slidingStyle == fullscreen_mac::OMNIBOX_TABS_HIDDEN) {
-    if (IsCGFloatEqual(menubarFraction_, kShowFraction))
+    shouldShowToolbarWithoutMenu_ =
+        shouldShowToolbarWithoutMenu_ ||
+        (isMenuBarDisappearing && [self isToolbarFocused]);
+
+    if (IsCGFloatEqual(menubarFraction_, kShowFraction)) {
       [self setupTrackingArea];
+      hiddenToolbarState_ = fullscreen_mac::TOOLBAR_SHOWN;

[erikchen, 2016/08/30 23:46:48]

This logic confuses me. Can we please move more logic into -toolbarFraction, and
less logic in the callbacks from AppKit?

It seems like we want something along the lines of:

1) If an NSMenuBar animation is in progress, use a state based on the progress
of the menubar.
2) Otherwise, if a toolbar animation is in progress, use a state based on the
progress of the animation.
3) Otherwise, use shouldShowToolbarWithoutMenu_.

Ditto for shouldShowToolbarWithoutMenu_. It's really not clear to me what the
ivar means. From the logic, it looks like it means:

If the NSMenuBar is in animation, and there is no toolbar animation in progress,
should we show the full height of the toolbar, or just part of the height.

But this can be simplified:
Let's say that we have the state: enum NSMenuBarState { HIDDEN, SHOWING,
TRANSITIONING}. And if state = TRANSITIONING, there are two further states:
CGFloat menubarFraction_
enum NSMenuBarAnimationDirection { IN, OUT}

Then 
"""
return shouldShowToolbarWithoutMenu_ ? kShowFraction : menubarFraction_;
"""

becomes:
"""
return direction == OUT ? kShowFraction : menubarFraction_
"""

AFAICT, that will have the same behavior, but without needing this intermediate
variable of shouldShowToolbarWithoutMenu_. 

Looking at your code, right now the question I'm having trouble answering is:
What happens when there's a Chrome animation in progress, and the NSMenuBar is
animating? hiddenToolbarState_ is changed by both of those processes.

+    }
 
-    // If the menubar is disappearing from the screen, check if the mouse
-    // is still interacting with the toolbar. If it is, don't set
-    // |toolbarFractionFromMenuProgress_| so that the the toolbar will remain
-    // on the screen.
-    BOOL isMenuBarDisappearing =
-        menubarFraction_ < toolbarFractionFromMenuProgress_;
-    if (!(isMenuBarDisappearing && [self mouseInsideTrackingArea]))
-      toolbarFractionFromMenuProgress_ = progress;
+    // If |shouldShowToolbarWithoutMenu_| is true, then |hiddenToolbarState_|
+    // should remain fullscreen_mac::TOOLBAR_SHOWN.
+    if (!shouldShowToolbarWithoutMenu_) {
+      hiddenToolbarState_ = IsCGFloatEqual(menubarFraction_, kHideFraction)
+                                ? fullscreen_mac::TOOLBAR_HIDDEN
+                                : fullscreen_mac::TOOLBAR_TRANSITIONING;
+    }
   }
 
   // If an animation is not running, then -layoutSubviews will not be called
@@ -532,6 +563,13 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
   return eachScreenShouldHaveMenuBar ?: [self isWindowOnPrimaryScreen];
 }
 
+- (BOOL)isToolbarFocused {
+  if ([browserController_ isBarVisibilityLockedForOwner:nil])
+    return YES;
+
+  return [self mouseInsideTrackingArea] || menubarFraction_ == kShowFraction;
+}
+
 - (BOOL)isWindowOnPrimaryScreen {
   NSScreen* screen = [[browserController_ window] screen];
   NSScreen* primaryScreen = [[NSScreen screens] firstObject];
@@ -571,6 +609,7 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
   [self removeTrackingAreaIfNecessary];
 
   [currentAnimation_ startAnimation];
+  hiddenToolbarState_ = fullscreen_mac::TOOLBAR_TRANSITIONING;
 }
 
 - (void)cancelHideTimer {
@@ -582,9 +621,7 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
   DCHECK_EQ(hideTimer_, timer);  // This better be our hide timer.
   [hideTimer_ invalidate];       // Make sure it doesn't repeat.
   hideTimer_.reset();            // And get rid of it.
-  shouldAnimateToolbarOut_ = YES;
   [self animateToolbarVisibility:NO];
-  shouldAnimateToolbarOut_ = NO;
 }
 
 - (void)cleanup {