[Mac] Reverse bookmark buttons and menus in RTL (take 2)

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

 // 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.
 
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h"
 
 #include "base/logging.h"
 #include "base/metrics/user_metrics.h"
 #include "base/strings/sys_string_conversions.h"
 #include "chrome/browser/ui/cocoa/bookmarks/bookmark_bar_constants.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_button.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_context_menu_cocoa_controller.h"
+#include "chrome/browser/ui/cocoa/l10n_util.h"
 #include "chrome/grit/generated_resources.h"
 #import "components/bookmarks/browser/bookmark_model.h"
 #import "ui/base/cocoa/nsview_additions.h"
 #include "ui/base/l10n/l10n_util_mac.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/resources/grit/ui_resources.h"
 
 using base::UserMetricsAction;
 using bookmarks::BookmarkNode;
 
 namespace {
 
-// Padding on the right side of the arrow icon.
-const int kHierarchyButtonRightPadding = 4;
+// Padding on the trailing side of the arrow icon.
+const int kHierarchyButtonTrailingPadding = 4;
 
-// Padding on the left side of the arrow icon.
-const int kHierarchyButtonLeftPadding = 11;
+// Padding on the leading side of the arrow icon.
+const int kHierarchyButtonLeadingPadding = 11;
 
 const int kIconTextSpacer = 4;
-const int kTextRightPadding = 4;
-const int kIconLeftPadding = 4;
+const int kTextTrailingPadding = 4;
+const int kIconLeadingPadding = 4;
 
 const int kDefaultFontSize = 12;
 
 // Kerning value for the title text.
 const CGFloat kKernAmount = 0.2;
 
 };  // namespace
 
 // TODO(lgrey): Bake setting the chevron image into this
 // class instead of setting it externally.
