Adjustments to bookmark bar folder menu placement issues.

1) Sub-menus should expose as much as possible without needing to be scrolled up when the dock is showing at the bottom.
2) Fix how menu bottom is misplaced and window is foreshortened when that last scroll-up is performed (for a menu which fits on the screen after being completely scrolled up).
3) Align the right edge of the menu with the right edge of the bookmark bar button from which it springs.
4) Tighten up the menu item spacing.

BUG=54701,69418,69346,59057
TEST=1) Place window near bottom of screen with dock showing. Pop up a folder menu (one which will completely fit on the screen after being fully scrolled up) and scroll fully up. Verify that the bottom it properly placed and all folder contents are shown. Verify that the menu right edge is aligned with the right edge of the button.
2) Have a bookmark folder menu with a submenu near the bottom. Pop up the folder menu and then the submenu. Verify all of the submenu appears.
3) All BookmarkBarFolderControllerTests menu unit tests pass.
4) All BookmarkBarFolderControllerMenuTests menu unit tests pass.

Review URL: http://codereview.chromium.org/6257005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@71889 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/cocoa/bookmarks/bookmark_bar_constants.h

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -33,6 +33,9 @@ const int kNTPBookmarkBarPadding =
 // The height of buttons in the bookmark bar.
 const int kBookmarkButtonHeight = kBookmarkBarHeight + kVisualHeightOffset;
 
+// The height of buttons in a bookmark bar folder menu.
+const CGFloat kBookmarkFolderButtonHeight = 24.0;
+
 // The radius of the corner curves on the menu. Also used for sizing the shadow
 // window behind the menu window at times when the menu can be scrolled.
 const CGFloat kBookmarkBarMenuCornerRadius = 4.0;

chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -54,16 +54,6 @@ const CGFloat kBookmarkVerticalPadding = 2.0;
 const CGFloat kBookmarkMenuButtonMinimumWidth = 100.0;
 const CGFloat kBookmarkMenuButtonMaximumWidth = 485.0;
 
-// TODO(mrossetti): Add constant (kBookmarkVerticalSeparation) for the gap
-// between buttons in a folder menu. Right now we're using
-// kBookmarkVerticalPadding, which is dual purpose and wrong.
-// http://crbug.com/59057
-
-// Convenience constant giving the vertical distance from the top extent of one
-// folder button to the next button.
-const CGFloat kBookmarkButtonVerticalSpan =
-    kBookmarkButtonHeight + kBookmarkVerticalPadding;
-
 // The minimum separation between a folder menu and the edge of the screen.
 // If the menu gets closer to the edge of the screen (either right or left)
 // then it is pops up in the opposite direction.
@@ -91,6 +81,12 @@ const CGFloat kScrollWindowVerticalMargin = 6.0;
 // is set just above the bar so that it become distinctive when drawn.
 const CGFloat kBookmarkBarMenuOffset = 2.0;
 
+// How far to offset a folder menu's left edge horizontally in relation to
+// the left edge of the button from which it springs. Because of drawing
+// differences, simply aligning the |frame| of each does not render the
+// pproper result, so we have to offset.
+const CGFloat kBookmarkBarButtonOffset = 2.0;
+
 // Delay before opening a subfolder (and closing the previous one)
 // when hovering over a folder button.
 const NSTimeInterval kHoverOpenDelay = 0.3;

chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -29,11 +29,11 @@ const NSTimeInterval kBookmarkBarFolderScrollInterval = 0.1;
 // Amount to scroll by per timer fire.  We scroll rather slowly; to
 // accomodate we do several at a time.
 const CGFloat kBookmarkBarFolderScrollAmount =
-    3 * bookmarks::kBookmarkButtonVerticalSpan;
+    3 * bookmarks::kBookmarkFolderButtonHeight;
 
 // Amount to scroll for each scroll wheel roll.
 const CGFloat kBookmarkBarFolderScrollWheelAmount =
-    1 * bookmarks::kBookmarkButtonVerticalSpan;
+    1 * bookmarks::kBookmarkFolderButtonHeight;
 
 // Determining adjustments to the layout of the folder menu window in response
 // to resizing and scrolling relies on many visual factors. The following
