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

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

Index: chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
index 693ef769066bf8a0d1a04a57c0c23069142b8bb1..60f770988fb336260023aed05ce7add37bd47275 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
@@ -152,6 +152,17 @@ 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,
@@ -363,6 +374,10 @@ NSAttributedString* CreateClassifiedAttributedString(
     }
 
     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];