[Mac] Reverse the omnibox in RTL

chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm

 // Copyright (c) 2012 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/location_bar/autocomplete_text_field_cell.h"
 
 #include <stddef.h>
 
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/mac_logging.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/themes/theme_service.h"
+#include "chrome/browser/ui/cocoa/l10n_util.h"
 #import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.h"
 #import "chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h"
 #import "chrome/browser/ui/cocoa/themed_window.h"
 #include "chrome/grit/theme_resources.h"
 #import "extensions/common/feature_switch.h"
 #import "third_party/mozilla/NSPasteboard+Utils.h"
 #import "ui/base/cocoa/appkit_utils.h"
 #import "ui/base/cocoa/tracking_area.h"
 #import "ui/base/cocoa/nsview_additions.h"
 #include "ui/base/cocoa/scoped_cg_context_smooth_fonts.h"
 #include "ui/base/material_design/material_design_controller.h"
 
 using extensions::FeatureSwitch;
 
 namespace {
 
 // Matches the clipping radius of |GradientButtonCell|.
 const CGFloat kCornerRadius = 3.0;
 
 // How far to inset the left- and right-hand decorations from the field's
 // bounds.
-const CGFloat kRightDecorationXOffset = 2.0;
-const CGFloat kLeftDecorationXOffset = 1.0;
+const CGFloat kTrailingDecorationXPadding = 2.0;
+const CGFloat kLeadingDecorationXPadding = 1.0;
 
-// How much the text frame needs to overlap the rightmost left decoration.
+// How much the text frame needs to overlap the front-most leading

[Sidney San Martín, 2016/12/16 00:05:54]

The use of “front” and “back” in this CL are confusing to me. Maybe “outermost”
and “innermost” would be more clear, or you can think of something even better?

[Sidney San Martín, 2016/12/16 00:09:40]

Sorry, I didn't mean to imply "front" → "outermost". I just meant "front-most" →
"outermost".

[lgrey, 2016/12/20 19:41:03]

Done.

+// decoration.
 const CGFloat kTextFrameDecorationOverlap = 5.0;
 
 // How long to wait for mouse-up on the location icon before assuming
 // that the user wants to drag.
 const NSTimeInterval kLocationIconDragTimeout = 0.25;
 
 // Calculate the positions for a set of decorations.  |frame| is the
 // overall frame to do layout in, |remaining_frame| will get the
 // left-over space.  |all_decorations| is the set of decorations to
 // lay out, |decorations| will be set to the decorations which are
@@ skipping 57 matching lines @@
 }
 
 // Helper function for calculating placement of decorations w/in the cell.
 // |frame| is the cell's boundary rectangle, |remaining_frame| will get any
 // space left after decorations are laid out (for text).  |left_decorations| is
 // a set of decorations for the left-hand side of the cell, |right_decorations|
 // for the right-hand side.
 // |decorations| will contain the resulting visible decorations, and
 // |decoration_frames| will contain their frames in the same coordinates as
 // |frame|.  Decorations will be ordered left to right. As a convenience returns
 // the index of the first right-hand decoration.

[Sidney San Martín, 2016/12/16 00:05:54]

This comment needs to be updated too.

[lgrey, 2016/12/20 19:41:03]

Done.

 size_t CalculatePositionsInFrame(
     NSRect frame,
-    const std::vector<LocationBarDecoration*>& left_decorations,
-    const std::vector<LocationBarDecoration*>& right_decorations,
+    const std::vector<LocationBarDecoration*>& leading_decorations,
+    const std::vector<LocationBarDecoration*>& trailing_decorations,
     std::vector<LocationBarDecoration*>* decorations,
     std::vector<NSRect>* decoration_frames,
     NSRect* remaining_frame) {
   decorations->clear();
   decoration_frames->clear();
 
-  // Layout |left_decorations| against the LHS.
-  CalculatePositionsHelper(frame, left_decorations, NSMinXEdge,
-                           kLeftDecorationXOffset, decorations,
-                           decoration_frames, &frame);
+  BOOL is_rtl = cocoa_l10n_util::ShouldDoExperimentalRTLLayout();

[Sidney San Martín, 2016/12/16 00:05:54]

The large number of is_rtl checks in the CL smell funny to me, and it's hard for
me to keep track of when we're working in leading/trailing space versus
left/right space.

I would probably approach this by converting to "real" coordinates as late as
possible. For example, imagine adding this utility function:

    CGRect CGRectFlipXInRect(CGRect rect, CGRect inRect) {
      CGAffineTransform transform = CGAffineTransformScale(
          CGAffineTransformMakeTranslation(inRect.size.width, 0), -1, 1);
      return CGRectApplyAffineTransform(rect, transform);
    }

Instead of all of the new checks, you could then just have something like this
at the end of CalculatePositionInFrame:

    if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout()) {
      for (NSRect& decorationFrame : *decoration_frames) {
        decorationFrame = CGRectFlipXInRect(decorationFrame, frame);
      }
      *text_frame = CGRectFlipXInRect(*text_frame, frame);
    }