@@ skipping 67 matching lines @@
 }
 
 + (id)offTheSideButtonCell {
   return [[[OffTheSideButtonCell alloc] init] autorelease];
 }
 
 + (CGFloat)cellWidthForNode:(const bookmarks::BookmarkNode*)node
                       image:(NSImage*)image {
   NSString* title =
       [self cleanTitle:base::SysUTF16ToNSString(node->GetTitle())];
-  CGFloat width = kIconLeftPadding + [image size].width;
+  CGFloat width = kIconLeadingPadding + [image size].width;
   if ([title length] > 0) {
     CGSize titleSize = [title sizeWithAttributes:@{
       NSParagraphStyleAttributeName : [self paragraphStyleForBookmarkBarCell],
       NSKernAttributeName : @(kKernAmount),
       NSFontAttributeName : [self fontForBookmarkBarCell],
     }];
-    width += kIconTextSpacer + std::ceil(titleSize.width) + kTextRightPadding;
+    width +=
+        kIconTextSpacer + std::ceil(titleSize.width) + kTextTrailingPadding;
   } else {
-    width += kIconLeftPadding;
+    width += kIconLeadingPadding;

[Elly Fong-Jones, 2017/05/03 17:03:14]

why is this leading padding twice? shouldn't it always be kTextTrailingPadding,
even if there is no text?

[lgrey, 2017/05/03 21:32:48]

Done.

   }
   return width;
 }
 
 - (id)initForNode:(const BookmarkNode*)node
              text:(NSString*)text
             image:(NSImage*)image
    menuController:(BookmarkContextMenuCocoaController*)menuController {
   if ((self = [super initTextCell:text])) {
     menuController_ = menuController;
@@ skipping 47 matching lines @@
 
 - (BOOL)isOffTheSideButtonCell {
   return NO;
 }
 
 // Perform all normal init routines specific to the BookmarkButtonCell.
 - (void)configureBookmarkButtonCell {
   [self setButtonType:NSMomentaryPushInButton];
   [self setShowsBorderOnlyWhileMouseInside:YES];
   [self setControlSize:NSSmallControlSize];
-  [self setAlignment:NSLeftTextAlignment];
   [self setFont:[[self class] fontForBookmarkBarCell]];
+  [self setAlignment:NSNaturalTextAlignment];

[Elly Fong-Jones, 2017/05/03 17:03:14]

why did this move? is it important that these be done in this order?

[lgrey, 2017/05/03 21:32:48]

I think this was an accident, doesn't seem to make a difference. Reverted.

   [self setBordered:NO];
   [self setBezeled:NO];
   [self setWraps:NO];
   // NSLineBreakByTruncatingMiddle seems more common on OSX but let's
   // try to match Windows for a bit to see what happens.
   [self setLineBreakMode:NSLineBreakByTruncatingTail];
 
   // The overflow button chevron bitmap is not 16 units high, so it'd be scaled
   // at paint time without this.
   [self setImageScaling:NSImageScaleNone];
@@ skipping 16 matching lines @@
   NSSize size = [self cellSize];
   size.width = std::min(aRect.size.width, size.width);
   size.height = std::min(aRect.size.height, size.height);
   return size;
 }
 
 - (void)setBookmarkCellText:(NSString*)title
                       image:(NSImage*)image {
   title = [[self class] cleanTitle:title];
   if ([title length] && ![self isOffTheSideButtonCell]) {
-    [self setImagePosition:NSImageLeft];
+    [self setImagePosition:cocoa_l10n_util::LeadingCellImagePosition()];
     [self setTitle:title];
   } else if ([self isFolderButtonCell]) {
     // Left-align icons for bookmarks within folders, regardless of whether
     // there is a title.
-    [self setImagePosition:NSImageLeft];
+    [self setImagePosition:cocoa_l10n_util::LeadingCellImagePosition()];
   } else {
     // For bookmarks without a title that aren't visible directly in the
     // bookmarks bar, squeeze things tighter by displaying only the image.
     // By default, Cocoa leaves extra space in an attempt to display an
     // empty title.
     [self setImagePosition:NSImageOnly];
   }
 
   if (image)
     [self setImage:image];
@@ skipping 69 matching lines @@
 // See comment above mouseEntered:, above.
 - (void)mouseExited:(NSEvent*)event {
   [[self controlView] mouseExited:event];
   [super mouseExited:event];
 }
 
 - (void)setDrawFolderArrow:(BOOL)draw {
   drawFolderArrow_ = draw;
   if (draw && !arrowImage_) {
     ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-    arrowImage_.reset(
-        [rb.GetNativeImageNamed(IDR_MENU_HIERARCHY_ARROW).ToNSImage() retain]);
+    NSImage* image =
+        rb.GetNativeImageNamed(IDR_MENU_HIERARCHY_ARROW).ToNSImage();
+    if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout())
+      image = cocoa_l10n_util::FlippedImage(image);
+    arrowImage_.reset([image retain]);
   }
 }
 
 - (NSDictionary*)titleTextAttributes {
   NSParagraphStyle* style = [[self class] paragraphStyleForBookmarkBarCell];
   NSColor* textColor = textColor_.get();
   if (!textColor) {
     textColor = [NSColor blackColor];
   }
   if (![self isEnabled]) {
@@ skipping 15 matching lines @@
 - (NSString*)visibleTitle {
   return [self imagePosition] != NSImageOnly ? [self title] : @"";
 }
 
 // Add extra size for the arrow so it doesn't overlap the text.
 // Does not sanity check to be sure this is actually a folder node.
 - (NSSize)cellSize {
   NSSize cellSize = NSZeroSize;
   // Return the space needed to display the image and title, with a little
   // distance between them.
-  cellSize = NSMakeSize(kIconLeftPadding + [[self image] size].width,
+  cellSize = NSMakeSize(kIconLeadingPadding + [[self image] size].width,
                         bookmarks::kBookmarkButtonHeight);
   NSString* title = [self visibleTitle];
   if ([title length] > 0) {
     CGFloat textWidth =
         [title sizeWithAttributes:[self titleTextAttributes]].width;
     cellSize.width +=
-        kIconTextSpacer + std::ceil(textWidth) + kTextRightPadding;
+        kIconTextSpacer + std::ceil(textWidth) + kTextTrailingPadding;
   } else {
     // Make buttons without visible titles 20pts wide (18 plus padding).

[Elly Fong-Jones, 2017/05/03 17:03:14]

please delete this comment! comments that duplicate constants like this are
basically pleading to become inaccurate

[lgrey, 2017/05/03 21:32:48]

Done.

-    cellSize.width += kIconLeftPadding;
+    cellSize.width += kIconLeadingPadding;
   }
 
   if (drawFolderArrow_) {
     cellSize.width += [arrowImage_ size].width +
-                      kHierarchyButtonLeftPadding +
-                      kHierarchyButtonRightPadding;
+                      kHierarchyButtonLeadingPadding +
+                      kHierarchyButtonTrailingPadding;
   }
   return cellSize;
 }
 
 - (NSRect)imageRectForBounds:(NSRect)theRect {
   NSRect imageRect = [super imageRectForBounds:theRect];
-  // Add a little space between the image and the button's left edge, but only
-  // if there's a visible title.
+  const CGFloat inset = [self insetInView:[self controlView]];
   imageRect.origin.y -= 1;
-  imageRect.origin.x = kIconLeftPadding;
+  imageRect.origin.x =
+      cocoa_l10n_util::ShouldDoExperimentalRTLLayout()
+          ? NSMaxX(theRect) - kIconLeadingPadding - NSWidth(imageRect) + inset
+          : kIconLeadingPadding;
   return imageRect;
 }
 
-- (CGFloat)textStartXOffset {
-  return kIconLeftPadding + [[self image] size].width + kIconTextSpacer;
+- (NSRect)titleRectForBounds:(NSRect)theRect {
+  NSRect textRect = [super titleRectForBounds:theRect];
+  NSRect imageRect = [self imageRectForBounds:theRect];
+  if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout()) {
+    textRect.origin.x = drawFolderArrow_ ? [arrowImage_ size].width +
+                                               kHierarchyButtonTrailingPadding +
+                                               kTextTrailingPadding
+                                         : kTextTrailingPadding;
+    textRect.size.width =
+        NSMinX(imageRect) - textRect.origin.x - kIconTextSpacer;
+  } else {
+    textRect.origin.x = NSMaxX(imageRect) + kIconTextSpacer;
+  }
+  return textRect;
 }
 
 - (void)drawFocusRingMaskWithFrame:(NSRect)cellFrame
                             inView:(NSView*)controlView {
   // We have to adjust the focus ring slightly for the chevron and regular
   // bookmark icons.
   if ([self isOffTheSideButtonCell]) {
     cellFrame.origin.y -= 2;
   } else if ([self visibleTitle].length > 0) {
     cellFrame.origin.x += 4;
@@ skipping 11 matching lines @@
 
   // If asked to do so, and if a folder, draw the arrow.
   if (!drawFolderArrow_)
     return;
   BookmarkButton* button = static_cast<BookmarkButton*>([self controlView]);
   DCHECK([button respondsToSelector:@selector(isFolder)]);
   if ([button isFolder]) {
     NSRect imageRect = NSZeroRect;
     imageRect.size = [arrowImage_ size];
     const CGFloat kArrowOffset = 1.0;  // Required for proper centering.
-    CGFloat dX =
-        NSWidth(cellFrame) - NSWidth(imageRect) - kHierarchyButtonRightPadding;
+    CGFloat dX = cocoa_l10n_util::ShouldDoExperimentalRTLLayout()
+                     ? kHierarchyButtonTrailingPadding
+                     : NSWidth(cellFrame) - NSWidth(imageRect) -
+                           kHierarchyButtonTrailingPadding;
     CGFloat dY = (NSHeight(cellFrame) / 2.0) - (NSHeight(imageRect) / 2.0) +
         kArrowOffset;
     NSRect drawRect = NSOffsetRect(imageRect, dX, dY);
     [arrowImage_ drawInRect:drawRect
                     fromRect:imageRect
                    operation:NSCompositeSourceOver
                     fraction:[self isEnabled] ? 1.0 : 0.5
               respectFlipped:YES
                        hints:nil];
   }
@@ skipping 27 matching lines @@
 
 // Returns |title| with newlines and line feeds replaced with
 // spaces.
 + (NSString*)cleanTitle:(NSString*)title {
   title = [title stringByReplacingOccurrencesOfString:@"\n" withString:@" "];
   title = [title stringByReplacingOccurrencesOfString:@"\r" withString:@" "];
   return title;
 }
 
 @end