@@ -473,9 +473,9 @@ struct LayoutMetrics {
         [[parentButton_ window]
             convertBaseToScreen:[[parentButton_ superview]
                                     convertPoint:NSZeroPoint toView:nil]];
-    newWindowTopLeft = NSMakePoint(buttonBottomLeftInScreen.x,
-                                   bookmarkBarBottomLeftInScreen.y +
-                                      bookmarks::kBookmarkBarMenuOffset);
+    newWindowTopLeft = NSMakePoint(
+        buttonBottomLeftInScreen.x + bookmarks::kBookmarkBarButtonOffset,
+        bookmarkBarBottomLeftInScreen.y + bookmarks::kBookmarkBarMenuOffset);
     // Make sure the window is on-screen; if not, push left.  It is
     // intentional that top level folders "push left" slightly
     // different than subfolders.
@@ -486,11 +486,11 @@ struct LayoutMetrics {
                                     NSMinX(screenFrame));
     }
   } else {
-    // Parent is a folder; grow right/left.
+    // Parent is a folder: expose as much as we can vertically; grow right/left.
     newWindowTopLeft.x = [self childFolderWindowLeftForWidth:windowWidth];
     NSPoint topOfWindow = NSMakePoint(0,
-                                      (NSMaxY([parentButton_ frame]) +
-                                          bookmarks::kBookmarkVerticalPadding));
+                                      NSMaxY([parentButton_ frame]) -
+                                          bookmarks::kBookmarkVerticalPadding);
     topOfWindow = [[parentButton_ window]
                    convertBaseToScreen:[[parentButton_ superview]
                                         convertPoint:topOfWindow toView:nil]];
