[Mac] Retain location bar focus on switching to fullscreen and back.

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

Index: chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm
diff --git a/chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm b/chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm
index f80e083fb15fb17e8d608eba1fa2eee3944f7475..6b553bbeb6dc3ffbfa6b67e4c07e9bba32254c8c 100644
--- a/chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm
+++ b/chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm
@@ -47,6 +47,14 @@ const CGFloat kShowFraction = 1.0;
   DCHECK(!inFullscreenMode_);
   inFullscreenMode_ = YES;
 
+  // If the location bar is the first responder, don't let the toolbar hide.
+  // This is a special signal to -mustShowFullscreenToolbar that the toolbar
+  // should remain visible during the transition to fullscreen.
+  if ([browserController_ locationBarHasFocus]) {

[spqchan, 2017/02/08 19:21:51]

Would it possible to move this out of fullscreen_toolbar_controller?
I prefer that this class is more focused on managing the different parts of
fullscreen and calculating the layout.

[shrike, 2017/02/08 19:51:42]

I could maybe do that with a fourth toolbar style (see my comment below),
otherwise I don't think it's possible. The fullscreen toolbar controller
controls when the toolbar is visible, so the code at this level needs to arrange
to keep the toolbar around. I can't arrange that from another class unless I use
a fourth toolbar style.

[spqchan, 2017/02/09 01:50:34]

Will something like this work?

In browser_window_controller_private.mm, change L785 to:

if ([self locationBarHasFocus])
    [self lockToolbarVisibilityForOwner:self withAnimation:NO];
[fullscreenToolbarController_ enterFullscreenMode];
[self releaseToolbarVisibilityForOwner:self withAnimation:NO];


But yeah, please don't introduce a fourth style.

[shrike, 2017/02/09 02:18:29]

This may work, but unfortunately I still need a check inside of
mustShowFullscreenToolbar that would read

    if ([visibilityLockController_
isToolbarVisibilityLockedForOwner:browserWindowController_])...

so that would mean the fullscreen toolbar controller knowing that the browser
window controller might be locking visibility (ugh).

Or I could call lockToolbarVisibilityForOwner:... in BWC passing in the FTC as
the owner but that's also kind of yucky. More discussion below.

[spqchan, 2017/02/09 05:09:36]

Ah gotcha, so we're locking the toolbar as a special case for the omnibox. Since
the toolbar is locked whenever something in it is focused, perhaps we should
show the toolbar regardless of the style if something on it is focused before it
entered fullscreen. A similar issue could happen with bookmarks if the user is
focusing on it before we enter fullscreen.

In that case, how about we change mustShowFullscreen so that it will always
return true if something, anything, is locking the toolbar? That way we don't
need to worry about checking if BWC is locked here and it'll fix the focusing
issue with other parts of the toolbar.

+    [visibilityLockController_ lockToolbarVisibilityForOwner:self
+                                               withAnimation:NO];
+  }
+
   [self updateToolbarStyleExitingTabFullscreen:NO];
 
   if ([browserController_ isInImmersiveFullscreen]) {
@@ -60,6 +68,11 @@ const CGFloat kShowFraction = 1.0;
         initWithFullscreenToolbarController:self
                         animationController:animationController_.get()]);
   }
+
+  // Let the location bar keep the toolbar locked in place from here on out
+  // (if it's the first responder).
+  [visibilityLockController_ releaseToolbarVisibilityForOwner:self
+                                                withAnimation:YES];
 }
 
 - (void)exitFullscreenMode {
@@ -132,6 +145,12 @@ const CGFloat kShowFraction = 1.0;
   if (!inFullscreenMode_)
     return NO;
 
+  // Check if the fullscreen toolbar controller wants the toolbar to remain
+  // visible.
+  if ([visibilityLockController_ isToolbarVisibilityLockedForOwner:self]) {

[spqchan, 2017/02/08 19:21:51]

Remove: This is redundant, L163 should cover this.

[shrike, 2017/02/08 19:51:42]

I have to have this here, otherwise the code early outs on L157. The issue is
that there are three toolbar style modes (NONE, PRESENT, HIDDEN), but I need a
fourth style which would mean "keep the toolbar around as you go into fullscreen
mode". It seemed kind of ugly to add a fourth style so I opted to lock the
toolbar temporarily and to check for that lock in this method,

[spqchan, 2017/02/09 01:50:34]

I'm a bit confused, why do you want to show the toolbar when it's not supposed
to? AFAIK, if you fullscreen a youtube video, the toolbar is not supposed to
appear.

If it's supposed to appear though, we can just check the toolbar visibility
locks before checking the toolbar style, but I'm not sure why we want to show
the toolbar for TOOLBAR_NONE.

[shrike, 2017/02/09 02:18:29]

If the toolbar is TOOLBAR_NONE then the toolbar should not appear. But if you go
into fullscreen mode with TOOLBAR_NONE and then you make the toolbar appear and
then click in the omnibox, the toolbar won't go away until you're done editing.
So even though the browser is configured for TOOLBAR_NONE, the toolbar needs to
be visible in this case. So far so good.

The problem is that we're trying to preserve first responder state in the
omnibox when you transition to fullscreen. So in this case we want the toolbar
to remain visible even though we're configured for TOOLBAR_NONE.

The crux of the problem is that -mustShowFullscreenToolbar determines whether or
not the toolbar should be visible at the start of fullscreen. Because we're
editing the omnibox we want it to return YES, but because we're configured for
TOOLBAR_NONE it returns NO. This happens even if the omnibox is locking
visibility because the visibility lock check occurs after we check for
TOOLBAR_NONE. So we need a special case to get -mustShowFullscreenToolbar to
return YES before it checks for TOOLBAR_NONE. I assume the check for
TOOLBAR_NONE is required before the general check for the visibility lock (i.e.
that you had a specific reason for ordering these checks this way), and that
moving the visibility check before the TOOLBAR_NONE check will break something.
If that's not the case, then I think that would be an easy fix.

[spqchan, 2017/02/09 05:09:36]

See comment from above.

+    return YES;
+  }
+
   if (toolbarStyle_ == FullscreenToolbarStyle::TOOLBAR_PRESENT)
     return YES;