[Mac] Reverse the omnibox in RTL

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

Index: chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
diff --git a/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm b/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
index 91bc402bde8ff5b71a8e8de464672df461ab74a1..353618ca7e4a8313b36520a2feb8f15e71cc356e 100644
--- a/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
+++ b/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
@@ -11,6 +11,7 @@
 #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"
@@ -32,10 +33,11 @@ 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
@@ -116,38 +118,49 @@ void CalculatePositionsHelper(
 // 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());
@@ -191,24 +204,24 @@ size_t CalculatePositionsInFrame(
 }
 
 - (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);
@@ -224,8 +237,9 @@ size_t CalculatePositionsInFrame(
   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 =
@@ -247,9 +261,12 @@ size_t CalculatePositionsInFrame(
                       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);
 }
 
@@ -259,8 +276,9 @@ size_t CalculatePositionsInFrame(
   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
@@ -282,9 +300,9 @@ size_t CalculatePositionsInFrame(
   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);
@@ -383,8 +401,9 @@ size_t CalculatePositionsInFrame(
   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
@@ -403,12 +422,25 @@ size_t CalculatePositionsInFrame(
   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;
   }
 
@@ -439,8 +471,9 @@ size_t CalculatePositionsInFrame(
   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))
@@ -659,8 +692,9 @@ static NSString* UnusedLegalNameForNewDropFile(NSURL* saveLocation,
   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) {