@@ -508,8 +508,8 @@ struct LayoutMetrics {
 - (int)menuHeightForButtonCount:(int)buttonCount {
   // This does not take into account any padding which may be required at the
   // top and/or bottom of the window.
-  return (buttonCount * bookmarks::kBookmarkButtonVerticalSpan) +
-      bookmarks::kBookmarkVerticalPadding;
+  return (buttonCount * bookmarks::kBookmarkFolderButtonHeight) +
+      2 * bookmarks::kBookmarkVerticalPadding;
 }
 
 - (void)adjustWindowLeft:(CGFloat)windowLeft
@@ -569,10 +569,10 @@ struct LayoutMetrics {
   metrics.canScrollDown = metrics.folderTop > maximumY;
 
   // Accommodate changes in the bottom of the menu.
-  [self adjustMetricsForMenuTopChanges:layoutMetrics];
+  [self adjustMetricsForMenuBottomChanges:layoutMetrics];
 
   // Accommodate changes in the top of the menu.
-  [self adjustMetricsForMenuBottomChanges:layoutMetrics];
+  [self adjustMetricsForMenuTopChanges:layoutMetrics];
 
   metrics.scrollerFrame.origin.y += metrics.deltaScrollerY;
   metrics.scrollerFrame.size.height += metrics.deltaScrollerHeight;
@@ -586,7 +586,7 @@ struct LayoutMetrics {
   metrics.windowFrame.size.width = metrics.windowSize.width;
 }
 
-- (void)adjustMetricsForMenuTopChanges:(LayoutMetrics*)layoutMetrics {
+- (void)adjustMetricsForMenuBottomChanges:(LayoutMetrics*)layoutMetrics {
   LayoutMetrics& metrics(*layoutMetrics);
   if (metrics.canScrollUp) {
     if (!metrics.couldScrollUp) {
@@ -598,7 +598,7 @@ struct LayoutMetrics {
       metrics.deltaScrollerY = verticalScrollArrowHeight_;
       metrics.deltaScrollerHeight = -metrics.deltaScrollerY;
       // Adjust the scroll delta if we've grown the window and it is
-      // now scroll-up-able, but don't adjust it factor if we've
+      // now scroll-up-able, but don't adjust it if we've
       // scrolled down and it wasn't scroll-up-able but now is.
       if (metrics.canScrollDown == metrics.couldScrollDown) {
         CGFloat deltaScroll = metrics.deltaWindowY + metrics.deltaScrollerY +
@@ -613,17 +613,12 @@ struct LayoutMetrics {
       // Could -> Can't
       metrics.deltaWindowY = metrics.folderY - metrics.oldWindowY;
       metrics.deltaWindowHeight = -metrics.deltaWindowY;
-      metrics.deltaVisibleY = -bookmarks::kScrollWindowVerticalMargin;
+      metrics.deltaVisibleY = -metrics.visibleFrame.origin.y;
       metrics.deltaVisibleHeight = -metrics.deltaVisibleY;
       metrics.deltaScrollerY = -verticalScrollArrowHeight_;
       metrics.deltaScrollerHeight = -metrics.deltaScrollerY;
-      // Adjust the scroll delta if we are no longer scroll-up-able
-      // and the scroll-down-able-ness hasn't changed.
-      if (metrics.canScrollDown == metrics.couldScrollDown) {
-        CGFloat deltaScroll = metrics.deltaWindowY + metrics.deltaScrollerY +
-                              metrics.deltaVisibleY;
-        metrics.scrollPoint.y += deltaScroll;
-      }
+      // We are no longer scroll-up-able so the scroll point drops to zero.
+      metrics.scrollPoint.y = 0.0;
     } else {
       // Couldn't -> Can't
       // Check for menu height change by looking at the relative tops of the
@@ -635,7 +630,7 @@ struct LayoutMetrics {
   }
 }
 
-- (void)adjustMetricsForMenuBottomChanges:(LayoutMetrics*)layoutMetrics {
+- (void)adjustMetricsForMenuTopChanges:(LayoutMetrics*)layoutMetrics {
   LayoutMetrics& metrics(*layoutMetrics);
   if (metrics.canScrollDown == metrics.couldScrollDown) {
     if (!metrics.canScrollDown) {
@@ -733,9 +728,10 @@ struct LayoutMetrics {
   // http://crbug.com/35966
   NSRect buttonsOuterFrame = NSMakeRect(
       0,
-      (height - bookmarks::kBookmarkButtonVerticalSpan),
+      height - bookmarks::kBookmarkFolderButtonHeight -
+          bookmarks::kBookmarkVerticalPadding,
       bookmarks::kDefaultBookmarkWidth,
-      bookmarks::kBookmarkButtonHeight);
+      bookmarks::kBookmarkFolderButtonHeight);
 
   // TODO(jrg): combine with addNodesToButtonList: code from
   // bookmark_bar_controller.mm (but use y offset)
@@ -755,7 +751,7 @@ struct LayoutMetrics {
                                                  frame:buttonsOuterFrame];
       [buttons_ addObject:button];
       [folderView_ addSubview:button];
-      buttonsOuterFrame.origin.y -= bookmarks::kBookmarkButtonVerticalSpan;
+      buttonsOuterFrame.origin.y -= bookmarks::kBookmarkFolderButtonHeight;
     }
   }
   [self layOutWindowWithHeight:height];
@@ -770,8 +766,10 @@ struct LayoutMetrics {
   // Make sure as much of a submenu is exposed (which otherwise would be a
   // problem if the parent button is close to the bottom of the screen).
   if ([parentController_ isKindOfClass:[self class]]) {
-    newWindowTopLeft.y = MAX(newWindowTopLeft.y,
-                             height + bookmarks::kScrollWindowVerticalMargin);
+    CGFloat minimumY = NSMinY([[[self window] screen] visibleFrame]) +
+                       bookmarks::kScrollWindowVerticalMargin +
+                       height;
+    newWindowTopLeft.y = MAX(newWindowTopLeft.y, minimumY);
   }
   NSWindow* window = [self window];
   NSRect windowFrame = NSMakeRect(newWindowTopLeft.x,
@@ -1449,7 +1447,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   // topmost button's frame (and there will always be one) offset upward in
   // anticipation of insertion.
   NSRect newButtonFrame = [[buttons_ objectAtIndex:0] frame];
-  newButtonFrame.origin.y += bookmarks::kBookmarkButtonVerticalSpan;
+  newButtonFrame.origin.y += bookmarks::kBookmarkFolderButtonHeight;
   // When adding a button to an empty folder we must remove the 'empty'
   // placeholder button. This can be detected by checking for a parent
   // child count of 1.
@@ -1473,7 +1471,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
     // which is where the new button will be located.
     newButtonFrame = [button frame];
     NSRect buttonFrame = [button frame];
-    buttonFrame.origin.y += bookmarks::kBookmarkButtonVerticalSpan;
+    buttonFrame.origin.y += bookmarks::kBookmarkFolderButtonHeight;
     [button setFrame:buttonFrame];
   }
   [[button cell] mouseExited:nil];  // De-highlight.
@@ -1545,7 +1543,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
       for (NSInteger i = fromIndex; i < toIndex; ++i) {
         BookmarkButton* button = [buttons_ objectAtIndex:i];
         NSRect frame = [button frame];
-        frame.origin.y += bookmarks::kBookmarkButtonVerticalSpan;
+        frame.origin.y += bookmarks::kBookmarkFolderButtonHeight;
         [button setFrameOrigin:frame.origin];
       }
     } else {
@@ -1554,7 +1552,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
       for (NSInteger i = fromIndex - 1; i >= toIndex; --i) {
         BookmarkButton* button = [buttons_ objectAtIndex:i];
         NSRect buttonFrame = [button frame];
-        buttonFrame.origin.y -= bookmarks::kBookmarkButtonVerticalSpan;
+        buttonFrame.origin.y -= bookmarks::kBookmarkFolderButtonHeight;
         [button setFrameOrigin:buttonFrame.origin];
       }
     }
@@ -1596,7 +1594,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   for (NSInteger i = 0; i < buttonIndex; ++i) {
     BookmarkButton* button = [buttons_ objectAtIndex:i];
     NSRect buttonFrame = [button frame];
-    buttonFrame.origin.y -= bookmarks::kBookmarkButtonVerticalSpan;
+    buttonFrame.origin.y -= bookmarks::kBookmarkFolderButtonHeight;
     [button setFrame:buttonFrame];
   }
   // Search for and adjust submenus, if necessary.
@@ -1614,13 +1612,8 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
     // If all nodes have been removed from this folder then add in the
     // 'empty' placeholder button.
     NSRect buttonFrame =
-        NSMakeRect(0,
-                   bookmarks::kBookmarkButtonHeight -
-                       (bookmarks::kBookmarkBarHeight -
-                        bookmarks::kBookmarkVerticalPadding),
-                   bookmarks::kDefaultBookmarkWidth,
-                   (bookmarks::kBookmarkBarHeight -
-                       2 * bookmarks::kBookmarkVerticalPadding));
+        NSMakeRect(0.0, 0.0, bookmarks::kDefaultBookmarkWidth,
+                   bookmarks::kBookmarkFolderButtonHeight);
     BookmarkButton* button = [self makeButtonForNode:nil
                                                frame:buttonFrame];
     [buttons_ addObject:button];

chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -44,7 +44,7 @@
     } else {
       CGFloat nextVerticalOffset = [button frame].origin.y;
       EXPECT_CGFLOAT_EQ(lastVerticalOffset -
-                            bookmarks::kBookmarkButtonVerticalSpan,
+                            bookmarks::kBookmarkFolderButtonHeight,
                         nextVerticalOffset);
       lastVerticalOffset = nextVerticalOffset;
     }
@@ -483,9 +483,11 @@ TEST_F(BookmarkBarFolderControllerTest, SimpleScroll) {
       break;
     targetButton = button;
   }
+  EXPECT_TRUE(targetButton != nil);
   NSPoint hitPoint = [targetButton frame].origin;
   hitPoint.x += 50.0;
-  hitPoint.y += (bookmarks::kBookmarkButtonHeight / 2.0) - scrollPoint.y;
+  hitPoint.y += (bookmarks::kBookmarkFolderButtonHeight / 2.0) - scrollPoint.y;
+  hitPoint = [targetButton convertPoint:hitPoint toView:scrollView];
 
   for (int i = 0; i < 3; i++) {
     CGFloat height = NSHeight([window frame]);
@@ -557,7 +559,7 @@ TEST_F(BookmarkBarFolderControllerTest, MenuPlacementWhileScrollingDeleting) {
   // the top of the window has moved up, then delete a visible button and
   // make sure the top has not moved.
   oldTop = newTop;
-  const CGFloat scrollOneBookmark = bookmarks::kBookmarkButtonHeight +
+  const CGFloat scrollOneBookmark = bookmarks::kBookmarkFolderButtonHeight +
       bookmarks::kBookmarkVerticalPadding;
   NSUInteger buttonCounter = 0;
   NSUInteger extraButtonLimit = 3;
@@ -729,10 +731,8 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragMoveBarBookmarkToFolder) {
   // and grown vertically.
   NSRect expectedToWindowFrame = oldToWindowFrame;
   expectedToWindowFrame.origin.x -= horizontalShift;
-  CGFloat diff = (bookmarks::kBookmarkBarHeight +
-                  2*bookmarks::kBookmarkVerticalPadding);
-  expectedToWindowFrame.origin.y -= diff;
-  expectedToWindowFrame.size.height += diff;
+  expectedToWindowFrame.origin.y -= bookmarks::kBookmarkFolderButtonHeight;
+  expectedToWindowFrame.size.height += bookmarks::kBookmarkFolderButtonHeight;
   EXPECT_NSRECT_EQ(expectedToWindowFrame, newToWindowFrame);
 
   // Check button spacing.
@@ -796,10 +796,8 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragCopyBarBookmarkToFolder) {
   EXPECT_NSRECT_EQ(oldToFolderFrame, newToFolderFrame);
   // The toWindow should have shifted down vertically and grown vertically.
   NSRect expectedToWindowFrame = oldToWindowFrame;
-  CGFloat diff = (bookmarks::kBookmarkBarHeight +
-                  2*bookmarks::kBookmarkVerticalPadding);
-  expectedToWindowFrame.origin.y -= diff;
-  expectedToWindowFrame.size.height += diff;
+  expectedToWindowFrame.origin.y -= bookmarks::kBookmarkFolderButtonHeight;
+  expectedToWindowFrame.size.height += bookmarks::kBookmarkFolderButtonHeight;
   EXPECT_NSRECT_EQ(expectedToWindowFrame, newToWindowFrame);
 
   // Copy the button back to the bar after "3b".
@@ -875,10 +873,9 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragMoveBarBookmarkToSubfolder) {
   EXPECT_NSRECT_EQ(oldToWindowFrame, newToWindowFrame);
   NSRect newToSubwindowFrame = [toSubwindow frame];
   NSRect expectedToSubwindowFrame = oldToSubwindowFrame;
-  expectedToSubwindowFrame.origin.y -=
-      (bookmarks::kBookmarkButtonHeight + bookmarks::kVisualHeightOffset);
+  expectedToSubwindowFrame.origin.y -= bookmarks::kBookmarkFolderButtonHeight;
   expectedToSubwindowFrame.size.height +=
-      (bookmarks::kBookmarkButtonHeight + bookmarks::kVisualHeightOffset);
+      bookmarks::kBookmarkFolderButtonHeight;
   EXPECT_NSRECT_EQ(expectedToSubwindowFrame, newToSubwindowFrame);
 }
 
@@ -1181,7 +1178,7 @@ TEST_F(BookmarkBarFolderControllerMenuTest, MenuSizingAndScrollArrows) {
   EXPECT_TRUE(folderController);
   NSWindow* folderWindow = [folderController window];
   EXPECT_TRUE(folderWindow);
-  CGFloat expectedHeight = (CGFloat)bookmarks::kBookmarkButtonHeight +
+  CGFloat expectedHeight = (CGFloat)bookmarks::kBookmarkFolderButtonHeight +
       (2*bookmarks::kBookmarkVerticalPadding);
   NSRect windowFrame = [folderWindow frame];
   CGFloat windowHeight = NSHeight(windowFrame);

chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -231,10 +231,11 @@
   if ([button isFolder]) {
     NSRect imageRect = NSZeroRect;
     imageRect.size = [arrowImage_ size];
-    NSRect drawRect = NSOffsetRect(imageRect,
-                                   NSWidth(cellFrame) - NSWidth(imageRect),
-                                   (NSHeight(cellFrame) / 2.0) -
-                                   (NSHeight(imageRect) / 2.0));
+    const CGFloat kArrowOffset = 1.0;  // Required for proper centering.
+    CGFloat dX = NSWidth(cellFrame) - NSWidth(imageRect);
+    CGFloat dY = (NSHeight(cellFrame) / 2.0) - (NSHeight(imageRect) / 2.0) +
+        kArrowOffset;
+    NSRect drawRect = NSOffsetRect(imageRect, dX, dY);
     [arrowImage_ drawInRect:drawRect
                     fromRect:imageRect
                    operation:NSCompositeSourceOver