Maybe there’s a way to do it even later, or a different approach. The one in
this CL just seems hard to reason about/maintain, though :/.

[lgrey, 2016/12/20 19:41:03]

Done.

+  // Layout left decorations (leading in LTR, trailing in RTL) against the LHS.
+  CGFloat left_padding =
+      is_rtl ? kTrailingDecorationXPadding : kLeadingDecorationXPadding;
+  const std::vector<LocationBarDecoration*>& left_decorations =
+      is_rtl ? trailing_decorations : leading_decorations;
+  CalculatePositionsHelper(frame, left_decorations, NSMinXEdge, left_padding,
+                           decorations, decoration_frames, &frame);
   DCHECK_EQ(decorations->size(), decoration_frames->size());
 
   // Capture the number of visible left-hand decorations.
   const size_t left_count = decorations->size();

[Sidney San Martín, 2016/12/16 00:05:54]

It was unexpected to see "left" pop up here. It looks like it's only used by one
caller (textCursorFrameForFrame:), and it's unclear if it's the correct behavior
for RTL. Is there any way to make that code direction-agnostic and/or get rid of
the need for this one-off return value? It's not a huge deal, though… I won't
push back if you think it's fine.

[lgrey, 2016/12/20 19:41:03]

Done.

 
+  // Layout right decorations against the RHS.
+  CGFloat right_padding =
+      is_rtl ? kLeadingDecorationXPadding : kTrailingDecorationXPadding;
+  const std::vector<LocationBarDecoration*>& right_decorations =
+      is_rtl ? leading_decorations : trailing_decorations;
+
+  CalculatePositionsHelper(frame, right_decorations, NSMaxXEdge, right_padding,
+                           decorations, decoration_frames, &frame);
+  DCHECK_EQ(decorations->size(), decoration_frames->size());
+
+  // Reverse the trailing decorations so that overall everything is
+  // sorted back to front.
+  const size_t leading_count =
+      is_rtl ? decorations->size() - left_count : left_count;
   // Extend the text frame so that it slightly overlaps the rightmost left
   // decoration.
-  if (left_count) {
-    frame.origin.x -= kTextFrameDecorationOverlap;
+  if (leading_count) {
+    if (!is_rtl)
+      frame.origin.x -= kTextFrameDecorationOverlap;

[Sidney San Martín, 2016/12/16 00:05:54]

This is also a good example. It takes some thought to understand why this tweak
is skipped in RTL. If we were staying in leading/trailing space until later, it
wouldn't be needed.

[lgrey, 2016/12/20 19:41:03]

Done.

     frame.size.width += kTextFrameDecorationOverlap;
   }
 
-  // Layout |right_decorations| against the RHS.
-  CalculatePositionsHelper(frame, right_decorations, NSMaxXEdge,
-                           kRightDecorationXOffset, decorations,
-                           decoration_frames, &frame);
-  DCHECK_EQ(decorations->size(), decoration_frames->size());
-
-  // Reverse the right-hand decorations so that overall everything is
-  // sorted left to right.
   std::reverse(decorations->begin() + left_count, decorations->end());
   std::reverse(decoration_frames->begin() + left_count,
                decoration_frames->end());
 
   *remaining_frame = frame;
   return left_count;
 }
 
 }  // namespace
 
@@ skipping 23 matching lines @@
 }
 
 - (void)clearTrackingArea {
   for (auto& decoration : mouseTrackingDecorations_)
     decoration->RemoveTrackingArea();
 
   mouseTrackingDecorations_.clear();
 }
 
 - (void)clearDecorations {
-  leftDecorations_.clear();
-  rightDecorations_.clear();
+  leadingDecorations_.clear();
+  trailingDecorations_.clear();
   [self clearTrackingArea];
 }
 
