Mac (Cocoa) Omnibox: Force text field to LTR context if it is a URL.

chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm

 // Copyright 2013 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/omnibox/omnibox_popup_cell.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <cmath>
@@ skipping 134 matching lines @@
   // Calculate a slightly smaller font. The ratio here is somewhat arbitrary.
   // Proportions from 5/9 to 5/7 all look pretty good.
   CGFloat size = [font pointSize] * 5.0 / 9.0;
   NSFontDescriptor* descriptor = [font fontDescriptor];
   return [NSFont fontWithDescriptor:descriptor size:size];
 }
 NSFont* SmallFont() {
   return OmniboxViewMac::GetSmallFont();
 }
 
+void SetTextDirectionForRange(NSMutableAttributedString* attributedString,

[tapted, 2016/07/21 04:37:05]

This should be something like SetURLTextDirection, and the comment below should
be attached to the function. E.g. there's no point passing in |direction| if
it's always NSWritingDirectionLeftToRight, and it's not really clear why this
function exists without that comment

[Matt Giuca, 2016/07/21 04:55:37]

It isn't necessarily setting the text direction for a URL (it operates on an
arbitrary text). That it is only ever used on URLs is irrelevant to this
function, and potentially confusing (calling it "SetURLTextDirection" might
imply it does nothing if it is not a URL). I'd prefer to not use the word URL
here.
That's kind of the same thing as the naming: this function is not specific to
URLs and it just looks confusing to me to have a function that is technically
generic, but named and documented after its intended usage, rather than what it
actually does (which is set the writing direction).

I added a comment, but it doesn't mention URLs.
I have a |direction| argument because it seems unnecessarily specific to have a
function that sets it to LTR when this can easily be generalized.
I suppose this could go the other way and be FormatTextAsURL, but then I'd
expect it to do all the other things to format like a URL (like making it blue
or whatever). I think it's better to not make this a function about formatting
URLs and have it be about setting the text direction of an AttributedString.

+                              NSWritingDirection direction,
+                              NSRange range) {
+  base::scoped_nsobject<NSMutableParagraphStyle> paragraph_style(
+      [[NSMutableParagraphStyle alloc] init]);
+  [paragraph_style setBaseWritingDirection:direction];
+  [attributedString addAttribute:NSParagraphStyleAttributeName
+                           value:paragraph_style
+                           range:range];
+}

[tapted, 2016/07/21 04:37:05]

nit: any reason to have this declared up here, rather than just above
CreateClassifiedAttributedString?

[Matt Giuca, 2016/07/21 04:55:37]

Well kind of along similar lines, it isn't specific to
CreateClassifiedAttributedString, just gets used there. I can move it there if
you like.

+
 NSAttributedString* CreateAnswerStringHelper(const base::string16& text,
                                              NSInteger style_type,
                                              bool is_bold,
                                              BOOL is_dark_theme) {
   NSDictionary* answer_style = nil;
   NSFont* answer_font = nil;
   bool is_mode_material = ui::MaterialDesignController::IsModeMaterial();
   switch (style_type) {
     case SuggestionAnswer::TOP_ALIGNED:
       answer_style = @{
@@ skipping 191 matching lines @@
     const NSRange range =
         NSMakeRange(location, std::min(length, match_length - location));
 
     if (0 != (i->style & ACMatchClassification::MATCH)) {
       [attributedString addAttribute:NSFontAttributeName
                                value:BoldFieldFont()
                                range:range];
     }
 
     if (0 != (i->style & ACMatchClassification::URL)) {
+      // URLs have their text direction set to to LTR (avoids RTL characters
+      // making the URL render from right to left, as per RFC 3987 Section 4.1).
+      SetTextDirectionForRange(attributedString, NSWritingDirectionLeftToRight,
+                               range);
       [attributedString addAttribute:NSForegroundColorAttributeName
                                value:URLTextColor(is_dark_theme)
                                range:range];
     } else if (0 != (i->style & ACMatchClassification::DIM)) {
       [attributedString addAttribute:NSForegroundColorAttributeName
                                value:DimTextColor(is_dark_theme)
                                range:range];
     }
   }
 
@@ skipping 343 matching lines @@
   base::string16 raw_separator =
       l10n_util::GetStringUTF16(IDS_AUTOCOMPLETE_MATCH_DESCRIPTION_SEPARATOR);
   return CreateAttributedString(raw_separator, DimTextColor(isDarkTheme));
 }
 
 + (CGFloat)getContentAreaWidth:(NSRect)cellFrame {
   return NSWidth(cellFrame) - TextStartOffset();
 }
 
 @end