-- (void)addLeftDecoration:(LocationBarDecoration*)decoration {
-  leftDecorations_.push_back(decoration);
+- (void)addLeadingDecoration:(LocationBarDecoration*)decoration {
+  leadingDecorations_.push_back(decoration);
 }
 
-- (void)addRightDecoration:(LocationBarDecoration*)decoration {
-  rightDecorations_.push_back(decoration);
+- (void)addTrailingDecoration:(LocationBarDecoration*)decoration {
+  trailingDecorations_.push_back(decoration);
 }
 
 - (CGFloat)availableWidthInFrame:(const NSRect)frame {
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
-  CalculatePositionsInFrame(frame, leftDecorations_, rightDecorations_,
+  CalculatePositionsInFrame(frame, leadingDecorations_, trailingDecorations_,
                             &decorations, &decorationFrames, &textFrame);
 
   return NSWidth(textFrame);
 }
 
 - (NSRect)frameForDecoration:(const LocationBarDecoration*)aDecoration
                      inFrame:(NSRect)cellFrame {
   // Short-circuit if the decoration is known to be not visible.
   if (aDecoration && !aDecoration->IsVisible())
     return NSZeroRect;
 
   // Layout the decorations.
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
-  CalculatePositionsInFrame(cellFrame, leftDecorations_, rightDecorations_,
-                            &decorations, &decorationFrames, &textFrame);
+  CalculatePositionsInFrame(cellFrame, leadingDecorations_,
+                            trailingDecorations_, &decorations,
+                            &decorationFrames, &textFrame);
 
   // Find our decoration and return the corresponding frame.
   std::vector<LocationBarDecoration*>::const_iterator iter =
       std::find(decorations.begin(), decorations.end(), aDecoration);
   if (iter != decorations.end()) {
     const size_t index = iter - decorations.begin();
     return decorationFrames[index];
   }
 
   // Decorations which are not visible should have been filtered out
   // at the top, but return |NSZeroRect| rather than a 0-width rect
   // for consistency.
   NOTREACHED();
   return NSZeroRect;
 }
 
 - (NSRect)backgroundFrameForDecoration:(LocationBarDecoration*)decoration
                                inFrame:(NSRect)cellFrame
                       isLeftDecoration:(BOOL*)isLeftDecoration {
   NSRect decorationFrame =
       [self frameForDecoration:decoration inFrame:cellFrame];
+  std::vector<LocationBarDecoration*>& left_decorations =
+      cocoa_l10n_util::ShouldDoExperimentalRTLLayout() ? trailingDecorations_
+                                                       : leadingDecorations_;
   *isLeftDecoration =
-      std::find(leftDecorations_.begin(), leftDecorations_.end(), decoration) !=
-      leftDecorations_.end();
+      std::find(left_decorations.begin(), left_decorations.end(), decoration) !=
+      left_decorations.end();
   return decoration->GetBackgroundFrame(decorationFrame);
 }
 
 // Overriden to account for the decorations.
 - (NSRect)textFrameForFrame:(NSRect)cellFrame {
   // Get the frame adjusted for decorations.
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame = [super textFrameForFrame:cellFrame];
-  CalculatePositionsInFrame(textFrame, leftDecorations_, rightDecorations_,
-                            &decorations, &decorationFrames, &textFrame);
+  CalculatePositionsInFrame(textFrame, leadingDecorations_,
+                            trailingDecorations_, &decorations,
+                            &decorationFrames, &textFrame);
 
   // The text needs to be slightly higher than its default position to match the
   // Material Design spec. It turns out this adjustment is equal to the single
   // pixel line width (so 1 on non-Retina, 0.5 on Retina). Make this adjustment
   // after computing decoration positions because the decorations are already
   // correctly positioned. The spec also calls for positioning the text 1pt to
   // the right of its default position.
   textFrame.origin.x += 1;
   textFrame.size.width -= 1;
   textFrame.origin.y -= singlePixelLineWidth_;
 
   // NOTE: This function must closely match the logic in
   // |-drawInteriorWithFrame:inView:|.
 
   return textFrame;
 }
 
 - (NSRect)textCursorFrameForFrame:(NSRect)cellFrame {
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
-  size_t left_count =
-      CalculatePositionsInFrame(cellFrame, leftDecorations_, rightDecorations_,
-                                &decorations, &decorationFrames, &textFrame);
+  size_t left_count = CalculatePositionsInFrame(
+      cellFrame, leadingDecorations_, trailingDecorations_, &decorations,
+      &decorationFrames, &textFrame);
 
   // Determine the left-most extent for the i-beam cursor.
   CGFloat minX = NSMinX(textFrame);
   for (size_t index = left_count; index--; ) {
     if (decorations[index]->AcceptsMousePress())
       break;
 
     // If at leftmost decoration, expand to edge of cell.
     if (!index)
       minX = NSMinX(cellFrame);
@@ skipping 78 matching lines @@
 
 - (void)drawInteriorWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
   ui::ScopedCGContextSmoothFonts fontSmoothing;
   [super drawInteriorWithFrame:cellFrame inView:controlView];
 
   // NOTE: This method must closely match the logic in |-textFrameForFrame:|.
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect workingFrame;
 
-  CalculatePositionsInFrame(cellFrame, leftDecorations_, rightDecorations_,
-                            &decorations, &decorationFrames, &workingFrame);
+  CalculatePositionsInFrame(cellFrame, leadingDecorations_,
+                            trailingDecorations_, &decorations,
+                            &decorationFrames, &workingFrame);
 
   // Draw the decorations. Do this after drawing the interior because the
   // field editor's background rect overlaps the right edge of the security
   // decoration's hover rounded rect.
   for (size_t i = 0; i < decorations.size(); ++i) {
     if (decorations[i]) {
       decorations[i]->DrawWithBackgroundInFrame(decorationFrames[i],
                                                 controlView);
     }
   }
 }
 
 - (BOOL)canDropAtLocationInWindow:(NSPoint)location
                            ofView:(AutocompleteTextField*)controlView {
   NSRect cellFrame = [controlView bounds];
   const NSPoint locationInView =
       [controlView convertPoint:location fromView:nil];
 
-  // If we have decorations, the drop can't occur at their horizontal padding.
-  if (!leftDecorations_.empty() && locationInView.x < kLeftDecorationXOffset)
+  CGFloat leftEdge = 0;
+  CGFloat rightEdge = NSWidth(cellFrame);
+  BOOL isRTL = cocoa_l10n_util::ShouldDoExperimentalRTLLayout();

[Sidney San Martín, 2016/12/16 00:05:54]

is_rtl — but it would be nicer if it could be eliminated.

[lgrey, 2016/12/20 19:41:03]

Done.

+  if (!leadingDecorations_.empty()) {
+    if (isRTL)
+      rightEdge -= kLeadingDecorationXPadding;
+    else
+      leftEdge += kLeadingDecorationXPadding;
+  }
+  if (!trailingDecorations_.empty()) {
+    if (isRTL)
+      leftEdge += kTrailingDecorationXPadding;
+    else
+      rightEdge -= kTrailingDecorationXPadding;
+  }
+  if (locationInView.x < leftEdge)
     return false;
 
-  if (!rightDecorations_.empty() &&
-      locationInView.x > NSWidth(cellFrame) - kRightDecorationXOffset) {
+  if (locationInView.x > rightEdge) {
     return false;
   }
 
   LocationBarDecoration* decoration =
       [self decorationForLocationInWindow:location
                                    inRect:cellFrame
                                    ofView:controlView];
   return !decoration;
 }
 
@@ skipping 10 matching lines @@
                                                  inRect:(NSRect)cellFrame
                                                  ofView:(AutocompleteTextField*)
                                                             controlView {
   const BOOL flipped = [controlView isFlipped];
   const NSPoint locationInView =
       [controlView convertPoint:location fromView:nil];
 
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
-  CalculatePositionsInFrame(cellFrame, leftDecorations_, rightDecorations_,
-                            &decorations, &decorationFrames, &textFrame);
+  CalculatePositionsInFrame(cellFrame, leadingDecorations_,
+                            trailingDecorations_, &decorations,
+                            &decorationFrames, &textFrame);
 
   for (size_t i = 0; i < decorations.size(); ++i) {
     if (NSMouseInRect(locationInView, decorationFrames[i], flipped))
       return decorations[i];
   }
 
   return NULL;
 }
 
 - (NSMenu*)decorationMenuForEvent:(NSEvent*)theEvent
@@ skipping 198 matching lines @@
 - (NSDragOperation)draggingSourceOperationMaskForLocal:(BOOL)isLocal {
   return NSDragOperationCopy;
 }
 
 - (void)updateMouseTrackingAndToolTipsInRect:(NSRect)cellFrame
                                       ofView:
                                           (AutocompleteTextField*)controlView {
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
-  CalculatePositionsInFrame(cellFrame, leftDecorations_, rightDecorations_,
-                            &decorations, &decorationFrames, &textFrame);
+  CalculatePositionsInFrame(cellFrame, leadingDecorations_,
+                            trailingDecorations_, &decorations,
+                            &decorationFrames, &textFrame);
   [self clearTrackingArea];
 
   for (size_t i = 0; i < decorations.size(); ++i) {
     CrTrackingArea* trackingArea =
         decorations[i]->SetupTrackingArea(decorationFrames[i], controlView);
     if (trackingArea)
       mouseTrackingDecorations_.push_back(decorations[i]);
 
     NSString* tooltip = decorations[i]->GetToolTip();
     if ([tooltip length] > 0)
@@ skipping 30 matching lines @@
 
 @end
 
 @implementation AutocompleteTextFieldCell (TestingAPI)
 
 - (const std::vector<LocationBarDecoration*>&)mouseTrackingDecorations {
   return mouseTrackingDecorations_;
 }
 
 @end